fix(gax): unconditionally wrap logging tracer

westarle · westarle · commit bc74103aaf84 · 2026-03-23T12:07:40.000-04:00
diff --git a/gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java b/gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java
@@ -278,10 +278,8 @@ public static ClientContext create(StubSettings settings) throws IOException {
             .setLibraryMetadata(settings.getLibraryMetadata())
             .build();
     ApiTracerFactory apiTracerFactory = settings.getTracerFactory();
-    if (apiTracerFactory instanceof SpanTracerFactory) {
-      apiTracerFactory = apiTracerFactory.withContext(apiTracerContext);
-    }
-    if (com.google.api.gax.logging.LoggingUtils.isLoggingEnabled()) {
+    apiTracerFactory = apiTracerFactory.withContext(apiTracerContext);
+    if (!(apiTracerFactory instanceof com.google.api.gax.tracing.CompositeTracerFactory)) {
       com.google.api.gax.tracing.ApiTracerFactory loggingTracerFactory =
           new com.google.api.gax.tracing.LoggingTracerFactory().withContext(apiTracerContext);
       apiTracerFactory =
diff --git a/gax-java/gax/src/main/java/com/google/api/gax/tracing/CompositeTracer.java b/gax-java/gax/src/main/java/com/google/api/gax/tracing/CompositeTracer.java
@@ -44,6 +44,10 @@ public CompositeTracer(List<ApiTracer> tracers) {
     this.tracers = tracers;
   }
 
+  public List<ApiTracer> getTracers() {
+    return tracers;
+  }
+
   @Override
   public Scope inScope() {
     // Returning a scope that closes all sub-scopes
@@ -52,7 +56,8 @@ public Scope inScope() {
       scopes[i] = tracers.get(i).inScope();
     }
     return () -> {
-      for (Scope scope : scopes) {
+      for (int i = scopes.length - 1; i >= 0; i--) {
+        Scope scope = scopes[i];
         if (scope != null) {
           scope.close();
         }
@@ -62,22 +67,22 @@ public Scope inScope() {
 
   @Override
   public void operationSucceeded() {
-    for (ApiTracer tracer : tracers) {
-      tracer.operationSucceeded();
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).operationSucceeded();
     }
   }
 
   @Override
   public void operationCancelled() {
-    for (ApiTracer tracer : tracers) {
-      tracer.operationCancelled();
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).operationCancelled();
     }
   }
 
   @Override
   public void operationFailed(Throwable error) {
-    for (ApiTracer tracer : tracers) {
-      tracer.operationFailed(error);
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).operationFailed(error);
     }
   }
 
@@ -105,65 +110,65 @@ public void attemptStarted(Object request, int attemptNumber) {
 
   @Override
   public void attemptSucceeded() {
-    for (ApiTracer tracer : tracers) {
-      tracer.attemptSucceeded();
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).attemptSucceeded();
     }
   }
 
   @Override
   public void attemptCancelled() {
-    for (ApiTracer tracer : tracers) {
-      tracer.attemptCancelled();
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).attemptCancelled();
     }
   }
 
   @Override
   @SuppressWarnings("deprecation")
   public void attemptFailed(Throwable error, org.threeten.bp.Duration delay) {
-    for (ApiTracer tracer : tracers) {
-      tracer.attemptFailed(error, delay);
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).attemptFailed(error, delay);
     }
   }
 
   @Override
   public void attemptFailedDuration(Throwable error, java.time.Duration delay) {
-    for (ApiTracer tracer : tracers) {
-      tracer.attemptFailedDuration(error, delay);
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).attemptFailedDuration(error, delay);
     }
   }
 
   @Override
   public void attemptFailedRetriesExhausted(Throwable error) {
-    for (ApiTracer tracer : tracers) {
-      tracer.attemptFailedRetriesExhausted(error);
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).attemptFailedRetriesExhausted(error);
     }
   }
 
   @Override
   public void attemptPermanentFailure(Throwable error) {
-    for (ApiTracer tracer : tracers) {
-      tracer.attemptPermanentFailure(error);
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).attemptPermanentFailure(error);
     }
   }
 
   @Override
   public void lroStartFailed(Throwable error) {
-    for (ApiTracer tracer : tracers) {
-      tracer.lroStartFailed(error);
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).lroStartFailed(error);
     }
   }
 
   @Override
   public void lroStartSucceeded() {
-    for (ApiTracer tracer : tracers) {
-      tracer.lroStartSucceeded();
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).lroStartSucceeded();
     }
   }
 
   @Override
   public void responseReceived() {
-    for (ApiTracer tracer : tracers) {
-      tracer.responseReceived();
+    for (int i = tracers.size() - 1; i >= 0; i--) {
+      tracers.get(i).responseReceived();
     }
   }
 
diff --git a/gax-java/gax/src/main/java/com/google/api/gax/tracing/CompositeTracerFactory.java b/gax-java/gax/src/main/java/com/google/api/gax/tracing/CompositeTracerFactory.java
@@ -63,21 +63,33 @@ private CompositeTracerFactory(
   @Override
   public ApiTracer newTracer(ApiTracer parent, SpanName spanName, OperationType operationType) {
     List<ApiTracer> tracers = new ArrayList<>(factories.size());
-    for (ApiTracerFactory factory : factories) {
-      tracers.add(factory.newTracer(parent, spanName, operationType));
+    for (int i = 0; i < factories.size(); i++) {
+      ApiTracer subParent = getSubParent(parent, i);
+      tracers.add(factories.get(i).newTracer(subParent, spanName, operationType));
     }
     return new CompositeTracer(tracers);
   }
 
   @Override
   public ApiTracer newTracer(ApiTracer parent, ApiTracerContext context) {
     List<ApiTracer> tracers = new ArrayList<>(factories.size());
-    for (ApiTracerFactory factory : factories) {
-      tracers.add(factory.newTracer(parent, context));
+    for (int i = 0; i < factories.size(); i++) {
+      ApiTracer subParent = getSubParent(parent, i);
+      tracers.add(factories.get(i).newTracer(subParent, context));
     }
     return new CompositeTracer(tracers);
   }
 
+  private ApiTracer getSubParent(ApiTracer parent, int index) {
+    if (parent instanceof CompositeTracer) {
+      CompositeTracer compositeParent = (CompositeTracer) parent;
+      if (index < compositeParent.getTracers().size()) {
+        return compositeParent.getTracers().get(index);
+      }
+    }
+    return parent;
+  }
+
   @Override
   public ApiTracerContext getApiTracerContext() {
     return apiTracerContext;
diff --git a/gax-java/gax/src/main/java/com/google/api/gax/tracing/LoggingTracer.java b/gax-java/gax/src/main/java/com/google/api/gax/tracing/LoggingTracer.java
@@ -116,7 +116,10 @@ private void recordActionableError(Throwable error) {
       }
     }
 
-    String message = error.getMessage() != null ? error.getMessage() : error.getClass().getName();
+    String message = "Unknown Error";
+    if (error != null) {
+      message = error.getMessage() != null ? error.getMessage() : error.getClass().getName();
+    }
     LoggingUtils.logActionableError(logContext, LOGGER_PROVIDER, message);
   }
 }
diff --git a/gax-java/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java b/gax-java/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java
@@ -1304,7 +1304,8 @@ void testCreate_withTracerFactoryReturningNullWithContext() throws IOException {
     Mockito.doReturn(apiTracerFactory).when(settings).getTracerFactory();
 
     ClientContext context = ClientContext.create(settings);
-    assertThat(context.getTracerFactory()).isSameInstanceAs(apiTracerFactory);
+    assertThat(context.getTracerFactory())
+        .isInstanceOf(com.google.api.gax.tracing.CompositeTracerFactory.class);
     verify(apiTracerFactory, times(1)).withContext(Mockito.any());
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -44,6 +44,10 @@ public CompositeTracer(List<ApiTracer> tracers) {`
`44`	`44`	`this.tracers = tracers;`
`45`	`45`	`}`
`46`	`46`
	`47`	`+ public List<ApiTracer> getTracers() {`
	`48`	`+ return tracers;`
	`49`	`+ }`
	`50`	`+`
`47`	`51`	`@Override`
`48`	`52`	`public Scope inScope() {`
`49`	`53`	`// Returning a scope that closes all sub-scopes`
`@@ -52,7 +56,8 @@ public Scope inScope() {`
`52`	`56`	`scopes[i] = tracers.get(i).inScope();`
`53`	`57`	`}`
`54`	`58`	`return () -> {`
`55`		`- for (Scope scope : scopes) {`
	`59`	`+ for (int i = scopes.length - 1; i >= 0; i--) {`
	`60`	`+ Scope scope = scopes[i];`
`56`	`61`	`if (scope != null) {`
`57`	`62`	`scope.close();`
`58`	`63`	`}`
`@@ -62,22 +67,22 @@ public Scope inScope() {`
`62`	`67`
`63`	`68`	`@Override`
`64`	`69`	`public void operationSucceeded() {`
`65`		`- for (ApiTracer tracer : tracers) {`
`66`		`- tracer.operationSucceeded();`
	`70`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`71`	`+ tracers.get(i).operationSucceeded();`
`67`	`72`	`}`
`68`	`73`	`}`
`69`	`74`
`70`	`75`	`@Override`
`71`	`76`	`public void operationCancelled() {`
`72`		`- for (ApiTracer tracer : tracers) {`
`73`		`- tracer.operationCancelled();`
	`77`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`78`	`+ tracers.get(i).operationCancelled();`
`74`	`79`	`}`
`75`	`80`	`}`
`76`	`81`
`77`	`82`	`@Override`
`78`	`83`	`public void operationFailed(Throwable error) {`
`79`		`- for (ApiTracer tracer : tracers) {`
`80`		`- tracer.operationFailed(error);`
	`84`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`85`	`+ tracers.get(i).operationFailed(error);`
`81`	`86`	`}`
`82`	`87`	`}`
`83`	`88`
`@@ -105,65 +110,65 @@ public void attemptStarted(Object request, int attemptNumber) {`
`105`	`110`
`106`	`111`	`@Override`
`107`	`112`	`public void attemptSucceeded() {`
`108`		`- for (ApiTracer tracer : tracers) {`
`109`		`- tracer.attemptSucceeded();`
	`113`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`114`	`+ tracers.get(i).attemptSucceeded();`
`110`	`115`	`}`
`111`	`116`	`}`
`112`	`117`
`113`	`118`	`@Override`
`114`	`119`	`public void attemptCancelled() {`
`115`		`- for (ApiTracer tracer : tracers) {`
`116`		`- tracer.attemptCancelled();`
	`120`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`121`	`+ tracers.get(i).attemptCancelled();`
`117`	`122`	`}`
`118`	`123`	`}`
`119`	`124`
`120`	`125`	`@Override`
`121`	`126`	`@SuppressWarnings("deprecation")`
`122`	`127`	`public void attemptFailed(Throwable error, org.threeten.bp.Duration delay) {`
`123`		`- for (ApiTracer tracer : tracers) {`
`124`		`- tracer.attemptFailed(error, delay);`
	`128`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`129`	`+ tracers.get(i).attemptFailed(error, delay);`
`125`	`130`	`}`
`126`	`131`	`}`
`127`	`132`
`128`	`133`	`@Override`
`129`	`134`	`public void attemptFailedDuration(Throwable error, java.time.Duration delay) {`
`130`		`- for (ApiTracer tracer : tracers) {`
`131`		`- tracer.attemptFailedDuration(error, delay);`
	`135`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`136`	`+ tracers.get(i).attemptFailedDuration(error, delay);`
`132`	`137`	`}`
`133`	`138`	`}`
`134`	`139`
`135`	`140`	`@Override`
`136`	`141`	`public void attemptFailedRetriesExhausted(Throwable error) {`
`137`		`- for (ApiTracer tracer : tracers) {`
`138`		`- tracer.attemptFailedRetriesExhausted(error);`
	`142`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`143`	`+ tracers.get(i).attemptFailedRetriesExhausted(error);`
`139`	`144`	`}`
`140`	`145`	`}`
`141`	`146`
`142`	`147`	`@Override`
`143`	`148`	`public void attemptPermanentFailure(Throwable error) {`
`144`		`- for (ApiTracer tracer : tracers) {`
`145`		`- tracer.attemptPermanentFailure(error);`
	`149`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`150`	`+ tracers.get(i).attemptPermanentFailure(error);`
`146`	`151`	`}`
`147`	`152`	`}`
`148`	`153`
`149`	`154`	`@Override`
`150`	`155`	`public void lroStartFailed(Throwable error) {`
`151`		`- for (ApiTracer tracer : tracers) {`
`152`		`- tracer.lroStartFailed(error);`
	`156`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`157`	`+ tracers.get(i).lroStartFailed(error);`
`153`	`158`	`}`
`154`	`159`	`}`
`155`	`160`
`156`	`161`	`@Override`
`157`	`162`	`public void lroStartSucceeded() {`
`158`		`- for (ApiTracer tracer : tracers) {`
`159`		`- tracer.lroStartSucceeded();`
	`163`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`164`	`+ tracers.get(i).lroStartSucceeded();`
`160`	`165`	`}`
`161`	`166`	`}`
`162`	`167`
`163`	`168`	`@Override`
`164`	`169`	`public void responseReceived() {`
`165`		`- for (ApiTracer tracer : tracers) {`
`166`		`- tracer.responseReceived();`
	`170`	`+ for (int i = tracers.size() - 1; i >= 0; i--) {`
	`171`	`+ tracers.get(i).responseReceived();`
`167`	`172`	`}`
`168`	`173`	`}`
`169`	`174`
Original file line number	Diff line number	Diff line change
`@@ -116,7 +116,10 @@ private void recordActionableError(Throwable error) {`
`116`	`116`	`}`
`117`	`117`	`}`
`118`	`118`
`119`		`- String message = error.getMessage() != null ? error.getMessage() : error.getClass().getName();`
	`119`	`+ String message = "Unknown Error";`
	`120`	`+ if (error != null) {`
	`121`	`+ message = error.getMessage() != null ? error.getMessage() : error.getClass().getName();`
	`122`	`+ }`
`120`	`123`	`LoggingUtils.logActionableError(logContext, LOGGER_PROVIDER, message);`
`121`	`124`	`}`
`122`	`125`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1304,7 +1304,8 @@ void testCreate_withTracerFactoryReturningNullWithContext() throws IOException {`
`1304`	`1304`	`Mockito.doReturn(apiTracerFactory).when(settings).getTracerFactory();`
`1305`	`1305`
`1306`	`1306`	`ClientContext context = ClientContext.create(settings);`
`1307`		`- assertThat(context.getTracerFactory()).isSameInstanceAs(apiTracerFactory);`
	`1307`	`+ assertThat(context.getTracerFactory())`
	`1308`	`+ .isInstanceOf(com.google.api.gax.tracing.CompositeTracerFactory.class);`
`1308`	`1309`	`verify(apiTracerFactory, times(1)).withContext(Mockito.any());`
`1309`	`1310`	`}`
`1310`	`1311`	`}`