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

Allow tags to point to a custom level defined in pino's customLevels #125

Merged
merged 2 commits into from
Oct 8, 2021

Conversation

aalimovs
Copy link
Contributor

@aalimovs aalimovs commented Nov 24, 2020

You can define customLevels with pino, e.g.

customLevels: {
  bar: 123
}

Meaning when logging with logger.bar('hello world') it results in { level: 123, ... }.

hapi-pino accepts an option of tags, which is a map of hapi log tags to pino's level. However, the library has a hardcoded list of pino levels:

const levels = ['trace', 'debug', 'info', 'warn', 'error']

It means you cannot define tags which would point to a custom level defined via customLevels:

options: {
    customLevels: {
        bar: 123
    },
    tags: {
        foo: 'bar',
    }
}

Above throws an error invalid tag levels because it checks level bar against ['trace', 'debug', 'info', 'warn', 'error'].

This PR compares bar against Object.keys(logger.levels.values), which is the valid array of levels that pino accepts, including those defined via customLevels.

PR also adds a missing fatal level to levelTags object, fixing server.log('fatal', 'bye world'); resulting in level: 30/info.

…level defined in pino's customLevels instead of a hardcoded list
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@coveralls
Copy link

coveralls commented Nov 24, 2020

Coverage Status

Coverage increased (+0.01%) to 99.098% when pulling 9a5bc12 on aalimovs:master into fb70517 on pinojs:master.

@mcollina
Copy link
Collaborator

CI is failing

@aalimovs
Copy link
Contributor Author

Hm it seems completely unrelated to the changes (happens pre-PR too). Something internally in hapi's lib/request.js changed starting from v19 where doing server.inject with simulate close true like:

server.inject({
  url: '/',
  method: 'POST',
  simulate: {
    close: true
  }
})

Throws an error at @hapi/hapi/lib/request.js:497:31

@mcollina
Copy link
Collaborator

Can you send another PR that fixes those? I'm not really using this module and I'm not making a release with red CI.

@aalimovs
Copy link
Contributor Author

aalimovs commented Oct 7, 2021

@mcollina this should be fixed now pulling in the merged #137

However, I'm not sure what's the deal now with the new pipelines.

Fails my end with unrelated Coveralls Parallel errors, https://github.com/aalimovs/hapi-pino/runs/3831798686?check_suite_focus=true

And it's waiting for approval here https://github.com/pinojs/hapi-pino/actions/runs/1317758243

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina requested a review from lauraseidler October 8, 2021 07:47
@mcollina
Copy link
Collaborator

mcollina commented Oct 8, 2021

@lauraseidler can you take a look?

Copy link
Collaborator

@lauraseidler lauraseidler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - not sure what went wrong with the tests there in the beginning, but looks fine now?

@lauraseidler
Copy link
Collaborator

Ah I see - it's failing on your fork because that's a different repo and a different token.. not sure how to solve this rn, will take a look later. But as it's running fine on this repo, I guess it's okay for now.

@lauraseidler lauraseidler merged commit 2b88862 into hapijs:master Oct 8, 2021
@lauraseidler
Copy link
Collaborator

I published a new version 8.5.0 that includes these changes - thanks!

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 this pull request may close these issues.

4 participants