Apply err serializer everywhere.

[mcollina]

Apply the error serializer if the error object is top-level. It does not seem to cause any performance regressions from my checks.

Fixes #895

[YouriT]

Be careful that this PR is bringing a breaking change if people were to leverage current behaviour.

[jsumners]

Be careful that this PR is bringing a breaking change if people were to leverage current behaviour.

Agreed.

[mcollina]

Be careful that this PR is bringing a breaking change if people were to leverage current behaviour.

TBH, I would consider that a bug in anybody codebase :(.

---

@jsumners I think this is almost good to go. I do not see any other way in which we can fix the change without breaking anybody.

[YouriT]

Be careful that this PR is bringing a breaking change if people were to leverage current behaviour.

TBH, I would consider that a bug in anybody codebase :(.

@jsumners I think this is almost good to go. I do not see any other way in which we can fix the change without breaking anybody.

Maybe it deserves a version bump with a breaking change notice in changelog.

[jsumners]

LGTM, but do we think this is a semver major?

[mcollina]

I'm torn. I don't see why anybody would not want this change, however we have recently bumped major and I do not want this to sit there for a long cycle.

This is be breaking if folks are relying on the difference between things. However this pops every now and then.. and I do not see why somebody would not want this. In fact, it is number 1 reason why I do not recommend logger.info(err).

[davidmarkclements]

what versions did you find there was no slow down? I thought instanceof was expensive? Is that no longer true in 12 and 14?

[YouriT]

instanceof was already there. Now there is even a branching being skipped in opposition to the original code.

[davidmarkclements]

do we need docs for this?

pino/browser.js serializer API needs to be synced with this

[davidmarkclements]

So the breaking change will be that if someone is using a custom error serializer and then do logger.info(err) the log output will now be different?

[jsumners]

do we need docs for this?

pino/browser.js serializer API needs to be synced with this

Yes, we need docs. I forgot to block on that 🤦

So the breaking change will be that if someone is using a custom error serializer and then do logger.info(err) the log output will now be different?

The change is that if someone does logger.info(err) they will now potentially get serialized output instead of nothing.

[YouriT]

The change is that if someone does logger.info(err) they will now potentially get serialized output instead of nothing.

AFAIK there isn't nothing at the moment, there is the error logged without passing it through the serializer. Hence message, stack, type and all other keys of the Error object are linked to the root of the log entry.

[jsumners]

The change is that if someone does logger.info(err) they will now potentially get serialized output instead of nothing.

AFAIK there isn't nothing at the moment, there is the error logged without passing it through the serializer. Hence message, stack, type and all other keys of the Error object are linked to the root of the log entry.

Typically, Error.prototype.toString() only returns the message.

[naseemkullah]

There seems to be no documentation with this PR, so I'll just ask here if that is ok:

does this mean we do not have to call the error object err anymore for pino to treat it special and serialize it as an error?

i.e. after this pr is merged:

const foo = new Error('hello');
const bar = {a: 'b'};
logger.error({foo, bar}, foo.message);

will result in foo being serialized?

[jsumners]

No. This PR addresses log.error(Error('message')).

[mcollina]

@davidmarkclements the browser version requires no changes as far as I understand: https://github.com/pinojs/pino/blob/master/test/browser-serializers.test.js#L47-L66. Please confirm

[davidmarkclements]

@mcollina 🤣 ok great, yeah confirmed. We've synchronised server pino with browser pino here instead hahaha.

[davidmarkclements]

👍

[mcollina]

@davidmarkclements @jsumners semver-major or semver-minor?

[jsumners]

I thought I asked first?

[mcollina]

You did! I'm passing that to @davidmarkclements!

[mcollina]

Any objection in shipping this in v6? @jsumners @davidmarkclements @delvedor @watson and possibly any others?

[jsumners]

If we use the justification that this fixes expected behavior, then we can do a minor release. Ideally, we'd issue a major out of an abundance of caution. But I am okay either way.

[mcollina]

I'm ok either way. We'll see if we can bump the major in Fastify. Let's do a major.

[delvedor]

I think it can be shipped as a minor, maybe with a note in the docs for showing how to get the old behavior?

[mcollina]

I think it can be shipped as a minor, maybe with a note in the docs for showing how to get the old behavior?

You can't get the old behavior, that's part of the problem.

[xamgore]

When nestedKey is defined and Error is passed to the logger, no fields will be displayed in the output. Does this PR fix it? Do I correctly understand @watson is the only person left to review the code?

export const logger = pino({
  nestedKey: 'payload',
})

process.on('uncaughtException', final(logger, (err, finalLogger) => {
  finalLogger.error(err, 'uncaughtException')
  process.exit()
}))

throw Error('wow') // { level: 50, pid, hostname, payload: {}, message: "uncaughtException"}

[mcollina]

Landed in the next branch, prepping for v7.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.