-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Clearer API for custom log levels #33960
base: master
Are you sure you want to change the base?
Conversation
Add AbstractLogLevel as the supertype of all log level types. Introduce "log level severity" including a `severity` function which maps any user-defined log level to an integer log level severity. This more cleanly separates out the ordinal notion of log severity from the categorical aspects of the log level. Log levels can be ordered by comparing their severities. This gives a certian type of pre-order which is reflexive, transitive and total, but not antisymmetric. This is the same as the ordering of complex numbers by their magnitude. Change to using `print` rather than `show` for formatting log levels for display. This seems to resolve the old conundrum of "do we print Warn as it appears in the program as "Warn" or in human-friendly form as "Warning""? The early (global) decision about log level filtering within the macro is now lowered to `shouldlog(level)` which is more natural and more customizable.
@@ -97,16 +129,13 @@ The log level provides a key against which potential log records may be | |||
filtered, before any other work is done to construct the log record data | |||
structure itself. | |||
""" | |||
struct LogLevel | |||
struct LogLevel <: AbstractLogLevel | |||
level::Int32 |
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.
Can we just change this to Int
?
Or make severity
return an Int32
I feel like the overhead of converting it each time is not worth it.
elseif level == Error print(io, "Error") | ||
elseif level == AboveMaxLevel print(io, "AboveMaxLevel") | ||
else print(io, "LogLevel($(level.level))") | ||
end | ||
end | ||
|
||
show(io::IO, level::LogLevel) = print(io, level == Warn ? "Warn" : level) |
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.
Reasonable.
@@ -44,14 +45,18 @@ function handle_message end | |||
|
|||
Return true when `logger` accepts a message at `level`, generated for | |||
`_module`, `group` and with unique log identifier `id`. | |||
|
|||
For very early filtering of custom log levels, users may override | |||
`shouldlog(level)`. |
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.
Is there a practical example other than severity(level) >= _min_enabled_severity[]
?
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.
Never mind. I guess one application might be some kind of "zero-cost @debug
" like ToggleableAsserts.jl https://discourse.julialang.org/t/ann-toggleableasserts-jl/31291
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.
You got it, this is exactly what I had in mind!
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.
By the way, this has been re-invented multiple times, for example https://discourse.julialang.org/t/defensive-programming-assert/8383/11?u=chris_foster :-D
I don't have time ATM to review this in detail (sorry). I just went through the OP and the code quickly but it looks good to me. For example, I think recommending
It may be a super nit-picking (in the sense that this doesn't matter in writing the code and docs), but I find it strange to treat the categorical part as the main concept of log level. According to Merriam-Webster one of the meaning is:
So, I think conceptualizing |
Thanks @tkf I always appreciate your thoughtful and insightful input. I think we reached consensus in #33418, on the subject of
By default this is true, but I have in mind some "interesting" uses for I was planning to use @oxinabox's write-up from the following comment as a starting point for the documentation: #33418 (comment) |
I commented on log "level" because I thought maybe it makes sense to use some other concept instead of "level" if the log level is not really about ordering. But log level is a very familiar concept for those who used other logging systems so I am not sure if it is a good idea to invent something new here. (BTW, |
re: depwarn, another way to implement it is:
That also compiles away. |
Indeed, this approach has many benefits and of course we'll continue to support it.
In what sense? I don't know of any way this could compile away. What I mean when I say "compile away" is that the compiler is able to optimize code like if shouldlog(stuff...)
more_logging_stuff
end into something like if false
more_logging_stuff
end via constant propagation / inlining etc, which then gets removed via dead code elimination. That means that the result of In your example |
How about |
Thinking about how to use log levels as filtering (ref JuliaLogging/ProgressLogging.jl#9, SciML/DiffEqBase.jl#450), I think it would be nice to have a way to declare the "default value" of supportedby(level::AbstractLogLevel, logger) = true
supports(logger::AbstractLogger, level) = supportedby(level, logger) with real_shouldlog_used_by_the_macro(logger, level, _module, group, id) =
supports(logger, level) && shouldlog(logger, level, _module, group, id) Proposed API
|
Oops, posted a half-done comment here by mistake! Re-posting something more complete... this has been languishing because I'm not sure whether we should address the categorical nature of |
This resolves #33418 by implementing something similar to #33418 (comment), but including various improvements which came up in the conversation.
The premise here is that log levels are a combination of two different things:
<
applied to their severities.Here I've introduced an
AbstractLogLevel
as the appropriate supertype for user defined log levels, and added aseverity
function which can be applied to any log level to compute the (integer) severity. Theprint
function is now recommended to compute log level labels from the level instance.This is an improvement on using
my_level = LogLevel(some_int)
because it allows custom levels to be represented with an appropriate label in pretty printing, and allows various modules to define custom levels independently without risk of confusion in the logging backends. It's also an improvement over the previous (undocumented) convention of allowing any custom type for a level because theseverity
function is more clearly defined.Detail
Add AbstractLogLevel as the supertype of all log level types.
Introduce "log level severity" including a
severity
function whichmaps any user-defined log level to an integer log level severity. This
more cleanly separates out the ordinal notion of log severity from the
categorical aspects of the log level.
Log levels can be ordered by comparing their severities. This gives a
certian type of pre-order which is reflexive, transitive and total, but
not antisymmetric. This is the same as the ordering of complex numbers
by their magnitude.
Change to using
print
rather thanshow
for formatting log levels fordisplay. This seems to resolve the old conundrum of "do we print Warn as
it appears in the program as "Warn" or in human-friendly form as
"Warning""?
The early (global) decision about log level filtering within the macro
is now lowered to
shouldlog(level)
which is more natural and morecustomizable.
A bug in
ConsoleLogger
was fixed where custom log levels could not actually be used with it easily.Compatibility
User defined log level types worked previously, but were undocumented and had a less sensible API. Nevertheless, I've attempted to include some fallback definitions so that this older pattern can continue to work. However, I have not
depwarn
ed these fallbacks yet becauseLogging
is still released in lockstep withBase
and this might make it hard for package authors to support both <=1.3 as well as 1.4 (/ 1.5 whenever this gets merged).TODO