Move servlet instrumentations around (#601)

README.md

@@ -146,7 +146,7 @@ provide the path to a JAR file including an SPI implementation using the system
 | [Rediscala](https://github.com/etaty/rediscala)                                                                                       | 1.8+                           |
 | [RMI](https://docs.oracle.com/en/java/javase/11/docs/api/java.rmi/java/rmi/package-summary.html)                                      | Java 7+                        |
 | [RxJava](https://github.com/ReactiveX/RxJava)                                                                                         | 1.0+                           |
-| [Servlet](https://javaee.github.io/javaee-spec/javadocs/javax/servlet/package-summary.html)                                           | 2.3+                           |
+| [Servlet](https://javaee.github.io/javaee-spec/javadocs/javax/servlet/package-summary.html)                                           | 2.2+                           |
 | [Spark Web Framework](https://github.com/perwendel/spark)                                                                             | 2.3+                           |
 | [Spring Data](https://spring.io/projects/spring-data)                                                                                 | 1.8+                           |
 | [Spring Scheduling](https://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/scheduling/package-summary.html)       | 3.1+                           |

instrumentation-core/servlet/servlet.gradle

@@ -0,0 +1,13 @@
+group = 'io.opentelemetry.instrumentation'
+
+apply from: "$rootDir/gradle/java.gradle"
+
+dependencies {
+  api deps.opentelemetryApi
+  api project(':auto-bootstrap')
+
+  implementation deps.slf4j
+
+  compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.3'
+}
+

instrumentation-core/servlet/src/main/java/io/opentelemetry/instrumentation/servlet/HttpServletRequestGetter.java

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-package io.opentelemetry.auto.instrumentation.servlet;
+package io.opentelemetry.instrumentation.servlet;
 
 import io.opentelemetry.context.propagation.HttpTextFormat;
 import javax.servlet.http.HttpServletRequest;

instrumentation-core/servlet/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-package io.opentelemetry.auto.instrumentation.servlet;
+package io.opentelemetry.instrumentation.servlet;
 
 import io.grpc.Context;
 import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerTracer;

instrumentation/jetty-8.0/src/main/java/io/opentelemetry/auto/instrumentation/jetty/JettyHandlerInstrumentation.java

@@ -56,8 +56,8 @@ public final class JettyHandlerInstrumentation extends Instrumenter.Default {
       packageName + ".JettyHttpServerTracer",
       "io.opentelemetry.auto.instrumentation.servlet.v3_0.Servlet3HttpServerTracer",
       "io.opentelemetry.auto.instrumentation.servlet.v3_0.TagSettingAsyncListener",
-      "io.opentelemetry.auto.instrumentation.servlet.ServletHttpServerTracer",
-      "io.opentelemetry.auto.instrumentation.servlet.HttpServletRequestGetter",
+      "io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer",
+      "io.opentelemetry.instrumentation.servlet.HttpServletRequestGetter",
     };
   }
 

instrumentation/servlet/README.md

@@ -1,5 +1,19 @@
 # Instrumentation for Java Servlets
 
+## A word about version
+
+We support Servlet API starting from version 2.2.
+But various instrumentations apply to different versions of the API.
+They are divided into 3 sub-modules:
+
+`servlet-common` contains instrumentations applicable to all API versions that we support.
+
+`servlet-2.2` contains instrumentations applicable to Servlet API 2.2, but not to to 3+.
+
+`servlet-3.0` contains instrumentations that require Servlet API 3.0 or newer.
+
+## Implementation details
+
 In order to fully understand how java servlet instrumentation work,
 let us first take a look at the following stacktrace from Spring PetClinic application.
 Unimportant frames are redacted, points of interests are highlighted and discussed below.
@@ -66,9 +80,23 @@ Span created by Spring MVC instrumentation will have `kind=INTERNAL` and named `
 The state described above has one significant problem.
 Observability backends usually aggregate traces based on their root spans.
 This means that ALL traces from any application deployed to Servlet container will be grouped together.
-Becaue their root spans will all have the same named based on common entry point.
+Because their root spans will all have the same named based on common entry point.
 In order to alleviate this problem, instrumentations for specific frameworks, such as Spring MVC here,
 _update_ name of the span corresponding to the entry point.
 Each framework instrumentation can decide what is the best span name based on framework implementation details.
 Of course, still adhering to OpenTelemetry
 [semantic conventions](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md).
+
+## Additional instrumentations
+`RequestDispatcherInstrumentation` instruments `javax.servlet.RequestDispatcher.forward` and
+`javax.servlet.RequestDispatcher.include` methods to create new `INTERNAL` spans around their
+invocations.
+
+`ServletContextInstrumentation` instruments `javax.servlet.ServletContext.getRequestDispatcher` and
+`javax.servlet.ServletContext.getNamedDispatcher`. The only job of this instrumentation is to
+preserve the input parameter of those methods and to make that available for `RequestDispatcherInstrumentation`
+described above. The latter uses that name for `dispatcher.target` span attribute.
+
+`HttpServletResponseInstrumentation` instruments `javax.servlet.http.HttpServletResponse.sendError`
+and `javax.servlet.http.HttpServletResponse.sendRedirect` methods to create new `INTERNAL` spans
+around their invocations.

instrumentation/servlet/servlet-2.2/servlet-2.2.gradle

@@ -24,7 +24,7 @@ testSets {
 
 dependencies {
   compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.2'
-  implementation(project(':instrumentation:servlet:servlet-common'))
+  api(project(':instrumentation-core:servlet'))
 
   testImplementation(project(':testing')) {
     exclude group: 'org.eclipse.jetty', module: 'jetty-server'

instrumentation/servlet/servlet-2.2/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_2/Servlet2Advice.java

@@ -14,9 +14,9 @@
  * limitations under the License.
  */
 
-package io.opentelemetry.auto.instrumentation.servlet.v2_3;
+package io.opentelemetry.auto.instrumentation.servlet.v2_2;
 
-import static io.opentelemetry.auto.instrumentation.servlet.v2_3.Servlet2HttpServerTracer.TRACER;
+import static io.opentelemetry.auto.instrumentation.servlet.v2_2.Servlet2HttpServerTracer.TRACER;
 
 import io.opentelemetry.auto.bootstrap.InstrumentationContext;
 import io.opentelemetry.context.Scope;

instrumentation/servlet/servlet-2.2/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_2/Servlet2HttpServerTracer.java

@@ -14,14 +14,14 @@
  * limitations under the License.
  */
 
-package io.opentelemetry.auto.instrumentation.servlet.v2_3;
+package io.opentelemetry.auto.instrumentation.servlet.v2_2;
 
-import io.opentelemetry.auto.instrumentation.servlet.ServletHttpServerTracer;
+import io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer;
 
 public class Servlet2HttpServerTracer extends ServletHttpServerTracer {
   public static final Servlet2HttpServerTracer TRACER = new Servlet2HttpServerTracer();
 
   protected String getInstrumentationName() {
-    return "io.opentelemetry.auto.servlet-2.3";
+    return "io.opentelemetry.auto.servlet-2.2";
   }
 }

instrumentation/servlet/servlet-2.2/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_2/Servlet2Instrumentation.java

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-package io.opentelemetry.auto.instrumentation.servlet.v2_3;
+package io.opentelemetry.auto.instrumentation.servlet.v2_2;
 
 import static io.opentelemetry.auto.tooling.ClassLoaderMatcher.hasClassesNamed;
 import static io.opentelemetry.auto.tooling.bytebuddy.matcher.AgentElementMatchers.safeHasSuperType;
@@ -61,8 +61,8 @@ public final class Servlet2Instrumentation extends Instrumenter.Default {
   @Override
   public String[] helperClassNames() {
     return new String[] {
-      "io.opentelemetry.auto.instrumentation.servlet.ServletHttpServerTracer",
-      "io.opentelemetry.auto.instrumentation.servlet.HttpServletRequestGetter",
+      "io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer",
+      "io.opentelemetry.instrumentation.servlet.HttpServletRequestGetter",
       packageName + ".Servlet2HttpServerTracer"
     };
   }

instrumentation/servlet/servlet-2.2/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_2/Servlet2ResponseStatusInstrumentation.java

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-package io.opentelemetry.auto.instrumentation.servlet.v2_3;
+package io.opentelemetry.auto.instrumentation.servlet.v2_2;
 
 import static io.opentelemetry.auto.tooling.ClassLoaderMatcher.hasClassesNamed;
 import static io.opentelemetry.auto.tooling.bytebuddy.matcher.AgentElementMatchers.safeHasSuperType;

instrumentation/servlet/servlet-2.2/src/test/groovy/JettyServlet2Test.groovy

@@ -17,19 +17,20 @@
 import io.opentelemetry.auto.instrumentation.api.MoreAttributes
 import io.opentelemetry.auto.test.asserts.TraceAssert
 import io.opentelemetry.auto.test.base.HttpServerTest
+import io.opentelemetry.sdk.trace.data.SpanData
+import javax.servlet.http.HttpServletRequest
 import io.opentelemetry.trace.attributes.SemanticAttributes
 import org.eclipse.jetty.server.Server
 import org.eclipse.jetty.server.handler.ErrorHandler
 import org.eclipse.jetty.servlet.ServletContextHandler
 
-import javax.servlet.http.HttpServletRequest
-
 import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED
 import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.ERROR
 import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
 import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
 import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.REDIRECT
 import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.SUCCESS
+import static io.opentelemetry.trace.Span.Kind.INTERNAL
 import static io.opentelemetry.trace.Span.Kind.SERVER
 
 class JettyServlet2Test extends HttpServerTest<Server> {
@@ -83,6 +84,18 @@ class JettyServlet2Test extends HttpServerTest<Server> {
     false
   }
 
+  @Override
+  void responseSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) {
+    trace.span(index) {
+      operationName endpoint == REDIRECT ? "HttpServletResponse.sendRedirect" : "HttpServletResponse.sendError"
+      spanKind INTERNAL
+      errored false
+      childOf((SpanData) parent)
+      attributes {
+      }
+    }
+  }
+
   // parent span must be cast otherwise it breaks debugging classloading (junit loads it early)
   void serverSpan(TraceAssert trace, int index, String traceID = null, String parentID = null, String method = "GET", ServerEndpoint endpoint = SUCCESS) {
     trace.span(index) {

instrumentation/servlet/servlet-3.0/servlet-3.0.gradle

@@ -22,8 +22,8 @@ testSets {
 }
 
 dependencies {
-  api(project(':instrumentation:servlet:servlet-common'))
   compileOnly group: 'javax.servlet', name: 'javax.servlet-api', version: '3.0.1'
+  api(project(':instrumentation-core:servlet'))
 
   testImplementation(project(':testing')) {
     exclude group: 'org.eclipse.jetty', module: 'jetty-server'

instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/AsyncContextInstrumentation.java

@@ -48,8 +48,8 @@ public final class AsyncContextInstrumentation extends Instrumenter.Default {
   @Override
   public String[] helperClassNames() {
     return new String[] {
-      "io.opentelemetry.auto.instrumentation.servlet.HttpServletRequestGetter",
-      "io.opentelemetry.auto.instrumentation.servlet.ServletHttpServerTracer",
+      "io.opentelemetry.instrumentation.servlet.HttpServletRequestGetter",
+      "io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer",
       packageName + ".Servlet3HttpServerTracer"
     };
   }

instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java

@@ -19,7 +19,7 @@ package io.opentelemetry.auto.instrumentation.servlet.v3_0;
 import static io.opentelemetry.trace.TracingContextUtils.getSpan;
 
 import io.grpc.Context;
-import io.opentelemetry.auto.instrumentation.servlet.ServletHttpServerTracer;
+import io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer;
 import io.opentelemetry.trace.Span;
 import io.opentelemetry.trace.Status;
 import javax.servlet.http.HttpServletRequest;

instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Instrumentation.java

@@ -52,8 +52,8 @@ public final class Servlet3Instrumentation extends Instrumenter.Default {
   @Override
   public String[] helperClassNames() {
     return new String[] {
-      "io.opentelemetry.auto.instrumentation.servlet.HttpServletRequestGetter",
-      "io.opentelemetry.auto.instrumentation.servlet.ServletHttpServerTracer",
+      "io.opentelemetry.instrumentation.servlet.HttpServletRequestGetter",
+      "io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer",
       packageName + ".Servlet3HttpServerTracer",
       packageName + ".TagSettingAsyncListener"
     };

instrumentation/servlet/servlet-common/servlet-common.gradle

@@ -15,7 +15,7 @@ testSets {
 }
 
 dependencies {
-  compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.2'
+  compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.3'
 
   testImplementation(project(':testing')) {
     exclude group: 'org.eclipse.jetty', module: 'jetty-server'

instrumentation/servlet/servlet-common/src/test/groovy/HttpServletResponseTest.groovy

@@ -221,6 +221,11 @@ class HttpServletResponseTest extends AgentTestRunner {
       return null
     }
 
+    @Override
+    String getContentType() {
+      return null
+    }
+
     @Override
     ServletOutputStream getOutputStream() throws IOException {
       return null
@@ -231,6 +236,11 @@ class HttpServletResponseTest extends AgentTestRunner {
       return null
     }
 
+    @Override
+    void setCharacterEncoding(String charset) {
+
+    }
+
     @Override
     void setContentLength(int i) {
 

instrumentation/servlet/servlet-common/src/test/groovy/RequestDispatcherUtils.java

@@ -68,6 +68,11 @@ public class RequestDispatcherUtils {
   }
 
   class TestContext implements ServletContext {
+    @Override
+    public String getContextPath() {
+      return null;
+    }
+
     @Override
     public ServletContext getContext(final String s) {
       return null;

instrumentation/servlet/servlet.gradle

@@ -1,31 +0,0 @@
-apply from: "$rootDir/gradle/instrumentation.gradle"
-
-muzzle {
-  pass {
-    group = "javax.servlet"
-    module = 'javax.servlet-api'
-    versions = "[,]"
-    assertInverse = true
-  }
-  pass {
-    group = "javax.servlet"
-    module = 'servlet-api'
-    versions = "[,]"
-    skipVersions += '0'
-    assertInverse = true
-  }
-}
-
-dependencies {
-  compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.3'
-
-  testImplementation group: 'javax.servlet', name: 'servlet-api', version: '2.3'
-
-  // servlet request instrumentation required for linking request to response.
-  testImplementation project(':instrumentation:servlet:servlet-2.3')
-
-  // Don't want to conflict with jetty from the test server.
-  testImplementation(project(':testing')) {
-    exclude group: 'org.eclipse.jetty', module: 'jetty-server'
-  }
-}

instrumentation/spring-webmvc-3.1/spring-webmvc-3.1.gradle

@@ -32,7 +32,6 @@ dependencies {
   }
 
   // Include servlet instrumentation for verifying the tomcat requests
-  testImplementation project(':instrumentation:servlet')
   testImplementation project(':instrumentation:servlet:servlet-3.0')
 
   testImplementation group: 'javax.validation', name: 'validation-api', version: '1.1.0.Final'

instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy

@@ -58,11 +58,6 @@ class SpringBootBasedTest extends HttpServerTest<ConfigurableApplicationContext>
     true
   }
 
-  @Override
-  boolean hasResponseSpan(ServerEndpoint endpoint) {
-    endpoint == REDIRECT
-  }
-
   @Override
   boolean hasRenderSpan(ServerEndpoint endpoint) {
     endpoint == REDIRECT
@@ -103,14 +98,8 @@ class SpringBootBasedTest extends HttpServerTest<ConfigurableApplicationContext>
       trace(0, 1) {
         basicSpan(it, 0, "TEST_SPAN")
       }
-      trace(1, 2) {
+      trace(1, 1) {
         serverSpan(it, 0, null, null, "POST", LOGIN)
-        span(1) {
-          operationName "HttpServletResponse.sendRedirect"
-          childOf span(0)
-          attributes {
-          }
-        }
       }
     }
 

instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy

@@ -29,7 +29,6 @@ import test.boot.SecurityConfig
 import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.ERROR
 import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
 import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
-import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.REDIRECT
 import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.SUCCESS
 import static io.opentelemetry.trace.Span.Kind.INTERNAL
 import static io.opentelemetry.trace.Span.Kind.SERVER
@@ -55,11 +54,6 @@ class ServletFilterTest extends HttpServerTest<ConfigurableApplicationContext> {
     false
   }
 
-  @Override
-  boolean hasResponseSpan(ServerEndpoint endpoint) {
-    endpoint == REDIRECT || endpoint == ERROR
-  }
-
   @Override
   boolean hasErrorPageSpans(ServerEndpoint endpoint) {
     endpoint == ERROR || endpoint == EXCEPTION
@@ -83,13 +77,16 @@ class ServletFilterTest extends HttpServerTest<ConfigurableApplicationContext> {
   }
 
   @Override
-  void responseSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) {
+  void handlerSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) {
     trace.span(index) {
-      operationName endpoint == REDIRECT ? "HttpServletResponse.sendRedirect" : "HttpServletResponse.sendError"
+      operationName "TestController.${endpoint.name().toLowerCase()}"
       spanKind INTERNAL
-      errored false
+      errored endpoint == EXCEPTION
       childOf((SpanData) parent)
       attributes {
+        if (endpoint == EXCEPTION) {
+          errorTags(Exception, EXCEPTION.body)
+        }
       }
     }
   }
@@ -130,19 +127,10 @@ class ServletFilterTest extends HttpServerTest<ConfigurableApplicationContext> {
   @Override
   void errorPageSpans(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) {
     trace.span(index) {
-      operationName "/error"
-      spanKind INTERNAL
-      errored false
-      childOf((SpanData) parent)
-      attributes {
-        "dispatcher.target" "/error"
-      }
-    }
-    trace.span(index + 1) {
       operationName "BasicErrorController.error"
       spanKind INTERNAL
       errored false
-      childOf trace.spans[index]
+      childOf((SpanData) parent)
       attributes {
       }
     }

settings.gradle

@@ -133,10 +133,9 @@ include ':instrumentation:reactor-3.1'
 include ':instrumentation:rediscala-1.8'
 include ':instrumentation:rmi'
 include ':instrumentation:rxjava-1.0'
-include ':instrumentation:servlet'
 include ':instrumentation:servlet:glassfish-testing'
 include ':instrumentation:servlet:servlet-common'
-include ':instrumentation:servlet:servlet-2.3'
+include ':instrumentation:servlet:servlet-2.2'
 include ':instrumentation:servlet:servlet-3.0'
 // FIXME this instrumentation relied on scope listener
 // include ':instrumentation:slf4j-mdc'
@@ -151,6 +150,7 @@ include ':instrumentation:vertx-3.0'
 include ':instrumentation:vertx-reactive-3.5'
 
 include ':instrumentation-core:aws-sdk:aws-sdk-2.2'
+include ':instrumentation-core:servlet'
 include ':instrumentation-core:spring'
 
 // exporter adapters

testing/src/main/groovy/io/opentelemetry/auto/test/base/HttpServerTest.groovy

@@ -389,7 +389,7 @@ abstract class HttpServerTest<SERVER> extends AgentTestRunner {
         spanCount++
       }
       if (hasErrorPageSpans(endpoint)) {
-        spanCount += 2
+        spanCount ++
       }
     }
     assertTraces(size * 2) {