move servlet context path handling

open-telemetry · Feb 10, 2021 · d03281e · d03281e
1 parent bb98885
commit d03281e
Show file tree

Hide file tree

Showing 6 changed files with 49 additions and 124 deletions.
diff --git a/...t-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/...t-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java
@@ -9,6 +9,7 @@
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.propagation.TextMapPropagator.Getter;
 import io.opentelemetry.instrumentation.api.servlet.AppServerBridge;
+import io.opentelemetry.instrumentation.api.servlet.ServletContextPath;
 import io.opentelemetry.instrumentation.api.tracer.HttpServerTracer;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.net.URI;
@@ -25,7 +26,16 @@ public abstract class ServletHttpServerTracer<RESPONSE>
   private static final Logger log = LoggerFactory.getLogger(ServletHttpServerTracer.class);
 
   public Context startSpan(HttpServletRequest request) {
-    return startSpan(request, request, request, getSpanName(request));
+    Context context = startSpan(request, request, request, getSpanName(request));
+    return addServletContextPath(context, request);
+  }
+
+  private static Context addServletContextPath(Context context, HttpServletRequest request) {
+    String contextPath = request.getContextPath();
+    if (contextPath != null && !contextPath.isEmpty() && !contextPath.equals("/")) {
+      return context.with(ServletContextPath.CONTEXT_KEY, contextPath);
+    }
+    return context;
   }
 
   @Override
@@ -153,11 +163,13 @@ public static String getSpanName(HttpServletRequest request) {
    * forward and other scenarios, where servlet path may change, but we don't want this to be
    * reflected in the span name.
    */
-  public void updateServerSpanNameOnce(Context attachedContext, HttpServletRequest request) {
-    if (AppServerBridge.shouldUpdateServerSpanName(attachedContext)) {
-      updateSpanName(Span.fromContext(attachedContext), request);
-      AppServerBridge.setServletUpdatedServerSpanName(attachedContext, true);
+  public Context runOnceUnderAppServer(Context context, HttpServletRequest request) {
+    if (AppServerBridge.shouldUpdateServerSpanName(context)) {
+      updateSpanName(Span.fromContext(context), request);
+      AppServerBridge.setServletUpdatedServerSpanName(context, true);
+      return addServletContextPath(context, request);
     }
+    return context;
   }
 
   public void updateSpanName(HttpServletRequest request) {

diff --git a/...src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java b/...src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java
@@ -37,7 +37,11 @@ public static void onEnter(
 
     Context serverContext = tracer().getServerContext(httpServletRequest);
     if (serverContext != null) {
-      tracer().updateServerSpanNameOnce(serverContext, httpServletRequest);
+      Context updatedContext = tracer().runOnceUnderAppServer(serverContext, httpServletRequest);
+      if (updatedContext != serverContext) {
+        // runOnceUnderAppServer updated context, need to re-scope
+        scope = updatedContext.makeCurrent();
+      }
       return;
     }
 

diff --git a/...src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java b/...src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java
@@ -36,19 +36,35 @@ public static void onEnter(
 
     Context attachedContext = tracer().getServerContext(httpServletRequest);
     if (attachedContext != null) {
+      // We are inside nested servlet/filter/app-server span, don't create new span
       if (Servlet3HttpServerTracer.needsRescoping(attachedContext)) {
+        attachedContext = tracer().runOnceUnderAppServer(attachedContext, httpServletRequest);
         scope = attachedContext.makeCurrent();
+        return;
       }
 
-      tracer().updateServerSpanNameOnce(attachedContext, httpServletRequest);
-      // We are inside nested servlet/filter/app-server span, don't create new span
+      // We already have attached context to request but this could have been done by app server
+      // instrumentation, if needed update span with info from current request.
+      Context currentContext = Java8BytecodeBridge.currentContext();
+      Context updatedContext = tracer().runOnceUnderAppServer(currentContext, httpServletRequest);
+      if (updatedContext != currentContext) {
+        // runOnceUnderAppServer updated context, need to re-scope
+        scope = updatedContext.makeCurrent();
+      }
       return;
     }
 
-    Context parentContext = Java8BytecodeBridge.currentContext();
-    if (parentContext != null && Java8BytecodeBridge.spanFromContext(parentContext).isRecording()) {
-      tracer().updateServerSpanNameOnce(parentContext, httpServletRequest);
-      // We are inside nested servlet/filter/app-server span, don't create new span
+    Context currentContext = Java8BytecodeBridge.currentContext();
+    if (currentContext != null
+        && Java8BytecodeBridge.spanFromContext(currentContext).isRecording()) {
+      // We already have a span but it was not created by servlet instrumentation.
+      // In case it was created by app server integration we need to update it with info from
+      // current request.
+      Context updatedContext = tracer().runOnceUnderAppServer(currentContext, httpServletRequest);
+      if (currentContext != updatedContext) {
+        // runOnceUnderAppServer updated context, need to re-scope
+        scope = updatedContext.makeCurrent();
+      }
       return;
     }
 

diff --git a/...avaagent/instrumentation/servlet/contextpath/ServletContextPathInstrumentationModule.java b/...avaagent/instrumentation/servlet/contextpath/ServletContextPathInstrumentationModule.java
diff --git a/...c/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatHandlerTest.groovy b/...c/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatHandlerTest.groovy
@@ -5,7 +5,7 @@
 
 package io.opentelemetry.javaagent.instrumentation.tomcat7
 
-import static io.opentelemetry.api.trace.Span.Kind.INTERNAL
+import static io.opentelemetry.api.trace.SpanKind.INTERNAL
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT

diff --git a/...g-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy b/...g-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy
@@ -445,23 +445,23 @@ abstract class HttpServerTest<SERVER> extends AgentInstrumentationSpecification
   void redirectSpan(TraceAssert trace, int index, Object parent) {
     trace.span(index) {
       name ~/\.sendRedirect$/
-      kind Span.Kind.INTERNAL
+      kind SpanKind.INTERNAL
       childOf((SpanData) parent)
     }
   }
 
   void sendErrorSpan(TraceAssert trace, int index, Object parent) {
     trace.span(index) {
       name ~/\.sendError$/
-      kind Span.Kind.INTERNAL
+      kind SpanKind.INTERNAL
       childOf((SpanData) parent)
     }
   }
 
   void forwardSpan(TraceAssert trace, int index, Object parent, String errorMessage = null, Class exceptionClass = Exception) {
     trace.span(index) {
       name ~/\.forward$/
-      kind Span.Kind.INTERNAL
+      kind SpanKind.INTERNAL
       errored errorMessage != null
       if (errorMessage) {
         errorEvent(exceptionClass, errorMessage)
@@ -473,7 +473,7 @@ abstract class HttpServerTest<SERVER> extends AgentInstrumentationSpecification
   void includeSpan(TraceAssert trace, int index, Object parent, String errorMessage = null, Class exceptionClass = Exception) {
     trace.span(index) {
       name ~/\.include$/
-      kind Span.Kind.INTERNAL
+      kind SpanKind.INTERNAL
       errored errorMessage != null
       if (errorMessage) {
         errorEvent(exceptionClass, errorMessage)