Capture servlet dispatch as INTERNAL span
MODIFY
instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet3/AsyncContextInstrumentation.java
instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet3/AsyncContextInstrumentation.java
package io.opentelemetry.auto.instrumentation.servlet3;
import static io.opentelemetry.auto.decorator.HttpServerDecorator.SPAN_ATTRIBUTE;
-import static io.opentelemetry.auto.instrumentation.servlet3.HttpServletRequestInjectAdapter.SETTER;
+import static io.opentelemetry.auto.instrumentation.servlet3.Servlet3Decorator.TRACER;
import static io.opentelemetry.auto.tooling.ByteBuddyElementMatchers.safeHasSuperType;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isInterface;
@@ -17,7 +17,6 @@ import io.opentelemetry.trace.Span;
import java.util.Map;
import javax.servlet.AsyncContext;
import javax.servlet.ServletRequest;
-import javax.servlet.http.HttpServletRequest;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
@@ -36,8 +35,7 @@ public final class AsyncContextInstrumentation extends Instrumenter.Default {
"io.opentelemetry.auto.decorator.BaseDecorator",
"io.opentelemetry.auto.decorator.ServerDecorator",
"io.opentelemetry.auto.decorator.HttpServerDecorator",
- packageName + ".Servlet3Decorator",
- packageName + ".HttpServletRequestInjectAdapter"
+ packageName + ".Servlet3Decorator"
};
}
@@ -68,18 +66,6 @@ public final class AsyncContextInstrumentation extends Instrumenter.Default {
return false;
}
- final ServletRequest request = context.getRequest();
- final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE);
- if (spanAttr instanceof Span) {
- request.removeAttribute(SPAN_ATTRIBUTE);
- final Span span = (Span) spanAttr;
- // Override propagation headers by injecting attributes from the current span
- // into the new request
- if (request instanceof HttpServletRequest) {
- Servlet3Decorator.TRACER
- .getHttpTextFormat()
- .inject(span.getContext(), (HttpServletRequest) request, SETTER);
- }
final String path;
if (args.length == 1 && args[0] instanceof String) {
path = (String) args[0];
@@ -88,8 +74,25 @@ public final class AsyncContextInstrumentation extends Instrumenter.Default {
} else {
path = "true";
}
- span.setAttribute("servlet.dispatch", path);
+
+ final ServletRequest request = context.getRequest();
+
+ // this tells the dispatched servlet that it is already part of an existing servlet request,
+ // so that it will only capture an INTERNAL span to represent the dispatch work
+ request.setAttribute("io.opentelemetry.auto.servlet.dispatch", path);
+
+ final Span currentSpan = TRACER.getCurrentSpan();
+ if (currentSpan.getContext().isValid()) {
+ // this tells the dispatched servlet to use the current span as the parent for its work
+ // (if the currentSpan is not valid for some reason, the original servlet span should still
+ // be present in the same request attribute, and so that will be used)
+ //
+ // the original servlet span stored in the same request attribute does not need to be saved
+ // and restored on method exit, because dispatch() hands off control of the request
+ // processing, and nothing can be done with the request anymore after this
+ request.setAttribute(SPAN_ATTRIBUTE, currentSpan);
}
+
return true;
}

MODIFY
instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet3/HttpServletRequestExtractAdapter.java
instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet3/HttpServletRequestExtractAdapter.java
@@ -10,14 +10,6 @@ public class HttpServletRequestExtractAdapter implements HttpTextFormat.Getter<H
@Override
public String get(final HttpServletRequest carrier, final String key) {
- /*
- * Read from the attributes and override the headers.
- * This is used by HttpServletRequestSetter when a request is async-dispatched.
- */
- final Object attribute = carrier.getAttribute(key);
- if (attribute instanceof String) {
- return (String) attribute;
- }
return carrier.getHeader(key);
}
}

DELETE
instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet3/HttpServletRequestInjectAdapter.java
-package io.opentelemetry.auto.instrumentation.servlet3;
-
-import io.opentelemetry.context.propagation.HttpTextFormat;
-import javax.servlet.http.HttpServletRequest;
-
-/** Inject into request attributes since the request headers can't be modified. */
-public class HttpServletRequestInjectAdapter implements HttpTextFormat.Setter<HttpServletRequest> {
-
- public static final HttpServletRequestInjectAdapter SETTER =
- new HttpServletRequestInjectAdapter();
-
- @Override
- public void put(final HttpServletRequest carrier, final String key, final String value) {
- carrier.setAttribute(key, value);
- }
-}

MODIFY
instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet3/Servlet3Advice.java
instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet3/Servlet3Advice.java
@@ -28,14 +28,46 @@ public class Servlet3Advice {
@Advice.This final Object servlet,
@Advice.Argument(0) final ServletRequest request,
@Advice.Argument(1) final ServletResponse response) {
- final boolean hasActiveTrace = TRACER.getCurrentSpan().getContext().isValid();
- final boolean hasServletTrace = request.getAttribute(SPAN_ATTRIBUTE) instanceof Span;
+ if (!(request instanceof HttpServletRequest)) {
+ return null;
+ }
+ final Object spanAttr = request.getAttribute(SPAN_ATTRIBUTE);
+ if (spanAttr instanceof Span) {
+ // inside of an existing servlet span already
+
+ final Span span = (Span) spanAttr;
+ final Object dispatch = request.getAttribute("io.opentelemetry.auto.servlet.dispatch");
+
+ if (dispatch instanceof String) {
+ // inside of a dispatched servlet/filter
- final boolean invalidRequest = !(request instanceof HttpServletRequest);
- if (invalidRequest || (hasActiveTrace && hasServletTrace)) {
- // Tracing might already be applied by the FilterChain. If so ignore this.
+ // remove the dispatch attribute so that it won't trigger any additional dispatch spans
+ // beyond this one
+ request.removeAttribute("io.opentelemetry.auto.servlet.dispatch");
+
+ // start an INTERNAL span to group the dispatched work under
+ final Span dispatchSpan =
+ TRACER.spanBuilder("servlet.dispatch").setParent(span).startSpan();
+ DECORATE.afterStart(dispatchSpan);
+ dispatchSpan.setAttribute(MoreTags.RESOURCE_NAME, (String) dispatch);
+ return new SpanWithScope(dispatchSpan, TRACER.withSpan(dispatchSpan));
+ }
+
+ final boolean spanContextWasLost =
+ !TRACER.getCurrentSpan().getContext().getTraceId().equals(span.getContext().getTraceId());
+ if (spanContextWasLost) {
+ // either there is no current span, or there is a current span but it is left over from some
+ // other trace
+
+ // re-scope the current work using the span in the request attribute
+ return new SpanWithScope(null, TRACER.withSpan(span));
+ } else {
+ // everything is good, just inside of a nested servlet/filter
+
+ // do not capture anything
return null;
}
+ }
final HttpServletRequest httpServletRequest = (HttpServletRequest) request;
final Span.Builder builder = TRACER.spanBuilder("servlet.request").setSpanKind(SERVER);
@@ -82,12 +114,17 @@ public class Servlet3Advice {
return;
}
+ final Span span = spanWithScope.getSpan();
+ if (span == null) {
+ // this was just a re-scoping of the current thread using the span in the request attribute
+ spanWithScope.closeScope();
+ return;
+ }
+
if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) {
final HttpServletRequest req = (HttpServletRequest) request;
final HttpServletResponse resp = (HttpServletResponse) response;
- final Span span = spanWithScope.getSpan();
-
if (throwable != null) {
DECORATE.onResponse(span, resp);
if (resp.getStatus() == HttpServletResponse.SC_OK) {

MODIFY
instrumentation/servlet/request-3.0/src/test/groovy/AbstractServlet3Test.groovy
instrumentation/servlet/request-3.0/src/test/groovy/AbstractServlet3Test.groovy
@@ -91,7 +91,6 @@ abstract class AbstractServlet3Test<SERVER, CONTEXT> extends HttpServerTest<SERV
"servlet.context" "/$context"
"servlet.path" { it == endpoint.path || it == "/dispatch$endpoint.path" }
if (hasDispatchSpan) {
- "servlet.dispatch" endpoint.path
"span.origin.type" String
} else {
"span.origin.type" { it == servlet.name || it == ApplicationFilterChain.name }

MODIFY
instrumentation/servlet/request-3.0/src/test/groovy/JettyServlet3Test.groovy
instrumentation/servlet/request-3.0/src/test/groovy/JettyServlet3Test.groovy
@@ -16,7 +16,7 @@ import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.EXCE
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.SERVER
+import static io.opentelemetry.trace.Span.Kind.INTERNAL
abstract class JettyServlet3Test extends AbstractServlet3Test<Server, ServletContextHandler> {
@@ -219,32 +219,20 @@ abstract class JettyDispatchTest extends JettyServlet3Test {
@Override
void dispatchSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) {
trace.span(index) {
- operationName expectedOperationName()
- spanKind SERVER
+ operationName "servlet.dispatch"
+ spanKind INTERNAL
childOf((SpanData) parent)
errored endpoint.errored
tags {
"$MoreTags.SPAN_TYPE" SpanTypes.HTTP_SERVER
+ "$MoreTags.RESOURCE_NAME" endpoint.path
"$Tags.COMPONENT" serverDecorator.getComponentName()
- "$Tags.PEER_HOSTNAME" { it == "localhost" || it == "127.0.0.1" }
- "$Tags.PEER_HOST_IPV4" { it == null || it == "127.0.0.1" } // Optional
- "$Tags.PEER_PORT" Long
- "$Tags.HTTP_URL" "${endpoint.resolve(address).toString().replace("/dispatch", "")}"
- "$Tags.HTTP_METHOD" "GET"
"$Tags.HTTP_STATUS" endpoint.status
- "servlet.context" "/$context"
- "servlet.path" endpoint.path
- "span.origin.type" {
- it == TestServlet3.DispatchImmediate.name || it == TestServlet3.DispatchAsync.name || it == TestServlet3.Sync.name || it == TestServlet3.Async.name
- }
if (endpoint.errored) {
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }
}
- if (endpoint.query) {
- "$MoreTags.HTTP_QUERY" endpoint.query
- }
}
}
}

MODIFY
instrumentation/servlet/request-3.0/src/test/groovy/TomcatServlet3Test.groovy
instrumentation/servlet/request-3.0/src/test/groovy/TomcatServlet3Test.groovy
@@ -7,7 +7,6 @@ import io.opentelemetry.sdk.trace.SpanData
import org.apache.catalina.Context
import org.apache.catalina.connector.Request
import org.apache.catalina.connector.Response
-import org.apache.catalina.core.ApplicationFilterChain
import org.apache.catalina.core.StandardHost
import org.apache.catalina.startup.Tomcat
import org.apache.catalina.valves.ErrorReportValve
@@ -23,7 +22,7 @@ import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.NOT_
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.SERVER
+import static io.opentelemetry.trace.Span.Kind.INTERNAL
abstract class TomcatServlet3Test extends AbstractServlet3Test<Tomcat, Context> {
@@ -261,32 +260,20 @@ abstract class TomcatDispatchTest extends TomcatServlet3Test {
@Override
void dispatchSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) {
trace.span(index) {
- operationName expectedOperationName()
- spanKind SERVER
+ operationName "servlet.dispatch"
+ spanKind INTERNAL
childOf((SpanData) parent)
errored endpoint.errored
tags {
"$MoreTags.SPAN_TYPE" SpanTypes.HTTP_SERVER
+ "$MoreTags.RESOURCE_NAME" endpoint.path
"$Tags.COMPONENT" serverDecorator.getComponentName()
- "$Tags.PEER_HOSTNAME" { it == "localhost" || it == "127.0.0.1" }
- "$Tags.PEER_HOST_IPV4" { it == null || it == "127.0.0.1" } // Optional
- "$Tags.PEER_PORT" Long
- "$Tags.HTTP_URL" "${endpoint.resolve(address).toString().replace("/dispatch", "")}"
- "$Tags.HTTP_METHOD" "GET"
"$Tags.HTTP_STATUS" endpoint.status
- "servlet.context" "/$context"
- "servlet.path" endpoint.path
- "span.origin.type" {
- it == TestServlet3.DispatchImmediate.name || it == TestServlet3.DispatchAsync.name || it == ApplicationFilterChain.name
- }
if (endpoint.errored) {
"error.msg" { it == null || it == EXCEPTION.body }
"error.type" { it == null || it == Exception.name }
"error.stack" { it == null || it instanceof String }
}
- if (endpoint.query) {
- "$MoreTags.HTTP_QUERY" endpoint.query
- }
}
}
}

DELETE
instrumentation/servlet/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletRequestSetter.java
-package io.opentelemetry.auto.instrumentation.servlet;
-
-import io.opentelemetry.context.propagation.HttpTextFormat;
-import javax.servlet.ServletRequest;
-
-/** Inject into request attributes since the request headers can't be modified. */
-public class ServletRequestSetter implements HttpTextFormat.Setter<ServletRequest> {
- public static final ServletRequestSetter SETTER = new ServletRequestSetter();
-
- @Override
- public void put(final ServletRequest carrier, final String key, final String value) {
- carrier.setAttribute(key, value);
- }
-}

MODIFY
instrumentation/servlet/src/main/java/io/opentelemetry/auto/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java
instrumentation/servlet/src/main/java/io/opentelemetry/auto/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java
package io.opentelemetry.auto.instrumentation.servlet.dispatcher;
import static io.opentelemetry.auto.decorator.HttpServerDecorator.SPAN_ATTRIBUTE;
-import static io.opentelemetry.auto.instrumentation.servlet.ServletRequestSetter.SETTER;
import static io.opentelemetry.auto.instrumentation.servlet.dispatcher.RequestDispatcherDecorator.DECORATE;
import static io.opentelemetry.auto.instrumentation.servlet.dispatcher.RequestDispatcherDecorator.TRACER;
import static io.opentelemetry.auto.tooling.ByteBuddyElementMatchers.safeHasSuperType;
@@ -35,9 +34,7 @@ public final class RequestDispatcherInstrumentation extends Instrumenter.Default
@Override
public String[] helperClassNames() {
return new String[] {
- "io.opentelemetry.auto.instrumentation.servlet.ServletRequestSetter",
- "io.opentelemetry.auto.decorator.BaseDecorator",
- packageName + ".RequestDispatcherDecorator",
+ "io.opentelemetry.auto.decorator.BaseDecorator", packageName + ".RequestDispatcherDecorator",
};
}
@@ -68,7 +65,7 @@ public final class RequestDispatcherInstrumentation extends Instrumenter.Default
public static SpanWithScope start(
@Advice.Origin("#m") final String method,
@Advice.This final RequestDispatcher dispatcher,
- @Advice.Local("_requestSpan") Object requestSpan,
+ @Advice.Local("_originalServletSpan") Object originalServletSpan,
@Advice.Argument(0) final ServletRequest request) {
if (!TRACER.getCurrentSpan().getContext().isValid()) {
// Don't want to generate a new top-level span
@@ -82,11 +79,11 @@ public final class RequestDispatcherInstrumentation extends Instrumenter.Default
InstrumentationContext.get(RequestDispatcher.class, String.class).get(dispatcher);
span.setAttribute(MoreTags.RESOURCE_NAME, target);
- // In case we lose context, inject trace into to the request.
- TRACER.getHttpTextFormat().inject(span.getContext(), request, SETTER);
+ // save the original servlet span before overwriting the request attribute, so that it can be
+ // restored on method exit
+ originalServletSpan = request.getAttribute(SPAN_ATTRIBUTE);
- // temporarily replace from request to avoid spring resource name bubbling up:
- requestSpan = request.getAttribute(SPAN_ATTRIBUTE);
+ // this tells the dispatched servlet to use the current span as the parent for its work
request.setAttribute(SPAN_ATTRIBUTE, span);
return new SpanWithScope(span, TRACER.withSpan(span));
@@ -95,17 +92,17 @@ public final class RequestDispatcherInstrumentation extends Instrumenter.Default
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void stop(
@Advice.Enter final SpanWithScope spanWithScope,
- @Advice.Local("_requestSpan") final Object requestSpan,
+ @Advice.Local("_originalServletSpan") final Object originalServletSpan,
@Advice.Argument(0) final ServletRequest request,
@Advice.Thrown final Throwable throwable) {
if (spanWithScope == null) {
return;
}
- if (requestSpan != null) {
- // now add it back...
- request.setAttribute(SPAN_ATTRIBUTE, requestSpan);
- }
+ // restore the original servlet span
+ // since spanWithScope is non-null here, originalServletSpan must have been set with the prior
+ // servlet span (as opposed to remaining unset)
+ request.setAttribute(SPAN_ATTRIBUTE, originalServletSpan);
final Span span = spanWithScope.getSpan();
DECORATE.onError(span, throwable);

MODIFY
instrumentation/servlet/src/main/java/io/opentelemetry/auto/instrumentation/servlet/http/HttpServletResponseInstrumentation.java
instrumentation/servlet/src/main/java/io/opentelemetry/auto/instrumentation/servlet/http/HttpServletResponseInstrumentation.java
@@ -12,7 +12,6 @@ import com.google.auto.service.AutoService;
import io.opentelemetry.auto.bootstrap.InstrumentationContext;
import io.opentelemetry.auto.instrumentation.api.MoreTags;
import io.opentelemetry.auto.instrumentation.api.SpanWithScope;
-import io.opentelemetry.auto.instrumentation.servlet.ServletRequestSetter;
import io.opentelemetry.auto.tooling.Instrumenter;
import io.opentelemetry.trace.Span;
import java.util.Map;
@@ -32,7 +31,6 @@ public final class HttpServletResponseInstrumentation extends Instrumenter.Defau
@Override
public String[] helperClassNames() {
return new String[] {
- "io.opentelemetry.auto.instrumentation.servlet.ServletRequestSetter",
"io.opentelemetry.auto.decorator.BaseDecorator",
packageName + ".HttpServletResponseDecorator",
};
@@ -77,9 +75,6 @@ public final class HttpServletResponseInstrumentation extends Instrumenter.Defau
span.setAttribute(MoreTags.RESOURCE_NAME, "HttpServletResponse." + method);
- // In case we lose context, inject trace into to the request.
- TRACER.getHttpTextFormat().inject(span.getContext(), req, ServletRequestSetter.SETTER);
-
return new SpanWithScope(span, TRACER.withSpan(span));
}

MODIFY
instrumentation/servlet/src/test/groovy/RequestDispatcherTest.groovy
instrumentation/servlet/src/test/groovy/RequestDispatcherTest.groovy
@@ -59,8 +59,6 @@ class RequestDispatcherTest extends AgentTestRunner {
}
}
- then:
- 1 * request.setAttribute("traceparent", _)
then:
1 * request.getAttribute(SPAN_ATTRIBUTE) >> mockSpan
then:
@@ -110,8 +108,6 @@ class RequestDispatcherTest extends AgentTestRunner {
}
}
- then:
- 1 * request.setAttribute("traceparent", _)
then:
1 * request.getAttribute(SPAN_ATTRIBUTE) >> mockSpan
then:

Capture servlet dispatch as INTERNAL span (#156)