Review requested:

• @nodejs/performance

thats my first performance results:

I think need improvement for child logger area, but ı'm so happy because other results it looks nice.
cc @mcollina @Qard @trentm @jsumners

node:logger vs pino

➜ node git:(mert/create-logger-api/node-core) ✗ ./node benchmark/logger/vs-pino.js n=100000
logger/vs-pino.js scenario="simple" logger="node-logger" n=100000: 5,240,540.823813018
logger/vs-pino.js scenario="child" logger="node-logger" n=100000: 2,635,847.7027229806
logger/vs-pino.js scenario="disabled" logger="node-logger" n=100000: 159,436,487.67795104
logger/vs-pino.js scenario="fields" logger="node-logger" n=100000: 3,619,336.304205216
logger/vs-pino.js scenario="simple" logger="pino" n=100000: 3,398,489.9761368227
logger/vs-pino.js scenario="child" logger="pino" n=100000: 4,489,799.803418606
logger/vs-pino.js scenario="disabled" logger="pino" n=100000: 119,772,384.56038144
logger/vs-pino.js scenario="fields" logger="pino" n=100000: 1,257,930.8609750536

I now learn this feat in Pino

pinojs/pino#2281

I will try add in node:logger

This will require support for serializers

This will require support for serializers

I wonder, should we name them like Pino does (built-in serializers), or go with something like standardSerializers ?

This will require support for serializers

I wonder, should we name them like Pino does (built-in serializers), or go with something like standardSerializers ?

Follow pino and we’ll change it

This will require support for serializers

I wonder, should we name them like Pino does (built-in serializers), or go with something like standardSerializers ?

Follow pino and we’ll change it

I tryed serializer implement for logger, and some bench result repaired

and fields and simple are experiencing a decline; I will try to resolve these

fields: 3.62M → 2.16M (-40%)
simple: 5.24M → 4.87M (-7%)

previously, the results for the child logger were quite slow at around 70%, but the new results have dropped to 18% and have actually improved significantly.

I continue to try new methods.

➜  node git:(mert/create-logger-api/node-core) ✗ ./node benchmark/logger/vs-pino.js n=100000
logger/vs-pino.js scenario="simple" logger="node-logger" n=100000: 4,868,164.032787085
logger/vs-pino.js scenario="child" logger="node-logger" n=100000: 3,894,327.425314102
logger/vs-pino.js scenario="disabled" logger="node-logger" n=100000: 160,503,080.85663706
logger/vs-pino.js scenario="fields" logger="node-logger" n=100000: 2,157,462.3927336666
logger/vs-pino.js scenario="simple" logger="pino" n=100000: 3,424,706.4418693925
logger/vs-pino.js scenario="child" logger="pino" n=100000: 4,753,595.477010947
logger/vs-pino.js scenario="disabled" logger="pino" n=100000: 122,100,122.10012211
logger/vs-pino.js scenario="fields" logger="pino" n=100000: 1,411,215.99189962
➜  node git:(mert/create-logger-api/node-core) ✗ 

This will require support for serializers

I wonder, should we name them like Pino does (built-in serializers), or go with something like standardSerializers ?

Follow pino and we’ll change it

I tryed serializer implement for logger, and some bench result repaired

and fields and simple are experiencing a decline; I will try to resolve these

fields: 3.62M → 2.16M (-40%) simple: 5.24M → 4.87M (-7%)

previously, the results for the child logger were quite slow at around 70%, but the new results have dropped to 18% and have actually improved significantly.

I continue to try new methods.

➜  node git:(mert/create-logger-api/node-core) ✗ ./node benchmark/logger/vs-pino.js n=100000
logger/vs-pino.js scenario="simple" logger="node-logger" n=100000: 4,868,164.032787085
logger/vs-pino.js scenario="child" logger="node-logger" n=100000: 3,894,327.425314102
logger/vs-pino.js scenario="disabled" logger="node-logger" n=100000: 160,503,080.85663706
logger/vs-pino.js scenario="fields" logger="node-logger" n=100000: 2,157,462.3927336666
logger/vs-pino.js scenario="simple" logger="pino" n=100000: 3,424,706.4418693925
logger/vs-pino.js scenario="child" logger="pino" n=100000: 4,753,595.477010947
logger/vs-pino.js scenario="disabled" logger="pino" n=100000: 122,100,122.10012211
logger/vs-pino.js scenario="fields" logger="pino" n=100000: 1,411,215.99189962
➜  node git:(mert/create-logger-api/node-core) ✗ 

I inspected pino and I learn some patterns, than I applied this commit and new results! 1ededc7

I used this patterns

removed the cost of serializing bindings in each log for the child logger,
skipped unnecessary serializer checks,
used direct string concatenation instead of object merging,
sped up type checking

simple: 6.06M vs 3.48M ops/s (+74% faster)
child: 5.76M vs 4.41M ops/s (+31% faster)
disabled: 174M vs 146M ops/s (+19% faster)
fields: 2.13M vs 1.36M ops/s (+56% faster)

➜  node git:(mert/create-logger-api/node-core) ✗ ./node benchmark/logger/vs-pino.js n=100000
logger/vs-pino.js scenario="simple" logger="node-logger" n=100000: 6,062,182.962986493
logger/vs-pino.js scenario="child" logger="node-logger" n=100000: 5,758,903.394222795
logger/vs-pino.js scenario="disabled" logger="node-logger" n=100000: 174,026,539.04720467
logger/vs-pino.js scenario="fields" logger="node-logger" n=100000: 2,126,059.37552321
logger/vs-pino.js scenario="simple" logger="pino" n=100000: 3,477,918.037575009
logger/vs-pino.js scenario="child" logger="pino" n=100000: 4,407,389.658686015
logger/vs-pino.js scenario="disabled" logger="pino" n=100000: 145,551,509.22359914
logger/vs-pino.js scenario="fields" logger="pino" n=100000: 1,363,125.197883181
➜  node git:(mert/create-logger-api/node-core) ✗ 

hello @mcollina do you any comments or suggestions for this end commits, I'm curious 🙏 .

Codecov Report

❌ Patch coverage is 95.67233% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.75%. Comparing base (76215dc) to head (7e478d9).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main   #60468      +/-   ##
==========================================
+ Coverage   89.73%   89.75%   +0.02%     
==========================================
  Files         676      678       +2     
  Lines      206069   206715     +646     
  Branches    39517    39605      +88     
==========================================
+ Hits       184909   185531     +622     
- Misses      13310    13330      +20     
- Partials     7850     7854       +4     

@RafaelGSS please take a look. 7 days passed since your review has been requested.

@RafaelGSS please take a look. 7 days passed since your review has been requested.

It's a 2.5k lines PR, it's hard to concilate time to review it properly.

It's missing tests for a significant part of the exposed API. Apart from my comments, why are we copying pino internals instead of vendoring pino (which is more robust and well tested) into core?

I'd love to hear @mcollina and @jsumners thoughts on this PR too.

The design is inspired by pino but intentionally minimal the diagnostics_channel consumer model is different from pino's streaming architecture. It's not a copy, it's a reimplementation of the same ideas adapted to Node.js internals

Core code must use primordials and internal APIs pino is written for userland. Using it directly in core would require either patching it or losing the prototype pollution protection that __proto__: null and primordials provide

I'll also add the missing test coverage for the uncovered,

Happy to hear @mcollina and @jsumners thoughts on the vendoring question,

Btw I edited your suggestion @RafaelGSS , thanks for reviews

Core code must use primordials and internal APIs pino is written for userland

Both statements are incorrect. Core code doesn't must use primordials, and node:logger is written for userland.

Core code must use primordials and internal APIs pino is written for userland

Both statements are incorrect. Core code doesn't must use primordials, and node:logger is written for userland.

You're right, primordials aren't strictly required, I should have said they're a common convention in core modules rather than a hard requirement.

The main reason for reimplementing rather than vendoring is the architectural difference: node:logger uses diagnostics_channel as its dispatch mechanism, which is fundamentally different from pino's streaming pipeline. Vendoring pino would mean either rewriting its internals to use diagnostics_channel or maintaining a diverging fork.

The current implementation also outperforms pino in benchmarks:

This is largely possible because of the diagnostics_channel architecture and tighter integration with Node.js internals like Utf8Stream.

On vedoring pino in core: IMHO it’s too opinionated to be in core. It works for me, but it uses some historical/non-standard conventions that makes it hard for some users to use it.

An option we could follow is to follow the same path of undici and create something new with the goal of bringing it in the fold.

It's missing tests for a significant part of the exposed API. Apart from my comments, why are we copying pino internals instead of vendoring pino (which is more robust and well tested) into core?

I'd love to hear @mcollina and @jsumners thoughts on this PR too.

As Matteo said, Pino has baggage that we can't realistically remove. I have provided a few tips along the way of this PR, but I really don't have the energy to devote to an actual review of it. So I can't speak to the implementation as it is today. The last time I looked at it, it was going in the right direction and I assume that has continued. If we are at the "go or no-go" point, I can attempt to use it and provide feedback on if it works as I would expect or not.

@mertcanaltin Can you update the description of this pull-request? Please provide a full description of all the changes you're bringing, the performance characteristics of this new module etc.

@mertcanaltin Can you update the description of this pull-request? Please provide a full description of all the changes you're bringing, the performance characteristics of this new module etc.

I Updated PR description

If we are at the "go or no-go" point, I can attempt to use it and provide feedback on if it works as I would expect or not.

I don't think there's any need to rush into this must land immediately. We can provide people the time to review it properly if anyone wants to do so. Personally, I think it's in good shape at this point, but it's also a somewhat big and critical system so it makes sense that we might want to wait a bit for deeper review to ensure we're fully confident in the architecture.

If we are at the "go or no-go" point, I can attempt to use it and provide feedback on if it works as I would expect or not.

I don't think there's any need to rush into this must land immediately. We can provide people the time to review it properly if anyone wants to do so. Personally, I think it's in good shape at this point, but it's also a somewhat big and critical system so it makes sense that we might want to wait a bit for deeper review to ensure we're fully confident in the architecture.

I don't understand this reply. I am not trying to rush anything. I am stating that the best I can do for a review is to try out the "done" product and provide feedback at that point. I have no idea if that point has been reached.