Code review sweep (run 24968439270) (#18325)

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

Author: otelbot[bot]

instrumentation/java-util-logging/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jul/JavaUtilLoggingHelper.java

@@ -29,7 +29,7 @@
 
 public class JavaUtilLoggingHelper {
 
-  private static final Formatter FORMATTER = new AccessibleFormatter();
+  private static final Formatter formatter = new AccessibleFormatter();
 
   private static final boolean captureExperimentalAttributes =
       DeclarativeConfigUtil.getInstrumentationConfig(GlobalOpenTelemetry.get(), "java_util_logging")
@@ -68,7 +68,7 @@ public static void capture(application.java.util.logging.Logger logger, LogRecor
    */
   private static void mapLogRecord(LogRecordBuilder builder, LogRecord logRecord) {
     // message
-    String message = FORMATTER.formatMessage(logRecord);
+    String message = formatter.formatMessage(logRecord);
     if (message != null) {
       builder.setBody(message);
     }
@@ -82,7 +82,7 @@ private static void mapLogRecord(LogRecordBuilder builder, LogRecord logRecord)
     Level level = logRecord.getLevel();
     if (level != null) {
       builder.setSeverity(levelToSeverity(level));
-      builder.setSeverityText(logRecord.getLevel().getName());
+      builder.setSeverityText(level.getName());
     }
 
     AttributesBuilder attributes = Attributes.builder();

instrumentation/java-util-logging/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/jul/JavaUtilLoggingTest.java

@@ -161,18 +161,18 @@ private static void performLogging(
   }
 
   @FunctionalInterface
-  interface LoggerMethod {
+  private interface LoggerMethod {
     void call(Logger logger, String msg);
   }
 
-  static String experimental(String value) {
+  private static String experimental(String value) {
     if (isExperimentalAttributesEnabled) {
       return value;
     }
     return null;
   }
 
-  static Long experimental(long value) {
+  private static Long experimental(long value) {
     if (isExperimentalAttributesEnabled) {
       return value;
     }

instrumentation/javalin/javalin-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/javalin/v5_0/JavalinInstrumentationModule.java

@@ -30,7 +30,8 @@ public List<TypeInstrumentation> typeInstrumentations() {
 
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
-    return hasClassesNamed("io.javalin.http.Handler")
+    // added in 5.0.0
+    return hasClassesNamed("io.javalin.config.JavalinConfig")
         // added in 7.0.0
         .and(not(hasClassesNamed("io.javalin.config.JavalinState")));
   }

instrumentation/javalin/testing/src/main/java/io/opentelemetry/instrumentation/javalin/AbstractJavalinTest.java

@@ -26,37 +26,39 @@
 import io.javalin.Javalin;
 import io.opentelemetry.api.trace.SpanKind;
 import io.opentelemetry.instrumentation.test.utils.PortUtils;
-import io.opentelemetry.instrumentation.testing.internal.AutoCleanupExtension;
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
 import io.opentelemetry.testing.internal.armeria.client.WebClient;
 import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpResponse;
+import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInstance;
-import org.junit.jupiter.api.extension.RegisterExtension;
 
 @TestInstance(PER_CLASS)
 public abstract class AbstractJavalinTest {
 
-  @RegisterExtension static final AutoCleanupExtension cleanup = AutoCleanupExtension.create();
-
   protected abstract InstrumentationExtension testing();
 
   protected abstract Javalin setupJavalin(int port);
 
   protected abstract String getJettyInstrumentation();
 
   private int port;
+  private Javalin app;
   private WebClient client;
 
   @BeforeAll
   void setup() {
     port = PortUtils.findOpenPort();
-    Javalin app = setupJavalin(port);
-    cleanup.deferAfterAll(app::stop);
+    app = setupJavalin(port);
     client = WebClient.of("http://localhost:" + port);
   }
 
+  @AfterAll
+  void stopJavalin() {
+    app.stop();
+  }
+
   @Test
   void testSpanNameAndHttpRouteSpanWithPathParamResponseSuccessful() {
     String id = "123";

instrumentation/jaxrs-client/jaxrs-client-1.1-testing/src/test/java/io/opentelemetry/javaagent/instrumentation/jaxrsclient/JaxRsClientV1Test.java

@@ -44,6 +44,7 @@ private static Client buildClient(boolean readTimeout) {
     client.addFilter(new LoggingFilter());
     client.addFilter(new GZIPContentEncodingFilter());
 
+    // shared across tests via @TestInstance(PER_CLASS), destroy after the class finishes
     cleanup.deferAfterAll(client::destroy);
     return client;
   }

instrumentation/jaxrs-client/jaxrs-client-2.0-testing/src/test/java/io/opentelemetry/javaagent/instrumentation/jaxrsclient/AbstractJaxRsClientTest.java

@@ -60,10 +60,13 @@ public int sendRequest(
     try {
       Entity<String> body = BODY_METHODS.contains(method) ? Entity.text("") : null;
       Response response = request.build(method, body).invoke();
-      // read response body to avoid broken pipe errors on the server side
-      response.readEntity(String.class);
-      response.close();
-      return response.getStatus();
+      try {
+        // read response body to avoid broken pipe errors on the server side
+        response.readEntity(String.class);
+        return response.getStatus();
+      } finally {
+        response.close();
+      }
     } catch (ProcessingException e) {
       if (e.getCause() instanceof Exception) {
         throw (Exception) e.getCause();
@@ -89,9 +92,13 @@ public void sendRequestWithCallback(
             new InvocationCallback<Response>() {
               @Override
               public void completed(Response response) {
-                // read response body
-                response.readEntity(String.class);
-                requestResult.complete(response.getStatus());
+                try {
+                  // read response body
+                  response.readEntity(String.class);
+                  requestResult.complete(response.getStatus());
+                } finally {
+                  response.close();
+                }
               }
 
               @Override

instrumentation/jaxrs-client/jaxrs-client-2.0-testing/src/test/java/io/opentelemetry/javaagent/instrumentation/jaxrsclient/JaxMultithreadedClientTest.java

@@ -17,6 +17,7 @@
 import io.opentelemetry.testing.internal.armeria.testing.junit5.server.ServerExtension;
 import java.net.URI;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicBoolean;
 import javax.ws.rs.client.Client;
 import org.glassfish.jersey.client.JerseyClientBuilder;
 import org.junit.jupiter.api.DisplayName;
@@ -41,20 +42,23 @@ protected void configure(ServerBuilder sb) {
 
   @SuppressWarnings("CatchingUnchecked")
   boolean checkUri(JerseyClientBuilder builder, URI uri) {
+    Client client = builder.build();
     try {
-      Client client = builder.build();
-      client.target(uri).request().get();
+      client.target(uri).request().get().close();
+      return false;
     } catch (Exception e) {
       return true;
+    } finally {
+      client.close();
     }
-    return false;
   }
 
   @DisplayName("multiple threads using the same builder works")
   @Test
   void testMultipleThreads() throws InterruptedException {
     URI uri = server.httpUri().resolve("/success");
     JerseyClientBuilder builder = new JerseyClientBuilder();
+    AtomicBoolean hadErrors = new AtomicBoolean();
 
     // Start 10 threads and do 50 requests each
     CountDownLatch latch = new CountDownLatch(10);
@@ -63,17 +67,22 @@ void testMultipleThreads() throws InterruptedException {
               new Runnable() {
                 @Override
                 public void run() {
-                  boolean hadErrors = false;
-                  for (int j = 0; j < 50; j++) {
-                    hadErrors = hadErrors || checkUri(builder, uri);
+                  try {
+                    for (int j = 0; j < 50; j++) {
+                      if (checkUri(builder, uri)) {
+                        hadErrors.set(true);
+                        return;
+                      }
+                    }
+                  } finally {
+                    latch.countDown();
                   }
-                  assertThat(hadErrors).isFalse();
-                  latch.countDown();
                 }
               })
           .start();
     }
 
     assertThat(latch.await(10, SECONDS)).isTrue();
+    assertThat(hadErrors).isFalse();
   }
 }

instrumentation/jaxrs-client/jaxrs-client-2.0-testing/src/test/java/io/opentelemetry/javaagent/instrumentation/jaxrsclient/ResteasySingleConnection.java

@@ -54,14 +54,16 @@ public int doRequest(String path, Map<String, String> headers) throws ExecutionE
     }
 
     Response response = requestBuilder.buildGet().invoke();
-    response.close();
+    try {
+      String responseId = response.getHeaderString(REQUEST_ID_HEADER);
+      if (!Objects.equals(requestId, responseId)) {
+        throw new IllegalStateException(
+            String.format("Received response with id %s, expected %s", responseId, requestId));
+      }
 
-    String responseId = response.getHeaderString(REQUEST_ID_HEADER);
-    if (!Objects.equals(requestId, responseId)) {
-      throw new IllegalStateException(
-          String.format("Received response with id %s, expected %s", responseId, requestId));
+      return response.getStatus();
+    } finally {
+      response.close();
     }
-
-    return response.getStatus();
   }
 }

instrumentation/jaxrs/jaxrs-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v1_0/HandlerData.java

@@ -13,7 +13,7 @@
 import javax.ws.rs.HttpMethod;
 import javax.ws.rs.Path;
 
-public class HandlerData {
+class HandlerData {
 
   private static final ClassValue<Map<Method, String>> serverSpanNames =
       new ClassValue<Map<Method, String>>() {
@@ -26,16 +26,16 @@ protected Map<Method, String> computeValue(Class<?> type) {
   private final Class<?> target;
   private final Method method;
 
-  public HandlerData(Class<?> target, Method method) {
+  HandlerData(Class<?> target, Method method) {
     this.target = target;
     this.method = method;
   }
 
-  public Class<?> codeClass() {
+  Class<?> codeClass() {
     return target;
   }
 
-  public String methodName() {
+  String methodName() {
     return method.getName();
   }
 

instrumentation/jaxrs/jaxrs-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v1_0/JaxrsServerSpanNaming.java

@@ -9,13 +9,13 @@
 import io.opentelemetry.javaagent.bootstrap.jaxrs.JaxrsContextPath;
 import io.opentelemetry.javaagent.bootstrap.servlet.ServletContextPath;
 
-public class JaxrsServerSpanNaming {
+class JaxrsServerSpanNaming {
 
   private static final HttpServerRouteGetter<HandlerData> serverSpanName =
       (context, handlerData) -> {
         String pathBasedSpanName = handlerData.getServerSpanName();
         // If path based name is empty skip prepending context path so that path based name would
-        // remain as an empty string for which we skip updating span name. Path base span name is
+        // remain as an empty string for which we skip updating span name. Path based span name is
         // empty when method and class don't have a jax-rs path annotation, this can happen when
         // creating an "abort" span, see RequestContextHelper.
         if (!pathBasedSpanName.isEmpty()) {
@@ -25,7 +25,7 @@ public class JaxrsServerSpanNaming {
         return pathBasedSpanName;
       };
 
-  public static HttpServerRouteGetter<HandlerData> serverSpanName() {
+  static HttpServerRouteGetter<HandlerData> serverSpanName() {
     return serverSpanName;
   }