pinojs / pino

🌲 super fast, all natural json logger
http://getpino.io
MIT License
14.21k stars 875 forks source link

Need both multistream and transport to accomplish a task #1189

Closed jsumners closed 2 years ago

jsumners commented 2 years ago

But they are mutually exclusive. Situation: I'm working on adding pino to a CLI tool where error logs will be written to stderr as straight messages (not ndjson), and program output will be written to stdout through a custom level (value: 100), e.g. log.output(csv).

First attempt is logging configuration like:

logger.js:

'use strict';

const path = require('path');
const pino = require('pino');

module.exports = pino({
  level: 'output',
  customLevels: {
    output: 100
  },
  tansport: {
    targets: [
      // { target: 'pino/file', level: 'debug', options: { destination: 2 } },
      { target: path.resolve(path.join(__dirname, 'stderr.transport.mjs')), level: 'debug' },
      { target: 'pino/file', level: 'output', options: { destination: 1 } }
    ]
  }
});

stderr.transport.mjs:

import { Writable } from 'stream';

export default (options) => {
  const myTransportStream = new Writable({
    autoDestroy: true,
    write(chunk, enc, cb) {
      const str = chunk.toString();
      let json = str;
      try {
        json = JSON.parse(str);
      } catch {}
      process.stderr.write(json.msg || json);
      cb();
    }
  });
  return myTransportStream;
};

But we know that won't work because setting the base logger to level: 100 will prevent anything from ever being sent to the stderr transport. So we would consider using the multistream feature instead. Except, that won't work because we need our stream to be a transport.

What do we think should be done to support such use cases?

mcollina commented 2 years ago

As you can see in https://github.com/pinojs/pino/blob/f16c98d7d758fa96155996bbc7604e4eee600d79/lib/worker.js#L51, our implementation for transport uses pino.multistream(), so anything that should be possible through it should be. It's definitely possible that we should be passing through more options and make things more configurable.

One more note, don't use process.stderr or process.stdout in workers, they do not work as you would expect, i.e. they send data back to the main thread for being printed out. Use pino.destination.

jsumners commented 2 years ago

One more note, don't use process.stderr or process.stdout in workers, they do not work as you would expect, i.e. they send data back to the main thread for being printed out. Use pino.destination.

I figured that was a bug, was going to get to it later. Fixed (locally).

our implementation for transport uses pino.multistream(), so anything that should be possible through it should be.

Unfortunately, that doesn't seem to be the case. If you use the above logger sample with:

const log = require('./logger')

log.error('boom')
// Not clear if I need to add an `await once()` somewhere myself or not...
process.exit(1)

Add a breakpoint in the stderr transport (on the .write), run the test, and notice that the breakpoint does not get hit.

mcollina commented 2 years ago

As you can see in https://getpino.io/#/docs/api?id=pinomultistreamoptions-gt-stream we need to pass a customLevel options to multistream in https://github.com/pinojs/pino/blob/f16c98d7d758fa96155996bbc7604e4eee600d79/lib/worker.js#L51. We should pass it through as we it is around.

github-actions[bot] commented 2 years ago

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.