✨ Add metadata filter support for context logger #297
Conversation
gabe-l-hart
left a comment
gabe-l-hart
left a comment
There was a problem hiding this comment.
Thanks for putting this together @gkumbhat! I think we want to look carefully at LoggerAdapter instead of logging.Filter, and I think the naming could be cleaned up a little. Also, it's probably worth using a separate scope object rather than just ScopedLog since that's explicitly designed to add Start/End blocks. Something like ScopedMetadata.
| """ | ||
| return logging.getLogger(channel) | ||
|
|
||
| ## Logger Filters ############################################################## |
There was a problem hiding this comment.
Hmmm, this is interesting. I have two reactions to this:
- I don't think this is what
logging.Filteris meant to do. I think LogAdapter is probably the more correct utility for this type of contextual information. - This makes me think that some of the
alogfunctionality around filtering should be refactored to uselogging.Filterin a more standard way
There was a problem hiding this comment.
I tried to use LogAdapter, however, but it modifies the logger which is read only inside _ScopedLog
There was a problem hiding this comment.
Filters are also designed / recommended for this use-case it seems: https://docs.python.org/3/howto/logging-cookbook.html#filters-contextual
There was a problem hiding this comment.
This makes me think that some of the alog functionality around filtering should be refactored to use logging.Filter in a more standard way
I was thinking same while exploring adapters and filtering.
There was a problem hiding this comment.
Also current implementation isn't thread safe. I think for it to be threadsafe, I'll need to save this metadata in a threadlng.local based class object similar to how we do it for indent. Was wondering if there would be any concerns for that?
There was a problem hiding this comment.
Yeah, this is the nastiest part IMO. I think we'd want some careful threading behavior:
- Independent threads manage independent
metadatacontexts - Spawned threads inherit context
metadatafrom parents - Closing a metadata scope removes the metadata from the thread where the scope was created
- I'm honestly not sure what should happen to threads spawned by this parent thread while the scope was open
Also, the same behavior should probably be true of spawned processes
| else: | ||
| log_record['message'] = str(record_args) | ||
|
|
||
| if hasattr(record, "additional_ctxt_metadata"): |
There was a problem hiding this comment.
If we're going to have a "special" string for this, I think it should be (a) a constant, and (b) a __dunder__ name
There was a problem hiding this comment.
Yes, certainly. Will update the PR
2 participants
Description
What does this PR do?
Changes
What changes are included in this PR?
Testing
How did you test this PR?
Related Issue(s)
List any issues related to this PR