feat(gax): prototype Approach B callee-managed scoping (LoggingTracer via SharedContext) - #14568
jinseopkim0 wants to merge 27 commits into
Conversation
… via SharedContext)
![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 introduces a mechanism to propagate the active OpenTelemetry attempt span to the LoggingTracer via a new SharedContext in ApiTracerContext. This ensures that logs emitted during actionable errors are correctly associated with the active tracing span. The feedback highlights several critical improvements: ensuring thread safety for the lazy initialization of sharedContext using double-checked locking, adding defensive null checks in LoggingTracer to prevent potential NullPointerExceptions, avoiding unnecessary side-effects in the merge method of ApiTracerContext, and overriding operationFailed in OpenTelemetryTracingTracer to prevent span reference leaks.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared context mechanism (SharedContext) within ApiTracerContext to share the active attemptSpan between sibling ApiTracer instances, allowing LoggingTracer to activate the span during actionable error logging. Corresponding integration and unit tests were added to verify span context capture and prevent leaks. The review feedback highlights critical issues with this implementation: defining sharedContext as a private mutable field inside an @AutoValue class breaks toBuilder() copying and equals/hashCode consistency; unconditionally overwriting sharedContext during a merge operation can discard active spans; and failing to override attemptFailed in OpenTelemetryTracingTracer leaves stale, ended spans in the shared context during retry backoff.
…ovalue integration
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to capture and propagate the OpenTelemetry span context during error logging by adding a mutable SharedContext to ApiTracerContext and updating LoggingTracer and OpenTelemetryTracingTracer accordingly. While the overall implementation is solid, there are two critical issues in ApiTracerContext.java that need to be addressed: first, the custom equals and hashCode overrides in SharedContext are highly dangerous for a mutable container and should be removed; second, the merging logic for SharedContext is flawed and can discard the active shared context reference when the attempt span is null.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to share the attempt span context between sibling ApiTracer instances belonging to the same attempt by adding a SharedContext class inside ApiTracerContext. This allows LoggingTracer to activate the attempt span during error logging, ensuring that logs are correctly associated with the active span. Feedback on these changes suggests returning an empty SharedContext instead of null in ApiTracerContext.sharedContext() to minimize null pointer risks and eliminate the need for defensive null checks.
…ks and preserve equality
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to share the active OpenTelemetry attempt span context with sibling tracers, such as LoggingTracer, by adding a mutable SharedContext to ApiTracerContext. This ensures that error logs capture the correct span context without active span leaks. Feedback on the changes focuses on code readability and maintainability, including importing Scope to avoid fully qualified names, simplifying the retrieval of attemptSpan, extracting duplicated span-clearing logic into a helper method in OpenTelemetryTracingTracer, and using a ternary operator to streamline the merging of sharedContext.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a SharedContext within ApiTracerContext to share the current attempt span between sibling tracers, allowing LoggingTracer to activate the span during error logging. Feedback on the changes highlights a bug in LoggingTracerTest where the use of ApiTracerContext.empty() prevents the mock span from being set, and suggests simplifying the try-with-resources block in LoggingTracer as well as adding bidirectional merge assertions in ApiTracerContextTest.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to share the active OpenTelemetry attempt span context among sibling tracers (such as OpenTelemetryTracingTracer and LoggingTracer) using a new SharedContext in ApiTracerContext. This ensures that actionable error logs are correctly associated with the active span context. The review feedback highlights a critical bug in the ApiTracerContext merge logic that could discard active shared contexts, suggests using compareAndSet in OpenTelemetryTracingTracer to prevent race conditions when clearing the shared span, and recommends importing Scope in LoggingTracer to avoid fully qualified class names.
…ve context during merge
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to capture and propagate the OpenTelemetry span context during error logging by adding a SharedContext to ApiTracerContext to share the attemptSpan between sibling tracers. Feedback on these changes highlights a potential bug in the merge logic of ApiTracerContext that could cause cross-attempt span leakage or discarded contexts, and suggests a more idiomatic approach in LoggingTracer to avoid passing null to a try-with-resources statement.
…source in LoggingTracer
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a SharedContext within ApiTracerContext to share the current attempt's OpenTelemetry Span among sibling ApiTracer instances, allowing LoggingTracer to activate the span context when logging actionable errors. Key feedback points out that the merge logic in ApiTracerContext should prefer the current instance's shared context to avoid orphaning existing tracers, and that adding a mutable SharedContext to an @autovalue class breaks its value-type semantics. Additionally, unconditionally clearing the shared attempt span on operation completion in OpenTelemetryTracingTracer is redundant and may introduce race conditions.
…t clear in operation methods
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to propagate and activate the current OpenTelemetry attempt span during the logging of actionable errors. It adds a mutable SharedContext to ApiTracerContext to share the attempt span between sibling tracers, which is then managed by OpenTelemetryTracingTracer and activated within LoggingTracer during error logging. Feedback on these changes highlights a potential thread-safety issue in ApiTracerContext.merge where a non-atomic check-then-act operation should be replaced with compareAndSetAttemptSpan, as well as several redundant no-op method overrides in OpenTelemetryTracingTracer that can be safely removed.
There was a problem hiding this comment.
Code Review
This pull request introduces a shared context mechanism (SharedContext) within ApiTracerContext to share mutable state, specifically the attempt scope provider, between sibling tracers. This allows LoggingTracer to activate the attempt span during error logging, ensuring the span context is captured without leaking spans. The changes are supported by new integration and unit tests. The review feedback suggests optimizing the EMPTY sentinel in SharedContext by overriding openAttemptScope() to return null directly, and improving test code readability by replacing fully qualified class names with imports.
There was a problem hiding this comment.
Code Review
This pull request introduces a shared context mechanism (SharedContext in ApiTracerContext) to propagate and activate the attempt span context during error logging in LoggingTracer. This ensures that logs emitted during an attempt are correctly associated with the active OpenTelemetry span. The feedback highlights two important concurrency concerns: first, the attemptScopeProvider field in OpenTelemetryTracingTracer should be declared volatile to prevent thread visibility issues when starting and ending attempts on different threads; second, sharing the same mutable SharedContext instance across toBuilder() copies could lead to race conditions or incorrect span associations in concurrent or hedged attempt scenarios, which should be documented or mitigated with an explicit fork mechanism.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to share attempt span scopes between sibling ApiTracer instances (such as OpenTelemetryTracingTracer and LoggingTracer) via a new SharedContext in ApiTracerContext. This ensures that LoggingTracer can activate the correct span context when logging actionable errors. Feedback on the changes highlights a potential NullPointerException in LoggingTracer if apiTracerContext.sharedContext() is null (e.g., when mocked in tests), and suggests adding a defensive null check.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared context mechanism within ApiTracerContext to propagate the active OpenTelemetry span scope to sibling tracers, specifically allowing LoggingTracer to activate the attempt span when logging actionable errors. This ensures logs are correctly correlated with traces. The changes also include corresponding integration and unit tests. The feedback recommends a minor cleanup in LoggingTracerTest.java to use the simple class name Span instead of its fully qualified name, as it is already imported.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared context mechanism (SharedContext) within ApiTracerContext to share attempt scope providers between sibling tracers, enabling LoggingTracer to open the attempt scope during actionable error logging. Corresponding tests and updates to TestAppender are added to verify span context capture. The review feedback points out a potential race condition where concurrent attempts sharing the same ApiTracerContext might overwrite each other's attemptScopeProvider. Additionally, the reviewer suggests initializing the Scope resource directly within the try-with-resources statement in LoggingTracer for safer resource management, and strengthening the test in LoggingTracerTest using atomic booleans to verify that the scope is correctly opened and closed.
…ingTracer try-with-resources
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a SharedContext mechanism in ApiTracerContext to share mutable state, such as the active attempt scope provider, between sibling ApiTracer instances. This allows LoggingTracer to activate the correct span context during error logging, which is managed by OpenTelemetryTracingTracer. The changes also include thread-safe test appenders and new test coverage. Feedback on the changes points out that the custom equals and hashCode implementations in SharedContext make all instances equal, which breaks the equality logic of the AutoValue class ApiTracerContext and should be replaced with default reference equality.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to capture and propagate the active OpenTelemetry span context during error logging by adding a SharedContext to ApiTracerContext. Sibling tracers within an attempt can now share scope state, allowing LoggingTracer to log actionable errors within the active attempt span. The review feedback highlights a potential concurrency issue with hedged requests sharing the same SharedContext instance, suggesting that each attempt should receive a fresh context. Additionally, several redundant null checks on the non-nullable sharedContext() method should be simplified across LoggingTracer and OpenTelemetryTracingTracer.
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to capture and propagate the active OpenTelemetry span context during error logging by adding a SharedContext to ApiTracerContext. Sibling tracers within an attempt can now share scope state, allowing LoggingTracer to log actionable errors within the active attempt span. The review feedback highlights a potential concurrency issue with hedged requests sharing the same SharedContext instance, suggesting that each attempt should receive a fresh context. Additionally, several redundant null checks on the non-nullable sharedContext() method should be simplified across LoggingTracer and OpenTelemetryTracingTracer.
I am having trouble creating individual review comments. Click here to see my feedback.
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/ApiTracerContext.java (414-416)
Concurrency / Hedging Issue with Shared Context
If concurrent attempts (such as in hedged requests) are executed, they will share the same ApiTracerContext instance if the callers (like BasicRetryingFuture) are untouched. Since toBuilder() preserves the SharedContext reference, concurrent attempts will share the same SharedContext instance, causing them to overwrite each other's active scope providers. This can lead to logs from one attempt being associated with another attempt's span.
To prevent this without modifying the callers, we can ensure that each attempt gets a fresh SharedContext. A great place to do this is in CompositeTracerFactory.newTracer, where we can call context.withNewSharedContext() before passing it to the delegate tracers. This ensures sibling tracers within the same attempt share the same SharedContext, while concurrent attempts remain completely isolated.
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/LoggingTracer.java (101-104)
The null check apiTracerContext.sharedContext() != null is redundant because sharedContext() is non-nullable in ApiTracerContext (it is an abstract property in an @AutoValue class without @Nullable and is initialized to a non-null value by default). We can simplify this expression.
try (ApiTracerContext.Scope scope =
apiTracerContext != null ? apiTracerContext.sharedContext().openAttemptScope() : null) {References
- Avoid adding defensive null checks or redundant null-handling logic for method return values that are guaranteed to be non-null by design, such as when an empty state or failure condition would already throw an exception earlier in the execution path.
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/OpenTelemetryTracingTracer.java (151)
The null check apiTracerContext.sharedContext() != null is redundant because sharedContext() is non-nullable. We can simplify this condition.
if (apiTracerContext != null) {
References
- Avoid adding defensive null checks or redundant null-handling logic for method return values that are guaranteed to be non-null by design, such as when an empty state or failure condition would already throw an exception earlier in the execution path.
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/OpenTelemetryTracingTracer.java (168-170)
The null check apiTracerContext.sharedContext() != null is redundant because sharedContext() is non-nullable. We can simplify this condition.
if (apiTracerContext != null && this.attemptScopeProvider != null) {References
- Avoid adding defensive null checks or redundant null-handling logic for method return values that are guaranteed to be non-null by design, such as when an empty state or failure condition would already throw an exception earlier in the execution path.
…ctory and remove redundant null checks
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to share attempt scope (span context) among sibling ApiTracer instances within the same attempt. This is achieved by adding a SharedContext class to ApiTracerContext and updating LoggingTracer to open the attempt scope when logging actionable errors, ensuring that logs are emitted within the active span context. OpenTelemetryTracingTracer manages setting and clearing this scope provider. Additionally, comprehensive integration and unit tests are added to verify the behavior and prevent span leaks. The feedback suggests explicitly overriding hasAttemptScope() to return false in the EMPTY sentinel of SharedContext for a more robust design.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to share tracing scope state across sibling ApiTracer instances during an attempt, allowing error logs emitted by LoggingTracer to be properly associated with the active OpenTelemetry attempt span. This is achieved by introducing a SharedContext class in ApiTracerContext and updating LoggingTracer and OpenTelemetryTracingTracer accordingly. Feedback on the changes suggests declaring the attemptScopeProvider field in OpenTelemetryTracingTracer as volatile to ensure thread safety and proper visibility across different threads.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to propagate and activate the OpenTelemetry attempt span scope during error logging within the LoggingTracer. It adds a SharedContext to ApiTracerContext to allow sibling tracers (such as LoggingTracer and OpenTelemetryTracingTracer) to share mutable state, specifically the active attempt span's scope provider. This ensures that logs emitted during an attempt are correctly associated with the active span context without leaking the span scope. Comprehensive integration and unit tests have been added to verify this behavior across HTTP/JSON and gRPC transports, as well as to validate the lifecycle and merging of the shared context. No review comments were provided, so there is no additional feedback.
|
|
This branch has not been deployed
1 participant
Prototype for Approach B (Callee-Managed Scoping) evaluated during the 2026-10-01 observability scoping meeting.
In this approach, ApiTracerContext carries a shared context container across tracer instances within the same attempt. OpenTelemetryTracingTracer publishes its active attempt span to this shared context upon start, and LoggingTracer explicitly enters the attempt span's scope when recording actionable error logs. Callers like BasicRetryingFuture and TraceFinisher remain completely untouched.