Code review sweep (run 24903865150) (open-telemetry#18267)

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/testing/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/TestServlet3.java

@@ -294,6 +294,7 @@ public static class DispatchRecursive extends HttpServlet {
     protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException {
       if (req.getServletPath().equals("/recursive")) {
         resp.getWriter().print("Hello Recursive");
+        return;
       }
 
       int depth = Integer.parseInt(req.getParameter("depth"));

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

@@ -56,7 +56,7 @@ public abstract class AbstractTomcatServlet3Test extends AbstractServlet3Test<To
           ERROR.getStatus(),
           ERROR.getBody(),
           false);
-  private TestAccessLogValve accessLogValue;
+  private TestAccessLogValve accessLogValve;
 
   @TempDir private static File tempDir;
 
@@ -110,8 +110,8 @@ protected Tomcat setupServer() throws Exception {
 
     ((StandardHost) tomcatServer.getHost())
         .setErrorReportValveClass(ErrorHandlerValve.class.getName());
-    accessLogValue = new TestAccessLogValve();
-    tomcatServer.getHost().getPipeline().addValve(accessLogValue);
+    accessLogValve = new TestAccessLogValve();
+    tomcatServer.getHost().getPipeline().addValve(accessLogValve);
 
     tomcatServer.start();
 
@@ -122,7 +122,7 @@ protected void configureServer(Context servletContext) {}
 
   @BeforeEach
   void setUp() {
-    accessLogValue.getLoggedIds().clear();
+    accessLogValve.getLoggedIds().clear();
     testing().clearAllExportedData();
   }
 
@@ -153,12 +153,12 @@ void accessLogHasIdsForCountRequests(int count) {
               assertThat(response.contentUtf8()).isEqualTo(ACCESS_LOG_SUCCESS.getBody());
             });
 
-    accessLogValue.waitForLoggedIds(count);
-    assertThat(accessLogValue.getLoggedIds()).hasSize(count);
+    accessLogValve.waitForLoggedIds(count);
+    assertThat(accessLogValve.getLoggedIds()).hasSize(count);
     List<String> loggedTraces =
-        accessLogValue.getLoggedIds().stream().map(Map.Entry::getKey).collect(toList());
+        accessLogValve.getLoggedIds().stream().map(Map.Entry::getKey).collect(toList());
     List<String> loggedSpans =
-        accessLogValue.getLoggedIds().stream().map(Map.Entry::getValue).collect(toList());
+        accessLogValve.getLoggedIds().stream().map(Map.Entry::getValue).collect(toList());
 
     testing()
         .waitAndAssertTraces(
@@ -201,7 +201,7 @@ void accessLogHasIdsForErrorRequest() {
                   .hasParent(trace.getSpan(1)));
     }
 
-    accessLogValue.waitForLoggedIds(1);
+    accessLogValve.waitForLoggedIds(1);
     testing()
         .waitAndAssertTraces(
             trace -> {
@@ -210,7 +210,7 @@ void accessLogHasIdsForErrorRequest() {
                       .map(e -> (Consumer<SpanDataAssert>) span -> e.accept(span, trace))
                       .collect(toList()));
               SpanData span = trace.getSpan(0);
-              Map.Entry<String, String> entry = accessLogValue.getLoggedIds().get(0);
+              Map.Entry<String, String> entry = accessLogValve.getLoggedIds().get(0);
               assertThat(entry.getKey()).isEqualTo(span.getTraceId());
               assertThat(entry.getValue()).isEqualTo(span.getSpanId());
             });

instrumentation/servlet/servlet-5.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/snippet/SnippetPrintWriterTest.java

@@ -149,19 +149,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-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletServiceAdvice.java

@@ -124,7 +124,7 @@ public static Object[] onEnter(
         new AdviceScope(
             CallDepth.forClass(AppServerBridge.getCallDepthKey()),
             servletOrFilter,
-            (HttpServletRequest) request,
+            httpServletRequest,
             response);
 
     return new Object[] {adviceScope, request, response};
@@ -134,7 +134,7 @@ public static Object[] onEnter(
   public static void stopSpan(
       @Advice.Argument(0) ServletRequest request,
       @Advice.Argument(1) ServletResponse response,
-      @Advice.Thrown Throwable throwable,
+      @Advice.Thrown @Nullable Throwable throwable,
       @Advice.Enter Object[] enterResult) {
 
     AdviceScope adviceScope = (AdviceScope) enterResult[0];

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

@@ -104,7 +104,8 @@ public List<String> getRequestHeaderValues(HttpServletRequest request, String na
 
   @Override
   public Iterable<String> getRequestHeaderNames(HttpServletRequest httpServletRequest) {
-    return Collections.list(httpServletRequest.getHeaderNames());
+    Enumeration<String> names = httpServletRequest.getHeaderNames();
+    return names == null ? emptyList() : Collections.list(names);
   }
 
   @Override

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

@@ -32,8 +32,6 @@
 
 public class TestServlet5 {
 
-  private TestServlet5() {}
-
   @WebServlet
   public static class Sync extends HttpServlet {
     @Override
@@ -305,4 +303,6 @@ protected void service(HttpServletRequest req, HttpServletResponse resp) throws
       }
     }
   }
+
+  private TestServlet5() {}
 }

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

@@ -56,10 +56,10 @@ public abstract class AbstractTomcatServlet5Test extends AbstractServlet5Test<To
           ERROR.getStatus(),
           ERROR.getBody(),
           false);
-  private TestAccessLogValve accessLogValve;
-
   @TempDir private static File tempDir;
 
+  private TestAccessLogValve accessLogValve;
+
   @Override
   protected void configure(HttpServerTestOptions options) {
     super.configure(options);

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

@@ -18,19 +18,18 @@
 import org.apache.catalina.connector.Response;
 import org.apache.catalina.valves.ValveBase;
 
-// public, because it's loaded by reflection
 public class TestAccessLogValve extends ValveBase implements AccessLog {
 
-  public List<Map.Entry<String, String>> getLoggedIds() {
-    return loggedIds;
-  }
-
   private final List<Map.Entry<String, String>> loggedIds = new ArrayList<>();
 
   public TestAccessLogValve() {
     super(true);
   }
 
+  public List<Map.Entry<String, String>> getLoggedIds() {
+    return loggedIds;
+  }
+
   @Override
   public void log(Request request, Response response, long time) {
     if (request.getParameter("access-log") == null) {