diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java index 34af542379..3ac1096a54 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java @@ -8,6 +8,7 @@ package io.opentelemetry.instrumentation.api.servlet; import io.opentelemetry.context.Context; import io.opentelemetry.context.ContextKey; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; /** * Helper container for Context attributes for transferring certain information between servlet @@ -39,7 +40,15 @@ public class AppServerBridge { * @return new context with AppServerBridge attached. */ public static Context init(Context ctx, boolean shouldRecordException) { - return ctx.with(AppServerBridge.CONTEXT_KEY, new AppServerBridge(shouldRecordException)); + Context context = + ctx.with(AppServerBridge.CONTEXT_KEY, new AppServerBridge(shouldRecordException)); + // Add context for storing servlet context path. Servlet context path is updated from servlet + // integration. + if (context.get(ServletContextPath.CONTEXT_KEY) == null) { + context = context.with(ServletContextPath.CONTEXT_KEY, new AtomicReference<>(null)); + } + + return context; } private final AtomicBoolean servletUpdatedServerSpanName = new AtomicBoolean(false); diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletContextPath.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletContextPath.java index d400349334..fcfcbafb89 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletContextPath.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletContextPath.java @@ -7,6 +7,7 @@ package io.opentelemetry.instrumentation.api.servlet; import io.opentelemetry.context.Context; import io.opentelemetry.context.ContextKey; +import java.util.concurrent.atomic.AtomicReference; /** * The context key here is used to propagate the servlet context path throughout the request, so @@ -22,11 +23,12 @@ public class ServletContextPath { // Keeps track of the servlet context path that needs to be prepended to the route when updating // the span name - public static final ContextKey CONTEXT_KEY = + public static final ContextKey> CONTEXT_KEY = ContextKey.named("opentelemetry-servlet-context-path-key"); public static String prepend(Context context, String spanName) { - String value = context.get(CONTEXT_KEY); + AtomicReference valueReference = context.get(CONTEXT_KEY); + String value = valueReference != null ? valueReference.get() : null; // checking isEmpty just to avoid unnecessary string concat / allocation if (value != null && !value.isEmpty()) { return value + spanName; diff --git a/instrumentation-core/servlet-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation-core/servlet-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index e1b96f7c55..ea89803fb4 100644 --- a/instrumentation-core/servlet-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation-core/servlet-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -15,6 +15,7 @@ import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; import java.net.URI; import java.net.URISyntaxException; import java.security.Principal; +import java.util.concurrent.atomic.AtomicReference; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import org.slf4j.Logger; @@ -27,9 +28,13 @@ public abstract class ServletHttpServerTracer public Context startSpan(HttpServletRequest request) { Context context = startSpan(request, request, request, getSpanName(request)); + return addContextPathContext(context, request); + } + + protected Context addContextPathContext(Context context, HttpServletRequest request) { String contextPath = request.getContextPath(); if (contextPath != null && !contextPath.isEmpty() && !contextPath.equals("/")) { - context = context.with(ServletContextPath.CONTEXT_KEY, contextPath); + context = context.with(ServletContextPath.CONTEXT_KEY, new AtomicReference<>(contextPath)); } return context; } @@ -153,19 +158,41 @@ public abstract class ServletHttpServerTracer return contextPath + servletPath; } + /** + * When server spans are managed by app server instrumentation servlet instrumentation should set + * information that was not available from app server instrumentation. + */ + public void updateServerSpan(Context attachedContext, HttpServletRequest request) { + updateServerSpanName(attachedContext, request); + updateContextPath(attachedContext, request); + } + /** * When server spans are managed by app server instrumentation, servlet must update server span * name only once and only during the first pass through the servlet stack. There are potential * 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) { + private void updateServerSpanName(Context attachedContext, HttpServletRequest request) { + // Update name only when server span wasn't created by servlet integration + // and has not been already updated if (AppServerBridge.shouldUpdateServerSpanName(attachedContext)) { updateSpanName(Span.fromContext(attachedContext), request); AppServerBridge.setServletUpdatedServerSpanName(attachedContext, true); } } + private void updateContextPath(Context attachedContext, HttpServletRequest request) { + // Update context path if it isn't already set + AtomicReference reference = attachedContext.get(ServletContextPath.CONTEXT_KEY); + if (reference != null && reference.get() == null) { + String contextPath = request.getContextPath(); + if (contextPath != null && !contextPath.isEmpty() && !contextPath.equals("/")) { + reference.compareAndSet(null, contextPath); + } + } + } + public void updateSpanName(HttpServletRequest request) { updateSpanName(getServerSpan(request), request); } diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java index 3120d1c014..72f86b9ac1 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java @@ -6,7 +6,6 @@ package io.opentelemetry.javaagent.instrumentation.liberty; import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; import io.opentelemetry.javaagent.instrumentation.servlet.v3_0.Servlet3HttpServerTracer; import javax.servlet.http.HttpServletRequest; @@ -23,11 +22,7 @@ public class LibertyHttpServerTracer extends Servlet3HttpServerTracer { // span name will be updated a bit later when calling request.getServletPath() works // see LibertyUpdateSpanAdvice Context context = startSpan(request, request, request, "HTTP " + request.getMethod()); - String contextPath = request.getContextPath(); - if (contextPath != null && !contextPath.isEmpty() && !contextPath.equals("/")) { - context = context.with(ServletContextPath.CONTEXT_KEY, contextPath); - } - return context; + return addContextPathContext(context, request); } @Override diff --git a/instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java b/instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java index 5496e2ab6e..fccd427897 100644 --- a/instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java +++ b/instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java @@ -37,7 +37,7 @@ public class Servlet2Advice { Context serverContext = tracer().getServerContext(httpServletRequest); if (serverContext != null) { - tracer().updateServerSpanNameOnce(serverContext, httpServletRequest); + tracer().updateServerSpan(serverContext, httpServletRequest); return; } diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java index c871350cd7..877620951e 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java @@ -40,14 +40,14 @@ public class Servlet3Advice { scope = attachedContext.makeCurrent(); } - tracer().updateServerSpanNameOnce(attachedContext, httpServletRequest); + tracer().updateServerSpan(attachedContext, httpServletRequest); // We are inside nested servlet/filter/app-server span, don't create new span return; } Context parentContext = Java8BytecodeBridge.currentContext(); if (parentContext != null && Java8BytecodeBridge.spanFromContext(parentContext).isRecording()) { - tracer().updateServerSpanNameOnce(parentContext, httpServletRequest); + tracer().updateServerSpan(parentContext, httpServletRequest); // We are inside nested servlet/filter/app-server span, don't create new span return; } diff --git a/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy b/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy index 8a7c388d07..a465273c48 100644 --- a/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy +++ b/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy @@ -14,6 +14,7 @@ import io.opentelemetry.semconv.trace.attributes.SemanticAttributes import javax.servlet.DispatcherType import org.apache.struts2.dispatcher.ng.filter.StrutsPrepareAndExecuteFilter import org.eclipse.jetty.server.Server +import org.eclipse.jetty.server.handler.HandlerCollection import org.eclipse.jetty.servlet.DefaultServlet import org.eclipse.jetty.servlet.ServletContextHandler import org.eclipse.jetty.util.resource.FileResource @@ -79,7 +80,11 @@ class Struts2ActionSpanTest extends HttpServerTest { context.setContextPath(getContextPath()) def resource = new FileResource(getClass().getResource("/")) context.setBaseResource(resource) - server.setHandler(context) + // jetty integration is disabled for some handler classes, using HandlerCollection here + // enables jetty integration + HandlerCollection handlerCollection = new HandlerCollection() + handlerCollection.addHandler(context) + server.setHandler(handlerCollection) context.addServlet(DefaultServlet, "/") context.addFilter(StrutsPrepareAndExecuteFilter, "/*", EnumSet.of(DispatcherType.REQUEST))