Merge branch 'main' into remove-more-deprecated-props

Author: trask

.github/agents/knowledge/gradle-conventions.md

@@ -102,6 +102,24 @@ should be active in the test agent.
 - **Sibling cross-version references are required**: every javaagent module in a version
   family must list all other sibling `:javaagent` modules via `testInstrumentation`.
 
+### Exception: modules already bundled into `agent-for-testing`
+
+A small set of javaagent modules are bundled directly into the main agent via
+`baseJavaagentLibs(...)` in `javaagent/build.gradle.kts`, and therefore into
+`agent-for-testing` as well. For these, the sibling cross-version rule does **not** apply:
+they are already loaded in every test JVM, so adding them via `testInstrumentation`
+from a sibling's `build.gradle.kts` is redundant and should be rejected in review.
+
+In particular, do not add `testInstrumentation(project(":instrumentation:opentelemetry-api:opentelemetry-api-1.N:javaagent"))`
+entries to sibling `opentelemetry-api-*` modules — all `opentelemetry-api-1.*:javaagent`
+versions are already bundled in the test agent. The same applies to the other modules listed
+under `baseJavaagentLibs` in `javaagent/build.gradle.kts` (e.g. `executors`,
+`opentelemetry-instrumentation-annotations-1.16`, and the `internal/*` modules).
+
+Before adding a `testInstrumentation` for a sibling, check
+[`javaagent/build.gradle.kts`](../../../javaagent/build.gradle.kts): if the sibling appears
+in a `baseJavaagentLibs(...)` line, omit the `testInstrumentation` entry.
+
 ### How to check for missing siblings (step by step)
 
 When reviewing a `javaagent/` module:

.github/agents/knowledge/testing-general-patterns.md

@@ -35,6 +35,8 @@
   `throws` clause. That noisy try/catch inside the lambda is worse than leaving
   `throws Exception` (or `throws IOException`) on the `@Test` method. Only introduce such
   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()`.
 
 ## Test Resource Cleanup
 

instrumentation-docs/build.gradle.kts

@@ -18,6 +18,21 @@ dependencies {
 }
 
 tasks {
+  test {
+    // DeclarativeConfigValidationTest walks ../instrumentation for metadata.yaml files,
+    // so changes to those files must invalidate this task's build cache entry.
+    // Eagerly resolve to a concrete file list (rather than passing a FileTree rooted at
+    // the instrumentation directory) to avoid Gradle's implicit-dependency validation
+    // flagging overlap with sibling subprojects' build/ output directories.
+    val metadataYamlFiles = fileTree(rootDir.resolve("instrumentation")) {
+      include("**/metadata.yaml")
+      exclude("**/build/**")
+    }.files
+    inputs.files(metadataYamlFiles)
+      .withPropertyName("instrumentationMetadataYamlFiles")
+      .withPathSensitivity(PathSensitivity.RELATIVE)
+  }
+
   val runAnalysis by registering(JavaExec::class) {
     dependsOn(classes)
 

instrumentation-docs/src/main/java/io/opentelemetry/instrumentation/docs/utils/FileManager.java

@@ -38,9 +38,10 @@ private static InstrumentationPath parseInstrumentationPath(String filePath) {
       return null;
     }
 
+    String normalized = normalizeSeparators(filePath);
     String instrumentationSegment = "/instrumentation/";
-    int startIndex = filePath.indexOf(instrumentationSegment) + instrumentationSegment.length();
-    String[] parts = filePath.substring(startIndex).split("/");
+    int startIndex = normalized.indexOf(instrumentationSegment) + instrumentationSegment.length();
+    String[] parts = normalized.substring(startIndex).split("/");
 
     if (parts.length < 2) {
       return null;
@@ -58,23 +59,28 @@ public static boolean isValidInstrumentationPath(String filePath) {
     if (filePath == null || filePath.isEmpty()) {
       return false;
     }
+    String normalized = normalizeSeparators(filePath);
     String instrumentationSegment = "instrumentation/";
 
-    if (!filePath.contains(instrumentationSegment)) {
+    if (!normalized.contains(instrumentationSegment)) {
       return false;
     }
 
-    int javaagentCount = filePath.split("/javaagent", -1).length - 1;
+    int javaagentCount = normalized.split("/javaagent", -1).length - 1;
     if (javaagentCount > 1) {
       return false;
     }
 
-    if (filePath.matches(
+    if (normalized.matches(
         ".*(/test/|/testing|/build/|-common/|-common-|common-|/compile-stub/|-testing|bootstrap/src).*")) {
       return false;
     }
 
-    return filePath.endsWith("javaagent") || filePath.endsWith("library");
+    return normalized.endsWith("javaagent") || normalized.endsWith("library");
+  }
+
+  private static String normalizeSeparators(String filePath) {
+    return filePath.replace('\\', '/');
   }
 
   public List<String> findBuildGradleFiles(String instrumentationDirectory) {
@@ -85,7 +91,7 @@ public List<String> findBuildGradleFiles(String instrumentationDirectory) {
           .filter(
               path ->
                   path.getFileName().toString().equals("build.gradle.kts")
-                      && !path.toString().contains("/testing/")
+                      && !normalizeSeparators(path.toString()).contains("/testing/")
                       && !isInNestedInstrumentationModule(path, rootPath))
           .map(Path::toString)
           .collect(toList());
@@ -105,13 +111,11 @@ public List<String> findBuildGradleFiles(String instrumentationDirectory) {
    */
   private static boolean isInNestedInstrumentationModule(Path filePath, Path rootPath) {
     Path relativePath = rootPath.relativize(filePath);
-    String relativeStr = relativePath.toString();
-
-    String[] segments = relativeStr.split("/");
 
     // Find the first javaagent or library segment
-    for (int i = 0; i < segments.length; i++) {
-      if (segments[i].equals("javaagent") || segments[i].equals("library")) {
+    for (int i = 0; i < relativePath.getNameCount(); i++) {
+      String segment = relativePath.getName(i).toString();
+      if (segment.equals("javaagent") || segment.equals("library")) {
         // If javaagent/library is not the first segment, it's a nested module
         return i > 0;
       }

instrumentation/opensearch/opensearch-rest-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/opensearch/rest/v1_0/OpenSearchRestSingletons.java

@@ -10,6 +10,7 @@
 import io.opentelemetry.javaagent.instrumentation.opensearch.rest.OpenSearchRestRequest;
 import io.opentelemetry.javaagent.instrumentation.opensearch.rest.OpenSearchRestResponse;
 import java.net.InetAddress;
+import javax.annotation.Nullable;
 import org.opensearch.client.Response;
 
 public class OpenSearchRestSingletons {
@@ -21,7 +22,11 @@ public static Instrumenter<OpenSearchRestRequest, OpenSearchRestResponse> instru
     return instrumenter;
   }
 
-  public static OpenSearchRestResponse convertResponse(Response response) {
+  @Nullable
+  public static OpenSearchRestResponse convertResponse(@Nullable Response response) {
+    if (response == null) {
+      return null;
+    }
     return new OpenSearchRestResponse() {
 
       @Override
@@ -30,6 +35,7 @@ public int getStatusCode() {
       }
 
       @Override
+      @Nullable
       public InetAddress getAddress() {
         return response.getHost().getAddress();
       }

instrumentation/opensearch/opensearch-rest-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/opensearch/rest/v3_0/RestClientInstrumentation.java

@@ -103,6 +103,7 @@ public void endWithListener(@Nullable Throwable throwable) {
   public static class PerformRequestAdvice {
 
     @Advice.OnMethodEnter(suppress = Throwable.class, inline = false)
+    @Nullable
     public static AdviceScope onEnter(@Advice.Argument(0) Request request) {
       return AdviceScope.start(request);
     }

instrumentation/opentelemetry-api/opentelemetry-api-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/ContextBridgeTest.java

@@ -21,7 +21,7 @@
 import io.opentelemetry.instrumentation.annotations.WithSpan;
 import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
-import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.atomic.AtomicReference;
 import org.junit.jupiter.api.DisplayName;
@@ -43,9 +43,12 @@ void agentPropagatesApplicationsContext() throws Exception {
     Context context = Context.current().with(ANIMAL, "cat");
     AtomicReference<String> captured = new AtomicReference<>();
     try (Scope ignored = context.makeCurrent()) {
-      Executors.newSingleThreadExecutor()
-          .submit(() -> captured.set(Context.current().get(ANIMAL)))
-          .get();
+      ExecutorService executor = Executors.newSingleThreadExecutor();
+      try {
+        executor.submit(() -> captured.set(Context.current().get(ANIMAL))).get();
+      } finally {
+        executor.shutdownNow();
+      }
     }
 
     // Then
@@ -97,12 +100,12 @@ void agentPropagatesApplicationsSpan() throws Exception {
 
     Span testSpan = tracer.spanBuilder("test").startSpan();
     try (Scope ignored = testSpan.makeCurrent()) {
-      Executors.newSingleThreadExecutor()
-          .submit(
-              () -> {
-                Span.current().setAttribute("cat", "yes");
-              })
-          .get();
+      ExecutorService executor = Executors.newSingleThreadExecutor();
+      try {
+        executor.submit(() -> Span.current().setAttribute("cat", "yes")).get();
+      } finally {
+        executor.shutdownNow();
+      }
     }
     testSpan.end();
 
@@ -158,19 +161,16 @@ void agentPropagatesApplicationsBaggage() throws Exception {
     // When
     Baggage testBaggage = Baggage.builder().put("cat", "yes").build();
     AtomicReference<Baggage> ref = new AtomicReference<>();
-    CountDownLatch latch = new CountDownLatch(1);
     try (Scope ignored = testBaggage.makeCurrent()) {
-      Executors.newSingleThreadExecutor()
-          .submit(
-              () -> {
-                ref.set(Baggage.current());
-                latch.countDown();
-              })
-          .get();
+      ExecutorService executor = Executors.newSingleThreadExecutor();
+      try {
+        executor.submit(() -> ref.set(Baggage.current())).get();
+      } finally {
+        executor.shutdownNow();
+      }
     }
 
     // Then
-    latch.await();
     assertThat(ref.get().size()).isEqualTo(1);
     assertThat(ref.get().getEntryValue("cat")).isEqualTo("yes");
   }

instrumentation/opentelemetry-api/opentelemetry-api-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/TracerTest.java

@@ -75,13 +75,11 @@ void captureSpanWithImplicitParentUsingTracerWithSpan() {
     // When
     Tracer tracer = GlobalOpenTelemetry.getTracer("test");
     Span parentSpan = tracer.spanBuilder("parent").startSpan();
-    Scope parentScope = Context.current().with(parentSpan).makeCurrent();
-
-    Span testSpan = tracer.spanBuilder("test").startSpan();
-    testSpan.end();
-
+    try (Scope parentScope = Context.current().with(parentSpan).makeCurrent()) {
+      Span testSpan = tracer.spanBuilder("test").startSpan();
+      testSpan.end();
+    }
     parentSpan.end();
-    parentScope.close();
 
     // Then
     testing.waitAndAssertTraces(
@@ -98,13 +96,11 @@ void captureSpanWithImplicitParentUsingMakeCurrent() {
     // When
     Tracer tracer = GlobalOpenTelemetry.getTracer("test");
     Span parentSpan = tracer.spanBuilder("parent").startSpan();
-    Scope parentScope = parentSpan.makeCurrent();
-
-    Span testSpan = tracer.spanBuilder("test").startSpan();
-    testSpan.end();
-
+    try (Scope parentScope = parentSpan.makeCurrent()) {
+      Span testSpan = tracer.spanBuilder("test").startSpan();
+      testSpan.end();
+    }
     parentSpan.end();
-    parentScope.close();
 
     // Then
     testing.waitAndAssertTraces(
@@ -123,13 +119,11 @@ void captureSpanWithImplicitParentUsingTracingContextUtilsWithSpanAndMakeCurrent
     Tracer tracer = GlobalOpenTelemetry.getTracer("test");
     Span parentSpan = tracer.spanBuilder("parent").startSpan();
     Context parentContext = Context.current().with(parentSpan);
-    Scope parentScope = parentContext.makeCurrent();
-
-    Span testSpan = tracer.spanBuilder("test").startSpan();
-    testSpan.end();
-
+    try (Scope parentScope = parentContext.makeCurrent()) {
+      Span testSpan = tracer.spanBuilder("test").startSpan();
+      testSpan.end();
+    }
     parentSpan.end();
-    parentScope.close();
 
     // Then
     testing.waitAndAssertTraces(
@@ -166,11 +160,11 @@ void captureSpanWithExplicitNoParent() {
     // When
     Tracer tracer = GlobalOpenTelemetry.getTracer("test");
     Span parentSpan = tracer.spanBuilder("parent").startSpan();
-    Scope parentScope = parentSpan.makeCurrent();
-    Span testSpan = tracer.spanBuilder("test").setNoParent().startSpan();
-    testSpan.end();
+    try (Scope parentScope = parentSpan.makeCurrent()) {
+      Span testSpan = tracer.spanBuilder("test").setNoParent().startSpan();
+      testSpan.end();
+    }
     parentSpan.end();
-    parentScope.close();
 
     // Then
     testing.waitAndAssertSortedTraces(
@@ -255,9 +249,9 @@ void captureNameUpdateUsingTracingContextUtilsGetCurrentSpan() {
     // When
     Tracer tracer = GlobalOpenTelemetry.getTracer("test");
     Span testSpan = tracer.spanBuilder("test").startSpan();
-    Scope testScope = Context.current().with(testSpan).makeCurrent();
-    Span.current().updateName("test2");
-    testScope.close();
+    try (Scope testScope = Context.current().with(testSpan).makeCurrent()) {
+      Span.current().updateName("test2");
+    }
     testSpan.end();
 
     // Then
@@ -273,9 +267,9 @@ void captureNameUpdateUsingTracingContextUtilsSpanFromContextCurrent() {
     // When
     Tracer tracer = GlobalOpenTelemetry.getTracer("test");
     Span testSpan = tracer.spanBuilder("test").startSpan();
-    Scope testScope = Context.current().with(testSpan).makeCurrent();
-    Span.fromContext(Context.current()).updateName("test2");
-    testScope.close();
+    try (Scope testScope = Context.current().with(testSpan).makeCurrent()) {
+      Span.fromContext(Context.current()).updateName("test2");
+    }
     testSpan.end();
 
     // Then

instrumentation/opentelemetry-api/opentelemetry-api-1.10/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/v1_10/OpenTelemetryInstrumentation.java

@@ -28,7 +28,7 @@ public void transform(TypeTransformer transformer) {
 
   @SuppressWarnings({"ReturnValueIgnored", "unused"})
   public static class InitAdvice {
-    @Advice.OnMethodEnter(suppress = Throwable.class, inline = false)
+    @Advice.OnMethodEnter(inline = false)
     public static void init() {
       // the sole purpose of this advice is to ensure that ApplicationOpenTelemetry110 is recognized
       // as helper class and injected into class loader

instrumentation/opentelemetry-api/opentelemetry-api-1.10/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/v1_10/metrics/CallbackGcCloseTest.java

@@ -159,7 +159,7 @@ protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundE
               resolveClass(c);
             }
             return c;
-          } catch (ClassNotFoundException e) {
+          } catch (ClassNotFoundException ignored) {
             // fall through to parent
           }
         }