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.

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

Bugfix using the getMessage method to get a LogRecord msg as a string #10

Merged
merged 1 commit into from
Oct 27, 2021

Conversation

bridgetrossiter
Copy link
Contributor

Pull request to fix a bug in the Ratelimitingfilter.

I noticed a bug when I attempted to call logger.exception(ValueError("This is a error")) . This is because in ratelimitingfilter.py, the log message is extracted from the LogRecord using LogRecord.msg. However, ratelimitingfilter assumes that this msg is always a string. This is not actually the case, particularly when it comes to exceptions.

Documentation on this can be found here: https://docs.python.org/3.9/howto/logging.html#arbitrary-object-messages

For example, change the last test in the code to the following:

def test_log_exception(self):
    logger = logging.getLogger('test')
    handler = logging.StreamHandler()
    ### Added configuration ###
    config = {'match': 'auto'}
    throttle = RateLimitingFilter(rate=1, per=1, burst=1, **config)
    ###########################
    handler.addFilter(throttle)
    logger.addHandler(handler)

    try:
        logger.error('First')
        raise RuntimeError('Expected exception')
    except RuntimeError as e:
        logger.exception(e)  # Should be throttled

And run the tests, you'll see the bug. Ratelimitingfilterfails at any point when it assumes that the msg is a string and attempts to iterate through it, as the LogRecord.msg is actually the RuntimeError exception object and is not iterable.

To fix this bug, I have changed references to LogRecord.msg to LogRecord.getMessage(), which runs str on the message before passing it to you. Details on the method are here: https://docs.python.org/3.9/library/logging.html#logging.LogRecord.getMessage

I have also doubled checked, and even with the changes I still get the full stack trace when logging an exception.

@wkeeling
Copy link
Owner

Many thanks for the PR! That's a good find on LogRecord.msg when using logger.exception() with auto throttling. Will merge and create a new release containing the fix.

@wkeeling wkeeling merged commit c72946d into wkeeling:master Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants