Logging API accepts optional Context with priority over trace_id etc, and LoggingHandler passes current Context #4597
Conversation
opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py
Outdated
Show resolved
Hide resolved
| ): ... | ||
|
|
||
| @overload | ||
| @deprecated( |
There was a problem hiding this comment.
LGTM! I think we want to make the same change in the API LogRecord
There was a problem hiding this comment.
Yes you're right. Updated in 172f1c5 but pyright typecheck fails. Will have a look
There was a problem hiding this comment.
Addressed the typecheck fail by forcing kwargs-only init of LogRecord (API) in 84c6b33 -- what do we think? Is this breaking?
The code that does init of LogRecord is SDK's LoggingHandler._translate and Event SDK, which use kwargs. Grep of LogRecord suggests the other inits are in the unit tests, and all tests pass.
There was a problem hiding this comment.
I've also installed this change into my vendor's setup and Log signals are still exporting.
| mock_context = Context() # no span in context | ||
|
|
||
| tracer = trace.TracerProvider().get_tracer(__name__) | ||
| < 8000 /td> | with tracer.start_as_current_span("test") as span: | |
| with patch( | ||
| "opentelemetry.sdk._logs._internal.get_current", | ||
| return_value=mock_context, | ||
| ): |
There was a problem hiding this comment.
I believe the mocking shouldn't be necessary since we are using the real context with start_as_current_span()
| mock_context = Context() # no span in context | |
| tracer = trace.TracerProvider().get_tracer(__name__) | |
| with tracer.start_as_current_span("test") as span: | |
| with patch( | |
| "opentelemetry.sdk._logs._internal.get_current", | |
| return_value=mock_context, | |
| ): | |
| tracer = trace.TracerProvider().get_tracer(__name__) | |
| with tracer.start_as_current_span("test") as span: |
There was a problem hiding this comment.
Sounds good! Updated in 172f1c5 and still passes.
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: