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

22.2.0 - TypeError with logger.exception and string interpolation #473

Closed
alangenfeld opened this issue Nov 21, 2022 · 6 comments · Fixed by #475
Closed

22.2.0 - TypeError with logger.exception and string interpolation #473

alangenfeld opened this issue Nov 21, 2022 · 6 comments · Fixed by #475

Comments

@alangenfeld
Copy link

_log_levels.py:156
unsupported operand type(s) for %: '<exception object>' and 'tuple'

sample stack https://gist.github.com/alangenfeld/2bc3f1d22e81af428fa487320fc6c797

@alangenfeld alangenfeld changed the title 22.2.0 - issue with logger.exception and string interpolation 22.2.0 - TypeError with logger.exception and string interpolation Nov 21, 2022
@fbraza
Copy link

fbraza commented Nov 21, 2022

Hello @alangenfeld

I confirm the bug. It happens here too. Our bitbucket pipelines kepts hanging for hours. Today bitbucket pipeline service was actually down. So I thought to wait the fix. But next still had the problem. It boiled down to this specific error.

Screenshot from 2022-11-21 20-25-25

I rolled back to a previous version and the pipeline passed. Because we used the logger extensively our pipeline just bursted I guess. They hanged 2 hours and then crash. I guess the stack trace was huge :-)

I unfortunately cannot share more specific code but can confirm that the error was basically screwing up our testing suite.

Cheers

@hynek
Copy link
Owner

hynek commented Nov 22, 2022

I would've been helpful if either of you gave an Short, Self Contained, Correct, Example or at least a call example.

Please help me understand this here, so far I'm guessing:

  1. @alangenfeld called:
    except Exception as exc:
        log.exception(exc)
  2. @fbraza called something like log.info(42)

?


So far it looks like a case of Hyrum's Law to me – the event argument was always documented as having to be a str. Case 2 should be wrapped in a str() but how exactly does case 1 work? Even stdlib's logging is documented to take a msg:

logging.exception(msg, *args, **kwargs)

@alangenfeld are you aware that if you're in an except block, you don't have to pass the exception into log.exception()? And if you're in an except block, the correct usage is log.exception("event", exc_info=exc) and the behavior is documented here: https://www.structlog.org/en/stable/api.html#structlog.processors.format_exc_info


I'm not sure what to do here there seem to be three bad options for me:

  • remove the feature
  • add isinstance(event, str) checks slowing down structlog for everybody
  • make everybody fix their code, however I don't know how many "everybody" is.

In any case, I should better document the exception handling. I've opened #474 for that.

hynek added a commit that referenced this issue Nov 22, 2022
Prevents assumptions about `event` being a string.

Fixes #473
hynek added a commit that referenced this issue Nov 22, 2022
Prevents assumptions about `event` being a string.

Fixes #473
@hynek
Copy link
Owner

hynek commented Nov 22, 2022

I think I've found a compromise that shouldn't have a measurable performance impact in #475 – can you two please check if it fixes your problems? Thank you.

@fbraza
Copy link

fbraza commented Nov 22, 2022

Hello @hynek

Many thanks for your prompt & thorough feedback. I'll come back to you with an answer today.

All the best

@alangenfeld
Copy link
Author

Apologies for the terse issue submission and thanks for the detailed response. You are correct that this is from call sites that pass the exception object in directly. #475 looks like it will handle that now as per the test case.

@fbraza
Copy link

fbraza commented Nov 23, 2022

Hello both,

Indeed it does.

Many thanks again.

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 a pull request may close this issue.

3 participants