pinojs / pino

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

formatters level set label cant transport #1353

Closed zhongshanxian closed 2 years ago

zhongshanxian commented 2 years ago

why set level => label, cant record the log, test.log is empty?

"pino": "^7.8.0"

const logger = pino({
    formatters: {
        level(label) {
            // if set level => label, cant record the log, test.log is empty
            // if set level => number, Everything is all right
            return { level: label };
        }
    },
    transport: {
        targets: [
            {
                target: 'pino/file',
                options: { destination: './test.log' }
            }
        ]
    }
});

export default logger
jsumners commented 2 years ago

It is not clear what you are asking. Please review https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#quoting-code and provide a minimal reproduction of your problem.

zhongshanxian commented 2 years ago

https://codesandbox.io/s/cool-sun-t395f0?file=/README.md

mcollina commented 2 years ago

pino.multistream() which underpins our transport system is incompatible with customizing the level. It would be better for pino to throw in that case.

guilhermelimak commented 2 years ago

Hey @mcollina, I've opened a pull request to solve this issue and added some info about it to the docs, could you review it and let me know if there's something wrong?

techmunk commented 2 years ago

The issue is actually just that level is not the numeric label. I was actually running with multiple transports, with a level formatter of

formatters: {
   level: (label, number) => ({ level: number, 'level-label': label })
},

And it was working fine. All transports got logged as expected.

Is there some other way to achieve an outcome such as this on the latest release of pino?

mcollina commented 2 years ago

I don't think so, the system is not designed in that way. A PR that improves on it would be awesome.

techmunk commented 2 years ago

@mcollina is there a reason the default formatter does not include the label, or an option to this effect? Would a change such as this be acceptable?

All I really want is the friendly level label to get to the logs so people don't need to know in logstash for example, that 30 is 'info'.

I'm happy to make a PR, but what kind of change would be acceptable here? Now that the code throws an error, I cannot do what I was doing before that worked just fine, which in my mind is a regression, even if it was not by design.

As I said, I'm happy to make a PR, but would appreciate some guidance on how such an outcome could be achieved.

mcollina commented 2 years ago

I'm happy to make a PR, but what kind of change would be acceptable here? Now that the code throws an error, I cannot do what I was doing before that worked just fine, which in my mind is a regression, even if it was not by design.

Instead of validating if the formatters.level is present when using multiple transports, you can call formatters.level('info', 30) and verify that the returned level property is 30.

techmunk commented 2 years ago

@mcollina That could work, and I'd be happy to do that. I was just going through the code, and I have two other options that would solve my issue that might be more appropriate.

  1. Add a new option such as "addLevelLabel, and check this option in thewrite()inproto.js`,
  2. I think the better and simpler approach, pass the level num into the mixin callback as an option.

Option 2 would allow me to easily call the instance.levels() method and achieve the same outcome cleanly.

Thoughts?

techmunk commented 2 years ago

@mcollina I've created a pull request for option 2. #1364

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.