Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Alternative approach to debug logging utilities. #64
Alternative approach to debug logging utilities. #64
Changes from all commits
677d925
8226ad9
99835e1
05fa158
0d549f2
8d0069c
fb99b13
a0a489e
84fa4ce
358652f
6d92532
8e84a78
73f17b8
caf7b94
b20b42e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This doesn't seem to override?
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.
The 1st way it overrides is by only returning its own
level
frommin_enabled_level
(other loggers typically return themax
of itself and child loggers). Also in this case, it's only checking its own level and then dispatching to it's nested loggershouldlog
; as far as I understand, loggers don't typically check levels inshouldlog
because it happens before w/min_enabled_level
.We're also just overriding the level; if there are other filters from the nested logger, then those will still be respected. If the nested logger does check levels in their
shouldlog
, then those will be respected. It's a bit tricky because it's hard to know exactly what the nested logger is going to do inshouldlog
, but from initial tests, this seems to be working as expected.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.
It looks like the
comp_shouldlog
which is called by all the composable loggers in the package checks bothlevel
andshouldlog
.LoggingExtras.jl/src/LoggingExtras.jl
Line 35 in 396afbb
There is a comment above it saying that because the loggering system itself won't check the
level
of child loggers.I think I ran into a bug before I had this with something like
TeeLogger(MinLevelFilter(Debug, ...), MinLevelFilter(Warn, ...))
Which should have the
TeeLogger
saying it's minlevel isDebug
since it needs to recieve Debug level messages.But then those need only to be accepted by one of the loggers and not by the other.
I think this can possibly be fixed by moving the logic from the recieving logger, up into the sending logger.
Which would better match how the logging system works outside the compositional structure.
So it only forwards to children that say they can accept it.
(vs right now where the child decides that it won't log it because of it's level)
I could be wrong here and this could all work
A test would prove it one way or the other
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 a test be added to check this case please
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.
This
shouldlog
method is exercised in the tests here:LoggingExtras.jl/test/runtests.jl
Line 254 in 73f17b8
Debug
level is "overriding" the TestLoggerInfo
level.Is there a more specific/different test desired here?
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.
yes,
I want to see:
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'm not sure this can be made to work due to the way MinLevelLogger has it's
shouldlog
andmin_level_enabled
defined; i.e. the presence of aMinLevelLogger
in a stack of loggers effectively "poisons" any other kind of level filtering. It'smin_enabled_level
callsmax
of its own level and its child logger, so it's going to raise the level of a LevelOverrideLogger. And because MinLevelLoggershouldlog
callscomp_shouldlog
of its child logger, and becausecomp_shouldlog
checks themin_enabled_level
of the child logger, it means LevelOverrideLogger gets overridden, even if we were to get back the min_enabled check.It's a similar issue if LevelOverride wraps a MinLevelLogger; the LevelOverride will pass the level check, but then it calls the
shouldlog
of its child logger and theMinLevelLogger
will "take over" as mentioned above.I don't see an immediate/easy way to detangle all that without going big with an overhaul of all the other compositional loggers here, so I'm inclined to just put a note in the docs that the LevelOverrideLogger should be used independently from other level-filtering loggers.
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 think I know how to fix it.
Which is a big overhaul of all the other compositional loggers.
But I don't think it will take me long.
Let's put that test case in.
Mark it broken, and open an issue cross referencing it, then merge this.
Then later tonight or tomorrow I will fix it.
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.
Ok, added the broken test.
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 see what you are doing here.
Can we find a nicer way to do it?
The way i have seen Keno do this is strip all line numebrs from generate code and then just interpolate
__source__
as the first statement.https://github.com/JuliaDiff/ChainRulesCore.jl/blob/2d75b4be102bb41ba3ac6df6dec8bb9617b20f0f/src/rule_definition_tools.jl#L178-L186
The way I have done it before is similar, but with creating the Expr directly so no line numbers endup there to begin with:
https://github.com/JuliaDiff/ChainRulesCore.jl/blob/2d75b4be102bb41ba3ac6df6dec8bb9617b20f0f/src/tangent_types/thunks.jl#L134
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 something problematic w/ this approach? I copied this from elsewhere; I can't remember if I wrote it or someone else.
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.
It's just very unclear what
expr.args[1].args[2]
isI guess something that would make it ok would be something like
As a form of self-checking documentation.
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.
Should be
right?
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 don't think so?
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.
Why not? Now this will not be within the Debug level range
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.
Or I guess,
+
instead of-
.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.
For me, I think of it like this: there are really two dimensions we're talking about: severity and verbosity. Sometimes, people view the log level severity spectrum as also corresponding to verbosity, but that's not quite right; the severity is more about the type of log, and how it should be seen in various scenarios. i.e.
Debug
is for non-normal/production cases where you're trying to see additional development-related information that's going on.Info
is general things that happen/major changes in state or milestones.Warn
is there's something non-fatal that was encountered.Error
is something bad happened.Verbosity, on the other hand, is the level of detail we want to see for a given severity. Now, I'll readily admit this principle doesn't seem super applicable/useful for non-Debug log levels, but for debugging, it really does make a strong point, and indeed many systems also include a
Trace
log level in this vein: sometimes you want even more debug information thanDebug
. But I would argue even forError
orWarn
, there are theoretical cases where you might want to emit more verbose messages than the default if requested by the user (e.g. for an@inbounds
error, maybe I want to see the full collection being indexed, where as by default you almost surely want some kind of compact printing).All that to say, instead of actually introducing two dimensions for severity and verbosity, we're hijacking the fact that the log levels themselves are so spread out in the integer domain to say that log levels in between the standard severity levels correspond to verbosity instead of actually different categorical/types of logs. So in the same vein of the severity spectrum at least roughly/partly ordered by verbosity (i.e. I would expect more
Trace
orDebug
messages thanError
orWarn
), we're saying thatDebug - 1
is likeDebug
, but one level more verbose. AndDebug - 2
is also like debug, but 2 levels more verbose, and 1 more level verbose thanDebug - 1
. So the levels less than the severity level, but greater than the next lower severity level constitute the possible "verbosity" levels for that severity 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.
this is actually kinda unfortunate positioning.
because we want
@debugv 0
to be the same as@debug
and to be the "most significant debug level"Ideally we would have that
Debug +0
is in the middle of the range that highlights asDebug
so thatDebug+1
andDebug-1
can both be considered as more or less significant debug-levelled values.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 looked briefly at changing this, but it didn't seem like an easy/obvious fix with my first attempt, mainly because of the directionality of log levels and filtering. Filtering always relies on filtering out lower levels, so it doesn't work to just invert our verbosity to be + instead of -.
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.
Yeah, that is why i wish
Debug +0
was in the middle of the range that highlights asDebug
.Can we have this changed up-stream in Julia, and then just live with the bad highlighting until 1.9 comes out?
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.
see: JuliaLang/julia#45708
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.
Thinking about this some more, I think I'd like to go back to the approach of using the
group
variable for verbosity. I think trying to leverage log levels has just muddied the waters too much. I also realized that there's one feature that "doesn't work" with the log-level approach: if I have a scenario like here, where, in the normal case, I just want to ignore any errors, but if I'm in a "verbose" mode, I'd like to at least log those kinds of errors, it doesn't work w/ our current log-level approach. Because@errorv 2 msg
just becomesLogLevel.Error - 2
, and that means it will always get printed, because it's "above" the defaultInfo
level.So I think this has clarified in my mind that verbosity is really a 2nd dimension, in addition to log level, that I want to be able to filter on separately. i.e. if my
verbosity=1
, then I don't want to see the overly verbose@errorv 2 msg
log. It also cements in my mind that log levels, while kind of a proxy for verbosity, just aren't. They're categorical values that give specific "types" to log messages.If no one is strongly opposed, I'm going to move the
LoggingExtras.@[x]v
macros back to that approach.I think another benefit of that approach is we should "fix" the log level printing color issue? Since we're not modifying the actual log level at all?