-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add option to increase the level of a logger #775
Conversation
Codecov Report
@@ Coverage Diff @@
## master #775 +/- ##
==========================================
+ Coverage 98.19% 98.21% +0.01%
==========================================
Files 42 43 +1
Lines 2275 2293 +18
==========================================
+ Hits 2234 2252 +18
Misses 33 33
Partials 8 8
Continue to review full report at Codecov.
|
I think this is related to #581 , but when I got the author correctly he wishes an option to change the loglevel on a per message basis rather than just having an easy way to create a new child logger which prints at a different log level. I'll leave this note here and reference here, so that issue as it might help some of the users stumbling across #581 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any way to guard against attempts to lower the level?
I'm concerned that despite naming, this will be very easy to misuse.
zapcore/level_filter.go
Outdated
// NewLevelCore creates a core that can be used to increase the level of an existing | ||
// Core. It cannot be used to decrease the logging level, as it acts as a filter | ||
// before calling the underlying core. | ||
func NewLevelCore(core Core, level LevelEnabler) Core { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this needs a similar name that implies increasing levels only.
We can't protect against it, but we can access the |
Logging to errorOutput seems reasonable. I agree that having this error out |
We've had a few requests on how to change the level of a logger. With the current APIs, it's not possible to reduce the log level (e.g., go from Info to Debug), but it is possible to increase the level using `WrapCore` with a custom filtering core. Since it's a common request, I think we should add an option to make this easy to use. I want to make sure "increase" is part of the API to avoid confusion on whether it can reduce the log level. Fixes #774.
b7c0c88
to
1653481
Compare
Updated with more explicit name + error if the level is decreased. Could definitely improve the message so looking for suggestions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New APIs look fine. Minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides prior comments
Co-Authored-By: Abhinav Gupta <[email protected]>
Co-Authored-By: Abhinav Gupta <[email protected]>
We've had a few requests on how to change the level of a logger. With the current APIs, it's not possible to reduce the log level (e.g., go from Info to Debug), but it is possible to increase the level using `WrapCore` with a custom filtering core. Since it's a common request, I think we should add an option to make this easy to use. I want to make sure "increase" is part of the API to avoid confusion on whether it can reduce the log level. Fixes uber-go#774.
We've had a few requests on how to change the level of a logger. With
the current APIs, it's not possible to reduce the log level (e.g., go
from Info to Debug), but it is possible to increase the level using
WrapCore
with a custom filtering core.Since it's a common request, I think we should add an option to make
this easy to use.
I want to make sure "increase" is part of the API to avoid confusion on
whether it can reduce the log level.
Fixes #774.