feat(gax): prototype Approach A caller-managed scoping (tracer.inScope()) by jinseopkim0 · Pull Request #14567 · googleapis/google-cloud-java

jinseopkim0 · 2026-10-01T20:30:47Z

Prototype implementation of Approach A (Caller-Managed Scoping) for OpenTelemetry attempt span propagation during error logging and telemetry callbacks.

In this approach, the caller lifecycle wrapper (BasicRetryingFuture) activates the attempt span context via tracer.inScope() during attempt completion handling. This ensures that active trace and span IDs are present in MDC/Logback during LoggingTracer failure logging and accessible across sibling tracers.

…e())

gemini-code-assist

Code Review

This pull request ensures that the active tracing span is correctly set in scope during retry attempts and callback executions (such as in BasicRetryingFuture and TraceFinisher), allowing logging frameworks to capture the correct span context. It implements the inScope() method in OpenTelemetryTracingTracer and adds comprehensive integration and unit tests to verify span context capture and prevent span leaks. Feedback on the changes suggests declaring the attemptSpan field as volatile in OpenTelemetryTracingTracer to guarantee proper visibility across threads.

… thread safety

jinseopkim0 · 2026-10-02T15:25:08Z

@gemini-code-assist review

gemini-code-assist

Code Review

This pull request introduces span context propagation during retry attempts and operation completions by implementing and utilizing ApiTracer.Scope in BasicRetryingFuture, TraceFinisher, and OpenTelemetryTracingTracer. The feedback highlights a critical check-then-act race condition in OpenTelemetryTracingTracer.inScope() involving the volatile attemptSpan field, suggesting a local variable copy to prevent a potential NullPointerException. Additionally, the reviewer notes that wrapping operations in TraceFinisher with inScope() is redundant or ineffective, recommending its removal.

…nisher inScope

jinseopkim0 · 2026-10-02T16:00:46Z

@gemini-code-assist review

gemini-code-assist

Code Review

This pull request activates the attempt span context during attempt completion handling by wrapping the execution in BasicRetryingFuture with tracer.inScope(). It also implements the inScope() method in OpenTelemetryTracingTracer to set the current attempt span as the active OpenTelemetry context, and adds corresponding tests to verify this behavior and prevent span leaks. Feedback points out that TraceFinisher.java was not updated as mentioned in the PR description, which may result in missing trace/span IDs for non-retried calls.

jinseopkim0 · 2026-10-02T16:05:26Z

@gemini-code-assist review

gemini-code-assist

Code Review

This pull request ensures that the current attempt span is installed as the active OpenTelemetry context on the calling thread during attempt handling (including error logging) by wrapping the attempt handling logic in BasicRetryingFuture with tracer.inScope(). It also implements the inScope() method in OpenTelemetryTracingTracer and adds integration and unit tests to verify span context capture and prevent span leaks. Feedback on the pull request suggests using CopyOnWriteArrayList instead of ArrayList in TestAppender to ensure thread safety, as log events can be appended from background threads while the main test thread reads or clears the lists.

jinseopkim0 · 2026-10-02T16:14:05Z

@gemini-code-assist review

gemini-code-assist

Code Review

This pull request introduces changes to propagate the current attempt span context into the active scope during retry attempts, allowing telemetry and logging to capture the correct trace and span IDs. It implements inScope() in OpenTelemetryTracingTracer and wraps the handleAttempt logic in BasicRetryingFuture with this scope. However, feedback points out that wrapping the entire handleAttempt method (including future completion calls like super.set and super.setException) can cause the attempt span context to leak into downstream user-defined callbacks. It is recommended to restrict the scope activation only to the specific telemetry and logging calls.

…xt to future listeners

jinseopkim0 · 2026-10-02T16:19:57Z

@gemini-code-assist review

jinseopkim0 · 2026-10-02T16:24:29Z

@gemini-code-assist review

gemini-code-assist

Code Review

This pull request implements the inScope() method in OpenTelemetryTracingTracer to activate the current attempt span in the OpenTelemetry context during retry attempts. It updates BasicRetryingFuture to wrap tracer lifecycle events (such as failures, cancellations, and successes) within the tracer's scope, ensuring correct context propagation. Additionally, it adds comprehensive unit and integration tests to verify that the span context is correctly captured during error logging and that no span leaks occur across threads. There are no review comments, so I have no feedback to provide.

gemini-code-assist

Code Review

This pull request ensures that the ApiTracer is properly in scope during retry attempts by implementing the inScope() method in OpenTelemetryTracingTracer and wrapping attempt callbacks in BasicRetryingFuture within try-with-resources blocks. It also adds corresponding unit and integration tests. The review feedback identifies a potential NullPointerException if tracer.inScope() returns null (e.g., with mock tracers in downstream tests) and suggests a null-safe fallback. Additionally, the feedback recommends refactoring the integration tests to avoid relying on the last element of the captured span contexts list, which can lead to flaky tests.

gemini-code-assist · 2026-10-02T16:27:41Z

+          try (ApiTracer.Scope ignored = tracer.inScope()) {
+            tracer.attemptCancelled();
+          }


Calling tracer.inScope() directly in a try-with-resources block can lead to a NullPointerException if the tracer is a mock (e.g., in downstream user tests) or a custom implementation that returns null. To ensure robust backward compatibility and prevent test failures when upgrading the library, we should defensively handle potential null returns from inScope(). Consider using a local variable and a null-safe fallback, or defining a private helper method in BasicRetryingFuture to wrap tracer.inScope() safely.

Suggested change

try (ApiTracer.Scope ignored = tracer.inScope()) {

tracer.attemptCancelled();

}

ApiTracer.Scope scope = tracer.inScope();

try (ApiTracer.Scope ignored = scope != null ? scope : () -> {}) {

tracer.attemptCancelled();

}

gemini-code-assist · 2026-10-02T16:27:41Z

+      io.opentelemetry.api.trace.SpanContext capturedContext =
+          testAppender.eventSpanContexts.get(testAppender.eventSpanContexts.size() - 1);


Relying on the last element of testAppender.eventSpanContexts can be fragile and lead to flaky tests if any unexpected background or cleanup logging occurs after the RPC call but before the assertion. A more robust approach is to find the specific log event corresponding to the failure (e.g., by filtering for a valid span context or matching the error message) and asserting on its associated span context.

Suggested change

io.opentelemetry.api.trace.SpanContext capturedContext =

testAppender.eventSpanContexts.get(testAppender.eventSpanContexts.size() - 1);

io.opentelemetry.api.trace.SpanContext capturedContext =

testAppender.eventSpanContexts.stream()

.filter(io.opentelemetry.api.trace.SpanContext::isValid)

.findFirst()

.orElse(io.opentelemetry.api.trace.SpanContext.getInvalid());

gemini-code-assist · 2026-10-02T16:27:41Z

+      io.opentelemetry.api.trace.SpanContext capturedContext =
+          testAppender.eventSpanContexts.get(testAppender.eventSpanContexts.size() - 1);


Relying on the last element of testAppender.eventSpanContexts can be fragile and lead to flaky tests if any unexpected background or cleanup logging occurs after the RPC call but before the assertion. A more robust approach is to find the specific log event corresponding to the failure (e.g., by filtering for a valid span context or matching the error message) and asserting on its associated span context.

Suggested change

io.opentelemetry.api.trace.SpanContext capturedContext =

testAppender.eventSpanContexts.get(testAppender.eventSpanContexts.size() - 1);

io.opentelemetry.api.trace.SpanContext capturedContext =

testAppender.eventSpanContexts.stream()

.filter(io.opentelemetry.api.trace.SpanContext::isValid)

.findFirst()

.orElse(io.opentelemetry.api.trace.SpanContext.getInvalid());

sonarqubecloud · 2026-10-02T16:29:56Z

Quality Gate passed for 'gapic-generator-java-root'

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

sonarqubecloud · 2026-10-02T16:31:50Z

Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

feat(gax): prototype Approach A caller-managed scoping (tracer.inScop…

61dc2eb

…e())

gemini-code-assist Bot reviewed Oct 1, 2026

View reviewed changes

Comment thread ...m-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/OpenTelemetryTracingTracer.java Outdated

fix(gax): make attemptSpan volatile in OpenTelemetryTracingTracer for…

18e14b3

… thread safety