Fix Enabled detection #22
Conversation
| @@ -27,6 +27,10 @@ import ( | |||
| "github.com/rs/zerolog" | |||
| ) | |||
|
|
|||
| const ( | |||
| defaultLevelDelta zerolog.Level = 1 | |||
There was a problem hiding this comment.
Can you use this magic variable to explain how zerolog handles log levels and how that maps to logr levels?
There was a problem hiding this comment.
Borrowed from here: https://github.com/bombsimon/logrusr/blob/d198d3e84a78c6115fb1e18c1ccb66193530e9c7/logrusr.go#L18. At the moment it's just an explicitly-named control knob instead of a magic number that has to appear multiple places. It could be offered as a parameter to the user if they want to shift the default level assignments (I could actually see myself doing this given the limited number of logging levels in zerolog). But I didn't offer a way to do that, so no magic yet.
| // Optimization: Info() will check level internally. | ||
| const traceLevel = 1 - int(zerolog.TraceLevel) | ||
| return level <= traceLevel | ||
| return ls.l.GetLevel() <= zerologLevel(level) |
There was a problem hiding this comment.
A unit test for this behavior would be good.
There was a problem hiding this comment.
I borrowed the tests from logrusr and adapted them for zerologr, then added tests for Enabled() and zerologLevel(). zerologr actually passed all of the logrusr tests (except the ones I commented) without much adaptation to the tests. I'll let you decide if you like to keep them. Otherwise, only the Enabled() test is what you asked for, so the file could be stripped down to just that one. The logrusr tests are a decent set, though.
binaryphile
force-pushed
the
master
branch
2 times, most recently
from
00e2a2f to
9476ad2
Compare
|
@binaryphile zerolog.Logger.GetLevel() itself is not enough since zerolog internally checks both local level and global level. https://github.com/rs/zerolog/blob/master/log.go#L469-L470 Alternatively, you can create new LogSink and embed zerologr.LogSink and overwrite Enabled() method to use the condition check in the link above. The downside is that your codepath will do the same logic twice since zerolog will check it again internally. Do you still need this PR open? |
|
Thanks but after this interaction, I will no longer try to help out with anything. I will not be suggesting this project to anyone else. You can keep your passive aggressive PR. |
|
I haven't followed the technical discussion closely enough, but it seems clear that zerologr has problems supporting verbosity the way that users are expecting. @hn8: if you don't want to change the implementation, can you at least extend the README so that it clearly states how go-logr verbosity levels are handled? I see "zerologLevel = 1 - logrLevel" there, but nothing about |
|
@pohly #25 fixed the document and added the option for logr.Enabled() lazy evaluaton and high verbosity support. Higher verbosity is possible but undocumented since it does not support some of zerolog's features like Hooks and Sampling. logr.Enabled() is not enforced by default but now documented because it is in the logging hotpath. Using logr.Enabled() as the condition for lazy evaluaton is possible but not popular in use, so it is better to keep happy path fast for majority of library logging users IMHO. Thanks. |
Unfortunately, the optimization in the LogSink#Enabled method breaks the intended use of that function, as it is exposed to the user by logr.Logger#Enabled. Currently, it's hardcoded to return true for any value between 0 and 2. However, if I'm bothering to call log.Enabled explicitly, it's for one reason: I want to check if logging is enabled prior to doing an expensive operation solely for the sake of logging, such as doing an http dump of a request, or a stack trace.
As the comment in the code notes, Info() will do that for me already. So I never need to call Enabled but for one reason: I need to know for sure that the message will be output prior to doing that expensive operation.
This implementation breaks that, since Enabled doesn't check the actual verbosity level of the logger implementation. If it is to be of any use whatsoever, the result of Enabled must match that of the underlying logsink implementation. The only way to check that is to...well, check it.
Since there's more than one place that the zerolog.Level is compared to the int level, I added a helper function to convert consistently. I also parameterized it slightly with a constant. Although it doesn't do anything now, that levelDelta value could be configurable in the future.