From a2dc3904ca6d5a53872da0de50c20a22c9cc57ab Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Fri, 14 May 2021 04:30:25 +0300 Subject: [PATCH 01/12] Attach servlet async listener with asyncStart instrumentation --- .../javaagent/jetty-common-javaagent.gradle | 1 + .../common/JettyHandlerAdviceHelper.java | 22 +------- .../javaagent/liberty-javaagent.gradle | 1 + .../liberty/LibertyHandleRequestAdvice.java | 15 +----- .../v3_0/Servlet3AsyncStartAdvice.java | 40 ++++++++++++++ .../v3_0/Servlet3InstrumentationModule.java | 4 +- .../servlet/v3_0/TagSettingAsyncListener.java | 54 ------------------- .../JakartaServletInstrumentationModule.java | 2 + .../servlet/v5_0/async/AsyncStartAdvice.java | 40 ++++++++++++++ .../v5_0/service/TagSettingAsyncListener.java | 54 ------------------- .../async/AsyncStartInstrumentation.java | 46 ++++++++++++++++ .../service/ServletAndFilterAdviceHelper.java | 43 ++++++++------- .../servlet/ServletHttpServerTracer.java | 18 +++++++ .../TomcatServerHandlerAdviceHelper.java | 34 +++--------- .../javaagent/tomcat-common-javaagent.gradle | 1 + 15 files changed, 188 insertions(+), 187 deletions(-) create mode 100644 instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java delete mode 100644 instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/TagSettingAsyncListener.java create mode 100644 instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncStartAdvice.java delete mode 100644 instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/TagSettingAsyncListener.java create mode 100644 instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/async/AsyncStartInstrumentation.java diff --git a/instrumentation/jetty/jetty-common/javaagent/jetty-common-javaagent.gradle b/instrumentation/jetty/jetty-common/javaagent/jetty-common-javaagent.gradle index f401290f4c33..bfc2052cd7d1 100644 --- a/instrumentation/jetty/jetty-common/javaagent/jetty-common-javaagent.gradle +++ b/instrumentation/jetty/jetty-common/javaagent/jetty-common-javaagent.gradle @@ -2,4 +2,5 @@ apply from: "$rootDir/gradle/instrumentation.gradle" dependencies { api(project(':instrumentation:servlet:servlet-common:library')) + implementation(project(':instrumentation:servlet:servlet-common:javaagent')) } diff --git a/instrumentation/jetty/jetty-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/common/JettyHandlerAdviceHelper.java b/instrumentation/jetty/jetty-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/common/JettyHandlerAdviceHelper.java index 3eed15c96e23..0af191a78854 100644 --- a/instrumentation/jetty/jetty-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/common/JettyHandlerAdviceHelper.java +++ b/instrumentation/jetty/jetty-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/common/JettyHandlerAdviceHelper.java @@ -7,10 +7,8 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; -import io.opentelemetry.instrumentation.servlet.ServletAccessor; import io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer; -import io.opentelemetry.instrumentation.servlet.TagSettingAsyncListener; -import java.util.concurrent.atomic.AtomicBoolean; +import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterAdviceHelper; public class JettyHandlerAdviceHelper { /** Shared method exit implementation for Jetty handler advices. */ @@ -45,23 +43,7 @@ public static void stopSpan( return; } - AtomicBoolean responseHandled = new AtomicBoolean(false); - ServletAccessor servletAccessor = tracer.getServletAccessor(); - - // In case of async servlets wait for the actual response to be ready - if (servletAccessor.isRequestAsyncStarted(request)) { - try { - servletAccessor.addRequestAsyncListener( - request, new TagSettingAsyncListener<>(tracer, responseHandled, context)); - } catch (IllegalStateException e) { - // org.eclipse.jetty.server.Request may throw an exception here if request became - // finished after check above. We just ignore that exception and move on. - } - } - - // Check again in case the request finished before adding the listener. - if (!servletAccessor.isRequestAsyncStarted(request) - && responseHandled.compareAndSet(false, true)) { + if (ServletAndFilterAdviceHelper.mustEndOnHandlerMethodExit(tracer, request)) { tracer.end(context, response); } } diff --git a/instrumentation/liberty/liberty/javaagent/liberty-javaagent.gradle b/instrumentation/liberty/liberty/javaagent/liberty-javaagent.gradle index dee3addc0bb3..e08c0bbb9dfd 100644 --- a/instrumentation/liberty/liberty/javaagent/liberty-javaagent.gradle +++ b/instrumentation/liberty/liberty/javaagent/liberty-javaagent.gradle @@ -6,5 +6,6 @@ apply from: "$rootDir/gradle/instrumentation.gradle" dependencies { compileOnly "javax.servlet:javax.servlet-api:3.0.1" + implementation project(':instrumentation:servlet:servlet-common:javaagent') implementation project(':instrumentation:servlet:servlet-3.0:javaagent') } diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHandleRequestAdvice.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHandleRequestAdvice.java index 28756b11ca0c..ba37b4e8bf6a 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHandleRequestAdvice.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHandleRequestAdvice.java @@ -9,8 +9,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; -import io.opentelemetry.javaagent.instrumentation.servlet.v3_0.TagSettingAsyncListener; -import java.util.concurrent.atomic.AtomicBoolean; +import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterAdviceHelper; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; @@ -68,17 +67,7 @@ public static void stopSpan( return; } - AtomicBoolean responseHandled = new AtomicBoolean(false); - - // In case of async servlets wait for the actual response to be ready - if (request.isAsyncStarted()) { - request - .getAsyncContext() - .addListener(new TagSettingAsyncListener(responseHandled, context), request, response); - } - - // Check again in case the request finished before adding the listener. - if (!request.isAsyncStarted() && responseHandled.compareAndSet(false, true)) { + if (ServletAndFilterAdviceHelper.mustEndOnHandlerMethodExit(tracer(), request)) { tracer().end(context, response); } } diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java new file mode 100644 index 000000000000..dead1551a594 --- /dev/null +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java @@ -0,0 +1,40 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.servlet.v3_0; + +import static io.opentelemetry.instrumentation.servlet.v3_0.Servlet3HttpServerTracer.tracer; + +import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap; +import javax.servlet.AsyncContext; +import javax.servlet.http.HttpServletRequest; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.implementation.bytecode.assign.Assigner; + +public class Servlet3AsyncStartAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void startAsyncEnter() { + // This allows to detect the outermost invocation of startAsync in method exit + CallDepthThreadLocalMap.incrementCallDepth(AsyncContext.class); + } + + @Advice.OnMethodExit(suppress = Throwable.class) + public static void startAsyncExit( + @Advice.This(typing = Assigner.Typing.DYNAMIC) HttpServletRequest request) { + + int callDepth = CallDepthThreadLocalMap.decrementCallDepth(AsyncContext.class); + + if (callDepth != 0) { + // This is not the innermost invocation, ignore. + return; + } + + if (request != null) { + if (!tracer().isAsyncListenerAttached(request)) { + tracer().attachAsyncListener(request); + } + } + } +} diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java index b3d8ea6f2ab9..46fbb2bd00ef 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java @@ -11,6 +11,7 @@ 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.async.AsyncStartInstrumentation; import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterInstrumentation; import java.util.List; @@ -30,7 +31,8 @@ public List typeInstrumentations() { BASE_PACKAGE, adviceClassName(".Servlet3Advice"), adviceClassName(".Servlet3InitAdvice"), - adviceClassName(".Servlet3FilterInitAdvice"))); + adviceClassName(".Servlet3FilterInitAdvice")), + new AsyncStartInstrumentation(BASE_PACKAGE, adviceClassName(".Servlet3AsyncStartAdvice"))); } private static String adviceClassName(String suffix) { diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/TagSettingAsyncListener.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/TagSettingAsyncListener.java deleted file mode 100644 index 89019ad782f1..000000000000 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/TagSettingAsyncListener.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.servlet.v3_0; - -import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.servlet.v3_0.Servlet3HttpServerTracer; -import java.util.concurrent.atomic.AtomicBoolean; -import javax.servlet.AsyncEvent; -import javax.servlet.AsyncListener; -import javax.servlet.http.HttpServletResponse; - -public class TagSettingAsyncListener implements AsyncListener { - private static final Servlet3HttpServerTracer servletHttpServerTracer = - Servlet3HttpServerTracer.tracer(); - - private final AtomicBoolean responseHandled; - private final Context context; - - public TagSettingAsyncListener(AtomicBoolean responseHandled, Context context) { - this.responseHandled = responseHandled; - this.context = context; - } - - @Override - public void onComplete(AsyncEvent event) { - if (responseHandled.compareAndSet(false, true)) { - servletHttpServerTracer.end(context, (HttpServletResponse) event.getSuppliedResponse()); - } - } - - @Override - public void onTimeout(AsyncEvent event) { - if (responseHandled.compareAndSet(false, true)) { - servletHttpServerTracer.onTimeout(context, event.getAsyncContext().getTimeout()); - } - } - - @Override - public void onError(AsyncEvent event) { - if (responseHandled.compareAndSet(false, true)) { - servletHttpServerTracer.endExceptionally( - context, event.getThrowable(), (HttpServletResponse) event.getSuppliedResponse()); - } - } - - /** Transfer the listener over to the new context. */ - @Override - public void onStartAsync(AsyncEvent event) { - event.getAsyncContext().addListener(this); - } -} 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 ab6407213882..68630df17fdc 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,6 +9,7 @@ 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.async.AsyncStartInstrumentation; import io.opentelemetry.javaagent.instrumentation.servlet.common.response.HttpServletResponseInstrumentation; import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterInstrumentation; import java.util.Arrays; @@ -27,6 +28,7 @@ public List typeInstrumentations() { return Arrays.asList( new AsyncContextInstrumentation( BASE_PACKAGE, adviceClassName(".async.AsyncDispatchAdvice")), + new AsyncStartInstrumentation(BASE_PACKAGE, adviceClassName(".async.AsyncStartAdvice")), new ServletAndFilterInstrumentation( BASE_PACKAGE, adviceClassName(".service.JakartaServletServiceAdvice"), diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncStartAdvice.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncStartAdvice.java new file mode 100644 index 000000000000..f4a28980d236 --- /dev/null +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncStartAdvice.java @@ -0,0 +1,40 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.async; + +import static io.opentelemetry.instrumentation.servlet.jakarta.v5_0.JakartaServletHttpServerTracer.tracer; + +import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap; +import jakarta.servlet.AsyncContext; +import jakarta.servlet.http.HttpServletRequest; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.implementation.bytecode.assign.Assigner; + +public class AsyncStartAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void startAsyncEnter() { + // This allows to detect the outermost invocation of startAsync in method exit + CallDepthThreadLocalMap.incrementCallDepth(AsyncContext.class); + } + + @Advice.OnMethodExit(suppress = Throwable.class) + public static void startAsyncExit( + @Advice.This(typing = Assigner.Typing.DYNAMIC) HttpServletRequest request) { + + int callDepth = CallDepthThreadLocalMap.decrementCallDepth(AsyncContext.class); + + if (callDepth != 0) { + // This is not the innermost invocation, ignore. + return; + } + + if (request != null) { + if (!tracer().isAsyncListenerAttached(request)) { + tracer().attachAsyncListener(request); + } + } + } +} diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/TagSettingAsyncListener.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/TagSettingAsyncListener.java deleted file mode 100644 index be1f98f3607b..000000000000 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/TagSettingAsyncListener.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.service; - -import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.servlet.jakarta.v5_0.JakartaServletHttpServerTracer; -import jakarta.servlet.AsyncEvent; -import jakarta.servlet.AsyncListener; -import jakarta.servlet.http.HttpServletResponse; -import java.util.concurrent.atomic.AtomicBoolean; - -public class TagSettingAsyncListener implements AsyncListener { - private static final JakartaServletHttpServerTracer tracer = - JakartaServletHttpServerTracer.tracer(); - - private final AtomicBoolean responseHandled; - private final Context context; - - public TagSettingAsyncListener(AtomicBoolean responseHandled, Context context) { - this.responseHandled = responseHandled; - this.context = context; - } - - @Override - public void onComplete(AsyncEvent event) { - if (responseHandled.compareAndSet(false, true)) { - tracer.end(context, (HttpServletResponse) event.getSuppliedResponse()); - } - } - - @Override - public void onTimeout(AsyncEvent event) { - if (responseHandled.compareAndSet(false, true)) { - tracer.onTimeout(context, event.getAsyncContext().getTimeout()); - } - } - - @Override - public void onError(AsyncEvent event) { - if (responseHandled.compareAndSet(false, true)) { - tracer.endExceptionally( - context, event.getThrowable(), (HttpServletResponse) event.getSuppliedResponse()); - } - } - - /** Transfer the listener over to the new context. */ - @Override - public void onStartAsync(AsyncEvent event) { - event.getAsyncContext().addListener(this); - } -} diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/async/AsyncStartInstrumentation.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/async/AsyncStartInstrumentation.java new file mode 100644 index 000000000000..14bc7136241b --- /dev/null +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/async/AsyncStartInstrumentation.java @@ -0,0 +1,46 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.servlet.common.async; + +import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface; +import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed; +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.returns; + +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 AsyncStartInstrumentation implements TypeInstrumentation { + private final String basePackageName; + private final String adviceClassName; + + public AsyncStartInstrumentation(String basePackageName, String adviceClassName) { + this.basePackageName = basePackageName; + this.adviceClassName = adviceClassName; + } + + @Override + public ElementMatcher classLoaderOptimization() { + return hasClassesNamed(basePackageName + ".Servlet"); + } + + @Override + public ElementMatcher typeMatcher() { + return implementsInterface(named(basePackageName + ".ServletRequest")); + } + + @Override + public Map, String> transformers() { + return singletonMap( + named("startAsync").and(returns(named(basePackageName + ".AsyncContext"))).and(isPublic()), + adviceClassName); + } +} diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterAdviceHelper.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterAdviceHelper.java index 0a8fe6a6b30d..12e53535ca82 100644 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterAdviceHelper.java +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterAdviceHelper.java @@ -8,12 +8,9 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.servlet.AppServerBridge; -import io.opentelemetry.instrumentation.servlet.ServletAccessor; import io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer; -import io.opentelemetry.instrumentation.servlet.TagSettingAsyncListener; import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap; import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; -import java.util.concurrent.atomic.AtomicBoolean; public class ServletAndFilterAdviceHelper { public static void stopSpan( @@ -49,23 +46,33 @@ public static void stopSpan( return; } - AtomicBoolean responseHandled = new AtomicBoolean(false); - ServletAccessor accessor = tracer.getServletAccessor(); - - // In case of async servlets wait for the actual response to be ready - if (accessor.isRequestAsyncStarted(request)) { - try { - accessor.addRequestAsyncListener( - request, new TagSettingAsyncListener<>(tracer, responseHandled, context)); - } catch (IllegalStateException e) { - // org.eclipse.jetty.server.Request may throw an exception here if request became - // finished after check above. We just ignore that exception and move on. - } + if (mustEndOnHandlerMethodExit(tracer, request)) { + tracer.end(context, response); } + } - // Check again in case the request finished before adding the listener. - if (!accessor.isRequestAsyncStarted(request) && responseHandled.compareAndSet(false, true)) { - tracer.end(context, response); + /** + * Helper method to determine whether the appserver handler/servlet service/servlet filter method + * that started a span must also end it, even if no error was detected. Extracted as a separate + * method to avoid duplicating the comments on the logic behind this choice. + */ + public static boolean mustEndOnHandlerMethodExit( + ServletHttpServerTracer tracer, REQUEST request) { + + if (tracer.isAsyncListenerAttached(request)) { + // This request is handled asynchronously and startAsync instrumentation has already attached + // the listener. + return false; } + + // Possible scenarios: + // 1) synchronously handled request + // 2) asynchronously handled request, but startAsync instrumentation is broken on this server + // (should be caught by tests for the specific server). + // In case of broken startAsync instrumentation, no fallback handling for asynchronous requests + // should be provided as handling it in the handler/service method is prone to race conditions + // (seen happening on Undertow) that may make some tests pass which should fail due to the + // possible race. + return true; } } diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index 4376f6eebb6d..7dbd7f3ef6e1 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -23,6 +23,7 @@ import java.net.URI; import java.net.URISyntaxException; import java.security.Principal; +import java.util.concurrent.atomic.AtomicBoolean; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -31,6 +32,9 @@ public abstract class ServletHttpServerTracer private static final Logger log = LoggerFactory.getLogger(ServletHttpServerTracer.class); + public static final String ASYNC_LISTENER_ATTRIBUTE = + ServletHttpServerTracer.class.getName() + ".AsyncListener"; + private static final boolean CAPTURE_EXPERIMENTAL_SPAN_ATTRIBUTES = Config.get() .getBooleanProperty("otel.instrumentation.servlet.experimental-span-attributes", false); @@ -142,6 +146,20 @@ public ServletAccessor getServletAccessor() { return accessor; } + public void attachAsyncListener(REQUEST request) { + Context context = getServerContext(request); + + if (context != null) { + accessor.addRequestAsyncListener( + request, new TagSettingAsyncListener<>(this, new AtomicBoolean(), context)); + accessor.setRequestAttribute(request, ASYNC_LISTENER_ATTRIBUTE, true); + } + } + + public boolean isAsyncListenerAttached(REQUEST request) { + return accessor.getRequestAttribute(request, ASYNC_LISTENER_ATTRIBUTE) != null; + } + public void addUnwrappedThrowable(Context context, Throwable throwable) { if (AppServerBridge.shouldRecordException(context)) { onException(context, throwable); diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java index a5c648935fb9..507ae28e5249 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java @@ -8,8 +8,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer; -import io.opentelemetry.instrumentation.servlet.TagSettingAsyncListener; -import java.util.concurrent.atomic.AtomicBoolean; +import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterAdviceHelper; import net.bytebuddy.asm.Advice; import org.apache.coyote.Request; import org.apache.coyote.Response; @@ -59,31 +58,12 @@ public static void stopSpan( Object note = request.getNote(1); if (note instanceof org.apache.catalina.connector.Request) { - AtomicBoolean responseHandled = new AtomicBoolean(false); - - org.apache.catalina.connector.Request servletRequest = - (org.apache.catalina.connector.Request) note; - if (servletRequest.isAsync()) { - try { - // The Catalina Request always implements the HttpServletRequest (either javax or jakarta) - // which is also what REQUEST generic parameter is. We cannot cast normally without a - // generic because this class is compiled against javax.servlet which would make it try - // to use request from javax.servlet when REQUEST is actually from jakarta.servlet. - //noinspection unchecked - servletTracer - .getServletAccessor() - .addRequestAsyncListener( - (REQUEST) servletRequest, - new TagSettingAsyncListener<>(servletTracer, responseHandled, context)); - } catch (IllegalStateException e) { - // thrown by newer tomcats if request was already handled while setting the listener. - tracer.end(context, response); - return; - } - } - - // In case the request finished before adding the listener on older tomcats... - if (!servletRequest.isAsyncStarted() && responseHandled.compareAndSet(false, true)) { + // The Catalina Request always implements the HttpServletRequest (either javax or jakarta) + // which is also what REQUEST generic parameter is. We cannot cast normally without a + // generic because this class is compiled against javax.servlet which would make it try + // to use request from javax.servlet when REQUEST is actually from jakarta.servlet. + //noinspection unchecked + if (ServletAndFilterAdviceHelper.mustEndOnHandlerMethodExit(servletTracer, (REQUEST) note)) { tracer.end(context, response); } } diff --git a/instrumentation/tomcat/tomcat-common/javaagent/tomcat-common-javaagent.gradle b/instrumentation/tomcat/tomcat-common/javaagent/tomcat-common-javaagent.gradle index 43b3581037a8..6776fe89bed8 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/tomcat-common-javaagent.gradle +++ b/instrumentation/tomcat/tomcat-common/javaagent/tomcat-common-javaagent.gradle @@ -2,5 +2,6 @@ apply from: "$rootDir/gradle/instrumentation.gradle" dependencies { api(project(':instrumentation:servlet:servlet-common:library')) + implementation(project(':instrumentation:servlet:servlet-common:javaagent')) compileOnly "org.apache.tomcat.embed:tomcat-embed-core:7.0.4" } From 3f57b9aa45ea65e407f2eedfcfe8ac7f8db6b78c Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Fri, 14 May 2021 04:53:25 +0300 Subject: [PATCH 02/12] Exclude Spring packages containing servlet request classes from global ignores --- .../tooling/matcher/AdditionalLibraryIgnoresMatcher.java | 1 + 1 file changed, 1 insertion(+) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/AdditionalLibraryIgnoresMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/AdditionalLibraryIgnoresMatcher.java index 64ad434e525f..6e6bc9d0d5d7 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/AdditionalLibraryIgnoresMatcher.java @@ -178,6 +178,7 @@ public boolean matches(TypeDescription target) { if (name.startsWith("org.springframework.web.")) { if (name.startsWith("org.springframework.web.servlet.") || name.startsWith("org.springframework.web.filter.") + || name.startsWith("org.springframework.web.multipart.") || name.startsWith("org.springframework.web.reactive.") || name.startsWith("org.springframework.web.context.request.async.") || name.equals( From 0af651be0c8ae86d2d7cd9f53e1496e9114bf6ce Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Fri, 14 May 2021 05:19:24 +0300 Subject: [PATCH 03/12] Exclude Tapestry HSR proxy with global ignore --- .../javaagent/tooling/matcher/GlobalIgnoresMatcher.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/GlobalIgnoresMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/GlobalIgnoresMatcher.java index f70d8564ea17..96009c71460e 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/GlobalIgnoresMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/GlobalIgnoresMatcher.java @@ -196,7 +196,10 @@ public boolean matches(TypeDescription target) { // want to instrument these methods in generated classes as this would create spans that // have the generated class name in them instead of the actual class that handles the call. || name.contains("__EJB31_Generated__") - || name.startsWith("org.springframework.core.$Proxy")) { + || name.startsWith("org.springframework.core.$Proxy") + // Tapestry Proxy, check only specific problematic class since there is no common prefix for + // its proxies other than "$" + || name.startsWith("$HttpServletRequest_")) { return true; } From 308ff19f1b0035c6b2ac4c5d67aa83ca668e676e Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Fri, 14 May 2021 08:45:04 +0300 Subject: [PATCH 04/12] Improve comments. --- .../servlet/v3_0/Servlet3AsyncStartAdvice.java | 2 +- .../instrumentation/servlet/v5_0/async/AsyncStartAdvice.java | 2 +- .../javaagent/tooling/matcher/GlobalIgnoresMatcher.java | 5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java index dead1551a594..60ecedbaf8a5 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java @@ -27,7 +27,7 @@ public static void startAsyncExit( int callDepth = CallDepthThreadLocalMap.decrementCallDepth(AsyncContext.class); if (callDepth != 0) { - // This is not the innermost invocation, ignore. + // This is not the outermost invocation, ignore. return; } diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncStartAdvice.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncStartAdvice.java index f4a28980d236..14e110a3eba1 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncStartAdvice.java +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/async/AsyncStartAdvice.java @@ -27,7 +27,7 @@ public static void startAsyncExit( int callDepth = CallDepthThreadLocalMap.decrementCallDepth(AsyncContext.class); if (callDepth != 0) { - // This is not the innermost invocation, ignore. + // This is not the outermost invocation, ignore. return; } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/GlobalIgnoresMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/GlobalIgnoresMatcher.java index 96009c71460e..c9a6db93142d 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/GlobalIgnoresMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/GlobalIgnoresMatcher.java @@ -197,8 +197,9 @@ public boolean matches(TypeDescription target) { // have the generated class name in them instead of the actual class that handles the call. || name.contains("__EJB31_Generated__") || name.startsWith("org.springframework.core.$Proxy") - // Tapestry Proxy, check only specific problematic class since there is no common prefix for - // its proxies other than "$" + // Tapestry Proxy, check only specific class that we know would be instrumented since there + // is no common prefix for its proxies other than "$". ByteBuddy fails to instrument this + // proxy, and as there is no reason why it should be instrumented anyway, exclude it. || name.startsWith("$HttpServletRequest_")) { return true; } From 26ce9520bd6683989e24f175ba4479a98934b06b Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Fri, 14 May 2021 13:03:30 +0300 Subject: [PATCH 05/12] Fix for Liberty - request response when adding async listener --- .../liberty/LibertyHandleRequestAdvice.java | 2 +- .../liberty/LibertyStartSpanAdvice.java | 4 ++++ .../liberty/ThreadLocalContext.java | 13 ++++++++++--- .../servlet/v2_2/Servlet2Accessor.java | 4 +++- .../servlet/v3_0/Servlet3Accessor.java | 12 ++++++++++-- .../jakarta/v5_0/JakartaServletAccessor.java | 12 ++++++++++-- .../instrumentation/servlet/ServletAccessor.java | 3 ++- .../servlet/ServletHttpServerTracer.java | 15 ++++++++++++++- .../library/src/test/java/RequestOnlyTracer.java | 2 +- 9 files changed, 55 insertions(+), 12 deletions(-) diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHandleRequestAdvice.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHandleRequestAdvice.java index ba37b4e8bf6a..123f69cd1c65 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHandleRequestAdvice.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHandleRequestAdvice.java @@ -32,7 +32,7 @@ public static void onEnter( // it is a bit too early to start span at this point because calling // some methods on HttpServletRequest will give a NPE // just remember the request and use it a bit later to start the span - ThreadLocalContext.startRequest(httpServletRequest); + ThreadLocalContext.startRequest(httpServletRequest, (HttpServletResponse) response); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java index 894187a774a2..2cd048e55461 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java @@ -26,5 +26,9 @@ public static void onEnter() { ctx.setContext(context); ctx.setScope(scope); + + // For startAsync instrumentation to work on Liberty, the listener needs to be added with a + // response object included, which is not available in that instrumentation. + tracer().setAsyncListenerResponse(ctx.getRequest(), ctx.getResponse()); } } diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/ThreadLocalContext.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/ThreadLocalContext.java index a2aafe1deefe..5e9f1cd3d517 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/ThreadLocalContext.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/ThreadLocalContext.java @@ -8,18 +8,21 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; public class ThreadLocalContext { private static final ThreadLocal local = new ThreadLocal<>(); private final HttpServletRequest req; + private final HttpServletResponse response; private Context context; private Scope scope; private boolean started; - private ThreadLocalContext(HttpServletRequest req) { + private ThreadLocalContext(HttpServletRequest req, HttpServletResponse response) { this.req = req; + this.response = response; } public Context getContext() { @@ -42,6 +45,10 @@ public HttpServletRequest getRequest() { return req; } + public HttpServletResponse getResponse() { + return response; + } + /** * Test whether span should be started. * @@ -53,8 +60,8 @@ public boolean startSpan() { return !b; } - public static void startRequest(HttpServletRequest req) { - ThreadLocalContext ctx = new ThreadLocalContext(req); + public static void startRequest(HttpServletRequest req, HttpServletResponse response) { + ThreadLocalContext ctx = new ThreadLocalContext(req, response); local.set(ctx); } diff --git a/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2Accessor.java b/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2Accessor.java index 6705f26ab38a..fdacc77af917 100644 --- a/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2Accessor.java +++ b/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2Accessor.java @@ -26,7 +26,9 @@ public boolean isRequestAsyncStarted(HttpServletRequest request) { @Override public void addRequestAsyncListener( - HttpServletRequest request, ServletAsyncListener listener) { + HttpServletRequest request, + ServletAsyncListener listener, + Object response) { throw new UnsupportedOperationException(); } diff --git a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3Accessor.java b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3Accessor.java index d4f74e4dcf31..8d502fa7853e 100644 --- a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3Accessor.java +++ b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3Accessor.java @@ -29,8 +29,16 @@ public boolean isRequestAsyncStarted(HttpServletRequest request) { @Override public void addRequestAsyncListener( - HttpServletRequest request, ServletAsyncListener listener) { - request.getAsyncContext().addListener(new Listener(listener)); + HttpServletRequest request, + ServletAsyncListener listener, + Object response) { + if (response instanceof HttpServletResponse) { + request + .getAsyncContext() + .addListener(new Listener(listener), request, (HttpServletResponse) response); + } else { + request.getAsyncContext().addListener(new Listener(listener)); + } } @Override diff --git a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java index 99c03fd4ea23..7e11b3acec33 100644 --- a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java +++ b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java @@ -107,8 +107,16 @@ public boolean isRequestAsyncStarted(HttpServletRequest request) { @Override public void addRequestAsyncListener( - HttpServletRequest request, ServletAsyncListener listener) { - request.getAsyncContext().addListener(new Listener(listener)); + HttpServletRequest request, + ServletAsyncListener listener, + Object response) { + if (response instanceof HttpServletResponse) { + request + .getAsyncContext() + .addListener(new Listener(listener), request, (HttpServletResponse) response); + } else { + request.getAsyncContext().addListener(new Listener(listener)); + } } @Override diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java index 49ec785a0298..d3ec79631121 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java @@ -52,7 +52,8 @@ public interface ServletAccessor { boolean isRequestAsyncStarted(REQUEST request); - void addRequestAsyncListener(REQUEST request, ServletAsyncListener listener); + void addRequestAsyncListener( + REQUEST request, ServletAsyncListener listener, Object response); int getResponseStatus(RESPONSE response); diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index 7dbd7f3ef6e1..07f03c266a1e 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -34,6 +34,8 @@ public abstract class ServletHttpServerTracer public static final String ASYNC_LISTENER_ATTRIBUTE = ServletHttpServerTracer.class.getName() + ".AsyncListener"; + public static final String ASYNC_LISTENER_RESPONSE_ATTRIBUTE = + ServletHttpServerTracer.class.getName() + ".AsyncListenerResponse"; private static final boolean CAPTURE_EXPERIMENTAL_SPAN_ATTRIBUTES = Config.get() @@ -146,12 +148,23 @@ public ServletAccessor getServletAccessor() { return accessor; } + /** + * Servlet containers where AsyncContext listener must be attached with request and response + * objects included, a response object to use for that purpose must be attached to the request + * with this method before attaching the listener. + */ + public void setAsyncListenerResponse(REQUEST request, RESPONSE response) { + accessor.setRequestAttribute(request, ASYNC_LISTENER_RESPONSE_ATTRIBUTE, response); + } + public void attachAsyncListener(REQUEST request) { Context context = getServerContext(request); if (context != null) { + Object response = accessor.getRequestAttribute(request, ASYNC_LISTENER_RESPONSE_ATTRIBUTE); + accessor.addRequestAsyncListener( - request, new TagSettingAsyncListener<>(this, new AtomicBoolean(), context)); + request, new TagSettingAsyncListener<>(this, new AtomicBoolean(), context), response); accessor.setRequestAttribute(request, ASYNC_LISTENER_ATTRIBUTE, true); } } diff --git a/instrumentation/servlet/servlet-javax-common/library/src/test/java/RequestOnlyTracer.java b/instrumentation/servlet/servlet-javax-common/library/src/test/java/RequestOnlyTracer.java index 83ce43de89a8..2dd58b88fa0a 100644 --- a/instrumentation/servlet/servlet-javax-common/library/src/test/java/RequestOnlyTracer.java +++ b/instrumentation/servlet/servlet-javax-common/library/src/test/java/RequestOnlyTracer.java @@ -24,7 +24,7 @@ public boolean isRequestAsyncStarted(HttpServletRequest request) { @Override public void addRequestAsyncListener( - HttpServletRequest request, ServletAsyncListener listener) { + HttpServletRequest request, ServletAsyncListener listener, Object response) { throw new UnsupportedOperationException(); } From a0377ee82e0d278486123e0d9b6943356a58ff2d Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Wed, 19 May 2021 13:20:17 +0300 Subject: [PATCH 06/12] Removed unused methods --- .../instrumentation/servlet/v2_2/Servlet2Accessor.java | 5 ----- .../instrumentation/servlet/v3_0/Servlet3Accessor.java | 5 ----- .../servlet/jakarta/v5_0/JakartaServletAccessor.java | 5 ----- .../instrumentation/servlet/ServletAccessor.java | 2 -- .../instrumentation/servlet/ServletHttpServerTracer.java | 4 ---- .../library/src/test/java/RequestOnlyTracer.java | 5 ----- 6 files changed, 26 deletions(-) diff --git a/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2Accessor.java b/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2Accessor.java index fdacc77af917..018a4933e32d 100644 --- a/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2Accessor.java +++ b/instrumentation/servlet/servlet-2.2/library/src/main/java/io/opentelemetry/instrumentation/servlet/v2_2/Servlet2Accessor.java @@ -19,11 +19,6 @@ public Integer getRequestRemotePort(HttpServletRequest httpServletRequest) { return null; } - @Override - public boolean isRequestAsyncStarted(HttpServletRequest request) { - return false; - } - @Override public void addRequestAsyncListener( HttpServletRequest request, diff --git a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3Accessor.java b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3Accessor.java index 8d502fa7853e..7abb9ca31606 100644 --- a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3Accessor.java +++ b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3Accessor.java @@ -22,11 +22,6 @@ public Integer getRequestRemotePort(HttpServletRequest request) { return request.getRemotePort(); } - @Override - public boolean isRequestAsyncStarted(HttpServletRequest request) { - return request.isAsyncStarted(); - } - @Override public void addRequestAsyncListener( HttpServletRequest request, diff --git a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java index 7e11b3acec33..3f67d54e106a 100644 --- a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java +++ b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java @@ -100,11 +100,6 @@ public Integer getRequestRemotePort(HttpServletRequest request) { return request.getRemotePort(); } - @Override - public boolean isRequestAsyncStarted(HttpServletRequest request) { - return request.isAsyncStarted(); - } - @Override public void addRequestAsyncListener( HttpServletRequest request, diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java index d3ec79631121..ff10d69402b8 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java @@ -50,8 +50,6 @@ public interface ServletAccessor { Integer getRequestRemotePort(REQUEST request); - boolean isRequestAsyncStarted(REQUEST request); - void addRequestAsyncListener( REQUEST request, ServletAsyncListener listener, Object response); diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index 07f03c266a1e..68a01d41e47d 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -144,10 +144,6 @@ protected int responseStatus(RESPONSE response) { @Override protected abstract TextMapGetter getGetter(); - public ServletAccessor getServletAccessor() { - return accessor; - } - /** * Servlet containers where AsyncContext listener must be attached with request and response * objects included, a response object to use for that purpose must be attached to the request diff --git a/instrumentation/servlet/servlet-javax-common/library/src/test/java/RequestOnlyTracer.java b/instrumentation/servlet/servlet-javax-common/library/src/test/java/RequestOnlyTracer.java index 2dd58b88fa0a..cd3a47e91cb4 100644 --- a/instrumentation/servlet/servlet-javax-common/library/src/test/java/RequestOnlyTracer.java +++ b/instrumentation/servlet/servlet-javax-common/library/src/test/java/RequestOnlyTracer.java @@ -17,11 +17,6 @@ public Integer getRequestRemotePort(HttpServletRequest httpServletRequest) { throw new UnsupportedOperationException(); } - @Override - public boolean isRequestAsyncStarted(HttpServletRequest request) { - throw new UnsupportedOperationException(); - } - @Override public void addRequestAsyncListener( HttpServletRequest request, ServletAsyncListener listener, Object response) { From 4fafd7f06dbef5e6ee736fe03fc5727e49feb4e9 Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Thu, 20 May 2021 15:50:57 +0300 Subject: [PATCH 07/12] Explicit response to async listeners on all servlet engines --- .../liberty/LibertyStartSpanAdvice.java | 2 - .../servlet/v3_0/Servlet3Advice.java | 2 + .../service/JakartaServletServiceAdvice.java | 2 + .../servlet/ServletHttpServerTracer.java | 6 +-- .../v10_0/Tomcat10ServerHandlerAdvice.java | 9 ++++ .../v10_0/Tomcat10ServletEntityProvider.java | 41 +++++++++++++++++++ .../v7_0/Tomcat7ServerHandlerAdvice.java | 16 +++++++- .../v7_0/Tomcat7ServletEntityProvider.java | 41 +++++++++++++++++++ .../TomcatServerHandlerAdviceHelper.java | 34 +++++++++------ .../common/TomcatServletEntityProvider.java | 24 +++++++++++ 10 files changed, 159 insertions(+), 18 deletions(-) create mode 100644 instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServletEntityProvider.java create mode 100644 instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7ServletEntityProvider.java create mode 100644 instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServletEntityProvider.java diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java index 2cd048e55461..be2bf45dd36b 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java @@ -27,8 +27,6 @@ public static void onEnter() { ctx.setContext(context); ctx.setScope(scope); - // For startAsync instrumentation to work on Liberty, the listener needs to be added with a - // response object included, which is not available in that instrumentation. tracer().setAsyncListenerResponse(ctx.getRequest(), ctx.getResponse()); } } 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 4b7c97221d77..6038158fa71c 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 @@ -81,6 +81,8 @@ public static void onEnter( context = tracer().startSpan(httpServletRequest, mappingResolver, servlet); scope = context.makeCurrent(); + + tracer().setAsyncListenerResponse(httpServletRequest, (HttpServletResponse) response); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletServiceAdvice.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletServiceAdvice.java index 5936fbc6f74d..c9886ba4d028 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletServiceAdvice.java +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletServiceAdvice.java @@ -82,6 +82,8 @@ public static void onEnter( context = tracer().startSpan(httpServletRequest, mappingResolver, servlet); scope = context.makeCurrent(); + + tracer().setAsyncListenerResponse(httpServletRequest, (HttpServletResponse) response); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index 68a01d41e47d..4ba803bc631c 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -145,9 +145,9 @@ protected int responseStatus(RESPONSE response) { protected abstract TextMapGetter getGetter(); /** - * Servlet containers where AsyncContext listener must be attached with request and response - * objects included, a response object to use for that purpose must be attached to the request - * with this method before attaching the listener. + * Response object must be attached to a request prior to {@link #attachAsyncListener(Object)} + * being called, as otherwise in some environments it is not possible to access response from + * async event in listeners. */ public void setAsyncListenerResponse(REQUEST request, RESPONSE response) { accessor.setRequestAttribute(request, ASYNC_LISTENER_RESPONSE_ATTRIBUTE, response); diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServerHandlerAdvice.java b/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServerHandlerAdvice.java index dabf325fd6ff..a7d8bad64fba 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServerHandlerAdvice.java +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServerHandlerAdvice.java @@ -11,6 +11,7 @@ import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.servlet.jakarta.v5_0.JakartaServletHttpServerTracer; import io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatServerHandlerAdviceHelper; +import jakarta.servlet.http.HttpServletRequest; import net.bytebuddy.asm.Advice; import org.apache.coyote.Request; import org.apache.coyote.Response; @@ -24,6 +25,7 @@ public class Tomcat10ServerHandlerAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter( @Advice.Argument(0) Request request, + @Advice.Argument(1) Response response, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { Context attachedContext = tracer().getServerContext(request); @@ -35,6 +37,12 @@ public static void onEnter( context = tracer().startServerSpan(request); scope = context.makeCurrent(); + + TomcatServerHandlerAdviceHelper.attachResponseToRequest( + Tomcat10ServletEntityProvider.INSTANCE, + JakartaServletHttpServerTracer.tracer(), + request, + response); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) @@ -47,6 +55,7 @@ public static void stopSpan( TomcatServerHandlerAdviceHelper.stopSpan( tracer(), + Tomcat10ServletEntityProvider.INSTANCE, JakartaServletHttpServerTracer.tracer(), request, response, diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServletEntityProvider.java b/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServletEntityProvider.java new file mode 100644 index 000000000000..6ef1fad4b3e4 --- /dev/null +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServletEntityProvider.java @@ -0,0 +1,41 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0; + +import io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatServletEntityProvider; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.coyote.Request; +import org.apache.coyote.Response; + +public class Tomcat10ServletEntityProvider + implements TomcatServletEntityProvider { + public static final Tomcat10ServletEntityProvider INSTANCE = new Tomcat10ServletEntityProvider(); + + private Tomcat10ServletEntityProvider() {} + + @Override + public HttpServletRequest getServletRequest(Request request) { + Object note = request.getNote(1); + + if (note instanceof HttpServletRequest) { + return (HttpServletRequest) note; + } else { + return null; + } + } + + @Override + public HttpServletResponse getServletResponse(Response response) { + Object note = response.getNote(1); + + if (note instanceof HttpServletResponse) { + return (HttpServletResponse) note; + } else { + return null; + } + } +} diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7ServerHandlerAdvice.java b/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7ServerHandlerAdvice.java index 550d949bc1f8..340cd9a13ffb 100644 --- a/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7ServerHandlerAdvice.java +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7ServerHandlerAdvice.java @@ -24,6 +24,7 @@ public class Tomcat7ServerHandlerAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter( @Advice.Argument(0) Request request, + @Advice.Argument(1) Response response, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { Context attachedContext = tracer().getServerContext(request); @@ -35,6 +36,12 @@ public static void onEnter( context = tracer().startServerSpan(request); scope = context.makeCurrent(); + + TomcatServerHandlerAdviceHelper.attachResponseToRequest( + Tomcat7ServletEntityProvider.INSTANCE, + Servlet3HttpServerTracer.tracer(), + request, + response); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) @@ -46,6 +53,13 @@ public static void stopSpan( @Advice.Local("otelScope") Scope scope) { TomcatServerHandlerAdviceHelper.stopSpan( - tracer(), Servlet3HttpServerTracer.tracer(), request, response, throwable, context, scope); + tracer(), + Tomcat7ServletEntityProvider.INSTANCE, + Servlet3HttpServerTracer.tracer(), + request, + response, + throwable, + context, + scope); } } diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7ServletEntityProvider.java b/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7ServletEntityProvider.java new file mode 100644 index 000000000000..644e8471ad1d --- /dev/null +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7ServletEntityProvider.java @@ -0,0 +1,41 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.v7_0; + +import io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatServletEntityProvider; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.apache.coyote.Request; +import org.apache.coyote.Response; + +public class Tomcat7ServletEntityProvider + implements TomcatServletEntityProvider { + public static final Tomcat7ServletEntityProvider INSTANCE = new Tomcat7ServletEntityProvider(); + + private Tomcat7ServletEntityProvider() {} + + @Override + public HttpServletRequest getServletRequest(Request request) { + Object note = request.getNote(1); + + if (note instanceof HttpServletRequest) { + return (HttpServletRequest) note; + } else { + return null; + } + } + + @Override + public HttpServletResponse getServletResponse(Response response) { + Object note = response.getNote(1); + + if (note instanceof HttpServletResponse) { + return (HttpServletResponse) note; + } else { + return null; + } + } +} diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java index 507ae28e5249..90157b5aafd1 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java @@ -9,7 +9,6 @@ import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer; import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterAdviceHelper; -import net.bytebuddy.asm.Advice; import org.apache.coyote.Request; import org.apache.coyote.Response; @@ -26,12 +25,13 @@ public class TomcatServerHandlerAdviceHelper { */ public static void stopSpan( TomcatTracer tracer, + TomcatServletEntityProvider servletEntityProvider, ServletHttpServerTracer servletTracer, Request request, Response response, - @Advice.Thrown Throwable throwable, - @Advice.Local("otelContext") Context context, - @Advice.Local("otelScope") Scope scope) { + Throwable throwable, + Context context, + Scope scope) { if (scope != null) { scope.close(); } @@ -56,16 +56,26 @@ public static void stopSpan( return; } - Object note = request.getNote(1); - if (note instanceof org.apache.catalina.connector.Request) { - // The Catalina Request always implements the HttpServletRequest (either javax or jakarta) - // which is also what REQUEST generic parameter is. We cannot cast normally without a - // generic because this class is compiled against javax.servlet which would make it try - // to use request from javax.servlet when REQUEST is actually from jakarta.servlet. - //noinspection unchecked - if (ServletAndFilterAdviceHelper.mustEndOnHandlerMethodExit(servletTracer, (REQUEST) note)) { + REQUEST servletRequest = servletEntityProvider.getServletRequest(request); + + if (servletRequest != null) { + if (ServletAndFilterAdviceHelper.mustEndOnHandlerMethodExit(servletTracer, servletRequest)) { tracer.end(context, response); } } } + + public static void attachResponseToRequest( + TomcatServletEntityProvider servletEntityProvider, + ServletHttpServerTracer servletTracer, + Request request, + Response response) { + + REQUEST servletRequest = servletEntityProvider.getServletRequest(request); + RESPONSE servletResponse = servletEntityProvider.getServletResponse(response); + + if (servletRequest != null && servletResponse != null) { + servletTracer.setAsyncListenerResponse(servletRequest, servletResponse); + } + } } diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServletEntityProvider.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServletEntityProvider.java new file mode 100644 index 000000000000..fe05238313c3 --- /dev/null +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServletEntityProvider.java @@ -0,0 +1,24 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.common; + +import org.apache.coyote.Request; +import org.apache.coyote.Response; + +/** + * Used to access servlet request/response classes from their Apache Coyote counterparts. As the + * Coyote classes are the same for all Tomcat versions, but newer Tomcat uses jakarta.servlet + * instead of javax.servlet, this allows accessing the servlet entities without unchecked casts in + * shared code where HttpServletRequest and/or HttpServletResponse are used as generic parameters. + * + * @param HttpServletRequest instance + * @param HttpServletResponse instance + */ +public interface TomcatServletEntityProvider { + REQUEST getServletRequest(Request request); + + RESPONSE getServletResponse(Response response); +} From b2552fa39a15ba396708dade1654a09562399fb0 Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Thu, 20 May 2021 15:55:50 +0300 Subject: [PATCH 08/12] Attach response to request on Jetty --- .../instrumentation/jetty/v11_0/Jetty11HandlerAdvice.java | 5 ++++- .../instrumentation/jetty/v8_0/Jetty8HandlerAdvice.java | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HandlerAdvice.java b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HandlerAdvice.java index 6e4d29b99cad..dbefc77c1214 100644 --- a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HandlerAdvice.java +++ b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HandlerAdvice.java @@ -19,7 +19,8 @@ public class Jetty11HandlerAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter( @Advice.This Object source, - @Advice.Argument(value = 2, readOnly = false) HttpServletRequest request, + @Advice.Argument(2) HttpServletRequest request, + @Advice.Argument(3) HttpServletResponse response, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { @@ -31,6 +32,8 @@ public static void onEnter( context = tracer().startServerSpan(request); scope = context.makeCurrent(); + + tracer().setAsyncListenerResponse(request, response); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) diff --git a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HandlerAdvice.java b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HandlerAdvice.java index 7f8e1a48f4b7..d89793b54516 100644 --- a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HandlerAdvice.java +++ b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HandlerAdvice.java @@ -19,7 +19,8 @@ public class Jetty8HandlerAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter( @Advice.This Object source, - @Advice.Argument(value = 2, readOnly = false) HttpServletRequest request, + @Advice.Argument(2) HttpServletRequest request, + @Advice.Argument(3) HttpServletResponse response, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { @@ -31,6 +32,8 @@ public static void onEnter( context = tracer().startServerSpan(request); scope = context.makeCurrent(); + + tracer().setAsyncListenerResponse(request, response); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) From 287b0f2df37b64c4e85137d28da41aef1fc70ce8 Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Fri, 21 May 2021 01:02:09 +0300 Subject: [PATCH 09/12] Fix broken build due to rebase, improved a comment --- .../common/async/AsyncStartInstrumentation.java | 8 +++----- .../common/service/ServletAndFilterAdviceHelper.java | 11 +++-------- .../tomcat/v10_0/Tomcat10ServerHandlerAdvice.java | 1 - 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/async/AsyncStartInstrumentation.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/async/AsyncStartInstrumentation.java index 14bc7136241b..6b247a3de568 100644 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/async/AsyncStartInstrumentation.java +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/async/AsyncStartInstrumentation.java @@ -7,14 +7,12 @@ import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface; import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed; -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.returns; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; -import java.util.Map; -import net.bytebuddy.description.method.MethodDescription; +import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -38,8 +36,8 @@ public ElementMatcher typeMatcher() { } @Override - public Map, String> transformers() { - return singletonMap( + public void transform(TypeTransformer transformer) { + transformer.applyAdviceToMethod( named("startAsync").and(returns(named(basePackageName + ".AsyncContext"))).and(isPublic()), adviceClassName); } diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterAdviceHelper.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterAdviceHelper.java index 12e53535ca82..a5c2645dbe73 100644 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterAdviceHelper.java +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterAdviceHelper.java @@ -65,14 +65,9 @@ public static boolean mustEndOnHandlerMethodExit( return false; } - // Possible scenarios: - // 1) synchronously handled request - // 2) asynchronously handled request, but startAsync instrumentation is broken on this server - // (should be caught by tests for the specific server). - // In case of broken startAsync instrumentation, no fallback handling for asynchronous requests - // should be provided as handling it in the handler/service method is prone to race conditions - // (seen happening on Undertow) that may make some tests pass which should fail due to the - // possible race. + // This means that startAsync was not called (assuming startAsync instrumentation works + // correctly on this servlet engine), therefore the request was handled synchronously, and + // handler method end must also end the span. return true; } } diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServerHandlerAdvice.java b/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServerHandlerAdvice.java index a7d8bad64fba..049aea25c7f2 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServerHandlerAdvice.java +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10ServerHandlerAdvice.java @@ -11,7 +11,6 @@ import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.servlet.jakarta.v5_0.JakartaServletHttpServerTracer; import io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatServerHandlerAdviceHelper; -import jakarta.servlet.http.HttpServletRequest; import net.bytebuddy.asm.Advice; import org.apache.coyote.Request; import org.apache.coyote.Response; From 995eba16c2481a8315cd2206b24ab8e3ad0d0737 Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Mon, 24 May 2021 22:31:01 +0300 Subject: [PATCH 10/12] Address PR comments --- .../jetty/v8_0/Jetty8HandlerAdvice.java | 1 + .../liberty/LibertyStartSpanAdvice.java | 1 + .../instrumentation/liberty/ThreadLocalContext.java | 12 ++++++------ .../common/TomcatServerHandlerAdviceHelper.java | 4 ++++ 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HandlerAdvice.java b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HandlerAdvice.java index d89793b54516..0fe4711b3df8 100644 --- a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HandlerAdvice.java +++ b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HandlerAdvice.java @@ -33,6 +33,7 @@ public static void onEnter( context = tracer().startServerSpan(request); scope = context.makeCurrent(); + // Must be set here since Jetty handlers can use startAsync outside of servlet scope. tracer().setAsyncListenerResponse(request, response); } diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java index be2bf45dd36b..017a528342d0 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyStartSpanAdvice.java @@ -27,6 +27,7 @@ public static void onEnter() { ctx.setContext(context); ctx.setScope(scope); + // Must be set here since Liberty RequestProcessors can use startAsync outside of servlet scope. tracer().setAsyncListenerResponse(ctx.getRequest(), ctx.getResponse()); } } diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/ThreadLocalContext.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/ThreadLocalContext.java index 5e9f1cd3d517..698887ec066d 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/ThreadLocalContext.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/ThreadLocalContext.java @@ -14,14 +14,14 @@ public class ThreadLocalContext { private static final ThreadLocal local = new ThreadLocal<>(); - private final HttpServletRequest req; + private final HttpServletRequest request; private final HttpServletResponse response; private Context context; private Scope scope; private boolean started; - private ThreadLocalContext(HttpServletRequest req, HttpServletResponse response) { - this.req = req; + private ThreadLocalContext(HttpServletRequest request, HttpServletResponse response) { + this.request = request; this.response = response; } @@ -42,7 +42,7 @@ public void setScope(Scope scope) { } public HttpServletRequest getRequest() { - return req; + return request; } public HttpServletResponse getResponse() { @@ -60,8 +60,8 @@ public boolean startSpan() { return !b; } - public static void startRequest(HttpServletRequest req, HttpServletResponse response) { - ThreadLocalContext ctx = new ThreadLocalContext(req, response); + public static void startRequest(HttpServletRequest request, HttpServletResponse response) { + ThreadLocalContext ctx = new ThreadLocalContext(request, response); local.set(ctx); } diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java index 90157b5aafd1..85f84ee1014f 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java @@ -65,6 +65,10 @@ public static void stopSpan( } } + /** + * Must be attached in Tomcat instrumentations since Tomcat valves can use startAsync outside of + * servlet scope. + */ public static void attachResponseToRequest( TomcatServletEntityProvider servletEntityProvider, ServletHttpServerTracer servletTracer, From 512dc97ad556f0405d6a26554d90bededc03cb3d Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Mon, 24 May 2021 22:33:21 +0300 Subject: [PATCH 11/12] Added a comment. --- .../instrumentation/jetty/v11_0/Jetty11HandlerAdvice.java | 1 + 1 file changed, 1 insertion(+) diff --git a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HandlerAdvice.java b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HandlerAdvice.java index dbefc77c1214..f5221d3806df 100644 --- a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HandlerAdvice.java +++ b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HandlerAdvice.java @@ -33,6 +33,7 @@ public static void onEnter( context = tracer().startServerSpan(request); scope = context.makeCurrent(); + // Must be set here since Jetty handlers can use startAsync outside of servlet scope. tracer().setAsyncListenerResponse(request, response); } From 763aef74da2a7f24aa57de4438cfafd0c844bd92 Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Tue, 25 May 2021 14:34:20 +0300 Subject: [PATCH 12/12] Addressed PR comments --- .../servlet/v3_0/Servlet3AsyncStartAdvice.java | 10 +++++----- .../tomcat/common/TomcatServerHandlerAdviceHelper.java | 7 +++---- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java index 60ecedbaf8a5..0d046357405a 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3AsyncStartAdvice.java @@ -9,9 +9,9 @@ import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap; import javax.servlet.AsyncContext; +import javax.servlet.ServletRequest; import javax.servlet.http.HttpServletRequest; import net.bytebuddy.asm.Advice; -import net.bytebuddy.implementation.bytecode.assign.Assigner; public class Servlet3AsyncStartAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) @@ -21,9 +21,7 @@ public static void startAsyncEnter() { } @Advice.OnMethodExit(suppress = Throwable.class) - public static void startAsyncExit( - @Advice.This(typing = Assigner.Typing.DYNAMIC) HttpServletRequest request) { - + public static void startAsyncExit(@Advice.This ServletRequest servletRequest) { int callDepth = CallDepthThreadLocalMap.decrementCallDepth(AsyncContext.class); if (callDepth != 0) { @@ -31,7 +29,9 @@ public static void startAsyncExit( return; } - if (request != null) { + if (servletRequest instanceof HttpServletRequest) { + HttpServletRequest request = (HttpServletRequest) servletRequest; + if (!tracer().isAsyncListenerAttached(request)) { tracer().attachAsyncListener(request); } diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java index 85f84ee1014f..5583e137fb4e 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerAdviceHelper.java @@ -58,10 +58,9 @@ public static void stopSpan( REQUEST servletRequest = servletEntityProvider.getServletRequest(request); - if (servletRequest != null) { - if (ServletAndFilterAdviceHelper.mustEndOnHandlerMethodExit(servletTracer, servletRequest)) { - tracer.end(context, response); - } + if (servletRequest != null + && ServletAndFilterAdviceHelper.mustEndOnHandlerMethodExit(servletTracer, servletRequest)) { + tracer.end(context, response); } }