Never serialize .cause Errors, rely on message + stack summary instead
#110
Conversation
This reverts commit 5b88f7e.
This reverts commit ae83956.
Instead rely on its content being appended to the message and the stack. This is an alternative fix to pinojs#94, replacing pinojs#105 and pinojs#108 + makes pinojs#109 not needed
|
Oh, just saw this PR, so I'll ask here as well/instead: could you elaborate on explicitly opting out of serializing Specifically, my original reason for making #105 and #108 was that serializing just the error messages was often not very useful, I needed a nested set of stack traces to get to the bottom of an error. |
|
Maybe I have just overlooked stack traces with "caused by" in them? I think I'm starting to see what your point is with your implementation. It seems like an issue to me that it was not clear in the output I saw, and that it didn't seem to cover the use case I had -- but with the understanding I'm now getting, I see what I must have missed, and how it works. |
|
I'm taking the opportunity to present you with a problem I'm having. Here is my scenario: class CustomErrorWithInfo extends VError {
constructor(cause) {
super(
{
cause,
info: {
foo: 'bar',
},
},
'Custom error with info'
);
}
}
class CustomErrorWithNoStackTrack extends VError {
constructor(cause) {
super(
{
cause,
},
'Custom error with no stack trace'
);
this.stack = undefined;
}
}
try {
try {
try {
throw new Error('first error');
} catch (err) {
throw new CustomErrorWithInfo(err);
}
} catch (err) {
throw new CustomErrorWithNoStackTrack(err);
}
} catch (err) {
logger.error({
err,
});
}I wrote a custom prettifier (supposedly to be open sourced soon) that made that output: I've just upgraded the project dependencies, and discovered that your lib now appends the error chain in the message. That's cool in most cases, however, it doesn't show the special As a summary: I don't think it's a good idea to loose the original |
|
I think that for 90% of the use cases a summary of the cause chain is good enough. For the remainder it is always possible to provide a custom error serializer to Pino and that serializer could then skip the message and stack summary in favor of recursively serializing the error chain. I think this serializer should cater to the 90% use case and ensure that the output strikes a good balance between size between and usefulness.
@hypesystem Yes, both stacks and messages are summarized, so you should get the full context, just not any extra data of the cause errors. I think this strikes a good balance between the size of the output and the usefulness of the data. pino-std-serializers/lib/err.js Lines 61 to 62 in d8ce240 |
@mcollina Yes, sorry, should of course have added that, done ✔️ |
|
That's what I indeed up doing. FTR the stack from the object passed as argument of the custom serializer already embeds all error stacks, therefore the first stack is extracted with With this PR's title I was afraid you'd drop access to the original |
|
@KoltesDigital Maybe one could add an option for toggling between the behaviors, not sure what @mcollina, @jsumners and such feels about that, as currently no such options are part of this module. |
I don't think that's a great idea. I would prefer to have this module as vanilla as possible. I'm happy to have a separate module catering to more advanced use cases, or export a different error serializer under a different file in this module. |
|
@voxpelli is this mergeable/releasable now? |
@KoltesDigital That is kind of what this change does. Any @mcollina With the exception of the above possible caveat, then yes. |
|
@jsumners you ok for this to ship? |
|
I feel like there are still some unresolved issues in the little back and forth in code we've had. If there's no rush, I'd love the time to formulate what I see (but it won't be for a few hours). |
|
Okay, finally had a moment after putting my kid to bed 😄 I think overall I'm a bit confused as to the design goals for these serializers, and it seems like that confusion is also what led to the back and forth here. So I'll try to talk from what I think are the design goals, but honestly I'm confused, and that confusion will probably lead to similar things in the future. I'm feeling a lack of alignment, so I'll try and at least explain how I see the problem space 😄 sorry if this is too much, I'm trying to be concise. I understand from @mcollina that the goal is to keep this library simple/"vanilla", and let people use their own more advanced error serializers if they need them. I assume that a good "simple/vanilla" solution will have some simple rules that just about support "normal" (common?) errors. In that case, the specific support for VError is a bit weird, but not too far out of scope. I think overall there are two approaches:
I would argue that people commonly use their own error objects that may or may not even be Pino already supports From this observation I think I have the following feedback on the current implementation + this PR:
I'm not necessarily saying the approach in my PR is the correct solution, but I think there are problems with the summarization approach that I haven't seen discussed anywhere. @voxpelli says the goal of the approach is to strike "a good balance between the size of the output and the usefulness of the data". If that is a design goal, I don't think that's reflected elsewhere in the code --- this would speak for more of a limited scope approach (see above and #27), and would certainly be reasonable! But it's not what I would expect from the other behavior of this library as is. Hope you can help me see more clearly what the aim of this library is, so I can contribute most helpfully 😄 (and make my own thing if I'm going in a different direction). |
|
I do not have time to "dig deep" into the ins-and-out of this change and the topic. Could you make some example? I'm a bit lost. @kibertoad could you chime in too? |
|
Sorry, I know that was long. Basically: the current error serializer is confusing and unpredictable. Do you want
These two design goals run counter to each other and that makes it hard to know what to expect from this library, and how to contribute. |
|
The expected behavior is
|
I concur. This module exports a function for wrapping the error serializer specifically so that the functionality can be extended without constant patches to this module -- https://github.com/pinojs/pino-std-serializers#exportswraperrorserializercustomserializer. We added the |
|
Cool, it seems there is consenaus among y'all, this PR definitely fits that consensus 😁 Do you also agree that removing serialization of every field is a good idea then? c.f. #27 — or has something in the observations regarding that changed? (I don't wanna do another PR that's instantly reverted 😅) |
That wrapper is what I linked above. Let's keep this simple and make the "standard serializer" only serialize standard |
|
This change has been quite detrimental to us. We use Restify & follow a restify-like pattern in many places, where we often see things like: Before this PR, our resultant error-handler logs would get useful information from the properties on the These logs were also of well structured data that could be easily read: Now we have: a) no properties/fields that we had before, b) unstructured stack traces that we cannot parse/walk/use.
This is a reasonable problem statement. But. I feel like we took the wrong path here, in deciding not to serialize errors. We should have gone down the other path available to us & started reducing the quantity of (not-so-machine-readable/not-so-structured) summary information we present. There are some legit improvements we could have made, that my example disuses. I'd far have preferred we serialize errors, & have each error print only it's piece of the stack next to it (not the whole stack). The original premise was we don't need to do both, and that's fine, but we chose the low density, lower information, less machine-usable pick. I'd like us to do better. I also think this PR sets us on the wrong path/precedent with another complex error, AggregateError.
|
|
@rektide You bring up similar criticism as @hypesystem did and which was discussed earlier in this thread with pointers on how one can cater to your needs but why it also makes sense for it to not be the default. Have you looked at eg #110 (comment) ? |
I've seen opinions but not justifications for what has been done here. Pino has, so far, been a great tool that gives users the data. A useless, text only, stack only log message with zero details (as my simple example shows) seems to be the new pivot, and it feels like a frigging joke. There's nothing that makes sense about any of this. This is such a harsh & radical u-turn on what pino has been, & I don't see any arguments made anywhere here to argue with: there's a seeming vast concensus that basically doing nothing for the user & concealing all relevant information is good, & the one dude who said, wait no please dont, got steamrolled already. There's literally no argumentation to disagree with about why doing absolutely nothing good for the user is beneficial, so there's no chance to make a concerted disagreement, no chance we ever make any other decisions, no chance for any other views to have a chance. Doing nothing but unreadable textual spamming inside JSON needs justification, and it hasn't. I've tried to show some very basic impact. But getting told "how one can cater to your needs" when my needs are- I want my logs to show some basic minimal information in a machine readable way/json-y way- is deeply cutting & a joke. I've gone to great lengths to show that this PR is a complete destruction of useful information at all levels, completely contrary to any modern logging practice in it's utter savaging of structured information. My needs are to show some basic sensible error messages. Your needs are to show nothing but text blobs??? You should do your own thing if you have those "particular" needs. I see other authors agree, but again, I just see zero justification. I can't argue with points of view that have never been justified AT ALL.
Again.... why did we not take the other path? Why show only a worthless text-only summary, instead of reducing the summary & cleanly nicely doing a very similar but structured & informative output? Many opinions, few arguments. In my honest opinion: much harm (with no rationalization for why). |
|
Configuring a different serializer is easy, if you have a different need you can configure your own. I do not understand why you are so upset. I'm not sure we need any justification to do a breaking change in a semver-major release. |
|
Hey, I made an alternate (exhaustive) error serializer that I'd love input on, that sounds like it fits your needs @rektide - pretty early stage for now: https://www.npmjs.com/package/serialize-every-error |
It makes me upset to see useful, helpful, structured information replaced by a massive stack trace that holds much of the same information in a unusable unstructured form. I don't see what could justify turning the good, usable information we had into bad, unusable information like this. Of the two paths we picked, we chose to keep the obviously worse path we had, rather than work with the good one:
|
This is an alternative fix to #94, replacing #105 and #108 and in the process make #109 not needed
Builds on my comment here #105 (comment)
In short:
Doing both a summary of the errors cause chain + serializing all the errors in the cause chain is doing the same thing twice basically.
And when on top of that doing summaries for each cause along the chain as well, then that's triple work.
In https://github.com/pinojs/pino-std-serializers/pull/78/files#diff-71c187d84f435004e68161a25e470d823f4de310a1d39b531cc02a90f3730149R56 I specifically opted out causes from recursion:
I think #94 simply reflects that it didn't do what it intended do do, which should have been this:
That way it will exclude all
Errorcauses from serialization, as intended, but keeping other causes for increased backwards compatibility.This PR reverts #105 and #108, then applies the above fix.