logging: Serialization of Native Types

[zbjornson]

One more logging question/possible bug...

Built-in JS objects including Date, Error, undefined, and RegExp are not serializable into the proto. Is the intent to support these? (Maybe depends if you consider these "objects" when reading the JSON spec.)

var entry = log.entry(resource, {
  someDate: new Date()
});
log.write(entry...) // Error: .google.protobuf.Value#timestampValue is not a field: undefined

var entry = log.entry(resource, {
  someErr: new Error("ohno")
});
log.write(entry...) // Error: Value of type object not recognized
// same error for RegExp

var entry = log.entry(resource, {
  someValue: undefined
});
log.write(entry...) // Error: Value of type undefined not recognized

At least Date and Error are common in logs and I think nice to support (otherwise as a consumer I'm manually casting these to strings). The others (RegExp, typed arrays, others?) I think are less common. For comparison, BSON allows Date.

[stephenplusplus]

The entry payload (the data object) is a Struct, defined here:

message Value {
  oneof kind {
    NullValue null_value = 1;
    double number_value = 2;
    string string_value = 3;
    bool bool_value = 4;
    Struct struct_value = 5;
    ListValue list_value = 6;
  }
}

So just these types are supported, evidently. Is there anything we can do to make it easier to work with Date and Error values?

[zbjornson]

Assuming I'm correct in thinking that this interface is intended to consume JSONs and transparently use protos on the backend, then one approach could be to add something like this somewhere in Entry, to delete undefined values and coerce native types. (One key difference from above would be mapping Date to proto's Timestamp type.)

Perhaps this should be up to the consumer, but I imagine many people will run into it.

[stephenplusplus]

@filipjs any thoughts about what we should do when a user of this library attempts to write one of these types-- RegExp, Date, Error, undefined-- in a log entry?

[filipjs]

Yes, we are only supporting JSON specification (so no date type unfortunately). Changing other objects to their string representation should work, like in the linked code sample.

[zbjornson]

That's reasonable. Date type is the only one I'm actually sad not to have, but when #1348 gets resolved then at least the entry timestamp will be settable and searchable.

[abawany]

BTW, this also happens with Node Error type (https://nodejs.org/api/errors.html). I have worked around it by logging err.stack but this is highly inconvenient. I wish the logger did not force me to watch out for its specific quirks - I use the project winston-googlecloud and it is extremely inconvenient to have to work around the gcloud limitation in my code whereas winston is able to log errors sanely.

[stephenplusplus]

Can you send a PR?

[abawany]

Unfortunately I don't have a PR to submit - I modified instances in my code to never invoke the log with an Error object. To add more context: I use the package winston-googlecloud to enable me to use winston for logging in my service.

[stephenplusplus]

I might be misunderstanding, but I'll try to summarize the problem:

We let you write log entries with Error objects, but when you try to read the entries back, it's just a string.

If that's correct, I believe that's because we are trying to be invisible with user-provided data. Unfortunately, we can't simply write the Error object to the Logging API, we have to convert it to a string. And we don't want to put it some library-specific flag like "GCN-ERROR: {error}.toString()". But without such a flag, we can't know what's an error and what's not while we're reading it back.

A flag like "GCN-ERROR" could be used in a layer on top of ours to make logging errors more sane, i.e. winston-googlecloud. But being on the bottom layer, we have to be as transparent as possible to enable other such layers to decorate on top of us, and not tamper with the data we get from the user.

Let me know if that's not what we're talking about, or if there's another way to solve this problem altogether from this library. We definitely want to make it as easy as possible here, working within the parameters defined above.

[zbjornson]

I haven't used Winston or winston-googlecloud, but I do use Bunyan (and help maintain bunyan-stackdriver). Bunyan has this bit that nicely JSON-ifies Errors: https://github.com/trentm/node-bunyan/blob/master/lib/bunyan.js#L1155-L1168. This seems like the right level to deal with non-JSON types (i.e. I agree there shouldn't be special handling in gcloud, because it's then much harder to override the behavior if the base API is the problem point).

[abawany]

True, I guess I can submit a patch to winston-googlecloud to detect instances of Error and invoke toString() in such cases. Effectively that's what I ended up doing in my service by grepping and scrubbing for such instances.