Code review sweep (run 24960889682) (#18314)

otelbot[bot] · trask · web-flow · commit 327c49684e8a · 2026-04-26T20:00:00.000Z
Co-authored-by: otelbot &lt;197425009+otelbot@users.noreply.github.com&gt;
Co-authored-by: Trask Stalnaker &lt;trask.stalnaker@gmail.com&gt;
diff --git a/.github/agents/knowledge/testing-general-patterns.md b/.github/agents/knowledge/testing-general-patterns.md
@@ -37,6 +37,11 @@
   wrapping when the lambda already needs its own error handling for behavioral reasons.
 - Do **not** choose `CompletableFuture.runAsync(...)` over the simpler
   `executor.submit(runnable).get()` just to avoid the checked exceptions thrown by `Future.get()`.
+- Do **not** wrap a call in `assertThatCode(() -> ...).doesNotThrowAnyException()`
+  solely to narrow a test method's `throws` clause by swallowing checked exceptions
+  into an `AssertionError`. If the call throws, the test already fails via its
+  `throws` clause. Prefer calling the method directly and leaving `throws Exception`
+  (or the narrowest checked type) on the `@Test` method.
 
 ## Test Resource Cleanup
 
diff --git a/instrumentation/elasticsearch/elasticsearch-transport-6.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/transport/v6_0/AbstractElasticsearch6NodeClientTest.java b/instrumentation/elasticsearch/elasticsearch-transport-6.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/transport/v6_0/AbstractElasticsearch6NodeClientTest.java
@@ -15,7 +15,6 @@
 import org.elasticsearch.client.Client;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.node.Node;
-import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
@@ -45,6 +44,7 @@ void setUp(@TempDir File esWorkingDir) {
             .put("discovery.type", "single-node")
             .build();
     testNode = getNodeFactory().newNode(settings);
+    cleanup.deferAfterAll(testNode);
     startNode(testNode);
 
     client = testNode.client();
@@ -75,11 +75,6 @@ void setUp(@TempDir File esWorkingDir) {
     testing.clearData();
   }
 
-  @AfterAll
-  void cleanUp() throws Exception {
-    testNode.close();
-  }
-
   protected abstract NodeFactory getNodeFactory();
 
   @Override
diff --git a/instrumentation/elasticsearch/elasticsearch-transport-6.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/transport/v6_0/AbstractElasticsearch6TransportClientTest.java b/instrumentation/elasticsearch/elasticsearch-transport-6.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/transport/v6_0/AbstractElasticsearch6TransportClientTest.java
@@ -19,7 +19,6 @@
 import org.elasticsearch.node.Node;
 import org.elasticsearch.transport.TransportService;
 import org.elasticsearch.transport.client.PreBuiltTransportClient;
-import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.io.TempDir;
 import org.slf4j.Logger;
@@ -46,6 +45,7 @@ void setUp(@TempDir File esWorkingDir) {
             .put("discovery.type", "single-node")
             .build();
     testNode = getNodeFactory().newNode(settings);
+    cleanup.deferAfterAll(testNode);
     startNode(testNode);
 
     tcpPublishAddress =
@@ -59,6 +59,7 @@ void setUp(@TempDir File esWorkingDir) {
                 .put("thread_pool.listener.size", 1)
                 .put(CLUSTER_NAME_SETTING.getKey(), clusterName)
                 .build());
+    cleanup.deferAfterAll(client);
     client.addTransportAddress(tcpPublishAddress);
     testing.runWithSpan(
         "setup",
@@ -88,12 +89,6 @@ void setUp(@TempDir File esWorkingDir) {
     testing.clearData();
   }
 
-  @AfterAll
-  void cleanUp() throws Exception {
-    client.close();
-    testNode.close();
-  }
-
   protected abstract NodeFactory getNodeFactory();
 
   @Override
diff --git a/instrumentation/executors/bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/executors/PropagatedContext.java b/instrumentation/executors/bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/executors/PropagatedContext.java
@@ -22,6 +22,7 @@ public final class PropagatedContext {
 
   // Used by AtomicReferenceFieldUpdater
   @SuppressWarnings("UnusedVariable")
+  @Nullable
   private volatile Context context;
 
   PropagatedContext() {}
diff --git a/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/executors/CallableInstrumentation.java b/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/executors/CallableInstrumentation.java
@@ -32,7 +32,7 @@ public ElementMatcher<TypeDescription> typeMatcher() {
   public void transform(TypeTransformer transformer) {
     transformer.applyAdviceToMethod(
         named("call").and(takesArguments(0)).and(isPublic()),
-        CallableInstrumentation.class.getName() + "$CallableAdvice");
+        getClass().getName() + "$CallableAdvice");
   }
 
   @SuppressWarnings("unused")
diff --git a/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/executors/FutureInstrumentation.java b/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/executors/FutureInstrumentation.java
@@ -92,7 +92,7 @@ public boolean matches(TypeDescription target) {
   public void transform(TypeTransformer transformer) {
     transformer.applyAdviceToMethod(
         named("cancel").and(returns(boolean.class)),
-        FutureInstrumentation.class.getName() + "$CanceledFutureAdvice");
+        getClass().getName() + "$CanceledFutureAdvice");
   }
 
   @SuppressWarnings("unused")
diff --git a/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/executors/JavaExecutorInstrumentation.java b/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/executors/JavaExecutorInstrumentation.java
@@ -53,45 +53,44 @@ public ElementMatcher<TypeDescription> typeMatcher() {
   public void transform(TypeTransformer transformer) {
     transformer.applyAdviceToMethod(
         named("execute").and(takesArgument(0, Runnable.class)).and(takesArguments(1)),
-        JavaExecutorInstrumentation.class.getName() + "$SetExecuteRunnableStateAdvice");
+        getClass().getName() + "$SetExecuteRunnableStateAdvice");
     // Netty uses addTask as the actual core of their submission; there are non-standard variations
     // like execute(Runnable,boolean) that aren't caught by standard instrumentation
     transformer.applyAdviceToMethod(
         named("addTask").and(takesArgument(0, Runnable.class)).and(takesArguments(1)),
-        JavaExecutorInstrumentation.class.getName() + "$SetExecuteRunnableStateAdvice");
+        getClass().getName() + "$SetExecuteRunnableStateAdvice");
     transformer.applyAdviceToMethod(
         named("execute").and(takesArgument(0, ForkJoinTask.class)),
-        JavaExecutorInstrumentation.class.getName() + "$SetJavaForkJoinStateAdvice");
+        getClass().getName() + "$SetJavaForkJoinStateAdvice");
     transformer.applyAdviceToMethod(
         named("submit")
             .and(takesArgument(0, Runnable.class))
             .and(returns(hasSuperType(is(Future.class)))),
-        JavaExecutorInstrumentation.class.getName() + "$SetSubmitRunnableStateAdvice");
+        getClass().getName() + "$SetSubmitRunnableStateAdvice");
     transformer.applyAdviceToMethod(
         named("submit")
             .and(takesArgument(0, Callable.class))
             .and(returns(hasSuperType(is(Future.class)))),
-        JavaExecutorInstrumentation.class.getName() + "$SetCallableStateAdvice");
+        getClass().getName() + "$SetCallableStateAdvice");
     transformer.applyAdviceToMethod(
         named("submit").and(takesArgument(0, ForkJoinTask.class)),
-        JavaExecutorInstrumentation.class.getName() + "$SetJavaForkJoinStateAdvice");
+        getClass().getName() + "$SetJavaForkJoinStateAdvice");
     transformer.applyAdviceToMethod(
         namedOneOf("invokeAny", "invokeAll").and(takesArgument(0, Collection.class)),
-        JavaExecutorInstrumentation.class.getName()
-            + "$SetCallableStateForCallableCollectionAdvice");
+        getClass().getName() + "$SetCallableStateForCallableCollectionAdvice");
     transformer.applyAdviceToMethod(
         named("invoke").and(takesArgument(0, ForkJoinTask.class)),
-        JavaExecutorInstrumentation.class.getName() + "$SetJavaForkJoinStateAdvice");
+        getClass().getName() + "$SetJavaForkJoinStateAdvice");
     transformer.applyAdviceToMethod(
         named("schedule")
             .and(takesArgument(0, Runnable.class))
             .and(returns(hasSuperType(is(Future.class)))),
-        JavaExecutorInstrumentation.class.getName() + "$SetSubmitRunnableStateAdvice");
+        getClass().getName() + "$SetSubmitRunnableStateAdvice");
     transformer.applyAdviceToMethod(
         named("schedule")
             .and(takesArgument(0, Callable.class))
             .and(returns(hasSuperType(is(Future.class)))),
-        JavaExecutorInstrumentation.class.getName() + "$SetCallableStateAdvice");
+        getClass().getName() + "$SetCallableStateAdvice");
   }
 
   @SuppressWarnings("unused")
diff --git a/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/executors/JavaForkJoinTaskInstrumentation.java b/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/executors/JavaForkJoinTaskInstrumentation.java
@@ -47,10 +47,9 @@ public ElementMatcher<TypeDescription> typeMatcher() {
   public void transform(TypeTransformer transformer) {
     transformer.applyAdviceToMethod(
         named("exec").and(takesArguments(0)).and(not(isAbstract())),
-        JavaForkJoinTaskInstrumentation.class.getName() + "$ForkJoinTaskAdvice");
+        getClass().getName() + "$ForkJoinTaskAdvice");
     transformer.applyAdviceToMethod(
-        named("fork").and(takesArguments(0)),
-        JavaForkJoinTaskInstrumentation.class.getName() + "$ForkAdvice");
+        named("fork").and(takesArguments(0)), getClass().getName() + "$ForkAdvice");
   }
 
   @SuppressWarnings("unused")
diff --git a/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/executors/RunnableInstrumentation.java b/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/executors/RunnableInstrumentation.java
@@ -31,7 +31,7 @@ public ElementMatcher<TypeDescription> typeMatcher() {
   public void transform(TypeTransformer transformer) {
     transformer.applyAdviceToMethod(
         named("run").and(takesArguments(0)).and(isPublic()),
-        RunnableInstrumentation.class.getName() + "$RunnableAdvice");
+        getClass().getName() + "$RunnableAdvice");
   }
 
   @SuppressWarnings("unused")
diff --git a/instrumentation/executors/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/executors/CompletableFutureTest.java b/instrumentation/executors/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/executors/CompletableFutureTest.java
@@ -32,35 +32,30 @@ void multipleCallbacks() {
     cleanup.deferCleanup(executor::shutdown);
     cleanup.deferCleanup(executor2::shutdown);
 
-    String result;
-    try {
-      result =
-          testing.runWithSpan(
-              "parent",
-              () ->
-                  CompletableFuture.supplyAsync(
-                          () -> {
-                            testing.runWithSpan("supplier", () -> {});
-                            try {
-                              Thread.sleep(1);
-                            } catch (InterruptedException e) {
-                              Thread.currentThread().interrupt();
-                              throw new AssertionError(e);
-                            }
-                            return "a";
-                          },
-                          executor)
-                      .thenCompose(
-                          s -> CompletableFuture.supplyAsync(new AppendingSupplier(s), executor2))
-                      .thenApply(
-                          s -> {
-                            testing.runWithSpan("function", () -> {});
-                            return s + "c";
-                          })
-                      .get());
-    } catch (Exception e) {
-      throw new AssertionError(e);
-    }
+    String result =
+        testing.runWithSpan(
+            "parent",
+            () ->
+                CompletableFuture.supplyAsync(
+                        () -> {
+                          testing.runWithSpan("supplier", () -> {});
+                          try {
+                            Thread.sleep(1);
+                          } catch (InterruptedException e) {
+                            Thread.currentThread().interrupt();
+                            throw new AssertionError(e);
+                          }
+                          return "a";
+                        },
+                        executor)
+                    .thenCompose(
+                        s -> CompletableFuture.supplyAsync(new AppendingSupplier(s), executor2))
+                    .thenApply(
+                        s -> {
+                          testing.runWithSpan("function", () -> {});
+                          return s + "c";
+                        })
+                    .join());
 
     assertThat(result).isEqualTo("abc");
 
diff --git a/instrumentation/executors/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/executors/LambdaContextPropagationTest.java b/instrumentation/executors/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/executors/LambdaContextPropagationTest.java
@@ -14,7 +14,6 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -134,7 +133,7 @@ void propagateContextInvokeAll() throws InterruptedException {
   }
 
   @Test
-  void propagateContextInvokeAny() throws InterruptedException, ExecutionException {
+  void propagateContextInvokeAny() throws Exception {
     ExecutorService executor = Executors.newSingleThreadExecutor();
 
     Baggage baggage = Baggage.builder().put("test", "test").build();

Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ public ElementMatcher<TypeDescription> typeMatcher() {`
`32`	`32`	`public void transform(TypeTransformer transformer) {`
`33`	`33`	`transformer.applyAdviceToMethod(`
`34`	`34`	`named("call").and(takesArguments(0)).and(isPublic()),`
`35`		`- CallableInstrumentation.class.getName() + "$CallableAdvice");`
	`35`	`+ getClass().getName() + "$CallableAdvice");`
`36`	`36`	`}`
`37`	`37`
`38`	`38`	`@SuppressWarnings("unused")`
Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ public boolean matches(TypeDescription target) {`
`92`	`92`	`public void transform(TypeTransformer transformer) {`
`93`	`93`	`transformer.applyAdviceToMethod(`
`94`	`94`	`named("cancel").and(returns(boolean.class)),`
`95`		`- FutureInstrumentation.class.getName() + "$CanceledFutureAdvice");`
	`95`	`+ getClass().getName() + "$CanceledFutureAdvice");`
`96`	`96`	`}`
`97`	`97`
`98`	`98`	`@SuppressWarnings("unused")`
Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,7 @@ public ElementMatcher<TypeDescription> typeMatcher() {`
`31`	`31`	`public void transform(TypeTransformer transformer) {`
`32`	`32`	`transformer.applyAdviceToMethod(`
`33`	`33`	`named("run").and(takesArguments(0)).and(isPublic()),`
`34`		`- RunnableInstrumentation.class.getName() + "$RunnableAdvice");`
	`34`	`+ getClass().getName() + "$RunnableAdvice");`
`35`	`35`	`}`
`36`	`36`
`37`	`37`	`@SuppressWarnings("unused")`