Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialize err.cause in a nice way when possible #78

Merged
merged 4 commits into from
Nov 24, 2021
Merged

Serialize err.cause in a nice way when possible #78

merged 4 commits into from
Nov 24, 2021

Conversation

voxpelli
Copy link
Contributor

Fixes #76, see that for a description

I did some other tweaks at #77, which I needed to get this patch working locally

@voxpelli
Copy link
Contributor Author

Opps, wrong content, will push update

@voxpelli
Copy link
Contributor Author

Fixed

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

Could you add some docs here?

Could you open a matching PR in the pino repo to document this too?

err.cause.cause = Error('abc')
const serialized = serializer(err)
t.is(serialized.type, 'Error')
t.is(serialized.message, 'foo: bar: abc')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the message a concatenation of the three errors? Can you show an example of the full serialized error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because all three describe the error that has happened, but at three different levels. An example:

If err consists of thrown and wrapped errors in these three steps:

  1. ENOENT, stat '/nonexistent'
  2. Failed to open cache
  3. Can't cache example.com/robots.txt

Then when someone does:

pino.warn(err, 'robots.txt lookup failed');

It will log:

{
  "message": "Can't cache example.com/robots.txt: Failed to open cache: ENOENT, stat '/nonexistent'"
  "stack": `Error: Can't cache example.com/robots.txt
    at Object.<anonymous> (/examples/fullStack.js:5:12)
caused by: Error: Failed to open cache:
    at Object.<anonymous> (/examples/fullStack.js:3:12)
caused by: Error: ENOENT, stat '/nonexistent'
    at Object.<anonymous> (/examples/fs.js:4:15)`
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the idea is that the top most error should only describe the error on its abstraction level, not on any of the lower levels, but rather refer to the error causes for adding those description at those levels.

This makes it easy to distinguish between Can't cache example.com/robots.txt errors caused by Failed to open cache if there's also another reason going around that's maybe Malformed cache file content.

Here's a real life example: https://github.com/dependency-check-team/dependency-check/blob/c0537d04452098208132c6633277374dc9ad0a8d/lib/resolve-target.js#L40-L43

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. My biggest concern was that I thought I was using the standard error serializer from this package and that this change in message shape would break me. But it seems I'm not using it. Still, this will be a semver major change unless it serializes like:

{
  "message": "three",
  "stack": "...",
  "cause": {
    "message": "two",
    "stack": "...",
    "cause": {
      "message": "one",
      "stack": "..."
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep the nested cause and only extend stack, then I think we can make it a minor, then follow up with message + removing redundant cause in a major?

Thoughts @jsumners @mcollina?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking a bit more about this, I think safest would be to simply make this a major change and make all changes at once

@jsumners
Copy link
Member

How much effort would it take to implement without a dependency? Going from zero to greater than zero dependencies is not ideal.

@voxpelli
Copy link
Contributor Author

How much effort would it take to implement without a dependency? Going from zero to greater than zero dependencies is not ideal.

Not a huge effort, simplest would be a copy and paste from my module: https://github.com/voxpelli/pony-cause/blob/1f1dff84a54a44e5253fa6382c9d32d37943d747/index.js#L60-L161

100 lines including type comments then

@mcollina
Copy link
Member

Let's do that

@jsumners
Copy link
Member

How much effort would it take to implement without a dependency? Going from zero to greater than zero dependencies is not ideal.

Not a huge effort, simplest would be a copy and paste from my module: https://github.com/voxpelli/pony-cause/blob/1f1dff84a54a44e5253fa6382c9d32d37943d747/index.js#L60-L161

100 lines including type comments then

If that is okay with you, let's do that.

@voxpelli
Copy link
Contributor Author

Moved relevant code from pony-cause into this module.

What I didn't move: All of the relevant tests: https://github.com/voxpelli/pony-cause/tree/main/test They are not needed here unless individual development of the code will be done here + they are written in Mocha/Chai rather than tap, so wouldn't be a simple 1:1 copy :)

Any additional thing I should change/do` @jsumners @mcollina?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Semver major.

@mcollina mcollina merged commit 49b6b3d into pinojs:master Nov 24, 2021
@mcollina
Copy link
Member

Should we bump it in pino@7? I think so.

@voxpelli
Copy link
Contributor Author

I should add some documentation around this as well, right? Shall we create an issue and assign it to me?

@jsumners
Copy link
Member

Should we bump it in pino@7? I think so.

I'm torn. This is a big change to the output of errors. But people shouldn't be parsing the text of logged errors anyway?

I should add some documentation around this as well, right? Shall we create an issue and assign it to me?

Sure.

voxpelli added a commit to voxpelli/mocha that referenced this pull request Feb 9, 2022
It would be great to get the full error stack chain for errors with causes, especially as all current browsers and Node.js >=16 supports it, see eg https://v8.dev/features/error-cause and https://dev.to/voxpelli/pony-cause-1-0-error-causes-2l2o

Eg. `pino` merged support for this as well: pinojs/pino-std-serializers#78
@voxpelli voxpelli deleted the add-support-for-error-causes branch March 2, 2022 10:16
voxpelli added a commit to voxpelli/mocha that referenced this pull request Feb 27, 2024
It would be great to get the full error stack chain for errors with causes, especially as all current browsers and Node.js >=16 supports it, see eg https://v8.dev/features/error-cause and https://dev.to/voxpelli/pony-cause-1-0-error-causes-2l2o

Eg. `pino` merged support for this as well: pinojs/pino-std-serializers#78
voxpelli added a commit to mochajs/mocha that referenced this pull request Mar 4, 2024
* Append the cause stacks to the main stack trace

It would be great to get the full error stack chain for errors with causes, especially as all current browsers and Node.js >=16 supports it, see eg https://v8.dev/features/error-cause and https://dev.to/voxpelli/pony-cause-1-0-error-causes-2l2o

Eg. `pino` merged support for this as well: pinojs/pino-std-serializers#78

* Fix tests

* Skip some string concatenation

* Don't export needlessly + improve docs

* Improved recursive filtering

* Added loop protection

* Same logic for "message" in cause trail as in top

* Apply suggestions from code review

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

* Revert "Apply suggestions from code review"

This reverts commit 04f7008.

---------

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make use of err.cause when available
3 participants