Make it possible to skip logging before the actual call #2745
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2745 +/- ##
=======================================
Coverage 56.69% 56.70%
=======================================
Files 602 602
Lines 43971 43980 +9
=======================================
+ Hits 24931 24937 +6
- Misses 19040 19043 +3 ☔ View full report in Codecov by Sentry. |
hmpf
force-pushed
the
skip-logging
branch
2 times, most recently
from
99827e4 to
28b1ee6
Compare
lunkwill42
left a comment
lunkwill42
left a comment
There was a problem hiding this comment.
I'm not sure I get the full connection between the PR title and the actual content. It doesn't seem to be as much about adding generic functionality to skip logging as to actually adding some more debug logging to remote user processing
| if not request: | ||
| return None | ||
|
|
||
| if settings.DEBUG or check_log_level(_logger, logging.DEBUG): |
There was a problem hiding this comment.
Is this extra function really necessary? I find the following to be just as clear:
| if settings.DEBUG or check_log_level(_logger, logging.DEBUG): | |
| if settings.DEBUG or _logger.getEffectiveLevel() <= logging.DEBUG: |
There was a problem hiding this comment.
The function makes for slightly easier branch testing if one is fond of mocks and patches. Also it might prevent someone from prettifying getEffectiveLevel() to level or what it was, which does not do what one thinks it does.
There was a problem hiding this comment.
settings.DEBUG is probably not even necessary since that doesn't flip debug-logging magically on.
There was a problem hiding this comment.
Well, this jogged my memory, and I recall NAV already has multiple examples of the (to me) even more legible pattern:
| if settings.DEBUG or check_log_level(_logger, logging.DEBUG): | |
| if settings.DEBUG or _logger.isEnabledFor(logging.DEBUG): |
I needed more logging and also found a way to scratch a very scratchy itch that has bothered me for years. Maybe it should have been two PRs. |
|
Kudos, SonarCloud Quality Gate passed!
|
|
hmpf
force-pushed
the
skip-logging
branch
2 times, most recently
from
66ba162 to
7e0c81a
Compare
| def check_log_level(logger, loglevel): | ||
| """ | ||
| Check that the level of the logger is equal to or below loglevel | ||
| Use before possibly expensive logging operations, like calling logging in | ||
| a loop or building an expensive object that will be logged and thrown away. | ||
| Avoid this classic mistake: | ||
| logger.info("%s", expensive_function()) | ||
| """ | ||
| level = logger.getEffectiveLevel() | ||
| return level <= loglevel |
There was a problem hiding this comment.
Referring back to my previous review comments: Why do we need this at all? Why not just use logger.isEnabledFor(loglevel) ?
There was a problem hiding this comment.
hmpf
force-pushed
the
skip-logging
branch
6 times, most recently
from
1ee3acc to
5edc150
Compare
|
Co-authored-by: Morten Brekkevold <[email protected]>
2 participants
Not particularly needed for where it is used in auth.remote_user but my sense of aesthetics was deeply offended by running a loop for no reason (if loglevel higher than DEBUG) and I wanted a general fix.