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

formatters level set label cant transport #1353

Closed
zhongshanxian opened this issue Mar 2, 2022 · 11 comments
Closed

formatters level set label cant transport #1353

zhongshanxian opened this issue Mar 2, 2022 · 11 comments

Comments

@zhongshanxian
Copy link

zhongshanxian commented Mar 2, 2022

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
Copy link
Member

jsumners commented Mar 2, 2022

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
Copy link
Author

@mcollina
Copy link
Member

mcollina commented Mar 4, 2022

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
Copy link
Contributor

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
Copy link
Contributor

techmunk commented Mar 9, 2022

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
Copy link
Member

mcollina commented Mar 9, 2022

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

@mcollina mcollina closed this as completed Mar 9, 2022
@techmunk
Copy link
Contributor

techmunk commented Mar 9, 2022

@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
Copy link
Member

mcollina commented Mar 9, 2022

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
Copy link
Contributor

techmunk commented Mar 9, 2022

@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 the write()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
Copy link
Contributor

techmunk commented Mar 9, 2022

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

@github-actions
Copy link

github-actions bot commented Apr 9, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants