fix: twice customProps

[youngkiu]

Hello!
I am the author of pino-web-hook.

I am using the NestJS framework, and I wanted to display the pinoHttp property, thinking that the main function of nestjs-pino is Middleware that connects pino-http.

    LoggerModule.forRoot({
      pinoHttp: {
        customProps: (req, res) => ({
          context: 'HTTP',
        }),
      },
    }),

However, I found the following bug, and also found out that there is an issue #216.

I tried the following to solve this problem.

1. I tried to use onchild-function, but stopped because the instance given as a parameter was a logger instance that could be known outside of OnChildCallback.

+   opts.onChild = (instance) => {
+     // Execute call back code for each newly created child.
+   }
    const logger = wrapChild(opts, theStream)

2. The second method is the current PR. Additional conditional statements can be written on one line, but for readability, I have declared local variables.

I debugged it with the following modified example.js file.

const logger = require('./')({
  customProps: (req, res) => ({
    context: 'HTTP'
  })
})

Thanks

[jmealo]

@youngkiu I thought this was by design to be fast... when the JSON is parsed, most parsers I have tested will keep the last entry logged, so there shouldn't be any dupes appearing in your log viewer? [however, the raw log output locally before it gets shipped/processed, would show the duplicates, possibly]... I don't see a way of fixing this that isn't a performance regression, and for most folks, I don't think it matters. I would have assumed a pretty printer when running locally would have fixed this? I'm not using cloud functions, but if the logging was JSON native, and you disable pretty printing, and let Google format it for you, it shouldn't have duplicates (if that's an option).

I just read all the threads on this, it looks like breaking up the customProps function may make sense. It'd be interesting to test your fix versus a WeakSet from a performance standpoint. I'm having a hard time reasoning about how your fix works, but it didn't break any tests which is promising. 🤷

[youngkiu]

@jmealo Thanks for your interest in my fix.

I understand that using pino-pretty will output customProps without duplication due to JSON parsing.
Especially in the production log, I think it's more important to save it raw than to look pretty. - We plan to write a program that can analyze the accumulated logs if necessary in the future.
I don't use a separate JSON parser, but I modified it because duplicate logs were inconvenient.

[mcollina]

Thanks for opening a PR! Can you please add a unit test?

[jmealo]

@youngkiu: If you need a hand with unit tests let me know. I think a failing test for the current implementation would be the easiest way to approach this.

[youngkiu]

@jmealo I'm still not sure how I was able to see logger[chindingsSym] in unit test. Can you help me write the test code?

[youngkiu]

@mcollina
In master branch, customProps is number 2,
From my fix, I added a test case to ensure that customProps is number 1.
Please review PR

[mcollina]

lgtm