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

Check a dict passed to ProcessorFormatter actually came from structlog. #188

Closed
wants to merge 2 commits into from
Closed

Check a dict passed to ProcessorFormatter actually came from structlog. #188

wants to merge 2 commits into from

Conversation

@wrouesnel
Copy link
Contributor

@wrouesnel wrouesnel commented Nov 23, 2018

ProcessorFormatter assumes certain attributes are attached by wrap_for_formatter
but originally only tested if it was receiving a dictionary as the log message.

This breaks if any libraries you use happen to pass dictionaries as log messages
expecting them to print using default string formatting.

Fortunately since the behavior of the LogRecord is different between the two
paths, we can resolve this by testing for the relevent attrs before choosing.

Fixes #187

wrouesnel added 2 commits Nov 23, 2018
ProcessorFormatter assumes certain attributes are attached by wrap_for_formatter
but originally only tested if it was receiving a dictionary as the log message.

This breaks if any libraries you use happen to pass dictionaries as log messages
expecting them to print using default string formatting.

Fortunately since the behavior of the LogRecord is different between the two
paths, we can resolve this by testing for the relevent attrs before choosing.

Fixes #187.
if isinstance(record.msg, dict):
if (
isinstance(record.msg, dict)
and hasattr(record, "_logger")

This comment has been minimized.

@hynek

hynek Nov 24, 2018
Owner

Please meet: https://hynek.me/articles/hasattr/ :)

If we pile on checks and assume that dicts are predominant, why don't we invert the whole thing and make it an try:…except (KeyError, AttributeError)? I reckon that’s considered more pythonic too.

This comment has been minimized.

@wrouesnel

wrouesnel Nov 24, 2018
Author Contributor

I would argue when you have the ProcessorLogger in the mix, dicts and strs aren't predominant one way or the other, so a try...except seems unnecessarily slow.

I wonder if instead the wrapper should convert LogRecord into WrappedLogRecord and just do a type-check since ultimately what we're looking for is a different object type.

This comment has been minimized.

@hynek

hynek Dec 2, 2018
Owner

try...except seems unnecessarily slow.

Hm, do you really think it's slower than multiple checks on each record? I think I'd like to see a benchmark on that since this is is hot code. If you have some spare it would be great if you could compare it using https://pypi.org/project/perf/ . I only care about modern Python versions FWIW.

I wonder if instead the wrapper should convert LogRecord into WrappedLogRecord and just do a type-check since ultimately what we're looking for is a different object type.

That sound clean, but really slow? 🤔

Sorry I'm being so anal about performance here but I take great pride that structlog isn't a handbrake like logging and I'd like it to stay that way. 😇

This comment has been minimized.

@wrouesnel

wrouesnel Dec 2, 2018
Author Contributor

No I agree about keeping it quick. I'll setup some tests.

This comment has been minimized.

@hynek

hynek Dec 2, 2018
Owner

Awesome thank you and sorry for the delay. My original plan was to run them myself but just didn't get around to it.

@hynek
Copy link
Owner

@hynek hynek commented Jan 17, 2019

bump? :)

@hynek hynek closed this in 8917d5d Feb 2, 2019
hynek added a commit that referenced this pull request Feb 2, 2019
@hynek
Copy link
Owner

@hynek hynek commented Feb 2, 2019

So I decided to use the approach from #189 but I added your test case using you as an author: 44ccae2

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants