logging: support native types

[stephenplusplus]

Fixes #1352

When writing a log entry, this will stringify Date, Error, and regex types... or more accurately, anything given will be passed to String(value).

Also, this adds a change that will take a raw "LogEntry" from "ListLogEntries()" and convert the verbose protobuf object into a normal JS object. (See {module:grpc-service#structToObj_} and {module:grpc-service#decodeValue_})

[stephenplusplus]

@zbjornson - could you take a look and see if this works the way you envisioned?

$ npm install --save stephenplusplus/gcloud-node#spp--1352

[zbjornson]

@stephenplusplus will do later today or this weekend, thanks! 😀

[zbjornson]

Kinda confused, because it looks like the existing code was intended to send bona fide proto Timestamps when Dates were passed. I would be hugely in favor of making that work if that was actually the intent, because then it becomes possible to query the logs temporally by any embedded timestamp property. Was something else preventing Dates from working?

[stephenplusplus]

It's probably me who is missing something, so please let me know! What I found was the json_payload of a LogEntry message is a struct. A struct can only contain these types:

message Struct {
  // Unordered map of dynamically typed values.
  map<string, Value> fields = 1;
}

message Value {
  // The kind of value.
  oneof kind {
    // Represents a null value.
    NullValue null_value = 1;
    // Represents a double value.
    double number_value = 2;
    // Represents a string value.
    string string_value = 3;
    // Represents a boolean value.
    bool bool_value = 4;
    // Represents a structured value.
    Struct struct_value = 5;
    // Represents a repeated `Value`.
    ListValue list_value = 6;
  }
}

So it didn't appear that embedded properties could be authentic Timestamps. Maybe I'm getting my wires crossed and the timestamps are supported in some other way?

[zbjornson]

Ah right. Based on https://github.com/GoogleCloudPlatform/gcloud-node/pull/1374/files#diff-d4c994c2d3998577634f0671d459bae5L335, I was hoping that the timestampValue key causes coercion server-side to a timestamp while still letting it be sent as a generic struct, but I see now that the xxxValue is used to pick the Value kind going into the proto.

(Follow-up before that chunk of code is removed: any possibility of using a different proto for LogEntry that adds timestamp_type?)

That and the CI failure aside, this seems to be working well so far.

[stephenplusplus]

any possibility of using a different proto for LogEntry that adds timestamp_type?

I don't believe so. I think our hands are tied. @filipjs?

[filipjs]

any possibility of using a different proto for LogEntry that adds timestamp_type?
I don't believe so. I think our hands are tied. @filipjs?

Correct, json_payload adheres to the JSON spec.
But there already is a timestamp field in the log entry, is there a specific use case for having more timestamps?

[zbjornson]

(Sorry to prolong this discussion in a PR.)

Timestamps in the entry itself are nice for querying, in cases like

• Long running tasks with begin and end times. (Ugly workaround is adding 'duration' and querying on entry timestamp - duration.)
• Log entries that contain database records with timestamps (e.g. if you log [part of] the DB record for the entity that was requested, and the record contains createdAt/updatedAt).

I'm a bit lost in this code, but under the hood this actually uses the protoPayload of LogEntry, right? Or based on this line, maybe it's just converting JSON to proto for transit. -- Point being, both javascript and Google's servers support Date/timestamp types when the protoPayload is used I think(?), so this appears to be a limitation of the API client only? The API docs are cryptic on what the "approved types" are for proto payloads. 😕

[stephenplusplus]

I'd agree having timestamps supported in the payload would be nice.

@filipjs:

Is there somewhere @zbjornson should open a feature request, if this is indeed something you'd consider?

And back to the technical part, we do use jsonPayload -- should we instead use protoPayload? Would that allow for timestamps?

[filipjs]

Hey, sorry for the late response.

I'm a bit lost in this code, but under the hood this actually uses the protoPayload of LogEntry, right?

No, this uses the jsonPayload field, which is the Struct proto mentioned earlier.
protoPayload is used by other Cloud services, which use predefined messages (e.g. AppEngine).

maybe it's just converting JSON to proto for transit

We receive JSON and change it to proto. Since timestamp representation is just a string, we wouldn't be able to tell what type the given field should be.

And back to the technical part, we do use jsonPayload -- should we instead use protoPayload?

No, jsonPayload is the correct field.

But if you put a timestamp as a string inside JSON, that will still work. When reading logs back using the API, you can parse the string back to timestamp, since you know the type of field.
The only place that's not supported would be the Logs Viewer UI, which would display the field a string without any "fancy" formatting. Is that the feature request you have in mind?

[zbjornson]

The only place that's not supported would be the Logs Viewer UI, which would display the field a string without any "fancy" formatting. Is that the feature request you have in mind?

Exactly. I haven't yet dumped logs into BigQuery, but that would be the other place the type would matter. Not sure if you can easily convert types when doing that.

[zbjornson]

Since timestamp representation is just a string, we wouldn't be able to tell what type the given field should be.

I was looking at this bit that is removed in this PR:

-      timestampValue: {
-        seconds: secondsRounded,
-        nanos: Math.floor((seconds - secondsRounded) * 1e9)
-      }

which matches up with the Timestamp proto. It seems like it should be possible to send that timestampValue object and convert back server-side regardless of whether jsonPayload or protoPayload is used? /me doesn't know how the server-side works

[stephenplusplus]

@callmehiphop PTAL.

[coveralls]

Coverage remained the same at 100.0% when pulling aa1e31a on stephenplusplus:spp--1352 into 50d465b on GoogleCloudPlatform:master.

[stephenplusplus]

ping @callmehiphop

[stephenplusplus]

I think this is safe to merge. // @callmehiphop