From 8b1da0f38a9f59df0e6b81cd46f6614ad4494cac Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 28 Jul 2021 15:54:21 +0300 Subject: [PATCH 1/3] Fix NullPointerException on tomcat --- .../servlet/v3_0/Servlet3Accessor.java | 2 - .../jakarta/v5_0/JakartaServletAccessor.java | 2 - .../v10_0/Tomcat10AttachResponseAdvice.java | 30 ++++ .../v10_0/Tomcat10InstrumentationModule.java | 9 +- .../v10_0/Tomcat10ServerHandlerAdvice.java | 6 - .../tomcat/v10_0/AsyncServlet.groovy | 74 ++++++++++ .../tomcat/v10_0/TomcatAsyncTest.groovy | 129 +++++++++++++++++ .../v7_0/Tomcat7AttachResponseAdvice.java | 30 ++++ .../v7_0/Tomcat7InstrumentationModule.java | 9 +- .../v7_0/Tomcat7ServerHandlerAdvice.java | 6 - .../tomcat/v7_0/AsyncServlet.groovy | 74 ++++++++++ .../tomcat/v7_0/TomcatAsyncTest.groovy | 130 ++++++++++++++++++ .../TomcatServerHandlerInstrumentation.java | 20 ++- 13 files changed, 493 insertions(+), 28 deletions(-) create mode 100644 instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10AttachResponseAdvice.java create mode 100644 instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy create mode 100644 instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatAsyncTest.groovy create mode 100644 instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7AttachResponseAdvice.java create mode 100644 instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/AsyncServlet.groovy create mode 100644 instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatAsyncTest.groovy 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 7abb9ca31606..5bd5900df897 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 @@ -31,8 +31,6 @@ public void addRequestAsyncListener( request .getAsyncContext() .addListener(new Listener(listener), request, (HttpServletResponse) response); - } else { - request.getAsyncContext().addListener(new Listener(listener)); } } 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 3f67d54e106a..690da46fad82 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 @@ -109,8 +109,6 @@ public void addRequestAsyncListener( request .getAsyncContext() .addListener(new Listener(listener), request, (HttpServletResponse) response); - } else { - request.getAsyncContext().addListener(new Listener(listener)); } } diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10AttachResponseAdvice.java b/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10AttachResponseAdvice.java new file mode 100644 index 000000000000..50ca5b66d2f3 --- /dev/null +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10AttachResponseAdvice.java @@ -0,0 +1,30 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0; + +import io.opentelemetry.instrumentation.servlet.jakarta.v5_0.JakartaServletHttpServerTracer; +import io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatServerHandlerAdviceHelper; +import net.bytebuddy.asm.Advice; +import org.apache.coyote.Request; +import org.apache.coyote.Response; + +public class Tomcat10AttachResponseAdvice { + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void attachResponse( + @Advice.Argument(0) Request request, + @Advice.Argument(2) Response response, + @Advice.Return boolean success) { + + if (success) { + TomcatServerHandlerAdviceHelper.attachResponseToRequest( + 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/Tomcat10InstrumentationModule.java b/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10InstrumentationModule.java index 6df4ab2e1ed5..7f77d7485af1 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10InstrumentationModule.java +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/Tomcat10InstrumentationModule.java @@ -6,12 +6,12 @@ package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0; import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed; +import static java.util.Collections.singletonList; 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.tomcat.common.TomcatServerHandlerInstrumentation; -import java.util.Collections; import java.util.List; import net.bytebuddy.matcher.ElementMatcher; @@ -30,9 +30,10 @@ public ElementMatcher.Junction classLoaderMatcher() { @Override public List typeInstrumentations() { - return Collections.singletonList( + String packageName = Tomcat10InstrumentationModule.class.getPackage().getName(); + return singletonList( new TomcatServerHandlerInstrumentation( - Tomcat10InstrumentationModule.class.getPackage().getName() - + ".Tomcat10ServerHandlerAdvice")); + packageName + ".Tomcat10ServerHandlerAdvice", + packageName + ".Tomcat10AttachResponseAdvice")); } } 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 e3c0b8fe1be5..23435036bd58 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 @@ -31,12 +31,6 @@ 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) diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy new file mode 100644 index 000000000000..4a1f208ba13f --- /dev/null +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy @@ -0,0 +1,74 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0 + +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +import io.opentelemetry.instrumentation.test.base.HttpServerTest +import jakarta.servlet.annotation.WebServlet +import jakarta.servlet.http.HttpServlet +import jakarta.servlet.http.HttpServletRequest +import jakarta.servlet.http.HttpServletResponse +import java.util.concurrent.Phaser + +@WebServlet(asyncSupported = true) +class AsyncServlet extends HttpServlet { + @Override + protected void service(HttpServletRequest req, HttpServletResponse resp) { + HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath) + def phaser = new Phaser(2) + def context = req.startAsync() + context.start { + try { + phaser.arriveAndAwaitAdvance() + HttpServerTest.controller(endpoint) { + resp.contentType = "text/plain" + switch (endpoint) { + case SUCCESS: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + context.complete() + break + case INDEXED_CHILD: + endpoint.collectSpanAttributes { req.getParameter(it) } + resp.status = endpoint.status + context.complete() + break + case QUERY_PARAM: + resp.status = endpoint.status + resp.writer.print(req.queryString) + context.complete() + break + case REDIRECT: + resp.sendRedirect(endpoint.body) + context.complete() + break + case ERROR: + resp.status = endpoint.status + resp.writer.print(endpoint.body) +// resp.sendError(endpoint.status, endpoint.body) + context.complete() + break + case EXCEPTION: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + context.complete() + throw new Exception(endpoint.body) + } + } + } finally { + phaser.arriveAndDeregister() + } + } + phaser.arriveAndAwaitAdvance() + phaser.arriveAndAwaitAdvance() + } +} diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatAsyncTest.groovy b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatAsyncTest.groovy new file mode 100644 index 000000000000..d0194bb8fbc5 --- /dev/null +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatAsyncTest.groovy @@ -0,0 +1,129 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0 + +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +import io.opentelemetry.instrumentation.test.AgentTestTrait +import io.opentelemetry.instrumentation.test.asserts.TraceAssert +import io.opentelemetry.instrumentation.test.base.HttpServerTest +import jakarta.servlet.Servlet +import jakarta.servlet.ServletException +import java.nio.file.Files +import org.apache.catalina.Context +import org.apache.catalina.startup.Tomcat +import org.apache.tomcat.JarScanFilter +import org.apache.tomcat.JarScanType +import spock.lang.Unroll + +@Unroll +class TomcatAsyncTest extends HttpServerTest implements AgentTestTrait { + + @Override + Tomcat startServer(int port) { + def tomcatServer = new Tomcat() + + def baseDir = Files.createTempDirectory("tomcat").toFile() + baseDir.deleteOnExit() + tomcatServer.setBaseDir(baseDir.getAbsolutePath()) + + tomcatServer.setPort(port) + tomcatServer.getConnector().enableLookups = true // get localhost instead of 127.0.0.1 + + File applicationDir = new File(baseDir, "/webapps/ROOT") + if (!applicationDir.exists()) { + applicationDir.mkdirs() + applicationDir.deleteOnExit() + } + Context servletContext = tomcatServer.addWebapp(contextPath, applicationDir.getAbsolutePath()) + // Speed up startup by disabling jar scanning: + servletContext.getJarScanner().setJarScanFilter(new JarScanFilter() { + @Override + boolean check(JarScanType jarScanType, String jarName) { + return false + } + }) + + setupServlets(servletContext) + + tomcatServer.start() + + return tomcatServer + } + + @Override + void stopServer(Tomcat server) { + server.stop() + server.destroy() + } + + @Override + String getContextPath() { + return "/tomcat-context" + } + + protected void setupServlets(Context context) { + def servlet = servlet() + + addServlet(context, SUCCESS.path, servlet) + addServlet(context, QUERY_PARAM.path, servlet) + addServlet(context, ERROR.path, servlet) + addServlet(context, EXCEPTION.path, servlet) + addServlet(context, REDIRECT.path, servlet) + addServlet(context, AUTH_REQUIRED.path, servlet) + addServlet(context, INDEXED_CHILD.path, servlet) + } + + void addServlet(Context servletContext, String path, Class servlet) { + String name = UUID.randomUUID() + Tomcat.addServlet(servletContext, name, servlet.newInstance()) + servletContext.addServletMappingDecoded(path, name) + } + + Class servlet() { + AsyncServlet + } + + @Override + boolean testException() { + // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807 + return false + } + + @Override + boolean testConcurrency() { + return true + } + + @Override + Class expectedExceptionClass() { + ServletException + } + + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == NOT_FOUND || endpoint == REDIRECT + } + + @Override + void responseSpan(TraceAssert trace, int index, Object parent, String method, ServerEndpoint endpoint) { + switch (endpoint) { + case REDIRECT: + redirectSpan(trace, index, parent) + break + case NOT_FOUND: + sendErrorSpan(trace, index, parent) + break + } + } +} diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7AttachResponseAdvice.java b/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7AttachResponseAdvice.java new file mode 100644 index 000000000000..229fd397d1da --- /dev/null +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7AttachResponseAdvice.java @@ -0,0 +1,30 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.v7_0; + +import io.opentelemetry.instrumentation.servlet.v3_0.Servlet3HttpServerTracer; +import io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatServerHandlerAdviceHelper; +import net.bytebuddy.asm.Advice; +import org.apache.coyote.Request; +import org.apache.coyote.Response; + +public class Tomcat7AttachResponseAdvice { + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void attachResponse( + @Advice.Argument(0) Request request, + @Advice.Argument(2) Response response, + @Advice.Return boolean success) { + + if (success) { + TomcatServerHandlerAdviceHelper.attachResponseToRequest( + Tomcat7ServletEntityProvider.INSTANCE, + Servlet3HttpServerTracer.tracer(), + request, + response); + } + } +} diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7InstrumentationModule.java b/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7InstrumentationModule.java index 89355093d552..1a3afb393f50 100644 --- a/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7InstrumentationModule.java +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/Tomcat7InstrumentationModule.java @@ -6,13 +6,13 @@ package io.opentelemetry.javaagent.instrumentation.tomcat.v7_0; import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed; +import static java.util.Collections.singletonList; import static net.bytebuddy.matcher.ElementMatchers.not; 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.tomcat.common.TomcatServerHandlerInstrumentation; -import java.util.Collections; import java.util.List; import net.bytebuddy.matcher.ElementMatcher; @@ -33,9 +33,10 @@ public ElementMatcher.Junction classLoaderMatcher() { public List typeInstrumentations() { // Tomcat 10+ is excluded by making sure Request does not have any methods returning // jakarta.servlet.ReadListener which is returned by getReadListener method on Tomcat 10+ - return Collections.singletonList( + String packageName = Tomcat7InstrumentationModule.class.getPackage().getName(); + return singletonList( new TomcatServerHandlerInstrumentation( - Tomcat7InstrumentationModule.class.getPackage().getName() - + ".Tomcat7ServerHandlerAdvice")); + packageName + ".Tomcat7ServerHandlerAdvice", + packageName + ".Tomcat7AttachResponseAdvice")); } } 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 ad3e33363ba7..de6c47efe6a1 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 @@ -31,12 +31,6 @@ 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) diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/AsyncServlet.groovy b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/AsyncServlet.groovy new file mode 100644 index 000000000000..b1b777626111 --- /dev/null +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/AsyncServlet.groovy @@ -0,0 +1,74 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.v7_0 + +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +import groovy.servlet.AbstractHttpServlet +import io.opentelemetry.instrumentation.test.base.HttpServerTest +import java.util.concurrent.Phaser +import javax.servlet.annotation.WebServlet +import javax.servlet.http.HttpServletRequest +import javax.servlet.http.HttpServletResponse + +@WebServlet(asyncSupported = true) +class AsyncServlet extends AbstractHttpServlet { + @Override + protected void service(HttpServletRequest req, HttpServletResponse resp) { + HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath) + def phaser = new Phaser(2) + def context = req.startAsync() + context.start { + try { + phaser.arriveAndAwaitAdvance() + HttpServerTest.controller(endpoint) { + resp.contentType = "text/plain" + switch (endpoint) { + case SUCCESS: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + context.complete() + break + case INDEXED_CHILD: + endpoint.collectSpanAttributes { req.getParameter(it) } + resp.status = endpoint.status + context.complete() + break + case QUERY_PARAM: + resp.status = endpoint.status + resp.writer.print(req.queryString) + context.complete() + break + case REDIRECT: + resp.sendRedirect(endpoint.body) + context.complete() + break + case ERROR: + resp.status = endpoint.status + resp.writer.print(endpoint.body) +// resp.sendError(endpoint.status, endpoint.body) + context.complete() + break + case EXCEPTION: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + context.complete() + throw new Exception(endpoint.body) + } + } + } finally { + phaser.arriveAndDeregister() + } + } + phaser.arriveAndAwaitAdvance() + phaser.arriveAndAwaitAdvance() + } +} diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatAsyncTest.groovy b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatAsyncTest.groovy new file mode 100644 index 000000000000..a66b39e2e2d3 --- /dev/null +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatAsyncTest.groovy @@ -0,0 +1,130 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.tomcat.v7_0 + +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +import io.opentelemetry.instrumentation.test.AgentTestTrait +import io.opentelemetry.instrumentation.test.asserts.TraceAssert +import io.opentelemetry.instrumentation.test.base.HttpServerTest +import java.nio.file.Files +import javax.servlet.Servlet +import javax.servlet.ServletException +import org.apache.catalina.Context +import org.apache.catalina.startup.Tomcat +import org.apache.tomcat.JarScanFilter +import org.apache.tomcat.JarScanType +import spock.lang.Unroll + + +@Unroll +class TomcatAsyncTest extends HttpServerTest implements AgentTestTrait { + + @Override + Tomcat startServer(int port) { + def tomcatServer = new Tomcat() + + def baseDir = Files.createTempDirectory("tomcat").toFile() + baseDir.deleteOnExit() + tomcatServer.setBaseDir(baseDir.getAbsolutePath()) + + tomcatServer.setPort(port) + tomcatServer.getConnector().enableLookups = true // get localhost instead of 127.0.0.1 + + File applicationDir = new File(baseDir, "/webapps/ROOT") + if (!applicationDir.exists()) { + applicationDir.mkdirs() + applicationDir.deleteOnExit() + } + Context servletContext = tomcatServer.addWebapp(contextPath, applicationDir.getAbsolutePath()) + // Speed up startup by disabling jar scanning: + servletContext.getJarScanner().setJarScanFilter(new JarScanFilter() { + @Override + boolean check(JarScanType jarScanType, String jarName) { + return false + } + }) + + setupServlets(servletContext) + + tomcatServer.start() + + return tomcatServer + } + + @Override + void stopServer(Tomcat server) { + server.stop() + server.destroy() + } + + @Override + String getContextPath() { + return "/tomcat-context" + } + + protected void setupServlets(Context context) { + def servlet = servlet() + + addServlet(context, SUCCESS.path, servlet) + addServlet(context, QUERY_PARAM.path, servlet) + addServlet(context, ERROR.path, servlet) + addServlet(context, EXCEPTION.path, servlet) + addServlet(context, REDIRECT.path, servlet) + addServlet(context, AUTH_REQUIRED.path, servlet) + addServlet(context, INDEXED_CHILD.path, servlet) + } + + void addServlet(Context servletContext, String path, Class servlet) { + String name = UUID.randomUUID() + Tomcat.addServlet(servletContext, name, servlet.newInstance()) + servletContext.addServletMappingDecoded(path, name) + } + + Class servlet() { + AsyncServlet + } + + @Override + boolean testException() { + // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807 + return false + } + + @Override + boolean testConcurrency() { + return true + } + + @Override + Class expectedExceptionClass() { + ServletException + } + + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == NOT_FOUND || endpoint == REDIRECT + } + + @Override + void responseSpan(TraceAssert trace, int index, Object parent, String method, ServerEndpoint endpoint) { + switch (endpoint) { + case REDIRECT: + redirectSpan(trace, index, parent) + break + case NOT_FOUND: + sendErrorSpan(trace, index, parent) + break + } + } +} diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerInstrumentation.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerInstrumentation.java index 863e7071b457..31efb2492ee0 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerInstrumentation.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerInstrumentation.java @@ -9,6 +9,7 @@ import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.returns; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; @@ -17,10 +18,13 @@ import net.bytebuddy.matcher.ElementMatcher; public class TomcatServerHandlerInstrumentation implements TypeInstrumentation { - private final String adviceClassName; + private final String handlerAdviceClassName; + private final String attachResponseAdviceClassName; - public TomcatServerHandlerInstrumentation(String adviceClassName) { - this.adviceClassName = adviceClassName; + public TomcatServerHandlerInstrumentation( + String handlerAdviceClassName, String attachResponseAdviceClassName) { + this.handlerAdviceClassName = handlerAdviceClassName; + this.attachResponseAdviceClassName = attachResponseAdviceClassName; } @Override @@ -36,6 +40,14 @@ public void transform(TypeTransformer transformer) { .and(named("service")) .and(takesArgument(0, named("org.apache.coyote.Request"))) .and(takesArgument(1, named("org.apache.coyote.Response"))), - adviceClassName); + handlerAdviceClassName); + + transformer.applyAdviceToMethod( + isMethod() + .and(named("postParseRequest")) + .and(takesArgument(0, named("org.apache.coyote.Request"))) + .and(takesArgument(2, named("org.apache.coyote.Response"))) + .and(returns(boolean.class)), + attachResponseAdviceClassName); } } From 4be8161a504239515b21e92746e9fff022e98aaa Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 28 Jul 2021 15:57:03 +0300 Subject: [PATCH 2/3] remove commented out line --- .../javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy | 1 - .../javaagent/instrumentation/tomcat/v7_0/AsyncServlet.groovy | 1 - 2 files changed, 2 deletions(-) diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy index 4a1f208ba13f..a5ecd74cc2af 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/AsyncServlet.groovy @@ -54,7 +54,6 @@ class AsyncServlet extends HttpServlet { case ERROR: resp.status = endpoint.status resp.writer.print(endpoint.body) -// resp.sendError(endpoint.status, endpoint.body) context.complete() break case EXCEPTION: diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/AsyncServlet.groovy b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/AsyncServlet.groovy index b1b777626111..7ce4bd9c40cd 100644 --- a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/AsyncServlet.groovy +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/AsyncServlet.groovy @@ -54,7 +54,6 @@ class AsyncServlet extends AbstractHttpServlet { case ERROR: resp.status = endpoint.status resp.writer.print(endpoint.body) -// resp.sendError(endpoint.status, endpoint.body) context.complete() break case EXCEPTION: From d3318ced940c93c811d906dce547254653e7c9c2 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 28 Jul 2021 18:10:13 +0300 Subject: [PATCH 3/3] instrument CoyoteAdapter --- .../tomcat/common/TomcatServerHandlerInstrumentation.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerInstrumentation.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerInstrumentation.java index 31efb2492ee0..96ce312d07fd 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerInstrumentation.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatServerHandlerInstrumentation.java @@ -5,7 +5,6 @@ package io.opentelemetry.javaagent.instrumentation.tomcat.common; -import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.named; @@ -29,7 +28,7 @@ public TomcatServerHandlerInstrumentation( @Override public ElementMatcher typeMatcher() { - return implementsInterface(named("org.apache.coyote.Adapter")); + return named("org.apache.catalina.connector.CoyoteAdapter"); } @Override