Skip to content

fix: Populate method level attributes in metrics recording #4149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blakeli0 merged 5 commits into from
Mar 17, 2026
Merged

fix: Populate method level attributes in metrics recording #4149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
64377a5
fix: Add method level ApiTracerContext to metrics.
blakeli0 Mar 16, 2026
1440456
fix: format.
blakeli0 Mar 16, 2026
427877e
fix: rename tests.
blakeli0 Mar 16, 2026
054f881
fix: Add verify in tests
blakeli0 Mar 17, 2026
eaac7ef
Merge branch 'main' into merge-apitracercontext
blakeli0 Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions gax-java/gax/src/main/java/com/google/api/gax/tracing/GoldenSignalsMetricsTracerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@
@InternalApi
public class GoldenSignalsMetricsTracerFactory implements ApiTracerFactory {

private ApiTracerContext apiTracerContext;
private ApiTracerContext clientLevelTracerContext;
private final OpenTelemetry openTelemetry;
private GoldenSignalsMetricsRecorder metricsRecorder;

public GoldenSignalsMetricsTracerFactory(OpenTelemetry openTelemetry) {
this.openTelemetry = openTelemetry;
this.apiTracerContext = ApiTracerContext.empty();
this.clientLevelTracerContext = ApiTracerContext.empty();
}

@Override
Expand All @@ -58,15 +58,26 @@ public ApiTracer newTracer(ApiTracer parent, SpanName spanName, OperationType op
// regular requests.
return new BaseApiTracer();
}
return new GoldenSignalsMetricsTracer(metricsRecorder, apiTracerContext);
return new GoldenSignalsMetricsTracer(metricsRecorder, clientLevelTracerContext);
}

@Override
public ApiTracer newTracer(ApiTracer parent, ApiTracerContext methodLevelTracerContext) {
if (metricsRecorder == null) {
// This should never happen, in case it happens, create a no-op api tracer to not block
// regular requests.
return new BaseApiTracer();
}
Comment on lines +66 to +70
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This null check for metricsRecorder is duplicated from the other newTracer method (lines 56-59). To improve maintainability and avoid code duplication, consider extracting this logic into a private helper method. For example:

private ApiTracer createTracerWithContext(ApiTracerContext context) {
  if (metricsRecorder == null) {
    // This should never happen...
    return new BaseApiTracer();
  }
  return new GoldenSignalsMetricsTracer(metricsRecorder, context);
}

Then both newTracer methods can call this helper.

ApiTracerContext mergedTracerContext = clientLevelTracerContext.merge(methodLevelTracerContext);
return new GoldenSignalsMetricsTracer(metricsRecorder, mergedTracerContext);
}

@Override
public ApiTracerFactory withContext(ApiTracerContext context) {
this.apiTracerContext = context;
this.clientLevelTracerContext = context;
this.metricsRecorder =
new GoldenSignalsMetricsRecorder(
openTelemetry, apiTracerContext.libraryMetadata().artifactName());
openTelemetry, clientLevelTracerContext.libraryMetadata().artifactName());
return this;
}
}
29 changes: 26 additions & 3 deletions ...a/gax/src/test/java/com/google/api/gax/tracing/GoldenSignalsMetricsTracerFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
package com.google.api.gax.tracing;

import static com.google.common.truth.Truth.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Answers.RETURNS_DEEP_STUBS;
import static org.mockito.Mockito.*;

import io.opentelemetry.api.OpenTelemetry;
import org.junit.jupiter.api.BeforeEach;
Expand All @@ -46,7 +47,7 @@ void setUp() {
}

@Test
void newTracer_createsTracer_successfully() {
void newTracerWithSpanName_shouldCreateTracer_ifMetricsRecorderIsNotNull() {
tracerFactory.withContext(ApiTracerContext.empty());
ApiTracer actual =
tracerFactory.newTracer(
Expand All @@ -55,10 +56,32 @@ void newTracer_createsTracer_successfully() {
}

@Test
void newTracer_createsBaseTracer_ifMetricsRecorderIsNull() {
void newTracerWithSpanName_shouldCreateBaseTracer_ifMetricsRecorderIsNull() {
ApiTracer actual =
tracerFactory.newTracer(
mock(ApiTracer.class), mock(SpanName.class), ApiTracerFactory.OperationType.Unary);
assertThat(actual).isInstanceOf(BaseApiTracer.class);
}

@Test
void newTracerWithApiTracerContext_shouldMergeApiTracerContext() {
ApiTracerContext clientLevelTracerContext = mock(ApiTracerContext.class, RETURNS_DEEP_STUBS);
ApiTracerContext methodLevelTracerContext = mock(ApiTracerContext.class);
when(clientLevelTracerContext.libraryMetadata().artifactName()).thenReturn("does not matter");
when(clientLevelTracerContext.merge(methodLevelTracerContext))
.thenReturn(clientLevelTracerContext);

tracerFactory.withContext(clientLevelTracerContext);
ApiTracer actual = tracerFactory.newTracer(mock(ApiTracer.class), methodLevelTracerContext);

verify(clientLevelTracerContext).merge(methodLevelTracerContext);
assertThat(actual).isInstanceOf(GoldenSignalsMetricsTracer.class);
Comment on lines +75 to +78
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This test correctly checks that a GoldenSignalsMetricsTracer is created, but it doesn't verify that the merge method was actually called on the clientLevelTracerContext. To make the test more robust, you should add a verification step using Mockito.verify().

You'll also need to add the following import:
import static org.mockito.Mockito.verify;

    ApiTracer actual = tracerFactory.newTracer(mock(ApiTracer.class), methodLevelTracerContext);

    verify(clientLevelTracerContext).merge(methodLevelTracerContext);
    assertThat(actual).isInstanceOf(GoldenSignalsMetricsTracer.class);

}

@Test
void newTracerWithApiTracerContext_shouldCreateBaseTracer_ifMetricsRecorderIsNull() {
ApiTracer actual = tracerFactory.newTracer(mock(ApiTracer.class), mock(ApiTracerContext.class));

assertThat(actual).isInstanceOf(BaseApiTracer.class);
}
}
Loading