feat(gax): add T3 client request operation spans to OpenTelemetryTracingTracer

[jinseopkim0]

This PR adds T3 client request operation spans to OpenTelemetryTracingTracer, establishing the logical operation span hierarchy and linking individual attempt spans as children.

Fixes b/564460690

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces support for tracking overall operations in OpenTelemetryTracingTracer by adding an operationSpan alongside the existing attemptSpan. It implements the lifecycle methods for operations (operationSucceeded, operationCancelled, and operationFailed), ensures attempt spans are correctly parented to the operation span, and updates trace context injection and scope management to fall back to the operation span when no attempt span is active. Additionally, corresponding unit tests have been added to verify these new behaviors. There are no review comments to address, and I have no further feedback to provide.

[gemini-code-assist]

Code Review

This pull request introduces tracking for an overall operation span in OpenTelemetryTracingTracer, establishing a parent-child relationship with individual attempt spans. It implements lifecycle methods for operations (succeeded, cancelled, failed) and updates trace context injection and scope management to fall back to the operation span when no attempt span is active. Corresponding unit tests are updated and added to verify these behaviors. The review feedback suggests adding a unit test to verify that ending an operation also correctly ends any active attempt span.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces support for an operation-level span in OpenTelemetryTracingTracer, establishing a parent-child relationship where attempt spans are nested under the operation span. It also implements lifecycle methods for the operation span and updates the corresponding unit tests to verify these behaviors. The review feedback correctly identifies an issue in recordErrorAndEndOperation, where calling endAttempt() directly fails to record the error on the active attempt span; updating this to recordErrorAndEndAttempt(error) is recommended to ensure proper error propagation.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces parent operation spans to the OpenTelemetryTracingTracer to trace overall operations in addition to individual attempts, implementing lifecycle methods for operations and updating corresponding unit tests. Feedback suggests improving resolveOperationSpanName to return "operation" instead of "attempt" when the attempt span name defaults to "attempt", preventing redundant and confusing span names in traces.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces an operationSpan to OpenTelemetryTracingTracer to track the overall lifecycle of an operation alongside individual attempt spans, updating trace context injection, scope management, and error recording accordingly. Feedback on these changes suggests guarding against empty span names when resolving the operation span name from "/attempt", and wrapping the attempt span cleanup in a try-finally block within recordErrorAndEndOperation to ensure exception safety and prevent resource leaks.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces operation-level tracing spans (operationSpan) alongside attempt-level spans in OpenTelemetryTracingTracer, managing their lifecycle and linking attempt spans to their parent operation span. The review feedback suggests simplifying the resolveOperationSpanName method by deriving the operation span name directly from the already resolved attemptSpanName, which reduces redundant checks and code duplication.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces operation span tracking to OpenTelemetryTracingTracer in gax-java, establishing a parent-child relationship where the overall operation span acts as the parent to individual attempt spans. It also updates context injection and scope management to fall back to the operation span when no attempt span is active, and adds corresponding unit tests. The review feedback highlights two important improvements: marking the mutable operationSpan and attemptSpan fields as volatile to ensure thread-safe visibility across threads, and refactoring resolveAttemptSpanName to append "/attempt" so that parent and child spans have distinct names in tracing visualization tools.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces an operationSpan to OpenTelemetryTracingTracer to track the overall operation lifecycle alongside individual attempt spans, updating trace context injection, scope management, and adding corresponding unit tests. The review feedback correctly identifies potential race conditions where volatile fields (attemptSpan and operationSpan) are read multiple times within single expressions or across try-finally blocks, and recommends copying these fields to local variables first to ensure thread-safe and consistent access.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces tracking for an overall operation span alongside individual attempt spans in OpenTelemetryTracingTracer, including fallback logic for context injection and scope management. Unit tests have been updated and expanded to cover the new operation lifecycle. A review comment correctly identifies a potential issue in resolveAttemptSpanName where an empty method name could bypass the null check and lead to an invalid span name, suggesting the use of Strings.isNullOrEmpty instead.

[jinseopkim0]

@gemini-code-assist review

[jinseopkim0]

@gemini-code-assist review

[jinseopkim0]

@gemini-code-assist review

[jinseopkim0]

@gemini-code-assist review

[sonarqubecloud]

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

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

See analysis details on SonarQube Cloud

[gemini-code-assist]

Code Review

This pull request introduces an operation-level span (operationSpan) to OpenTelemetryTracingTracer to track the overall lifecycle of an operation alongside individual attempt spans. It updates context injection, scope management, and lifecycle methods to handle the new operation span, and adds corresponding unit tests. The review feedback suggests avoiding the use of the fully qualified class name io.opentelemetry.api.trace.Span in the test file since Span is already imported, which would improve code readability.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces an operationSpan to the OpenTelemetryTracingTracer to track the overall lifecycle of an operation alongside individual attempt spans. It implements the inScope method, updates trace context injection to fallback to the operation span, and manages the operation span's lifecycle via success, cancellation, and failure callbacks. Comprehensive unit tests have been added to verify these changes. A review comment highlights a potential race condition in recordErrorAndEndOperation due to non-atomic operations on volatile fields, suggesting that operationSpan be set to null immediately after the null check to prevent double-ending the span.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces operation-level tracing alongside attempt-level tracing in OpenTelemetryTracingTracer by adding an operationSpan that acts as a parent to individual attempt spans. It also updates context injection and scope management to fall back to the operation span when no attempt span is active, and includes comprehensive unit tests for these new lifecycles. The review feedback suggests a thread-safety improvement in attemptStarted to assign the volatile operationSpan to a local variable before checking and using it, preventing potential race conditions.

[jinseopkim0]

@gemini-code-assist review

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces an operation-level span (operationSpan) to OpenTelemetryTracingTracer to wrap individual attempt spans, establishing a parent-child relationship between them and improving tracing resolution. It also implements operation lifecycle methods (operationSucceeded, operationCancelled, operationFailed) and updates the corresponding unit tests. The review feedback suggests capturing the OpenTelemetry Context at the tracer's creation time to ensure robust context propagation in asynchronous environments, and using explicit locks to prevent race conditions during concurrent cancellations and attempt completions.

[gemini-code-assist]

To ensure robust context propagation in asynchronous environments, we should capture the Context at the time the tracer is created (on the initiating thread) and use it as the parent context for all attempts. This prevents losing context (like Baggage or other trace correlation keys) when attempts are started on background executor threads where Context.current() is empty or unrelated.

First, let's declare a final io.opentelemetry.context.Context field to store the captured context.

Suggested change

	private final String operationSpanName;
	private final ApiTracerContext apiTracerContext;
	private @Nullable Span attemptSpan;
	private volatile @Nullable Span operationSpan;
	private volatile @Nullable Span attemptSpan;
	private final String operationSpanName;
	private final ApiTracerContext apiTracerContext;
	private final io.opentelemetry.context.Context operationContext;
	private volatile @Nullable Span operationSpan;
	private volatile @Nullable Span attemptSpan;

[gemini-code-assist]

Initialize the captured operationContext in the constructor using the current context combined with the newly started operationSpan.

Suggested change

	this.attemptSpanName = attemptSpanName;
	this.operationSpanName = resolveOperationSpanName(attemptSpanName);
	this.attemptAttributes = new HashMap<>();
	buildAttributes();
	startOperationSpan();
	}
	this.attemptSpanName = attemptSpanName;
	this.operationSpanName = resolveOperationSpanName(attemptSpanName);
	this.attemptAttributes = new HashMap<>();
	buildAttributes();
	startOperationSpan();
	this.operationContext = io.opentelemetry.context.Context.current().with(this.operationSpan);
	}

[gemini-code-assist]

Use the captured operationContext as the parent context for the attempt span instead of relying on Context.current() at the time the attempt starts, which may be executed on a background thread with an empty or unrelated context.

Suggested change

	Span localOperationSpan = operationSpan;
	if (localOperationSpan != null) {
	spanBuilder.setParent(io.opentelemetry.context.Context.current().with(localOperationSpan));
	if (operationSpan != null) {
	spanBuilder.setParent(operationContext);
	}

[gemini-code-assist]

To prevent race conditions between concurrent cancellation (e.g., from a timeout thread) and attempt completion (on the client thread), we should protect these operations. Since this is performance-sensitive tracing code, prefer using an explicit lock (such as ReentrantLock) over the synchronized keyword to protect shared state while ensuring thread safety and visibility.

References

1. In performance-sensitive code, prefer using explicit locks over the 'synchronized' keyword to protect shared state while ensuring thread safety and visibility.

[gemini-code-assist]

Code Review

This pull request introduces operation-level span tracking to OpenTelemetryTracingTracer alongside the existing attempt-level spans. It adds support for starting, ending, and propagating an operation span, and updates the inScope and injectTraceContext methods to fallback to the operation span when no attempt span is active. Corresponding unit tests have been added and updated to verify this new lifecycle. The review feedback suggests improving test code readability by replacing fully qualified org.mockito.Mockito calls with static imports, as Mockito static methods are already imported in the test file.

[gemini-code-assist]

Avoid using fully qualified class names like org.mockito.Mockito when there is no class name conflict. Since Mockito's static methods (like when and any) are already statically imported in this file, you can statically import lenient as well to significantly improve readability.

    lenient().when(tracer.spanBuilder(anyString())).thenReturn(spanBuilder);
    lenient()
        .when(spanBuilder.setSpanKind(any(SpanKind.class)))
        .thenReturn(spanBuilder);
    lenient().when(spanBuilder.setParent(any())).thenReturn(spanBuilder);
    lenient()
        .when(spanBuilder.setAllAttributes(any(Attributes.class)))
        .thenReturn(spanBuilder);
    lenient().when(spanBuilder.startSpan()).thenReturn(span);

    lenient()
        .when(operationSpanBuilder.setSpanKind(any(SpanKind.class)))
        .thenReturn(operationSpanBuilder);
    lenient()
        .when(operationSpanBuilder.setAllAttributes(any(Attributes.class)))
        .thenReturn(operationSpanBuilder);
    lenient().when(operationSpanBuilder.startSpan()).thenReturn(operationSpan);
    lenient()
        .when(operationSpan.storeInContext(any(io.opentelemetry.context.Context.class)))
        .thenAnswer(invocation -> invocation.getArgument(0));
    lenient()
        .when(tracer.spanBuilder("Service/Method"))
        .thenReturn(operationSpanBuilder);

References

1. Do not use fully qualified class names if there is no class name conflict in the file and the class is already imported, as it unnecessarily reduces code readability.

[gemini-code-assist]

Avoid using fully qualified class names like org.mockito.Mockito when there is no class name conflict. Statically importing mock and when makes the test code much cleaner and easier to read.

Suggested change

	io.opentelemetry.context.Scope mockScope =
	org.mockito.Mockito.mock(io.opentelemetry.context.Scope.class);
	org.mockito.Mockito.when(span.makeCurrent()).thenReturn(mockScope);
	io.opentelemetry.context.Scope mockScope =
	mock(io.opentelemetry.context.Scope.class);
	when(span.makeCurrent()).thenReturn(mockScope);

References

1. Do not use fully qualified class names if there is no class name conflict in the file and the class is already imported, as it unnecessarily reduces code readability.

[gemini-code-assist]

Avoid using fully qualified class names like org.mockito.Mockito when there is no class name conflict. Statically importing mock and when makes the test code much cleaner and easier to read.

Suggested change

	io.opentelemetry.context.Scope mockScope =
	org.mockito.Mockito.mock(io.opentelemetry.context.Scope.class);
	org.mockito.Mockito.when(operationSpan.makeCurrent()).thenReturn(mockScope);
	io.opentelemetry.context.Scope mockScope =
	mock(io.opentelemetry.context.Scope.class);
	when(operationSpan.makeCurrent()).thenReturn(mockScope);

References

1. Do not use fully qualified class names if there is no class name conflict in the file and the class is already imported, as it unnecessarily reduces code readability.

[gemini-code-assist]

Avoid using fully qualified class names like org.mockito.Mockito when there is no class name conflict. Statically importing when makes the test code much cleaner and easier to read.

Suggested change

	org.mockito.Mockito.when(operationSpanBuilder.startSpan()).thenReturn(realSpan);
	when(operationSpanBuilder.startSpan()).thenReturn(realSpan);

References

1. Do not use fully qualified class names if there is no class name conflict in the file and the class is already imported, as it unnecessarily reduces code readability.

[sonarqubecloud]

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

Issues
9 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud