Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pino.final does not work with pino-pretty #37

Closed
eadaniel opened this issue Oct 4, 2018 · 4 comments · Fixed by pinojs/pino#528
Closed

pino.final does not work with pino-pretty #37

eadaniel opened this issue Oct 4, 2018 · 4 comments · Fixed by pinojs/pino#528

Comments

@eadaniel
Copy link

eadaniel commented Oct 4, 2018

When using pino.final with pino-pretty following exception is thrown:

only compatible with loggers with pino.destination and pino.extreme targets

We are using pretty-printing on the dev boxes and it took me quite a while that pino.final works fine without pino-pretty. Below you find code to reproduce the issue.

Best,
Alexander

const pino = require('pino')

const logger = pino({
  prettyPrint: true
})

const run = async () => {
  process.on('SIGTERM', pino.final(logger, (err, finalLogger) => {
    finalLogger.info('Graceful shutdown 1')
    finalLogger.info('Graceful shutdown 2')
    process.exit(0)
  }))

  await delay(30)
}

const delay = (secs) => {
  return new Promise((resolve, reject) => {
    setTimeout(() => resolve(), secs * 1000)
  })
}

run().catch(err => {
  logger.error({ err })
})
@mcollina
Copy link
Member

mcollina commented Oct 4, 2018

This is indeed true. It would be hard to support such a feature.
prettyPrint: true  is essentially a development utility, not a production one.
pino.final is essentially a production utility, not a development one.
It should be fairly ok to disable one while using the other.

@jsumners
Copy link
Member

jsumners commented Oct 4, 2018

Maybe make pino.final a noop when pretty printing is enabled?

@mcollina
Copy link
Member

mcollina commented Oct 4, 2018

Making it a noop won't help - the function will not be fired. We can have pino.final be called with the passed-in logger in that case, and we should log another message as warn level: "The destination is not a pino.destination() or pino.extreme() and it does not support synchronous flushing". Would that be better?

@eadaniel
Copy link
Author

eadaniel commented Oct 4, 2018

@mcollina That's definitely better.

It would be nice to detect that pino-pretty is used and log a message explaining that pino.final can't be used with pino-pretty. At least that would have saved me time finding the root cause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants