logging: Serialization of circular structs

[zbjornson]

Think I've hit all the edge cases now...! :)

var circ = {a: 1};
circ.circ = circ;
var entry = log.entry(resource, circ);
log.write(entry...) // RangeError: Maximum call stack size exceeded

I think this should be handled in this library, not by the consumer?

[stephenplusplus]

I appreciate your help finding these edge cases! We can use https://www.npmjs.com/package/is-circular to check if there is a circular reference in the object. What do you think we should do if we catch one?

[zbjornson]

Rather than rejecting the payload, I think it would be useful to replace the circular reference with something like "[Circular]", which is what node does:

> var x = {a: 1}
undefined
> x.x = x
{ a: 1, x: [Circular] }

Maybe https://www.npmjs.com/package/destroy-circular (haven't tested myself -- all of the unit tests are simple).

[stephenplusplus]

Node does that for the sake of the output, but if you try to use that object (e.g. JSON.stringify), it will throw. The fact that a circular reference is there is likely an error, though I personally don't mind throwing to communicate that error to the user. It's possible where there is a circular reference, the user meant there to be something else entirely. If we delete it, it might be an unexpected bit of magic, making it harder for the user to catch their mistake.

@callmehiphop what do you think?

[zbjornson]

All true.

BTW I didn't hit this issue in production, just when trying to find edge cases. The node http request object has circular references in it, but the source I'm pulling from picks out some of the non-circular properties. Maybe detecting and throwing is sufficient.

[stephenplusplus]

@callmehiphop should we add the is-circular dep and throw if we get a circular reference, or let the user's script break with a RangeError: Maximum call stack size exceeded?

[callmehiphop]

@stephenplusplus I like the is-circular route, should make the error more specific/easier to debug, WDYT?

[zbjornson]

FWIW, the library dependent upon this one that I was working on when I filed this issue recently went with the destroy-circular option (ref). It seems nicer to fix the circular reference problem, rather than add another dependency that "just" clarifies the error, but with the change to bunyan-stackdriver the decision here doesn't affect me anymore.

[stephenplusplus]

Thanks for the update. I think that's a good option as well, and a better fit for a library that wraps ours. I'm only uncomfortable about modifying part of a user's input. A circular reference is definitely a user mistake, but it's not always an "oh yeah, get rid of that for me, would ya?" mistake. I'm thinking about the times it's a "oh, I can't do that? Okay, I'll have to think of another way to save this data".

@callmehiphop PR incoming!