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

fix for winston.config.syslog.levels which are not in the right priority order for winston #52

Closed
wants to merge 1 commit into from

Conversation

yonjah
Copy link
Contributor

@yonjah yonjah commented Oct 15, 2015

winston default logging priority order is ascending goes from lowest 0 (silly) to highest 5 (error)
but syslog priority order is descending from lowest 7 (debug) to highest 0 (emerg)

this means that when setting a log level for winston when using syslog
you will actually log below that level instead of above it
so setting logger.level='info' (which is the default)
will actually log debug and info but will not log notice warning and so on.

I'm not sure if this is the right place for this fix
but I opend an issue for winston a few days ago and didn't get any attention
since the bad config mostly affects winston-syslog users I thought it can be fixed within
winston-syslog a better fix might be to have the levels conf under Syslog namespace and not requiring winston config at all but that will require changing the docs

@indexzero
Copy link
Member

Fixed in [email protected] (see: winstonjs/winston#732). Could you let me know if you plan on upgrading? That will help us inform if this backwards compatibility layer would be necessary.

@yonjah
Copy link
Contributor Author

yonjah commented Nov 2, 2015

@indexzero thanks.
I'll probably try to upgrade in a week or two

@santigimeno
Copy link
Member

I've tested winston-syslog with latest [email protected] and the levels seem fine now

@indexzero
Copy link
Member

Thanks for the confirmation @santigimeno. Closing this.

@indexzero indexzero closed this Nov 4, 2015
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.

3 participants