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

Add ExtraAdder Processor function to structlog.stdlib #377

Closed
wants to merge 5 commits into from

Conversation

aucampia
Copy link
Contributor

@aucampia aucampia commented Dec 5, 2021

Summary

Added the structlog.stdlib.ExtraAdder processor that adds extra attributes of logging.LogRecord objects to the event dictionary.
This processor can be used for adding data passed in the extra parameter of the logging module's log methods to the event dictionary.

Fixes #209

Pull Request Check List

This is just a friendly reminder about the most common mistakes. Please make sure that you tick all boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing left to do.

  • Added tests for changed code.
  • New APIs are added to typing_examples.py.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) are documented in the changelog.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

@aucampia aucampia force-pushed the iwana-20211205T1316-add_extras branch from 40e5458 to 70d346a Compare December 8, 2021 00:04
@hynek
Copy link
Owner

hynek commented Dec 8, 2021

Since you refer to 209 and also note yourself in #378 that adding all of callsite info would be overwhelming, I'd like to refer to my comment.

I think adding all data ist just too much. We just have to decide if we want to take an allowlist or an disallowlist approach.

Since you've taken an allowlist approach in #380, I would say the same would be good here too? I usually like it better since it gives one more control.

We can't use enums of course, because we don't know what keys that user has added.

@aucampia
Copy link
Contributor Author

aucampia commented Dec 8, 2021

Since you've taken an allowlist approach in #380, I would say the same would be good here too? I usually like it better since it gives one more control.

We can't use enums of course, because we don't know what keys that user has added.

Okay sounds like a good idea, I will change this to a processor class and add an allow list, and also provide an option for allowing copying of all values.

@hynek
Copy link
Owner

hynek commented Dec 8, 2021

Yes, that's even better!

@hynek
Copy link
Owner

hynek commented Dec 8, 2021

maybe just make it allow=None == all?

i.e. allow: Optional[Sequence]

@aucampia
Copy link
Contributor Author

aucampia commented Dec 8, 2021

maybe just make it allow=None == all?

i.e. allow: Optional[Sequence]

This will be simplest, I will go for this, will update PR tonight (CET)

This processor function will add extra attributres from LogRecord
objects to the event_dict, and can be useful for making values passed
in the extra parameter of the logging module's log methods pass through
to output as additional properties.

Fixes hynek#209
- fix assert parameter order
- add `add_extra` to `__all__`
@aucampia aucampia marked this pull request as draft December 9, 2021 00:09
This is so ExtraAdder can be configured with what keys it should copy.
@aucampia aucampia force-pushed the iwana-20211205T1316-add_extras branch from 70d346a to 95857ae Compare December 9, 2021 00:10
@aucampia
Copy link
Contributor Author

aucampia commented Dec 9, 2021

Okay I converted it to a processor class, ExtraAdder and updated tests for this, still have to fix up documentation, will get to it tomorrow.

- Fix up documentation
- Improve tests
@aucampia aucampia force-pushed the iwana-20211205T1316-add_extras branch from 7eb8ec8 to 100b43d Compare December 9, 2021 23:30
@aucampia aucampia changed the title Add add_extra Processor function Add ExtraAdder Processor function to structlog.stdlib Dec 9, 2021
@aucampia
Copy link
Contributor Author

aucampia commented Dec 9, 2021

Documentation and changelog updated now also. I made the allow parameter a Collection instead of a Sequence so it can accept a set also.

@aucampia aucampia marked this pull request as ready for review December 9, 2021 23:33
Copy link
Owner

@hynek hynek left a comment

Choose a reason for hiding this comment

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

I've found a few nits, but they don't warrant another review round. This is great – thank you!

src/structlog/stdlib.py Show resolved Hide resolved
@hynek
Copy link
Owner

hynek commented Dec 10, 2021

(I've merged it in 9208a2c because I couldn't push into your branch – thanks again!)

@hynek hynek closed this Dec 10, 2021
@aucampia
Copy link
Contributor Author

@hynek I will add tests for the two uncovered lines tonight (again CET), did not realize that test coverage target is 100% but should have added a test for case where there is no log record.

I will also add a comment for [*allow] instead of allow - this is mainly to prevent any surprises if the collection is mutated afterwards - probably it is a bit defensive to do that, but at the very least it is good to clarify why it is happening.

And please mention other issues if there are, will update them as I get time.

I will also make a PR to update PULL_REQUEST_TEMPLATE with:

  • consider giving you permission to their PR branches
  • ensure coverage is 100%

@hynek
Copy link
Owner

hynek commented Dec 10, 2021

I've fixed the coverage; feel free to open a hock PR for the allow thing tho!

hynek added a commit that referenced this pull request Dec 10, 2021
@hynek
Copy link
Owner

hynek commented Dec 10, 2021

(i've also updated the template – sorry i've misread your comment on the go on a phone. i've only noticed you want to do it after looking up the PR id to refer to it)

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.

Log stdlib's logging extra-s
2 participants