moscajs / aedes-logging

Logging module for Aedes, based on Pino
MIT License
8 stars 4 forks source link

Pino initialisation might be wrong #4

Closed jyotman closed 7 years ago

jyotman commented 7 years ago

The Pino constructor has the following API -

.pino([options], [stream])

But in index.js, Pino is initialised in the following way -

  opts.stream = opts.stream || process.stdout
  var logger = pino(opts.stream, {
    level: opts.level,
    extreme: opts.extreme,
    safe: opts.safe
  })

This way Pino assumes that the options argument was skipped and only a stream argument was provided. This would lead to options never taking any effect.

Now to solve this if I simply interchange the 2 arguments then, Pino complains because we're not taking any Pino specific options from the user regarding those so ultimately we're passing the level, extreme and safe option as undefined to Pino. And Pino by does not set the default values for undefined fields.

So I guess a way to solve this is to provide an attribute called pinoOptions in the aedes-logging API like this -

logging({
  instance: instance,
  servers: servers,
  pinoOptions: {level: info // plus other properties}
})

And then simply pass that object to Pino in index.js -

  opts.stream = opts.stream || process.stdout
  var logger = pino(opts.pinoOptions || {}, opts.stream)
mcollina commented 7 years ago

It seems you have investigated the issues quite a bit, can you send a PR?

jyotman commented 7 years ago

Sure! Will submit a PR soon...