Support for pino-http multi instance

[GyanendroKh]

Adding support for the newly added pino-http multi instance middleware support in pinojs/pino-http#263

As a result of the above feature, pino-http@^8.3.0 no longer works with nestjs-pino when using the FastifyAdapter as Fastify by default sets the req.log with noop functions when disableRequestLogging is set to false

Fixes #1254

[iamolegga]

Thanks for opening this PR. Is it possible to add extra tests for a new feature and some documentation on it?

[GyanendroKh]

At the moment, I don't understand how or what tests to write for this.

This PR is more of a fix to the issue #1254.

This does not introduce new feature for this library, the changes is to support the new version of pino-http which added a new multi instance feature, breaking nestjs-pino's current usage of req.log for the logger.

As stated above, pinojs/pino-http#263 introduces the pino-http changes which as released in v8.3.0. The changes in pino-http only affects FastifyAdapter by default. pino-http now does not set the req.log property with the logger instance if req.log is already defined, but the instance is always pushed into req.allLogs[].

Why only FastifyAdapter?

Fastify by default can have request logging using pino, so the req.log is set by default and even if it is disabled the req.log property is still set with noop functions (disabled using disableRequestLogging in FastifyAdapter options), so pino-http ignores setting the req.log property.

The same happens with req.id which can be provided by pinoHttp.genReqId. But for this, Fastify's reqId generation can be disabled completely by providing genReqId: () => undefined in the FastifyAdapter options

---

Please let me know If I missed something

[iamolegga]

What I see is extra logic added:

    let log = req.log;

    if (!useExisting && (req.allLogs?.length ?? 0) > 0) {
      log = req.allLogs[req.allLogs.length - 1];
    }

so here are 2 variants what could be:

1. req.log is used
2. req.allLogs[req.allLogs.length - 1] is used

Here you can see that added line is uncovered

[GyanendroKh]

The uncovered line is 120. To be more specific it's the (req.allLogs?.length ?? 0) > 0 condition.

According to https://github.com/pinojs/pino-http/blob/master/logger.js#L158-L164 req.allLogs[] will always be present and never be empty starting from pino-http@^8.3.0 but req.allLogs will never be present in the previous versions. The length check is just an extra check for supporting previous versions.

But this has 100% coverage

if (!useExisting && req.allLogs) {
  log = req.allLogs[req.allLogs.length - 1];
}

And for using req.allLogs[req.allLogs.length - 1], if multiple pino-http is present, the newly added will always be pushed in the back so taking the last one

I think the uncovered line/condition can never be reached when using pino-http@^8.3.0. If you want 100% coverage, I can use the above code which works the same.

---

If you aren't happy with this, Sorry, I won't be able to extend the test to cover 100%

[iamolegga]

Thanks for clarification. Let's simplify then to make it 100% covered. I'll be happy with this 😀

[GyanendroKh]

Okay. I have pushed the new change.

Thanks.

[iamolegga]

published in 3.1.2

[GyanendroKh]

Thanks a lot for actively maintaining this library