Fix bug Global serializer not invoked for strings when base: null

[yellowbrickc]

No description provided.

[davidmarkclements]

thanks @yellowbrickc - can you show benchmarks for this vs master?

[mcollina]

Can you run the benchmarks before and after and see if there is any difference?

[yellowbrickc]

I run the both benchmarks a few times and the result was always different, even the measurements for bunyan and co. In the last run some numbers for pino are higher, some are lower.
I'm attaching the 2 text files here.
bench_master.txt
bench_issue_479.txt

[mcollina]

Numbers should be stable, maybe you had something else running on your computer? Docker, Chrome, Slack could all be responsible :/.

[davidmarkclements]

.. ok I'm just thinking this through.. can you please verify this assumption:

we're making a change to the metadata API (by adding a msg property) to solve the global serializer not being invoked when base is null?

@yellowbrickc can you also add comparison benchmarks – as @mcollina says try making sure you have minimum background processes. Also, choose benchmarks that would hit this path and feel free to comment out the benchmarks for other loggers (which also might help with clearer numbers). Even with background processes disabled, they'll always vary slightly, but you're looking for within ~1-2%

[yellowbrickc]

I run the file /internal/just-pino.bench.js twice per branch and the numbers are reliable now. They look good I think but it is your call of course
bench_issue.txt
bench_master.txt

[yellowbrickc]

we're making a change to the metadata API (by adding a msg property) to solve the global serializer not being invoked when base is null?

The only change regarding the metadata (the tests prove this) is that this.lastObj gets the property msg. The global serializer is applied to this object (before it became the lastObject ) This way it is ensured that the same logic is applied to the msg as to all the other properties to be logged.

But you are right, the connection to base=null is not visible. I debugged now again and found the culprit: https://github.com/pinojs/pino/blob/master/pino.js#L88
The serializer is called in tools.js in the method asChindings https://github.com/pinojs/pino/blob/master/lib/tools.js#L148

I think there is no connection between the base = null and the missing call to the serializer, the reproduction through console.log has led to false conclusion. Fact is, the prop msg was logged differently from the rest and this is the bug.

[yellowbrickc]

Hi @davidmarkclements @mcollina
what should we do with this PR, merge or throw away?
Maybe this bug is not really a bug because nobody seems missing a fix for it...

[mcollina]

LGTM, this is ok for me. I've just been extremely busy :/

@davidmarkclements @jsumners can you please check?

no activity, should be ok now.

[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.