refactor: add otel tracing filter to logging#859
Conversation
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
There was a problem hiding this comment.
Edit: I tried letting Claude review and post this time while I was working on a different task
Review: OTel Trace Context Injection
Clean, well-motivated addition. The no-op guard when OTel isn't installed is correct, the test coverage is thorough, and the _otel_span context manager is a solid test helper. A few issues worth addressing before merging.
Bugs:
- Unsafe
span_idaccess inJsonFormatter._build_log_dict— crashes ifspan_idis absent whentrace_idis present trace_id/span_idnot added toRESERVED_LOG_RECORD_ATTRS— silent overwrite risk fromset_log_context
Design / Clarity:
3. Integration tests mutate the MelleaLogger singleton without resetting it first — order-dependent flakiness risk
Nits:
- Blank line removal before
# Apply include/exclude filteringis an unrelated cosmetic change - Docstring update on private
_build_log_dictis redundant; the publicformat()already got the note
jakelorocco
left a comment
There was a problem hiding this comment.
A few questions but lgtm without more changes. Thank you
6624f82
|
I believe these are already addressed @ajbozarth
|
Yeah I deleted my comment, I didn't noticed that since the branch had been deleted already that I hadn't actually pulled latest code for Claude to look at |
* add custom logging filter using otel * add additional logging tests * handle edge case and testing * review feedback
Misc PR
Will need to update after #457 is merged to use MelleaLogger
Type of PR
Description
logging.Filterthat automatically extracts the current OpenTelemetry trace context and injectstrace_idandspan_idinto every log record #458Testing