Logging API accepts optional Context with priority over trace_id etc, and LoggingHandler passes current Context #4597
Conversation
tammy-baylis-swi
changed the title
| @@ -186,10 +189,22 @@ def __init__( | |||
| attributes: _ExtendedAttributes | None = None, | |||
| limits: LogLimits | None = _UnsetLogLimits, | |||
| ): | |||
| # Prioritizes context over trace_id / span_id / trace_flags. | |||
There was a problem hiding this comment.
I'd like to maybe be more aggressive here since Logs are not stable and I foresee more breaking changes. Can we just deprecated these off the bat and log a warning if they're passed? We can then remove them before stable release.
There was a problem hiding this comment.
I wonder if using a deprecated overload to signal to users would work
@overload
@deprecated
def __init__(
self,
trace_id: int | None = None,
span_id: int | None,
trace_flags: int | None,
**kwargs
): ...Might be a little tricky but should be possible
There was a problem hiding this comment.
we are not using warnings.deprecated in the codebase but I think it can works
There was a problem hiding this comment.
The @deprecated decorator isn't available Python < 3.13 , but I'll try that approach!
There was a problem hiding this comment.
Created our own @deprecated decorator and set up an overload in 676d9ff. An alternative could be to introduce Deprecated as an additional dependency. Thoughts? 🙂
There was a problem hiding this comment.
| ) | ||
| self.assertIsNotNone(log_record) | ||
| self.assertEqual( | ||
| log_record.trace_id, INVALID_SPAN_CONTEXT.trace_id |
There was a problem hiding this comment.
Would this pass even without mock_context? Maybe we could set a real specific span in context to get more signal
span_context = SpanContext(
trace_id=trace_id,
span_id=span_id,
...,
)
with use_span(NonRecordingSpan(span_context)):
...There was a problem hiding this comment.
Yes, no mock_context needed for this test; removed in a0c817c.
For real span, does test_log_record_trace_correlation further down look ok?
Description
Contextat LogRecord init.trace_id,span_id,trace_flagsprovided at init.LogRecord'sRemoved in 73e7b39 as discussed with Python SIG.to_jsondoes a serialization of what it can in Context (e.g. string, int) then casts to string for other values (e.g. Span).LoggingHandler'sLogging SDK (LogRecord init) defaults to current context in 676d9ff, as discussed with Python SIG._translateinits LogRecord with current context.Let me know if too many changes for one PR and I can break it up or remove anything out of scope.
Fixes #4328
Replaces #4584
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Contrib Repo Change?
Checklist: