fix(gax): manage OpenTelemetry scope in OpenTelemetryTracingTracer - #14457
jinseopkim0 wants to merge 1 commit into
Conversation
|
@gemini-code-assist review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request attempts to manage OpenTelemetry span scopes within OpenTelemetryTracingTracer by storing the Scope in an instance variable when an attempt starts and closing it when the attempt ends. It also adjusts the tracer factory ordering in ClientContext and adds corresponding unit tests. However, the feedback highlights a critical issue: storing and closing OpenTelemetry Scope instances across asynchronous boundaries violates the thread-bound contract of OpenTelemetry, leading to thread-local context leaks and corruption. It is recommended to remove this instance-level scope management, rely on ApiTracer.inScope() instead, revert the factory ordering change in ClientContext, and update the unit tests accordingly.
There was a problem hiding this comment.
Code Review
This pull request reorders the tracers in CompositeTracerFactory and introduces OpenTelemetry Scope management in OpenTelemetryTracingTracer to make the attempt span current during execution. However, storing the Scope as an instance variable and closing it in endAttempt introduces a severe thread-safety issue and context leak. Because Scope is backed by a ThreadLocal and API calls are often asynchronous, closing the scope on a different thread will not clean up the context on the initiator thread and can corrupt the callback thread's context. It is recommended to manage the scope locally within individual callback executions instead of using a long-lived instance variable.
bc5871d to
abd4e02
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request implements the inScope method in OpenTelemetryTracingTracer to manage OpenTelemetry span scopes, returning a no-op scope if the attempt span is null, and adds corresponding unit tests. Additionally, it adjusts the tracer order in ClientContext when creating a CompositeTracerFactory. There are no review comments, and I have no feedback to provide.
|
|
1 participant
Manage OpenTelemetry Scope lifecycle in OpenTelemetryTracingTracer so that log records emitted during attempt lifecycle can access the active span context, and order ApiTracerFactory before LoggingTracerFactory in CompositeTracerFactory to ensure span scope remains active during error logging.
Fixes: b/499388983