Refactor functions to use context object #452

jsumners · 2023-08-30T16:11:12Z

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 770f56a stops working when refactoring it to use the context object:

pino-pretty/index.js

Lines 117 to 122 in fa386a9

    
           if (minimumLevel) { 
        
             const condition = useOnlyCustomProps ? opts.customLevels : customLevelNames[minimumLevel] !== undefined 
        
             const minimum = (condition ? customLevelNames[minimumLevel] : LEVEL_NAMES[minimumLevel]) || Number(minimumLevel) 
        
             const level = log[levelKey === undefined ? LEVEL_KEY : levelKey] 
        
             if (level < minimum) return 
        
           }

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

pino-pretty/test/basic.test.js

Lines 642 to 664 in fa386a9

    
           t.test('filter some lines based on minimumLevel', (t) => { 
        
             t.plan(3) 
        
             const pretty = prettyFactory({ minimumLevel: 'info' }) 
        
             const expected = [ 
        
               undefined, 
        
               undefined, 
        
               `[${formattedEpoch}] INFO (${pid}): baz\n` 
        
             ] 
        
             const log = pino({}, new Writable({ 
        
               write (chunk, enc, cb) { 
        
                 const formatted = pretty(chunk.toString()) 
        
                 t.equal( 
        
                   formatted, 
        
                   expected.shift() 
        
                 ) 
        
                 cb() 
        
               } 
        
             })) 
        
             log.info({ msg: 'foo', level: 10 }) 
        
             log.info({ msg: 'bar', level: 20 }) 
        
             // only this line will be formatted 
        
             log.info({ msg: 'baz', level: 30 }) 
        
           })

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:

pino-pretty/lib/pretty.js

Lines 88 to 91 in a74b7b1

    
           // This is odd. The colorizer ends up relying on the value of 
        
           // `customProperties` instead of the original `customLevels` and 
        
           // `customLevelNames`. 
        
           ...this.context.customProperties

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 · 2023-08-31T12:33:12Z

@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:

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 · 2023-08-31T12:36:07Z

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 #451. I think the maintenance benefits will far outweigh any decrease in speed.

jsumners · 2023-08-31T12:37:34Z

index.js

@@ -37,42 +26,50 @@ const jsonParser = input => {
 * @property {boolean} [colorizeObjects=true] Apply coloring to rendered objects
 * when coloring is enabled.
 * @property {boolean} [crlf=false] End lines with `\r\n` instead of `\n`.
+ * @property {string|null} [customColors=null] A comma separated list of colors


This doc block got reorganized to order the properties in alphabetical order. No real change here.

jsumners · 2023-08-31T12:38:04Z

index.js

@@ -84,22 +81,25 @@ const defaultOptions = {
  colorize: isColorSupported,
  colorizeObjects: true,
  crlf: false,
+  customColors: null,


Same as with the doc block. Properties were reorganized to be in alphabetical order.

jsumners · 2023-08-31T12:38:39Z

index.js

+  const context = parseFactoryOptions(Object.assign({}, defaultOptions, options))
+  return pretty.bind({ ...context, context })


All of the implementation code was moved to lib/pretty.js.

jsumners · 2023-08-31T12:39:57Z

lib/pretty.js

New file that contains the original prettification code from the root index.js.

jsumners · 2023-08-31T12:40:45Z

lib/pretty.js

+  if (this.minimumLevel) {
+    // We need to figure out if the custom levels has the desired minimum
+    // level & use that one if found. If not, determine if the level exists
+    // in the standard levels. In both cases, make sure we have the level
+    // number instead of the level name.
+    let condition
+    if (this.useOnlyCustomProps) {
+      condition = this.customLevels
+    } else {
+      condition = this.customLevelNames[this.minimumLevel] !== undefined
+    }
+    let minimum
+    if (condition) {
+      minimum = this.customLevelNames[this.minimumLevel]
+    } else {
+      minimum = LEVEL_NAMES[this.minimumLevel]
+    }
+    if (!minimum) {
+      minimum = typeof this.minimumLevel === 'string'
+        ? LEVEL_NAMES[this.minimumLevel]
+        : LEVEL_NAMES[LEVELS[this.minimumLevel].toLowerCase()]
+    }
+
+    const level = log[this.levelKey === undefined ? LEVEL_KEY : this.levelKey]
+    if (level < minimum) return
+  }


This is a change from the original code. See the PR description for details on this block.

jsumners · 2023-08-31T12:42:15Z

lib/utils/parse-factory-options.js

This is a new file that contains the original code from prettyFactory that parsed the passed in options. This one parses the options and returns a standardized context object that we can pass around to the various prettifier functions.

jsumners · 2023-09-02T23:30:43Z

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

mcollina

great work much needed!

jsumners added 10 commits August 30, 2023 12:04

Refactor pretty to use context object

770f56a

Flesh out doc for context object

a4755d0

Move pretty function to own file

dcccb04

Refactor prettify-message to use context

b200bea

Refactor filter-log to use context

a4ca587

Refactor prettify-level to use context

860fbdf

Refactor prettify-metadata to use context

17c3bd3

Refactor prettify-time to use context

448253a

Refactor prettify-error-log to use context

b4190c6

Refactor prettify-object to use context

a74b7b1

jsumners marked this pull request as ready for review August 31, 2023 12:29

jsumners requested a review from mcollina August 31, 2023 12:29

jsumners commented Aug 31, 2023

View reviewed changes

mcollina approved these changes Sep 3, 2023

View reviewed changes

jsumners merged commit b490eb9 into improve-docs Sep 3, 2023
10 checks passed

jsumners deleted the refactor branch September 3, 2023 11:22

jsumners added a commit that referenced this pull request Sep 3, 2023

Refactor functions to use context object (#452)

d70582d

jsumners added a commit that referenced this pull request Sep 3, 2023

Refactor functions to use context object (#452)

ebe23b0

jsumners added a commit that referenced this pull request Sep 3, 2023

Refactor functions to use context object (#452)

c27157f

jsumners added a commit that referenced this pull request Sep 3, 2023

Refactor functions to use context object (#452)

b34e7c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor functions to use context object #452

Refactor functions to use context object #452

jsumners commented Aug 30, 2023 •

edited

Loading

jsumners commented Aug 31, 2023 •

edited

Loading

jsumners commented Aug 31, 2023

jsumners Aug 31, 2023

jsumners Aug 31, 2023

jsumners Aug 31, 2023

jsumners Aug 31, 2023

jsumners Aug 31, 2023

jsumners Aug 31, 2023

jsumners commented Sep 2, 2023

mcollina left a comment

	if (minimumLevel) {
	const condition = useOnlyCustomProps ? opts.customLevels : customLevelNames[minimumLevel] !== undefined
	const minimum = (condition ? customLevelNames[minimumLevel] : LEVEL_NAMES[minimumLevel]) \|\| Number(minimumLevel)
	const level = log[levelKey === undefined ? LEVEL_KEY : levelKey]
	if (level < minimum) return
	}

	t.test('filter some lines based on minimumLevel', (t) => {
	t.plan(3)
	const pretty = prettyFactory({ minimumLevel: 'info' })
	const expected = [
	undefined,
	undefined,
	`[${formattedEpoch}] INFO (${pid}): baz\n`
	]
	const log = pino({}, new Writable({
	write (chunk, enc, cb) {
	const formatted = pretty(chunk.toString())
	t.equal(
	formatted,
	expected.shift()
	)
	cb()
	}
	}))
	log.info({ msg: 'foo', level: 10 })
	log.info({ msg: 'bar', level: 20 })
	// only this line will be formatted
	log.info({ msg: 'baz', level: 30 })
	})

	// This is odd. The colorizer ends up relying on the value of
	// `customProperties` instead of the original `customLevels` and
	// `customLevelNames`.
	...this.context.customProperties

		const context = parseFactoryOptions(Object.assign({}, defaultOptions, options))
		return pretty.bind({ ...context, context })

Refactor functions to use context object #452

Refactor functions to use context object #452

Conversation

jsumners commented Aug 30, 2023 • edited Loading

jsumners commented Aug 31, 2023 • edited Loading

jsumners commented Aug 31, 2023

jsumners Aug 31, 2023

Choose a reason for hiding this comment

jsumners Aug 31, 2023

Choose a reason for hiding this comment

jsumners Aug 31, 2023

Choose a reason for hiding this comment

jsumners Aug 31, 2023

Choose a reason for hiding this comment

jsumners Aug 31, 2023

Choose a reason for hiding this comment

jsumners Aug 31, 2023

Choose a reason for hiding this comment

jsumners commented Sep 2, 2023

mcollina left a comment

Choose a reason for hiding this comment

jsumners commented Aug 30, 2023 •

edited

Loading

jsumners commented Aug 31, 2023 •

edited

Loading