search

pinojs / pino-pretty

🌲Basic prettifier for Pino log lines
MIT License
1.27k stars 150 forks source link

Refactor functions to use context object #452

Closed jsumners closed 1 year ago

jsumners commented 1 year ago

This PR refactors all of the prettify functions to use a context object. This removes the need to keep track of all of the parsed options and other items such functions need in order to perform their work. I believe this will make the code base easier to maintain going forward, and will make PRs like #445 easier to implement.

It's not clear to me why the following block of code in https://github.com/pinojs/pino-pretty/commit/770f56a44e0d4a30e2809c379ded5647aeba43df stops working when refactoring it to use the context object:

https://github.com/pinojs/pino-pretty/blob/fa386a964cce7d7c5cbeb4ce5bb8d28785001acd/index.js#L117-L122

The short of it is that the following test starts failing:

https://github.com/pinojs/pino-pretty/blob/fa386a964cce7d7c5cbeb4ce5bb8d28785001acd/test/basic.test.js#L642-L664

It starts failing because it ultimately invokes Number("info"), resulting in NaN, on line 119 of the current master code because this.customLevels defaults to an empty object. This is mysterious to me because the same default is present on the master branch.

Regardless, unrolling the conditionals is far easier to read and reason through. But I'm still likely to revisit this block before considering this PR ready.

Update: I thought it might be the same thing as this: https://github.com/pinojs/pino-pretty/blob/a74b7b1d37ff227f04e5aea8d11a1f33bf2d106b/lib/pretty.js#L88-L91

But it does not seem to be. I'm going to leave the unrolled block in place. I really don't like the conditionals nested in ternaries to begin with.

jsumners commented 1 year ago

@mcollina in order to avoid the situation in #449 that led to having to create #453, once this and #453 are approved I will be merging in the following order:

  1. 452

  2. 450

  3. 453

It will negate the approvals on those issues as I move through them, but nothing will be changing other than everything coming together. I broke this up into multiple PRs in an effort to make it a least somewhat easier to review.

jsumners commented 1 year ago

For what it's worth, I'm getting benchmark numbers like this with the PR:

basicLog*10000: 796.194ms
objectLog*10000: 648.642ms
coloredLog*10000: 764.881ms
customPrettifiers*10000: 767.826ms
logWithErrorObject*10000: 577.4ms
logRemappedMsgErrKeys*10000: 587.286ms
messageFormatString*10000: 967.363ms
basicLog*10000: 753.156ms
objectLog*10000: 643.996ms
coloredLog*10000: 764.156ms
customPrettifiers*10000: 798.113ms
logWithErrorObject*10000: 584.494ms
logRemappedMsgErrKeys*10000: 595.066ms
messageFormatString*10000: 982.404ms

Slightly different than the results discovered with https://github.com/pinojs/pino-pretty/pull/451. I think the maintenance benefits will far outweigh any decrease in speed.

jsumners commented 1 year ago

@mcollina when you get a chance, please review this one.