Record exception for async servlet invocations (open-telemetry#4677)

* Record exception for asyn servlet invocations * add back accidentally commented out line * rearrange test so that it passes on both jetty and tomcat
RashmiRam · May 23, 2022 · 454f408 · 454f408
1 parent e259ba2
commit 454f408
Show file tree

Hide file tree

Showing 18 changed files with 217 additions and 118 deletions.
diff --git a/...opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncContextStartAdvice.java b/...opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncContextStartAdvice.java
@@ -0,0 +1,27 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.servlet.v3_0;
+
+import static io.opentelemetry.javaagent.instrumentation.servlet.v3_0.Servlet3Singletons.helper;
+
+import javax.servlet.AsyncContext;
+import javax.servlet.ServletRequest;
+import javax.servlet.http.HttpServletRequest;
+import net.bytebuddy.asm.Advice;
+
+@SuppressWarnings("unused")
+public class Servlet3AsyncContextStartAdvice {
+
+  @Advice.OnMethodEnter(suppress = Throwable.class)
+  public static void start(
+      @Advice.This AsyncContext asyncContext,
+      @Advice.Argument(value = 0, readOnly = false) Runnable runnable) {
+    ServletRequest request = asyncContext.getRequest();
+    if (request instanceof HttpServletRequest) {
+      runnable = helper().wrapAsyncRunnable((HttpServletRequest) request, runnable);
+    }
+  }
+}
diff --git a/...o/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java b/...o/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java
@@ -12,6 +12,7 @@
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextInstrumentation;
+import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextStartInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncStartInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterInstrumentation;
 import java.util.List;
@@ -34,12 +35,14 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
   public List<TypeInstrumentation> typeInstrumentations() {
     return asList(
         new AsyncContextInstrumentation(BASE_PACKAGE, adviceClassName(".AsyncDispatchAdvice")),
+        new AsyncContextStartInstrumentation(
+            BASE_PACKAGE, adviceClassName(".Servlet3AsyncContextStartAdvice")),
+        new AsyncStartInstrumentation(BASE_PACKAGE, adviceClassName(".Servlet3AsyncStartAdvice")),
         new ServletAndFilterInstrumentation(
             BASE_PACKAGE,
             adviceClassName(".Servlet3Advice"),
             adviceClassName(".Servlet3InitAdvice"),
-            adviceClassName(".Servlet3FilterInitAdvice")),
-        new AsyncStartInstrumentation(BASE_PACKAGE, adviceClassName(".Servlet3AsyncStartAdvice")));
+            adviceClassName(".Servlet3FilterInitAdvice")));
   }
 
   private static String adviceClassName(String suffix) {

diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServlet3Test.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServlet3Test.groovy
@@ -42,9 +42,13 @@ abstract class JettyServlet3Test extends AbstractServlet3Test<Server, ServletCon
     ServletException
   }
 
+  boolean isAsyncTest() {
+    false
+  }
+
   @Override
   boolean hasResponseSpan(ServerEndpoint endpoint) {
-    return (IS_BEFORE_94 && endpoint == EXCEPTION) || super.hasResponseSpan(endpoint)
+    return (IS_BEFORE_94 && endpoint == EXCEPTION && !isAsyncTest()) || super.hasResponseSpan(endpoint)
   }
 
   @Override
@@ -140,9 +144,8 @@ class JettyServlet3TestAsync extends JettyServlet3Test {
   }
 
   @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
+  boolean isAsyncTest() {
+    true
   }
 }
 
@@ -152,12 +155,6 @@ class JettyServlet3TestFakeAsync extends JettyServlet3Test {
   Class<Servlet> servlet() {
     TestServlet3.FakeAsync
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 class JettyServlet3TestForward extends JettyDispatchTest {
@@ -223,6 +220,11 @@ class JettyServlet3TestDispatchImmediate extends JettyDispatchTest {
     TestServlet3.Sync
   }
 
+  @Override
+  boolean isAsyncTest() {
+    true
+  }
+
   @Override
   protected void setupServlets(ServletContextHandler context) {
     super.setupServlets(context)
@@ -237,12 +239,6 @@ class JettyServlet3TestDispatchImmediate extends JettyDispatchTest {
     addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet3.DispatchImmediate)
     addServlet(context, "/dispatch/recursive", TestServlet3.DispatchRecursive)
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 class JettyServlet3TestDispatchAsync extends JettyDispatchTest {
@@ -251,6 +247,11 @@ class JettyServlet3TestDispatchAsync extends JettyDispatchTest {
     TestServlet3.Async
   }
 
+  @Override
+  boolean isAsyncTest() {
+    true
+  }
+
   @Override
   protected void setupServlets(ServletContextHandler context) {
     super.setupServlets(context)
@@ -270,12 +271,6 @@ class JettyServlet3TestDispatchAsync extends JettyDispatchTest {
   boolean errorEndpointUsesSendError() {
     false
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 abstract class JettyDispatchTest extends JettyServlet3Test {

diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TestServlet3.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TestServlet3.groovy
@@ -5,7 +5,6 @@
 
 import groovy.servlet.AbstractHttpServlet
 import io.opentelemetry.instrumentation.test.base.HttpServerTest
-
 import javax.servlet.RequestDispatcher
 import javax.servlet.ServletException
 import javax.servlet.annotation.WebServlet
@@ -72,6 +71,9 @@ class TestServlet3 {
       HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath)
       def latch = new CountDownLatch(1)
       def context = req.startAsync()
+      if (endpoint == EXCEPTION) {
+        context.setTimeout(5000)
+      }
       context.start {
         try {
           HttpServerTest.controller(endpoint) {
@@ -111,8 +113,7 @@ class TestServlet3 {
               case EXCEPTION:
                 resp.status = endpoint.status
                 resp.writer.print(endpoint.body)
-                context.complete()
-                throw new Exception(endpoint.body)
+                throw new ServletException(endpoint.body)
             }
           }
         } finally {
@@ -157,7 +158,9 @@ class TestServlet3 {
               resp.sendError(endpoint.status, endpoint.body)
               break
             case EXCEPTION:
-              throw new Exception(endpoint.body)
+              resp.status = endpoint.status
+              resp.writer.print(endpoint.body)
+              throw new ServletException(endpoint.body)
           }
         }
       } finally {

diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy
@@ -319,12 +319,6 @@ class TomcatServlet3TestAsync extends TomcatServlet3Test {
   boolean errorEndpointUsesSendError() {
     false
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 class TomcatServlet3TestFakeAsync extends TomcatServlet3Test {
@@ -333,12 +327,6 @@ class TomcatServlet3TestFakeAsync extends TomcatServlet3Test {
   Class<Servlet> servlet() {
     TestServlet3.FakeAsync
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 class TomcatServlet3TestForward extends TomcatDispatchTest {
@@ -459,12 +447,6 @@ class TomcatServlet3TestDispatchAsync extends TomcatDispatchTest {
   boolean errorEndpointUsesSendError() {
     false
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 abstract class TomcatDispatchTest extends TomcatServlet3Test {

diff --git a/...telemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java b/...telemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java
@@ -9,6 +9,7 @@
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextInstrumentation;
+import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextStartInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncStartInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.response.HttpServletResponseInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterInstrumentation;
@@ -28,6 +29,8 @@ public List<TypeInstrumentation> typeInstrumentations() {
     return Arrays.asList(
         new AsyncContextInstrumentation(
             BASE_PACKAGE, adviceClassName(".async.AsyncDispatchAdvice")),
+        new AsyncContextStartInstrumentation(
+            BASE_PACKAGE, adviceClassName(".async.AsyncContextStartAdvice")),
         new AsyncStartInstrumentation(BASE_PACKAGE, adviceClassName(".async.AsyncStartAdvice")),
         new ServletAndFilterInstrumentation(
             BASE_PACKAGE,

diff --git a/...o/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncContextStartAdvice.java b/...o/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncContextStartAdvice.java
@@ -0,0 +1,27 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.async;
+
+import static io.opentelemetry.javaagent.instrumentation.servlet.v5_0.Servlet5Singletons.helper;
+
+import jakarta.servlet.AsyncContext;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.http.HttpServletRequest;
+import net.bytebuddy.asm.Advice;
+
+@SuppressWarnings("unused")
+public class AsyncContextStartAdvice {
+
+  @Advice.OnMethodEnter(suppress = Throwable.class)
+  public static void start(
+      @Advice.This AsyncContext asyncContext,
+      @Advice.Argument(value = 0, readOnly = false) Runnable runnable) {
+    ServletRequest request = asyncContext.getRequest();
+    if (request instanceof HttpServletRequest) {
+      runnable = helper().wrapAsyncRunnable((HttpServletRequest) request, runnable);
+    }
+  }
+}
diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/JettyServlet5Test.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/JettyServlet5Test.groovy
@@ -119,12 +119,6 @@ class JettyServlet5TestAsync extends JettyServlet5Test {
   boolean errorEndpointUsesSendError() {
     false
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 @IgnoreIf({ !jvm.java11Compatible })
@@ -134,12 +128,6 @@ class JettyServlet5TestFakeAsync extends JettyServlet5Test {
   Class<Servlet> servlet() {
     TestServlet5.FakeAsync
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 @IgnoreIf({ !jvm.java11Compatible })
@@ -222,12 +210,6 @@ class JettyServlet5TestDispatchImmediate extends JettyDispatchTest {
     addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet5.DispatchImmediate)
     addServlet(context, "/dispatch/recursive", TestServlet5.DispatchRecursive)
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 @IgnoreIf({ !jvm.java11Compatible })
@@ -256,12 +238,6 @@ class JettyServlet5TestDispatchAsync extends JettyDispatchTest {
   boolean errorEndpointUsesSendError() {
     false
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 abstract class JettyDispatchTest extends JettyServlet5Test {

diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TestServlet5.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TestServlet5.groovy
@@ -72,6 +72,9 @@ class TestServlet5 {
       HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath)
       def latch = new CountDownLatch(1)
       def context = req.startAsync()
+      if (endpoint == EXCEPTION) {
+        context.setTimeout(5000)
+      }
       context.start {
         try {
           HttpServerTest.controller(endpoint) {
@@ -111,8 +114,7 @@ class TestServlet5 {
               case EXCEPTION:
                 resp.status = endpoint.status
                 resp.writer.print(endpoint.body)
-                context.complete()
-                throw new Exception(endpoint.body)
+                throw new ServletException(endpoint.body)
             }
           }
         } finally {
@@ -157,7 +159,9 @@ class TestServlet5 {
               resp.sendError(endpoint.status, endpoint.body)
               break
             case EXCEPTION:
-              throw new Exception(endpoint.body)
+              resp.status = endpoint.status
+              resp.writer.print(endpoint.body)
+              throw new ServletException(endpoint.body)
           }
         }
       } finally {

diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5Test.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5Test.groovy
@@ -318,12 +318,6 @@ class TomcatServlet5TestAsync extends TomcatServlet5Test {
   boolean errorEndpointUsesSendError() {
     false
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 class TomcatServlet5TestFakeAsync extends TomcatServlet5Test {
@@ -332,12 +326,6 @@ class TomcatServlet5TestFakeAsync extends TomcatServlet5Test {
   Class<Servlet> servlet() {
     TestServlet5.FakeAsync
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 class TomcatServlet5TestForward extends TomcatDispatchTest {
@@ -458,12 +446,6 @@ class TomcatServlet5TestDispatchAsync extends TomcatDispatchTest {
   boolean errorEndpointUsesSendError() {
     false
   }
-
-  @Override
-  boolean testException() {
-    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
-    return false
-  }
 }
 
 abstract class TomcatDispatchTest extends TomcatServlet5Test {