Same level multistream logging is not working with "dedupe"

[frzsombor]

If I set dedupe to true, the option works as stated in the docs, but it also breaks the possibility to have multiple streams for the same level, as only the "first" stream of the given level will be used, not all the streams matching the level. I'm not sure if this is the intended way of working or this is something you might want to change. I understand that the way it currently works exactly means "deduping" but in my (and I think in most) use cases, we would expect dedupe to "send logs only to the stream with the higher level" but to all of them.

var fs = require('fs')
var pino = require('pino')
var pretty = require('pino-pretty')
var streams = [
  { level: 'info', stream: pretty() },
  { level: 'info', stream: fs.createWriteStream(`./log-info.log`) },
  { level: 'error', stream: fs.createWriteStream(`./log-error.log`) },
]

var multistreamOpts = {
    dedupe: true,
};

var log = pino({
  level: 'info' // this MUST be set at the lowest level of the destinations
}, pino.multistream(streams, multistreamOpts))

log.info('this will be written ONLY to the console by pino-pretty and NOT in ./log-info.log')
log.fatal('this will be written ONLY to ./log-error.log')

---

I think changing the current multistream write function to the following would do the trick:

function write (data) {
  //...
  for (let i = 0; i < streams.length; i++) {
    dest = streams[i]
    if (dest.level <= level) {
      //...
      if (!opts.dedupe || dest.level === level) { //change 'if' condition
        stream.write(data)
      }
    } else {
      break
    }
  }

  //TODO: remove
  //if (opts.dedupe && stream) {
  //  stream.write(data)
  //}
}

[mcollina]

The current implementation works as expected. If you would like to add a new feature, I'll be happy to review a PR.

[frzsombor]

Thanks for the response! How would you imagine this feature to be implemented?

A: Changing the 'if' condition and removing the stream.write that only runs for one eligible stream (my workaround above)
B: Introducing a new option with a new name that we can be checked beside 'dedupe' and update the docs (and tests?)

If you like the first one, and it will pass all the tests like before, I would be glad to send a PR, but if you would like to see new options, new tests and additions to the docs, I'm afraid I have to hand this task over to someone with more experience.

[mcollina]

Well, dedupe works as expected, so it's a new option.

No worries, I'll close this then.

[frzsombor]

All right, I understand if you don't want to change that and in this case, maybe I will try to create a PR for this some day.
Just let me add that, I don't think going with "A" above would invalidate the current meaning of dedupe or would go against the current docs, as first I even thought this is a bug, because I expected that dedupe is especially meant for "do not write logs to any stream with a level different from the specified" and not meant for "write logs randomly to one of the streams matching the target level".

[mcollina]

ah, I might have missed the problem then. a PR with A) will be good.

[frzsombor]

Sounds great, then here is a PR for that:

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.