Skip to content
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.

Sign up for GitHub

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

Make it possible to skip logging before the actual call #2745

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Make it possible to skip logging before the actual call #2745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65ff710
Make it possible to skip logging before the actual call
hmpf Nov 16, 2023
67ada1f
Add tests for new function
hmpf Nov 17, 2023
822ccb7
Change if-statement to boolean expression
hmpf Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions python/nav/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,3 +535,18 @@ def resource_bytes(package, filename):
"""
ref = resource_files(package) / filename
return ref.read_bytes()


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())
"""
lunkwill42 marked this conversation as resolved.
Show resolved Hide resolved
level = logger.getEffectiveLevel()
return level <= loglevel
Comment on lines +540 to +552
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referring back to my previous review comments: Why do we need this at all? Why not just use logger.isEnabledFor(loglevel) ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.python.org/3/library/logging.html#logging.Logger.isEnabledFor

7 changes: 7 additions & 0 deletions python/nav/web/auth/remote_user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,13 @@
from os.path import join
import secrets

from django.conf import settings

from nav.auditlog.models import LogEntry
from nav.config import NAVConfigParser
from nav.models.profiles import Account
from nav.web.auth.utils import set_account
from nav.util import check_log_level


__all__ = []
Expand Down Expand Up @@ -178,6 +181,10 @@
if not request:
return None

if settings.DEBUG or check_log_level(_logger, logging.DEBUG):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this extra function really necessary? I find the following to be just as clear:

Suggested change
if settings.DEBUG or check_log_level(_logger, logging.DEBUG):
if settings.DEBUG or _logger.getEffectiveLevel() <= logging.DEBUG:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

settings.DEBUG is probably not even necessary since that doesn't flip debug-logging magically on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this jogged my memory, and I recall NAV already has multiple examples of the (to me) even more legible pattern:

Suggested change
if settings.DEBUG or check_log_level(_logger, logging.DEBUG):
if settings.DEBUG or _logger.isEnabledFor(logging.DEBUG):

for metakey, value in request.META.items():
if metakey[0] == metakey[0].upper():
_logger.debug('%s: %s', metakey, value)

Check warning on line 187 in python/nav/web/auth/remote_user.py

View check run for this annotation

Codecov / codecov/patch

python/nav/web/auth/remote_user.py#L185-L187 

Added lines #L185 - L187 were not covered by tests
workaround = 'none'
try:
workaround_config = _config.get('remote-user', 'workaround')
Expand Down
17 changes: 17 additions & 0 deletions tests/unittests/util_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import logging

import pytest

from nav.util import check_log_level


class TestCheckLogLevel:
def test_actual_loglevel_above_input_should_return_false(self, *_):
logger = logging.getLogger()
logger.setLevel(logging.INFO)
assert check_log_level(logger, logging.DEBUG) is False

def test_actual_loglevel_below_input_should_return_true(self, *_):
logger = logging.getLogger()
logger.setLevel(logging.INFO)
assert check_log_level(logger, logging.WARNING)
Loading