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

Added structlog support #109

Merged
merged 13 commits into from
Feb 17, 2024
Merged

Conversation

will-ockmore
Copy link
Contributor

@will-ockmore will-ockmore commented Feb 14, 2024

  • Adds StructlogCapturer and related docs.

Follows loguru extension structure, borrowing inspiration from the structlog testing tools. I've matched the suggested version range in hynek/structlog#596.

Closes #108

@will-ockmore will-ockmore marked this pull request as draft February 14, 2024 22:49
@will-ockmore will-ockmore marked this pull request as ready for review February 14, 2024 23:16
@etianen
Copy link
Owner

etianen commented Feb 15, 2024

Amazing, thank you, I'll take a look ASAP.

I realise I've shot myself in the foot with listing 3rd party extensions in the readme tagline. That's not gonna scale, but highlighting a subset of them feels a bit arbitrary and unfair. Which are the "premium" integrations?! 😬 That's my problem though.

@will-ockmore
Copy link
Contributor Author

That is true, it's not going to scale! Happy to adjust the approach in this PR if you want to get ahead of it.

Copy link
Owner

@etianen etianen left a comment

Choose a reason for hiding this comment

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

This is very thorough, thank you! It makes me realise that I need to refactor the docs to make them scale beyond a few 3rd-party integrations. You had to touch too many files here! But that's for me to do later.

The structlog integration looks tricky. I have two main bits of feedback:

  • I think we can narrow the version range to reduce it's complexity. It's great that you're following the structlog author's suggested version range, but given his use of CalVer that's going to get wider each year! If we can constrain it a bit now to keep the code size down, I'd call that worthwhile.
  • The structlog log capturing doesn't behave quite like the other capturers. A capturer is supposed to leave any other log processing as-is, rather than intercept and squash. This allows multiple capture calls to coexist.

You've been a guinea pig here, and most of this I'll call out explicitly in the docs once this is merged. 🙇

pyproject.toml Show resolved Hide resolved
logot/_structlog.py Outdated Show resolved Hide resolved
logot/_structlog.py Outdated Show resolved Hide resolved
logot/_structlog.py Outdated Show resolved Hide resolved
logot/_structlog.py Outdated Show resolved Hide resolved
logot/_structlog.py Outdated Show resolved Hide resolved
tests/test_structlog.py Show resolved Hide resolved
@etianen
Copy link
Owner

etianen commented Feb 15, 2024

That is true, it's not going to scale! Happy to adjust the approach in this PR if you want to get ahead of it.

Let's leave it as-is, and I'll have a fiddle later.

@etianen etianen added the enhancement New feature or request label Feb 15, 2024
@etianen etianen changed the title Structlog support Added structlog support Feb 15, 2024
@etianen etianen merged commit e7349c6 into etianen:main Feb 17, 2024
16 checks passed
@etianen
Copy link
Owner

etianen commented Feb 17, 2024

Delightful! Thank you! This is worth making a 1.1 release for, so I will.

@etianen
Copy link
Owner

etianen commented Feb 17, 2024

Tadah! https://github.com/etianen/logot/releases/tag/1.1.0

I tweaked the log name matching to also support prefixes, matching the logging and loguru behaviour ... something that wasn't in the tests and I didn't spot in your PR.

That was a tricker integration than I expected. loguru was trivial by comparison, but loguru doesn't really do anything radically different to the stdlib here, it just has a nicer interface.

@will-ockmore will-ockmore deleted the structlog-support branch February 17, 2024 16:04
@will-ockmore
Copy link
Contributor Author

Nice! I didn't spot the prefix behaviour, although you referenced it in your comment. Thanks for picking that up, and for the fast release 🐎

@etianen
Copy link
Owner

etianen commented Feb 17, 2024

I was wondering... maybe the logot capture should be the first in the processor list?

From #117, I said:

logot is there to test that a logging call has been made, not that it's been output to any particular sink.

Which, when applied to structlog would seem to indicate we're only caring about the .bind(...).log(...) at the call site.

@will-ockmore
Copy link
Contributor Author

To me that does feel more appropriate. Reason I say this is that seeing the logged.info("my message") syntax immediately made me think of the call site, rather than the resultant event that ends up in a sink / logger.

Thinking of what it might look like if the ability to check for context on an event was added (hypothetical sytax to match the log in the example in https://www.structlog.org/en/stable/getting-started.html#your-first-log-entry ):

logot.wait_for(logged.info("hello, world!", key="value!", more_than_strings=[1, 2, 3]))

It's even clearer that the syntax could be very close to the call site (and not so close to whatever eventual format the event has post processing).

If someone wanted to test the processor chain as well, then the current implementation is better; however it's probably more suprising and certainly more complicated in practice as it can depend on more code.

@will-ockmore
Copy link
Contributor Author

See #118 for the above change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support structlog
2 participants