trentm / node-bunyan

a simple and fast JSON logging module for node.js services
Other
7.16k stars 515 forks source link

Bunyan outputs `{}` for Map objects even when they are fully populated #461

Open richardkiene opened 7 years ago

richardkiene commented 7 years ago

For example,

var a = new Map();
log.info(a, 'outputs {} which is correct at this point');
a.set('z', 1);
log.info(a, 'outputs {} which is incorrect at this point');

I believe this is because there is no toJSON method on Map. Monkey patching a toJSON method on to Map results in correct output.

Example monkey patch:

Map.prototype.toJSON = function ()
{
    var d={};
    this.forEach(function (v, k) {
        try {
            d[k]=v
        } catch (e) {
            /* uh-oh */
        } })
    return d;
};

Ideally Bunyan would output the full contents of a Map when logged, but that may be rather problematic given the diversity of types that can be in a map as keys and values. That said, Bunyan should at least output something that lets log readers understand that it can't actually output the contents of a Map so that they aren't mislead into thinking that a Map object with contents is empty.

Something like [Map (contents suppressed)] would work for me.

trentm commented 7 years ago

Thanks. Without a monkey-patched "Map.prototype.toJSON", bunyan won't currently be able to do anything for Map objects anywhere but at the top-level, e.g. like this:

var mymap = new Map();
mymap.set('a', 1);
// ...

log.info(mymap, 'a message');

With mymap as a top-level fields argument, Bunyan's current mkRecord function could guard against that being a Map instance. However for deeper stuff:

var o = {
    m: mymap
};
log.info(o, 'a message');

Bunyan doesn't currently inspect the type of each field of o. Doing so would have perf implications that i'd be vary adding to bunyan.

Even if that were done, if the Map instance were deeper (say, o.foo.bar.baz), bunyan's usage of JSON.stringify to serialize a record would have to significantly change. Note that there might might be some precedent here. Currently in bunyan if JSON.stringify blows up it can fallback to safe-json-stringify which IIUC does visit all nested objects. The purpose of this is to ensure bunyan logging does not crash, rather than to get a better serialization for some types. However, it could be a considered feature to have a beefed up (albeit slower) Bunyan serialization method that can have custom handling for specific types. There are other ES5/6 types to consider here as well, I believe: Map, Set, WeakMap, WeakSet, possibly others (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects).

The basic issue here is that Bunyan currently basically relies on the JavaScript's languages "JSON.stringify" to serialize types. JSON is specified, but new types are coming that don't have a specified representation in JSON. Also (I could be wrong), I think node.js sometimes defines toJSON methods for some objects? To illustrate:

> var f= function () {};
undefined
> f
[Function]
> JSON.stringify(f);
undefined
> var m = new Map();
undefined
> m
Map {}
> JSON.stringify(m)
'{}'
> var d = new Date();
undefined
> d
Wed Nov 16 2016 11:15:16 GMT-0800 (PST)
> JSON.stringify(d);
'"2016-11-16T19:15:16.251Z"'

So, possibilities here:

  1. Just handle special case of "Map" as a top-level log.info(mymap, 'a message'); and move on. That seems of limited help.
  2. Document for users of bunyan that for some sort of less surprising serialization of some types, they'd need to take some action at the app level. One possible action is to decide on a JSON serialization and monkey-patch a toJSON method on those types. IMO monkey patching must be at the app level to avoid modules stomping on each other, so bunyan won't do this itself by default. We should at least do this step.
  3. Consider a feature for optional beefier JSON serialization for Bunyan. I.e. a way to have a particular Bunyan logger instance not just use JSON.stringify, but instead a smarter serializer that will visit properties and do better for certain types.
trentm commented 7 years ago

I think we should:

  1. Add docs (aside: Bunyan's readme is getting a little heavy) warning users about Map, Set, et al not being rendered because of JS's or Node core's toJSON for those.

  2. Possibly offer functions that top-level apps can use to monkey-patch toJSON for these types:

    # myapp.js
    Map.prototype.toJSON = bunyan.monkeyPatchMapToJSON;
    Set.prototype.toJSON = bunyan.monkeyPatchSetToJSON;
    // This is intentionally wordy to force (a) it being a little painful
    // (monkey patching is evil) and (b) ensure that the assignment to
    // `toJSON` is in *application* code.
  3. Have support for a custom JSON stringifier (deps on #460) and have an example that handles these types (with the caveat that it'll be much slower).