Record exception for async servlet invocations (#4677)

* Record exception for asyn servlet invocations

* add back accidentally commented out line

* rearrange test so that it passes on both jetty and tomcat
This commit is contained in:
Lauri Tulmin 2021-11-22 21:17:59 +02:00 committed by GitHub
parent e01422736b
commit c53c8a0f01
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
18 changed files with 217 additions and 118 deletions

View File

@ -0,0 +1,27 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/
package io.opentelemetry.javaagent.instrumentation.servlet.v3_0;
import static io.opentelemetry.javaagent.instrumentation.servlet.v3_0.Servlet3Singletons.helper;
import javax.servlet.AsyncContext;
import javax.servlet.ServletRequest;
import javax.servlet.http.HttpServletRequest;
import net.bytebuddy.asm.Advice;
@SuppressWarnings("unused")
public class Servlet3AsyncContextStartAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void start(
@Advice.This AsyncContext asyncContext,
@Advice.Argument(value = 0, readOnly = false) Runnable runnable) {
ServletRequest request = asyncContext.getRequest();
if (request instanceof HttpServletRequest) {
runnable = helper().wrapAsyncRunnable((HttpServletRequest) request, runnable);
}
}
}

View File

@ -12,6 +12,7 @@ import com.google.auto.service.AutoService;
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextStartInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncStartInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterInstrumentation;
import java.util.List;
@ -34,12 +35,14 @@ public class Servlet3InstrumentationModule extends InstrumentationModule {
public List<TypeInstrumentation> typeInstrumentations() {
return asList(
new AsyncContextInstrumentation(BASE_PACKAGE, adviceClassName(".AsyncDispatchAdvice")),
new AsyncContextStartInstrumentation(
BASE_PACKAGE, adviceClassName(".Servlet3AsyncContextStartAdvice")),
new AsyncStartInstrumentation(BASE_PACKAGE, adviceClassName(".Servlet3AsyncStartAdvice")),
new ServletAndFilterInstrumentation(
BASE_PACKAGE,
adviceClassName(".Servlet3Advice"),
adviceClassName(".Servlet3InitAdvice"),
adviceClassName(".Servlet3FilterInitAdvice")),
new AsyncStartInstrumentation(BASE_PACKAGE, adviceClassName(".Servlet3AsyncStartAdvice")));
adviceClassName(".Servlet3FilterInitAdvice")));
}
private static String adviceClassName(String suffix) {

View File

@ -42,9 +42,13 @@ abstract class JettyServlet3Test extends AbstractServlet3Test<Server, ServletCon
ServletException
}
boolean isAsyncTest() {
false
}
@Override
boolean hasResponseSpan(ServerEndpoint endpoint) {
return (IS_BEFORE_94 && endpoint == EXCEPTION) || super.hasResponseSpan(endpoint)
return (IS_BEFORE_94 && endpoint == EXCEPTION && !isAsyncTest()) || super.hasResponseSpan(endpoint)
}
@Override
@ -140,9 +144,8 @@ class JettyServlet3TestAsync extends JettyServlet3Test {
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
boolean isAsyncTest() {
true
}
}
@ -152,12 +155,6 @@ class JettyServlet3TestFakeAsync extends JettyServlet3Test {
Class<Servlet> servlet() {
TestServlet3.FakeAsync
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
class JettyServlet3TestForward extends JettyDispatchTest {
@ -223,6 +220,11 @@ class JettyServlet3TestDispatchImmediate extends JettyDispatchTest {
TestServlet3.Sync
}
@Override
boolean isAsyncTest() {
true
}
@Override
protected void setupServlets(ServletContextHandler context) {
super.setupServlets(context)
@ -237,12 +239,6 @@ class JettyServlet3TestDispatchImmediate extends JettyDispatchTest {
addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch/recursive", TestServlet3.DispatchRecursive)
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
class JettyServlet3TestDispatchAsync extends JettyDispatchTest {
@ -251,6 +247,11 @@ class JettyServlet3TestDispatchAsync extends JettyDispatchTest {
TestServlet3.Async
}
@Override
boolean isAsyncTest() {
true
}
@Override
protected void setupServlets(ServletContextHandler context) {
super.setupServlets(context)
@ -270,12 +271,6 @@ class JettyServlet3TestDispatchAsync extends JettyDispatchTest {
boolean errorEndpointUsesSendError() {
false
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
abstract class JettyDispatchTest extends JettyServlet3Test {

View File

@ -5,7 +5,6 @@
import groovy.servlet.AbstractHttpServlet
import io.opentelemetry.instrumentation.test.base.HttpServerTest
import javax.servlet.RequestDispatcher
import javax.servlet.ServletException
import javax.servlet.annotation.WebServlet
@ -72,6 +71,9 @@ class TestServlet3 {
HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath)
def latch = new CountDownLatch(1)
def context = req.startAsync()
if (endpoint == EXCEPTION) {
context.setTimeout(5000)
}
context.start {
try {
HttpServerTest.controller(endpoint) {
@ -111,8 +113,7 @@ class TestServlet3 {
case EXCEPTION:
resp.status = endpoint.status
resp.writer.print(endpoint.body)
context.complete()
throw new Exception(endpoint.body)
throw new ServletException(endpoint.body)
}
}
} finally {
@ -157,7 +158,9 @@ class TestServlet3 {
resp.sendError(endpoint.status, endpoint.body)
break
case EXCEPTION:
throw new Exception(endpoint.body)
resp.status = endpoint.status
resp.writer.print(endpoint.body)
throw new ServletException(endpoint.body)
}
}
} finally {

View File

@ -319,12 +319,6 @@ class TomcatServlet3TestAsync extends TomcatServlet3Test {
boolean errorEndpointUsesSendError() {
false
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
class TomcatServlet3TestFakeAsync extends TomcatServlet3Test {
@ -333,12 +327,6 @@ class TomcatServlet3TestFakeAsync extends TomcatServlet3Test {
Class<Servlet> servlet() {
TestServlet3.FakeAsync
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
class TomcatServlet3TestForward extends TomcatDispatchTest {
@ -459,12 +447,6 @@ class TomcatServlet3TestDispatchAsync extends TomcatDispatchTest {
boolean errorEndpointUsesSendError() {
false
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
abstract class TomcatDispatchTest extends TomcatServlet3Test {

View File

@ -9,6 +9,7 @@ import com.google.auto.service.AutoService;
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextStartInstrumentation;
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;
@ -28,6 +29,8 @@ public class JakartaServletInstrumentationModule extends InstrumentationModule {
return Arrays.asList(
new AsyncContextInstrumentation(
BASE_PACKAGE, adviceClassName(".async.AsyncDispatchAdvice")),
new AsyncContextStartInstrumentation(
BASE_PACKAGE, adviceClassName(".async.AsyncContextStartAdvice")),
new AsyncStartInstrumentation(BASE_PACKAGE, adviceClassName(".async.AsyncStartAdvice")),
new ServletAndFilterInstrumentation(
BASE_PACKAGE,

View File

@ -0,0 +1,27 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/
package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.async;
import static io.opentelemetry.javaagent.instrumentation.servlet.v5_0.Servlet5Singletons.helper;
import jakarta.servlet.AsyncContext;
import jakarta.servlet.ServletRequest;
import jakarta.servlet.http.HttpServletRequest;
import net.bytebuddy.asm.Advice;
@SuppressWarnings("unused")
public class AsyncContextStartAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void start(
@Advice.This AsyncContext asyncContext,
@Advice.Argument(value = 0, readOnly = false) Runnable runnable) {
ServletRequest request = asyncContext.getRequest();
if (request instanceof HttpServletRequest) {
runnable = helper().wrapAsyncRunnable((HttpServletRequest) request, runnable);
}
}
}

View File

@ -119,12 +119,6 @@ class JettyServlet5TestAsync extends JettyServlet5Test {
boolean errorEndpointUsesSendError() {
false
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
@IgnoreIf({ !jvm.java11Compatible })
@ -134,12 +128,6 @@ class JettyServlet5TestFakeAsync extends JettyServlet5Test {
Class<Servlet> servlet() {
TestServlet5.FakeAsync
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
@IgnoreIf({ !jvm.java11Compatible })
@ -222,12 +210,6 @@ class JettyServlet5TestDispatchImmediate extends JettyDispatchTest {
addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet5.DispatchImmediate)
addServlet(context, "/dispatch/recursive", TestServlet5.DispatchRecursive)
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
@IgnoreIf({ !jvm.java11Compatible })
@ -256,12 +238,6 @@ class JettyServlet5TestDispatchAsync extends JettyDispatchTest {
boolean errorEndpointUsesSendError() {
false
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
abstract class JettyDispatchTest extends JettyServlet5Test {

View File

@ -72,6 +72,9 @@ class TestServlet5 {
HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath)
def latch = new CountDownLatch(1)
def context = req.startAsync()
if (endpoint == EXCEPTION) {
context.setTimeout(5000)
}
context.start {
try {
HttpServerTest.controller(endpoint) {
@ -111,8 +114,7 @@ class TestServlet5 {
case EXCEPTION:
resp.status = endpoint.status
resp.writer.print(endpoint.body)
context.complete()
throw new Exception(endpoint.body)
throw new ServletException(endpoint.body)
}
}
} finally {
@ -157,7 +159,9 @@ class TestServlet5 {
resp.sendError(endpoint.status, endpoint.body)
break
case EXCEPTION:
throw new Exception(endpoint.body)
resp.status = endpoint.status
resp.writer.print(endpoint.body)
throw new ServletException(endpoint.body)
}
}
} finally {

View File

@ -318,12 +318,6 @@ class TomcatServlet5TestAsync extends TomcatServlet5Test {
boolean errorEndpointUsesSendError() {
false
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
class TomcatServlet5TestFakeAsync extends TomcatServlet5Test {
@ -332,12 +326,6 @@ class TomcatServlet5TestFakeAsync extends TomcatServlet5Test {
Class<Servlet> servlet() {
TestServlet5.FakeAsync
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
class TomcatServlet5TestForward extends TomcatDispatchTest {
@ -458,12 +446,6 @@ class TomcatServlet5TestDispatchAsync extends TomcatDispatchTest {
boolean errorEndpointUsesSendError() {
false
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
}
abstract class TomcatDispatchTest extends TomcatServlet5Test {

View File

@ -35,7 +35,8 @@ public class AsyncRequestCompletionListener<REQUEST, RESPONSE>
if (responseHandled.compareAndSet(false, true)) {
ServletResponseContext<RESPONSE> responseContext =
new ServletResponseContext<>(response, null);
instrumenter.end(context, requestContext, responseContext, null);
Throwable throwable = servletHelper.getAsyncException(requestContext.request());
instrumenter.end(context, requestContext, responseContext, throwable);
}
}
@ -46,7 +47,8 @@ public class AsyncRequestCompletionListener<REQUEST, RESPONSE>
ServletResponseContext<RESPONSE> responseContext =
new ServletResponseContext<>(response, null);
responseContext.setTimeout(timeout);
instrumenter.end(context, requestContext, responseContext, null);
Throwable throwable = servletHelper.getAsyncException(requestContext.request());
instrumenter.end(context, requestContext, responseContext, throwable);
}
}

View File

@ -0,0 +1,37 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/
package io.opentelemetry.javaagent.instrumentation.servlet;
public class AsyncRunnableWrapper<REQUEST> implements Runnable {
private final ServletHelper<REQUEST, ?> helper;
private final REQUEST request;
private final Runnable runnable;
private AsyncRunnableWrapper(
ServletHelper<REQUEST, ?> helper, REQUEST request, Runnable runnable) {
this.helper = helper;
this.request = request;
this.runnable = runnable;
}
public static <REQUEST> Runnable wrap(
ServletHelper<REQUEST, ?> helper, REQUEST request, Runnable runnable) {
if (runnable == null || runnable instanceof AsyncRunnableWrapper) {
return runnable;
}
return new AsyncRunnableWrapper(helper, request, runnable);
}
@Override
public void run() {
try {
runnable.run();
} catch (Throwable throwable) {
helper.recordAsyncException(request, throwable);
throw throwable;
}
}
}

View File

@ -15,6 +15,8 @@ public class ServletHelper<REQUEST, RESPONSE> extends BaseServletHelper<REQUEST,
ServletHelper.class.getName() + ".AsyncListener";
private static final String ASYNC_LISTENER_RESPONSE_ATTRIBUTE =
ServletHelper.class.getName() + ".AsyncListenerResponse";
private static final String ASYNC_EXCEPTION_ATTRIBUTE =
ServletHelper.class.getName() + ".AsyncException";
public static final String CONTEXT_ATTRIBUTE = ServletHelper.class.getName() + ".Context";
public ServletHelper(
@ -42,6 +44,11 @@ public class ServletHelper<REQUEST, RESPONSE> extends BaseServletHelper<REQUEST,
// instrumentation and we have an uncaught throwable. Let's add it to the current span.
if (throwable != null) {
recordException(currentContext, throwable);
if (!mustEndOnHandlerMethodExit(request)) {
// We could be inside async dispatch. Unlike tomcat jetty does not call
// ServletAsyncListener.onError when exception is thrown inside async dispatch.
recordAsyncException(request, throwable);
}
}
}
@ -110,4 +117,16 @@ public class ServletHelper<REQUEST, RESPONSE> extends BaseServletHelper<REQUEST,
public boolean isAsyncListenerAttached(REQUEST request) {
return accessor.getRequestAttribute(request, ASYNC_LISTENER_ATTRIBUTE) != null;
}
public Runnable wrapAsyncRunnable(REQUEST request, Runnable runnable) {
return AsyncRunnableWrapper.wrap(this, request, runnable);
}
public void recordAsyncException(REQUEST request, Throwable throwable) {
accessor.setRequestAttribute(request, ASYNC_EXCEPTION_ATTRIBUTE, throwable);
}
public Throwable getAsyncException(REQUEST request) {
return (Throwable) accessor.getRequestAttribute(request, ASYNC_EXCEPTION_ATTRIBUTE);
}
}

View File

@ -0,0 +1,43 @@
/*
* 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.hasClassesNamed;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;
public class AsyncContextStartInstrumentation implements TypeInstrumentation {
private final String basePackageName;
private final String adviceClassName;
public AsyncContextStartInstrumentation(String basePackageName, String adviceClassName) {
this.basePackageName = basePackageName;
this.adviceClassName = adviceClassName;
}
@Override
public ElementMatcher<ClassLoader> classLoaderOptimization() {
return hasClassesNamed(basePackageName + ".Servlet");
}
@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return implementsInterface(named(basePackageName + ".AsyncContext"));
}
@Override
public void transform(TypeTransformer transformer) {
transformer.applyAdviceToMethod(
named("start").and(takesArguments(Runnable.class)).and(isPublic()), adviceClassName);
}
}

View File

@ -6,6 +6,7 @@
package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0
import io.opentelemetry.instrumentation.test.base.HttpServerTest
import jakarta.servlet.ServletException
import jakarta.servlet.annotation.WebServlet
import jakarta.servlet.http.HttpServlet
import jakarta.servlet.http.HttpServletRequest
@ -28,6 +29,9 @@ class AsyncServlet extends HttpServlet {
HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath)
def latch = new CountDownLatch(1)
def context = req.startAsync()
if (endpoint == EXCEPTION) {
context.setTimeout(5000)
}
context.start {
try {
HttpServerTest.controller(endpoint) {
@ -36,41 +40,36 @@ class AsyncServlet extends HttpServlet {
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 CAPTURE_HEADERS:
resp.setHeader("X-Test-Response", req.getHeader("X-Test-Request"))
resp.status = endpoint.status
resp.writer.print(endpoint.body)
context.complete()
break
case ERROR:
resp.status = endpoint.status
resp.writer.print(endpoint.body)
context.complete()
break
case EXCEPTION:
resp.status = endpoint.status
resp.writer.print(endpoint.body)
context.complete()
throw new Exception(endpoint.body)
throw new ServletException(endpoint.body)
}
}
} finally {
// complete at the end so the server span will end after the controller span
if (endpoint != EXCEPTION) {
context.complete()
}
latch.countDown()
}
}

View File

@ -108,12 +108,6 @@ class TomcatAsyncTest extends HttpServerTest<Tomcat> implements AgentTestTrait {
]
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
@Override
Class<?> expectedExceptionClass() {
ServletException

View File

@ -7,7 +7,8 @@ package io.opentelemetry.javaagent.instrumentation.tomcat.v7_0
import groovy.servlet.AbstractHttpServlet
import io.opentelemetry.instrumentation.test.base.HttpServerTest
import java.util.concurrent.CountDownLatch
import javax.servlet.ServletException
import javax.servlet.annotation.WebServlet
import javax.servlet.http.HttpServletRequest
import javax.servlet.http.HttpServletResponse
@ -25,7 +26,11 @@ class AsyncServlet extends AbstractHttpServlet {
@Override
protected void service(HttpServletRequest req, HttpServletResponse resp) {
HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath)
def latch = new CountDownLatch(1)
def context = req.startAsync()
if (endpoint == EXCEPTION) {
context.setTimeout(5000)
}
context.start {
try {
HttpServerTest.controller(endpoint) {
@ -58,13 +63,17 @@ class AsyncServlet extends AbstractHttpServlet {
case EXCEPTION:
resp.status = endpoint.status
resp.writer.print(endpoint.body)
throw new Exception(endpoint.body)
throw new ServletException(endpoint.body)
}
}
} finally {
// complete at the end so the server span will end after the controller span
context.complete()
if (endpoint != EXCEPTION) {
context.complete()
}
latch.countDown()
}
}
latch.await()
}
}

View File

@ -108,12 +108,6 @@ class TomcatAsyncTest extends HttpServerTest<Tomcat> implements AgentTestTrait {
]
}
@Override
boolean testException() {
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
return false
}
@Override
Class<?> expectedExceptionClass() {
ServletException