Code review sweep (run 25078700795) (#18389)

Co-authored-by: otelbot <197425009+otelbot@users.noreply.github.com>
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Author: otelbot[bot]

instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/SnippetPrintWriterTest.java

@@ -150,19 +150,15 @@ private static InMemoryHttpServletResponse createInMemoryHttpServletResponse(Str
 
   private static class InMemoryHttpServletResponse extends HttpServletResponseWrapper {
 
-    private PrintWriter printWriter;
-    private StringWriter stringWriter;
+    private final StringWriter stringWriter = new StringWriter();
+    private final PrintWriter printWriter = new PrintWriter(stringWriter);
 
     InMemoryHttpServletResponse(HttpServletResponse delegate) {
       super(delegate);
     }
 
     @Override
     public PrintWriter getWriter() {
-      if (printWriter == null) {
-        stringWriter = new StringWriter();
-        printWriter = new PrintWriter(stringWriter);
-      }
       return printWriter;
     }
 

instrumentation/servlet/servlet-3.0/library/build.gradle.kts

@@ -22,7 +22,7 @@ dependencies {
 }
 
 tasks {
-  withType<Test>().configureEach {
+  test {
     // required on jdk17
     jvmArgs("--add-opens=java.base/java.util=ALL-UNNAMED")
     jvmArgs("-XX:+IgnoreUnrecognizedVMOptions")

instrumentation/servlet/servlet-3.0/testing/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/tomcat/AbstractTomcatServlet3Test.java

@@ -122,7 +122,7 @@ protected void configureServer(Context servletContext) {}
 
   @BeforeEach
   void setUp() {
-    accessLogValve.getLoggedIds().clear();
+    accessLogValve.clearLoggedIds();
     testing().clearAllExportedData();
   }
 

instrumentation/servlet/servlet-3.0/testing/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/tomcat/TestAccessLogValve.java

@@ -28,7 +28,15 @@ public TestAccessLogValve() {
   }
 
   public List<Map.Entry<String, String>> getLoggedIds() {
-    return loggedIds;
+    synchronized (loggedIds) {
+      return new ArrayList<>(loggedIds);
+    }
+  }
+
+  public void clearLoggedIds() {
+    synchronized (loggedIds) {
+      loggedIds.clear();
+    }
   }
 
   @Override

instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncStartAdvice.java

@@ -11,6 +11,7 @@
 import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
 import jakarta.servlet.AsyncContext;
 import jakarta.servlet.http.HttpServletRequest;
+import javax.annotation.Nullable;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.implementation.bytecode.assign.Assigner;
 
@@ -25,8 +26,9 @@ public static CallDepth startAsyncEnter() {
     return callDepth;
   }
 
-  @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+  @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class, inline = false)
   public static void startAsyncExit(
+      @Advice.Thrown @Nullable Throwable throwable,
       @Advice.This(typing = Assigner.Typing.DYNAMIC) HttpServletRequest request,
       @Advice.Enter CallDepth callDepth) {
 
@@ -35,7 +37,7 @@ public static void startAsyncExit(
       return;
     }
 
-    if (request != null) {
+    if (throwable == null && request != null) {
       helper().attachAsyncListener(request, Java8BytecodeBridge.currentContext());
     }
   }

instrumentation/servlet/servlet-5.0/jetty11-testing/build.gradle.kts

@@ -20,7 +20,7 @@ otelJava {
 }
 
 tasks {
-  withType<Test>().configureEach {
+  test {
     jvmArgs("-Dotel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter")
   }
 }

instrumentation/servlet/servlet-5.0/jetty12-testing/build.gradle.kts

@@ -18,8 +18,12 @@ otelJava {
 }
 
 tasks {
-  withType<Test>().configureEach {
+  test {
     jvmArgs("-Dotel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter")
     systemProperty("collectMetadata", otelProps.collectMetadata)
+    systemProperty(
+      "metadataConfig",
+      "otel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter",
+    )
   }
 }

instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v5_0/internal/Servlet5TelemetryFilter.java

@@ -108,11 +108,11 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
   public void destroy() {}
 
   private class OtelHttpServletRequest extends HttpServletRequestWrapper {
-    final Context context;
-    final ServletRequestContext<HttpServletRequest> requestContext;
-    final ServletResponseContext<HttpServletResponse> responseContext;
-    boolean hasAsyncListener = false;
-    @Nullable Throwable asyncException;
+    private final Context context;
+    private final ServletRequestContext<HttpServletRequest> requestContext;
+    private final ServletResponseContext<HttpServletResponse> responseContext;
+    private boolean hasAsyncListener = false;
+    @Nullable private Throwable asyncException;
 
     OtelHttpServletRequest(
         HttpServletRequest request,
@@ -235,6 +235,7 @@ public void start(Runnable runnable) {
               runnable.run();
             } catch (Throwable t) {
               otelRequest.asyncException = t;
+              throw t;
             }
           });
     }

instrumentation/servlet/servlet-5.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/AbstractServlet5Test.java

@@ -20,6 +20,7 @@
 
 import io.opentelemetry.api.trace.SpanKind;
 import io.opentelemetry.instrumentation.api.internal.HttpConstants;
+import io.opentelemetry.instrumentation.testing.internal.AutoCleanupExtension;
 import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpServerTest;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpServerTestOptions;
 import io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint;
@@ -30,9 +31,12 @@
 import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpResponse;
 import jakarta.servlet.Servlet;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
 
 public abstract class AbstractServlet5Test<SERVER, CONTEXT> extends AbstractHttpServerTest<SERVER> {
 
+  @RegisterExtension static final AutoCleanupExtension cleanup = AutoCleanupExtension.create();
+
   public static final ServerEndpoint HTML_PRINT_WRITER =
       new ServerEndpoint(
           "HTML_PRINT_WRITER",
@@ -157,6 +161,8 @@ void snippetInjectionWithServletOutputStream() {
 
     ExperimentalSnippetHolder.setSnippet(
         "\n  <script type=\"text/javascript\"> Test Test</script>");
+    cleanup.deferCleanup(() -> ExperimentalSnippetHolder.setSnippet(""));
+
     AggregatedHttpRequest request = request(HTML_SERVLET_OUTPUT_STREAM, "GET");
     AggregatedHttpResponse response = client.execute(request).aggregate().join();
 
@@ -176,8 +182,6 @@ void snippetInjectionWithServletOutputStream() {
     assertThat(response.contentUtf8()).isEqualTo(result);
     assertThat(response.headers().contentLength()).isEqualTo(result.length());
 
-    ExperimentalSnippetHolder.setSnippet("");
-
     String expectedRoute = expectedHttpRoute(HTML_SERVLET_OUTPUT_STREAM, "GET");
     testing()
         .waitAndAssertTraces(
@@ -198,6 +202,8 @@ void snippetInjectionWithPrintWriter() {
     assumeTrue(isAgentTest());
 
     ExperimentalSnippetHolder.setSnippet("\n  <script type=\"text/javascript\"> Test </script>");
+    cleanup.deferCleanup(() -> ExperimentalSnippetHolder.setSnippet(""));
+
     AggregatedHttpRequest request = request(HTML_PRINT_WRITER, "GET");
     AggregatedHttpResponse response = client.execute(request).aggregate().join();
 
@@ -218,8 +224,6 @@ void snippetInjectionWithPrintWriter() {
     assertThat(response.contentUtf8()).isEqualTo(result);
     assertThat(response.headers().contentLength()).isEqualTo(result.length());
 
-    ExperimentalSnippetHolder.setSnippet("");
-
     String expectedRoute = expectedHttpRoute(HTML_PRINT_WRITER, "GET");
     testing()
         .waitAndAssertTraces(

instrumentation/servlet/servlet-5.0/tomcat-testing/build.gradle.kts

@@ -20,7 +20,7 @@ if (otelProps.testLatestDeps) {
 }
 
 tasks {
-  withType<Test>().configureEach {
+  test {
     jvmArgs("-Dotel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter")
   }
 }