diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java index 7ce396ecfc..ab64072138 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java @@ -9,7 +9,6 @@ import com.google.auto.service.AutoService; 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.dispatcher.RequestDispatcherInstrumentation; import io.opentelemetry.javaagent.instrumentation.servlet.common.response.HttpServletResponseInstrumentation; import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterInstrumentation; import java.util.Arrays; @@ -34,9 +33,7 @@ public class JakartaServletInstrumentationModule extends InstrumentationModule { adviceClassName(".service.JakartaServletInitAdvice"), adviceClassName(".service.JakartaServletFilterInitAdvice")), new HttpServletResponseInstrumentation( - BASE_PACKAGE, adviceClassName(".response.ResponseSendAdvice")), - new RequestDispatcherInstrumentation( - BASE_PACKAGE, adviceClassName(".dispatcher.RequestDispatcherAdvice"))); + BASE_PACKAGE, adviceClassName(".response.ResponseSendAdvice"))); } private static String adviceClassName(String suffix) { diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/dispatcher/RequestDispatcherAdvice.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/dispatcher/RequestDispatcherAdvice.java deleted file mode 100644 index 0ac70cb385..0000000000 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/dispatcher/RequestDispatcherAdvice.java +++ /dev/null @@ -1,62 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.dispatcher; - -import static io.opentelemetry.instrumentation.api.tracer.HttpServerTracer.CONTEXT_ATTRIBUTE; - -import io.opentelemetry.context.Context; -import io.opentelemetry.context.Scope; -import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; -import io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher.RequestDispatcherAdviceHelper; -import jakarta.servlet.RequestDispatcher; -import jakarta.servlet.ServletRequest; -import java.lang.reflect.Method; -import net.bytebuddy.asm.Advice; - -public class RequestDispatcherAdvice { - - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void start( - @Advice.Origin Method method, - @Advice.This RequestDispatcher dispatcher, - @Advice.Local("otelRequestContext") Context requestContext, - @Advice.Local("otelContext") Context context, - @Advice.Local("otelScope") Scope scope, - @Advice.Argument(0) ServletRequest request) { - - Context currentContext = Java8BytecodeBridge.currentContext(); - - Object requestContextAttr = request.getAttribute(CONTEXT_ATTRIBUTE); - requestContext = requestContextAttr instanceof Context ? (Context) requestContextAttr : null; - - context = RequestDispatcherAdviceHelper.getStartParentContext(currentContext, requestContext); - if (context == null) { - return; - } - - // this tells the dispatched servlet to use the current span as the parent for its work - request.setAttribute(CONTEXT_ATTRIBUTE, context); - - // TODO (trask) do we need this, since doing manual propagation above? - scope = context.makeCurrent(); - } - - @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - public static void stop( - @Advice.Argument(0) ServletRequest request, - @Advice.Local("otelRequestContext") Context requestContext, - @Advice.Local("otelContext") Context context, - @Advice.Local("otelScope") Scope scope, - @Advice.Thrown Throwable throwable) { - - scope.close(); - - if (requestContext != null) { - // restore the original request context - request.setAttribute(CONTEXT_ATTRIBUTE, requestContext); - } - } -} diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/dispatcher/RequestDispatcherAdviceHelper.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/dispatcher/RequestDispatcherAdviceHelper.java deleted file mode 100644 index ffccf91ef7..0000000000 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/dispatcher/RequestDispatcherAdviceHelper.java +++ /dev/null @@ -1,52 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher; - -import io.opentelemetry.api.trace.Span; -import io.opentelemetry.api.trace.SpanContext; -import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.tracer.HttpServerTracer; -import org.checkerframework.checker.nullness.qual.Nullable; - -public class RequestDispatcherAdviceHelper { - /** - * Determines if the advice for {@link RequestDispatcherInstrumentation} should create a new span - * and provides the context in which that span should be created. - * - * @param requestContext Value of the {@link HttpServerTracer#CONTEXT_ATTRIBUTE} attribute of the - * servlet request. - * @return The context in which the advice should create the dispatcher span in. Returns - * null in case a new span should not be created. - */ - // TODO (trask) do we need to guard against context leak here? - // this could be simplified by always using currentContext, only falling back to requestContext - // if currentContext does not have a valid span - public static @Nullable Context getStartParentContext( - Context currentContext, @Nullable Context requestContext) { - Span currentSpan = Span.fromContext(currentContext); - SpanContext currentSpanContext = currentSpan.getSpanContext(); - - if (!currentSpanContext.isValid()) { - return requestContext; - } - - if (requestContext == null) { - return currentContext; - } - - // at this point: currentContext has a valid span and requestContext is not null - - Span requestSpan = Span.fromContext(requestContext); - if (requestSpan.getSpanContext().getTraceId().equals(currentSpanContext.getTraceId())) { - // currentContext is part of the same trace, so return it - return currentContext; - } - - // currentContext is part of a different trace, so lets ignore it. - // This can happen with the way Tomcat does error handling. - return requestContext; - } -} diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/dispatcher/RequestDispatcherInstrumentation.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/dispatcher/RequestDispatcherInstrumentation.java deleted file mode 100644 index a10233dfac..0000000000 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/dispatcher/RequestDispatcherInstrumentation.java +++ /dev/null @@ -1,52 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher; - -import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface; -import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed; -import static io.opentelemetry.javaagent.extension.matcher.NameMatchers.namedOneOf; -import static java.util.Collections.singletonMap; -import static net.bytebuddy.matcher.ElementMatchers.isPublic; -import static net.bytebuddy.matcher.ElementMatchers.named; -import static net.bytebuddy.matcher.ElementMatchers.takesArgument; -import static net.bytebuddy.matcher.ElementMatchers.takesArguments; - -import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; -import java.util.Map; -import net.bytebuddy.description.method.MethodDescription; -import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.matcher.ElementMatcher; - -public class RequestDispatcherInstrumentation implements TypeInstrumentation { - private final String basePackageName; - private final String adviceClassName; - - public RequestDispatcherInstrumentation(String basePackageName, String adviceClassName) { - this.basePackageName = basePackageName; - this.adviceClassName = adviceClassName; - } - - @Override - public ElementMatcher classLoaderOptimization() { - return hasClassesNamed(basePackageName + ".RequestDispatcher"); - } - - @Override - public ElementMatcher typeMatcher() { - return implementsInterface(named(basePackageName + ".RequestDispatcher")); - } - - @Override - public Map, String> transformers() { - return singletonMap( - namedOneOf("forward", "include") - .and(takesArguments(2)) - .and(takesArgument(0, named(basePackageName + ".ServletRequest"))) - .and(takesArgument(1, named(basePackageName + ".ServletResponse"))) - .and(isPublic()), - adviceClassName); - } -} diff --git a/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/JavaxServletInstrumentationModule.java b/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/JavaxServletInstrumentationModule.java index 408fcdc1b8..5509ecd214 100644 --- a/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/JavaxServletInstrumentationModule.java +++ b/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/JavaxServletInstrumentationModule.java @@ -8,9 +8,8 @@ package io.opentelemetry.javaagent.instrumentation.servlet.javax; import com.google.auto.service.AutoService; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; -import io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher.RequestDispatcherInstrumentation; import io.opentelemetry.javaagent.instrumentation.servlet.common.response.HttpServletResponseInstrumentation; -import java.util.Arrays; +import java.util.Collections; import java.util.List; @AutoService(InstrumentationModule.class) @@ -23,11 +22,9 @@ public class JavaxServletInstrumentationModule extends InstrumentationModule { @Override public List typeInstrumentations() { - return Arrays.asList( + return Collections.singletonList( new HttpServletResponseInstrumentation( - BASE_PACKAGE, adviceClassName(".response.ResponseSendAdvice")), - new RequestDispatcherInstrumentation( - BASE_PACKAGE, adviceClassName(".dispatcher.RequestDispatcherAdvice"))); + BASE_PACKAGE, adviceClassName(".response.ResponseSendAdvice"))); } private static String adviceClassName(String suffix) { diff --git a/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/dispatcher/RequestDispatcherAdvice.java b/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/dispatcher/RequestDispatcherAdvice.java deleted file mode 100644 index cc557beb41..0000000000 --- a/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/dispatcher/RequestDispatcherAdvice.java +++ /dev/null @@ -1,60 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.servlet.javax.dispatcher; - -import static io.opentelemetry.instrumentation.api.tracer.HttpServerTracer.CONTEXT_ATTRIBUTE; - -import io.opentelemetry.context.Context; -import io.opentelemetry.context.Scope; -import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; -import io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher.RequestDispatcherAdviceHelper; -import java.lang.reflect.Method; -import javax.servlet.ServletRequest; -import net.bytebuddy.asm.Advice; - -public class RequestDispatcherAdvice { - - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void start( - @Advice.Origin Method method, - @Advice.Local("otelRequestContext") Context requestContext, - @Advice.Local("otelContext") Context context, - @Advice.Local("otelScope") Scope scope, - @Advice.Argument(0) ServletRequest request) { - - Context currentContext = Java8BytecodeBridge.currentContext(); - - Object requestContextAttr = request.getAttribute(CONTEXT_ATTRIBUTE); - requestContext = requestContextAttr instanceof Context ? (Context) requestContextAttr : null; - - context = RequestDispatcherAdviceHelper.getStartParentContext(currentContext, requestContext); - if (context == null) { - return; - } - - // this tells the dispatched servlet to use the current span as the parent for its work - request.setAttribute(CONTEXT_ATTRIBUTE, context); - - // TODO (trask) do we need this, since doing manual propagation above? - scope = context.makeCurrent(); - } - - @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - public static void stop( - @Advice.Argument(0) ServletRequest request, - @Advice.Local("otelRequestContext") Context requestContext, - @Advice.Local("otelContext") Context context, - @Advice.Local("otelScope") Scope scope, - @Advice.Thrown Throwable throwable) { - - scope.close(); - - if (requestContext != null) { - // restore the original request context - request.setAttribute(CONTEXT_ATTRIBUTE, requestContext); - } - } -} diff --git a/instrumentation/servlet/servlet-javax-common/javaagent/src/test/groovy/RequestDispatcherTest.groovy b/instrumentation/servlet/servlet-javax-common/javaagent/src/test/groovy/RequestDispatcherTest.groovy deleted file mode 100644 index 4c778cf2b5..0000000000 --- a/instrumentation/servlet/servlet-javax-common/javaagent/src/test/groovy/RequestDispatcherTest.groovy +++ /dev/null @@ -1,128 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -import static io.opentelemetry.instrumentation.test.utils.TraceUtils.basicSpan -import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace - -import io.opentelemetry.context.Context -import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification -import javax.servlet.ServletException -import javax.servlet.http.HttpServletRequest -import javax.servlet.http.HttpServletResponse - -class RequestDispatcherTest extends AgentInstrumentationSpecification { - - def request = Mock(HttpServletRequest) - def response = Mock(HttpServletResponse) - def dispatcher = new RequestDispatcherUtils(request, response) - - def "test dispatch no-parent"() { - when: - dispatcher.forward("") - dispatcher.include("") - - then: - assertTraces(2) { - traces.sort(orderByRootSpanName("forward-child", "include-child")) - trace(0, 1) { - basicSpan(it, 0, "forward-child") - } - trace(1, 1) { - basicSpan(it, 0, "include-child") - } - } - } - - def "test dispatcher #method with parent"() { - when: - runUnderTrace("parent") { - dispatcher."$method"(target) - } - - then: - assertTraces(1) { - trace(0, 2) { - basicSpan(it, 0, "parent") - basicSpan(it, 1, "$operation-child", span(0)) - } - } - - where: - operation | method - "forward" | "forward" - "forward" | "forwardNamed" - "include" | "include" - "include" | "includeNamed" - - target = "test-$method" - } - - def "test dispatcher #method with parent from request attribute"() { - setup: - Context context - runUnderTrace("parent") { - context = Context.current() - } - - when: - runUnderTrace("notParent") { - context.makeCurrent().withCloseable { - dispatcher."$method"(target) - } - } - - then: - assertTraces(2) { - traces.sort(orderByRootSpanName("parent", "notParent")) - trace(0, 2) { - basicSpan(it, 0, "parent") - basicSpan(it, 1, "$operation-child", span(0)) - } - trace(1, 1) { - basicSpan(it, 0, "notParent") - } - } - - where: - operation | method - "forward" | "forward" - "forward" | "forwardNamed" - "include" | "include" - "include" | "includeNamed" - - target = "test-$method" - } - - def "test dispatcher #method exception"() { - setup: - def ex = new ServletException("some error") - def dispatcher = new RequestDispatcherUtils(request, response, ex) - - when: - runUnderTrace("parent") { - dispatcher."$method"(target) - } - - then: - def th = thrown(ServletException) - th == ex - - assertTraces(1) { - trace(0, 2) { - basicSpan(it, 0, "parent", null, ex) - basicSpan(it, 1, "$operation-child", span(0)) - } - } - - where: - operation | method - "forward" | "forward" - "forward" | "forwardNamed" - "include" | "include" - "include" | "includeNamed" - - target = "test-$method" - } -} diff --git a/instrumentation/servlet/servlet-javax-common/javaagent/src/test/java/RequestDispatcherUtils.java b/instrumentation/servlet/servlet-javax-common/javaagent/src/test/java/RequestDispatcherUtils.java deleted file mode 100644 index efc7e72333..0000000000 --- a/instrumentation/servlet/servlet-javax-common/javaagent/src/test/java/RequestDispatcherUtils.java +++ /dev/null @@ -1,209 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace; - -import java.io.IOException; -import java.io.InputStream; -import java.net.URL; -import java.util.Enumeration; -import java.util.Set; -import java.util.concurrent.Callable; -import javax.servlet.RequestDispatcher; -import javax.servlet.Servlet; -import javax.servlet.ServletContext; -import javax.servlet.ServletException; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; - -public class RequestDispatcherUtils { - private final ServletRequest req; - private final ServletResponse resp; - private final ServletException toThrow; - - public RequestDispatcherUtils(ServletRequest req, ServletResponse resp) { - this.req = req; - this.resp = resp; - toThrow = null; - } - - public RequestDispatcherUtils( - ServletRequest req, ServletResponse resp, ServletException toThrow) { - this.req = req; - this.resp = resp; - this.toThrow = toThrow; - } - - /* RequestDispatcher can't be visible to groovy otherwise things break, so everything is - * encapsulated in here where groovy doesn't need to access it. - */ - - void forward(String target) throws ServletException, IOException { - new TestContext().getRequestDispatcher(target).forward(req, resp); - } - - void include(String target) throws ServletException, IOException { - new TestContext().getRequestDispatcher(target).include(req, resp); - } - - void forwardNamed(String target) throws ServletException, IOException { - new TestContext().getNamedDispatcher(target).forward(req, resp); - } - - void includeNamed(String target) throws ServletException, IOException { - new TestContext().getNamedDispatcher(target).include(req, resp); - } - - class TestContext implements ServletContext { - @Override - public String getContextPath() { - return null; - } - - @Override - public ServletContext getContext(String s) { - return null; - } - - @Override - public int getMajorVersion() { - return 0; - } - - @Override - public int getMinorVersion() { - return 0; - } - - @Override - public String getMimeType(String s) { - return null; - } - - @Override - public Set getResourcePaths(String s) { - return null; - } - - @Override - public URL getResource(String s) { - return null; - } - - @Override - public InputStream getResourceAsStream(String s) { - return null; - } - - @Override - public RequestDispatcher getRequestDispatcher(String s) { - return new TestDispatcher(); - } - - @Override - public RequestDispatcher getNamedDispatcher(String s) { - return new TestDispatcher(); - } - - @Override - public Servlet getServlet(String s) { - return null; - } - - @Override - public Enumeration getServlets() { - return null; - } - - @Override - public Enumeration getServletNames() { - return null; - } - - @Override - public void log(String s) {} - - @Override - public void log(Exception e, String s) {} - - @Override - public void log(String s, Throwable throwable) {} - - @Override - public String getRealPath(String s) { - return null; - } - - @Override - public String getServerInfo() { - return null; - } - - @Override - public String getInitParameter(String s) { - return null; - } - - @Override - public Enumeration getInitParameterNames() { - return null; - } - - @Override - public Object getAttribute(String s) { - return null; - } - - @Override - public Enumeration getAttributeNames() { - return null; - } - - @Override - public void setAttribute(String s, Object o) {} - - @Override - public void removeAttribute(String s) {} - - @Override - public String getServletContextName() { - return null; - } - } - - class TestDispatcher implements RequestDispatcher { - @Override - public void forward(ServletRequest servletRequest, ServletResponse servletResponse) - throws ServletException { - runUnderTrace( - "forward-child", - new Callable() { - @Override - public Object call() { - return null; - } - }); - if (toThrow != null) { - throw toThrow; - } - } - - @Override - public void include(ServletRequest servletRequest, ServletResponse servletResponse) - throws ServletException { - runUnderTrace( - "include-child", - new Callable() { - @Override - public Object call() { - return null; - } - }); - if (toThrow != null) { - throw toThrow; - } - } - } -}