diff --git a/instrumentation-core/servlet-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation-core/servlet-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index e1b96f7c55..ab84187221 100644 --- a/instrumentation-core/servlet-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation-core/servlet-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -27,9 +27,13 @@ public abstract class ServletHttpServerTracer public Context startSpan(HttpServletRequest request) { Context context = startSpan(request, request, request, getSpanName(request)); + return addServletContextPath(context, request); + } + + private static Context addServletContextPath(Context context, HttpServletRequest request) { String contextPath = request.getContextPath(); if (contextPath != null && !contextPath.isEmpty() && !contextPath.equals("/")) { - context = context.with(ServletContextPath.CONTEXT_KEY, contextPath); + return context.with(ServletContextPath.CONTEXT_KEY, contextPath); } return context; } @@ -159,11 +163,13 @@ public abstract class ServletHttpServerTracer * forward and other scenarios, where servlet path may change, but we don't want this to be * reflected in the span name. */ - public void updateServerSpanNameOnce(Context attachedContext, HttpServletRequest request) { - if (AppServerBridge.shouldUpdateServerSpanName(attachedContext)) { - updateSpanName(Span.fromContext(attachedContext), request); - AppServerBridge.setServletUpdatedServerSpanName(attachedContext, true); + public Context runOnceUnderAppServer(Context context, HttpServletRequest request) { + if (AppServerBridge.shouldUpdateServerSpanName(context)) { + updateSpanName(Span.fromContext(context), request); + AppServerBridge.setServletUpdatedServerSpanName(context, true); + return addServletContextPath(context, request); } + return context; } public void updateSpanName(HttpServletRequest request) { diff --git a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/jaxrs-2.0-jersey-2.0-javaagent.gradle b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/jaxrs-2.0-jersey-2.0-javaagent.gradle index 2bb9c839a7..376524bc21 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/jaxrs-2.0-jersey-2.0-javaagent.gradle +++ b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/jaxrs-2.0-jersey-2.0-javaagent.gradle @@ -17,6 +17,7 @@ dependencies { implementation project(':instrumentation:jaxrs:jaxrs-2.0:jaxrs-2.0-common:javaagent') testInstrumentation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') testImplementation project(':instrumentation:jaxrs:jaxrs-2.0:jaxrs-2.0-testing') diff --git a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/src/test/groovy/JerseyHttpServerTest.groovy b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/src/test/groovy/JerseyHttpServerTest.groovy index 79898e8cca..2a65c98306 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/src/test/groovy/JerseyHttpServerTest.groovy +++ b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/src/test/groovy/JerseyHttpServerTest.groovy @@ -30,4 +30,9 @@ class JerseyHttpServerTest extends JaxRsHttpServerTest { void stopServer(Server httpServer) { httpServer.stop() } + + @Override + boolean asyncCancelHasSendError() { + true + } } \ No newline at end of file diff --git a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.0/javaagent/jaxrs-2.0-resteasy-3.0-javaagent.gradle b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.0/javaagent/jaxrs-2.0-resteasy-3.0-javaagent.gradle index f6fc47fc31..8d0fce6da8 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.0/javaagent/jaxrs-2.0-resteasy-3.0-javaagent.gradle +++ b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.0/javaagent/jaxrs-2.0-resteasy-3.0-javaagent.gradle @@ -27,6 +27,7 @@ dependencies { implementation project(':instrumentation:jaxrs:jaxrs-2.0:jaxrs-2.0-common:javaagent') testInstrumentation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') testImplementation project(':instrumentation:jaxrs:jaxrs-2.0:jaxrs-2.0-testing') diff --git a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.1/javaagent/jaxrs-2.0-resteasy-3.1-javaagent.gradle b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.1/javaagent/jaxrs-2.0-resteasy-3.1-javaagent.gradle index 4bb97ecc2b..4c8fc45951 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.1/javaagent/jaxrs-2.0-resteasy-3.1-javaagent.gradle +++ b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.1/javaagent/jaxrs-2.0-resteasy-3.1-javaagent.gradle @@ -27,6 +27,7 @@ dependencies { implementation project(':instrumentation:jaxrs:jaxrs-2.0:jaxrs-2.0-common:javaagent') testInstrumentation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') testImplementation project(':instrumentation:jaxrs:jaxrs-2.0:jaxrs-2.0-testing') diff --git a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-testing/src/main/groovy/JaxRsHttpServerTest.groovy b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-testing/src/main/groovy/JaxRsHttpServerTest.groovy index 4c9adb183f..3083ea41f7 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-testing/src/main/groovy/JaxRsHttpServerTest.groovy +++ b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-testing/src/main/groovy/JaxRsHttpServerTest.groovy @@ -49,10 +49,18 @@ abstract class JaxRsHttpServerTest extends HttpServerTest { assert response.code() == statusCode assert bodyPredicate(response.body().string()) + def spanCount = 2 + def hasSendError = asyncCancelHasSendError() && action == "cancel" + if (hasSendError) { + spanCount++ + } assertTraces(1) { - trace(0, 2) { + trace(0, spanCount) { asyncServerSpan(it, 0, url, statusCode) handlerSpan(it, 1, span(0), "asyncOp", isCancelled, isError, errorMessage) + if (hasSendError) { + sendErrorSpan(it, 2, span(1)) + } } } @@ -117,6 +125,10 @@ abstract class JaxRsHttpServerTest extends HttpServerTest { true } + boolean asyncCancelHasSendError() { + false + } + private static boolean shouldTestCompletableStageAsync() { Boolean.getBoolean("testLatestDeps") } diff --git a/instrumentation/jetty-8.0/javaagent/jetty-8.0-javaagent.gradle b/instrumentation/jetty-8.0/javaagent/jetty-8.0-javaagent.gradle index 425bfde735..a0a3ad1306 100644 --- a/instrumentation/jetty-8.0/javaagent/jetty-8.0-javaagent.gradle +++ b/instrumentation/jetty-8.0/javaagent/jetty-8.0-javaagent.gradle @@ -13,6 +13,7 @@ muzzle { dependencies { library group: 'org.eclipse.jetty', name: 'jetty-server', version: '8.0.0.v20110901' implementation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') // Don't want to conflict with jetty from the test server. testImplementation(project(':testing-common')) { diff --git a/instrumentation/jetty-8.0/javaagent/src/test/groovy/JettyHandlerTest.groovy b/instrumentation/jetty-8.0/javaagent/src/test/groovy/JettyHandlerTest.groovy index 9013d465e3..f731ce7aa6 100644 --- a/instrumentation/jetty-8.0/javaagent/src/test/groovy/JettyHandlerTest.groovy +++ b/instrumentation/jetty-8.0/javaagent/src/test/groovy/JettyHandlerTest.groovy @@ -10,6 +10,7 @@ import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEn 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.asserts.TraceAssert import io.opentelemetry.instrumentation.test.base.HttpServerTest import javax.servlet.DispatcherType import javax.servlet.ServletException @@ -62,6 +63,23 @@ class JettyHandlerTest extends HttpServerTest { false } + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT || endpoint == ERROR + } + + @Override + void responseSpan(TraceAssert trace, int index, Object parent, String method, ServerEndpoint endpoint) { + switch (endpoint) { + case REDIRECT: + redirectSpan(trace, index, parent) + break + case ERROR: + sendErrorSpan(trace, index, parent) + break + } + } + static void handleRequest(Request request, HttpServletResponse response) { ServerEndpoint endpoint = ServerEndpoint.forPath(request.requestURI) controller(endpoint) { diff --git a/instrumentation/jsf/mojarra-1.2/javaagent/mojarra-1.2-javaagent.gradle b/instrumentation/jsf/mojarra-1.2/javaagent/mojarra-1.2-javaagent.gradle index 506d48efb0..80a0e75e11 100644 --- a/instrumentation/jsf/mojarra-1.2/javaagent/mojarra-1.2-javaagent.gradle +++ b/instrumentation/jsf/mojarra-1.2/javaagent/mojarra-1.2-javaagent.gradle @@ -68,6 +68,7 @@ dependencies { testImplementation project(':instrumentation:jsf:jsf-testing-common') testInstrumentation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') mojarra12TestImplementation group: 'javax.faces', name: 'jsf-impl', version: '1.2-20' mojarra12TestImplementation group: 'javax.faces', name: 'jsf-api', version: '1.2' diff --git a/instrumentation/jsf/myfaces-1.2/javaagent/myfaces-1.2-javaagent.gradle b/instrumentation/jsf/myfaces-1.2/javaagent/myfaces-1.2-javaagent.gradle index 4e3bf30f61..7b34878bbe 100644 --- a/instrumentation/jsf/myfaces-1.2/javaagent/myfaces-1.2-javaagent.gradle +++ b/instrumentation/jsf/myfaces-1.2/javaagent/myfaces-1.2-javaagent.gradle @@ -30,6 +30,7 @@ dependencies { testImplementation project(':instrumentation:jsf:jsf-testing-common') testInstrumentation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') myfaces12TestImplementation group: 'org.apache.myfaces.core', name: 'myfaces-impl', version: '1.2.12' myfaces12TestImplementation group: 'com.sun.facelets', name: 'jsf-facelets', version: '1.1.14' diff --git a/instrumentation/jsp-2.3/javaagent/jsp-2.3-javaagent.gradle b/instrumentation/jsp-2.3/javaagent/jsp-2.3-javaagent.gradle index 88a1361b38..fac4d1be91 100644 --- a/instrumentation/jsp-2.3/javaagent/jsp-2.3-javaagent.gradle +++ b/instrumentation/jsp-2.3/javaagent/jsp-2.3-javaagent.gradle @@ -18,6 +18,8 @@ dependencies { compileOnly group: 'javax.servlet', name: 'javax.servlet-api', version: '3.1.0' testInstrumentation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') + // using tomcat 7.0.37 because there seems to be some issues with Tomcat's jar scanning in versions < 7.0.37 // https://stackoverflow.com/questions/23484098/org-apache-tomcat-util-bcel-classfile-classformatexception-invalid-byte-tag-in testLibrary group: 'org.apache.tomcat.embed', name: 'tomcat-embed-core', version: '7.0.37' diff --git a/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationBasicTests.groovy b/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationBasicTests.groovy index 079ed8b8ff..2edcd1d6a6 100644 --- a/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationBasicTests.groovy +++ b/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationBasicTests.groovy @@ -332,7 +332,7 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification { then: assertTraces(1) { - trace(0, 3) { + trace(0, 4) { span(0) { hasNoParent() name "/$jspWebappContext/includes/includeHtml.jsp" @@ -366,6 +366,11 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification { "jsp.requestURL" reqUrl } } + span(3) { + childOf span(2) + name "ApplicationDispatcher.include" + errored false + } } } res.code() == 200 @@ -384,7 +389,7 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification { then: assertTraces(1) { - trace(0, 7) { + trace(0, 9) { span(0) { hasNoParent() name "/$jspWebappContext/includes/includeMulti.jsp" @@ -420,6 +425,11 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification { } span(3) { childOf span(2) + name "ApplicationDispatcher.include" + errored false + } + span(4) { + childOf span(3) name "Compile /common/javaLoopH2.jsp" errored false attributes { @@ -427,16 +437,21 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification { "jsp.compiler" "org.apache.jasper.compiler.JDTCompiler" } } - span(4) { - childOf span(2) + span(5) { + childOf span(3) name "Render /common/javaLoopH2.jsp" errored false attributes { "jsp.requestURL" reqUrl } } - span(5) { + span(6) { childOf span(2) + name "ApplicationDispatcher.include" + errored false + } + span(7) { + childOf span(6) name "Compile /common/javaLoopH2.jsp" errored false attributes { @@ -444,8 +459,8 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification { "jsp.compiler" "org.apache.jasper.compiler.JDTCompiler" } } - span(6) { - childOf span(2) + span(8) { + childOf span(6) name "Render /common/javaLoopH2.jsp" errored false attributes { diff --git a/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationForwardTests.groovy b/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationForwardTests.groovy index bff59a5b45..e9ba904f50 100644 --- a/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationForwardTests.groovy +++ b/instrumentation/jsp-2.3/javaagent/src/test/groovy/JspInstrumentationForwardTests.groovy @@ -80,7 +80,7 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { then: assertTraces(1) { - trace(0, 5) { + trace(0, 6) { span(0) { hasNoParent() name "/$jspWebappContext/$forwardFromFileName" @@ -116,6 +116,11 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { } span(3) { childOf span(2) + name "ApplicationDispatcher.forward" + errored false + } + span(4) { + childOf span(3) name "Compile /$forwardDestFileName" errored false attributes { @@ -123,8 +128,8 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { "jsp.compiler" "org.apache.jasper.compiler.JDTCompiler" } } - span(4) { - childOf span(2) + span(5) { + childOf span(3) name "Render /$forwardDestFileName" errored false attributes { @@ -155,7 +160,7 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { then: assertTraces(1) { - trace(0, 3) { + trace(0, 4) { span(0) { hasNoParent() name "/$jspWebappContext/forwards/forwardToHtml.jsp" @@ -189,6 +194,11 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { "jsp.requestURL" reqUrl } } + span(3) { + childOf span(2) + name "ApplicationDispatcher.forward" + errored false + } } } res.code() == 200 @@ -207,7 +217,7 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { then: assertTraces(1) { - trace(0, 9) { + trace(0, 12) { span(0) { hasNoParent() name "/$jspWebappContext/forwards/forwardToIncludeMulti.jsp" @@ -243,6 +253,11 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { } span(3) { childOf span(2) + name "ApplicationDispatcher.forward" + errored false + } + span(4) { + childOf span(3) name "Compile /includes/includeMulti.jsp" errored false attributes { @@ -250,8 +265,8 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { "jsp.compiler" "org.apache.jasper.compiler.JDTCompiler" } } - span(4) { - childOf span(2) + span(5) { + childOf span(3) name "Render /includes/includeMulti.jsp" errored false attributes { @@ -259,26 +274,13 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { "jsp.requestURL" baseUrl + "/includes/includeMulti.jsp" } } - span(5) { - childOf span(4) - name "Compile /common/javaLoopH2.jsp" - errored false - attributes { - "jsp.classFQCN" "org.apache.jsp.common.javaLoopH2_jsp" - "jsp.compiler" "org.apache.jasper.compiler.JDTCompiler" - } - } span(6) { - childOf span(4) - name "Render /common/javaLoopH2.jsp" + childOf span(5) + name "ApplicationDispatcher.include" errored false - attributes { - "jsp.forwardOrigin" "/forwards/forwardToIncludeMulti.jsp" - "jsp.requestURL" baseUrl + "/includes/includeMulti.jsp" - } } span(7) { - childOf span(4) + childOf span(6) name "Compile /common/javaLoopH2.jsp" errored false attributes { @@ -287,7 +289,30 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { } } span(8) { - childOf span(4) + childOf span(6) + name "Render /common/javaLoopH2.jsp" + errored false + attributes { + "jsp.forwardOrigin" "/forwards/forwardToIncludeMulti.jsp" + "jsp.requestURL" baseUrl + "/includes/includeMulti.jsp" + } + } + span(9) { + childOf span(5) + name "ApplicationDispatcher.include" + errored false + } + span(10) { + childOf span(9) + name "Compile /common/javaLoopH2.jsp" + errored false + attributes { + "jsp.classFQCN" "org.apache.jsp.common.javaLoopH2_jsp" + "jsp.compiler" "org.apache.jasper.compiler.JDTCompiler" + } + } + span(11) { + childOf span(9) name "Render /common/javaLoopH2.jsp" errored false attributes { @@ -313,7 +338,7 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { then: assertTraces(1) { - trace(0, 7) { + trace(0, 9) { span(0) { hasNoParent() name "/$jspWebappContext/forwards/forwardToJspForward.jsp" @@ -349,6 +374,11 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { } span(3) { childOf span(2) + name "ApplicationDispatcher.forward" + errored false + } + span(4) { + childOf span(3) name "Compile /forwards/forwardToSimpleJava.jsp" errored false attributes { @@ -356,8 +386,8 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { "jsp.compiler" "org.apache.jasper.compiler.JDTCompiler" } } - span(4) { - childOf span(2) + span(5) { + childOf span(3) name "Render /forwards/forwardToSimpleJava.jsp" errored false attributes { @@ -365,8 +395,13 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { "jsp.requestURL" baseUrl + "/forwards/forwardToSimpleJava.jsp" } } - span(5) { - childOf span(4) + span(6) { + childOf span(5) + name "ApplicationDispatcher.forward" + errored false + } + span(7) { + childOf span(6) name "Compile /common/loop.jsp" errored false attributes { @@ -374,8 +409,8 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { "jsp.compiler" "org.apache.jasper.compiler.JDTCompiler" } } - span(6) { - childOf span(4) + span(8) { + childOf span(6) name "Render /common/loop.jsp" errored false attributes { @@ -401,7 +436,7 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { then: assertTraces(1) { - trace(0, 4) { + trace(0, 5) { span(0) { hasNoParent() name "/$jspWebappContext/forwards/forwardToCompileError.jsp" @@ -439,6 +474,12 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { } span(3) { childOf span(2) + name "ApplicationDispatcher.forward" + errored true + errorEvent(JasperException, String) + } + span(4) { + childOf span(3) name "Compile /compileError.jsp" errored true errorEvent(JasperException, String) @@ -465,7 +506,7 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { then: assertTraces(1) { - trace(0, 3) { + trace(0, 5) { span(0) { hasNoParent() name "/$jspWebappContext/forwards/forwardToNonExistent.jsp" @@ -499,6 +540,14 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification { "jsp.requestURL" reqUrl } } + span(3) { + childOf span(2) + name "ApplicationDispatcher.forward" + } + span(4) { + childOf span(3) + name "ResponseFacade.sendError" + } } } res.code() == 404 diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java index 3120d1c014..3571da564a 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java @@ -6,7 +6,6 @@ package io.opentelemetry.javaagent.instrumentation.liberty; import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; import io.opentelemetry.javaagent.instrumentation.servlet.v3_0.Servlet3HttpServerTracer; import javax.servlet.http.HttpServletRequest; @@ -22,12 +21,7 @@ public class LibertyHttpServerTracer extends Servlet3HttpServerTracer { // using request method as span name as server isn't ready for calling request.getServletPath() // span name will be updated a bit later when calling request.getServletPath() works // see LibertyUpdateSpanAdvice - Context context = startSpan(request, request, request, "HTTP " + request.getMethod()); - String contextPath = request.getContextPath(); - if (contextPath != null && !contextPath.isEmpty() && !contextPath.equals("/")) { - context = context.with(ServletContextPath.CONTEXT_KEY, contextPath); - } - return context; + return startSpan(request, request, request, "HTTP " + request.getMethod()); } @Override diff --git a/instrumentation/servlet/glassfish-testing/glassfish-testing.gradle b/instrumentation/servlet/glassfish-testing/glassfish-testing.gradle index 4528c40117..714b5a3796 100644 --- a/instrumentation/servlet/glassfish-testing/glassfish-testing.gradle +++ b/instrumentation/servlet/glassfish-testing/glassfish-testing.gradle @@ -8,6 +8,7 @@ apply from: "$rootDir/gradle/instrumentation.gradle" dependencies { testInstrumentation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') testInstrumentation project(':instrumentation:grizzly-2.0:javaagent') testLibrary group: 'org.glassfish.main.extras', name: 'glassfish-embedded-all', version: '4.0' diff --git a/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy b/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy index 8e8b461936..92880c84d4 100644 --- a/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy +++ b/instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy @@ -3,6 +3,11 @@ * SPDX-License-Identifier: Apache-2.0 */ +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT + +import io.opentelemetry.instrumentation.test.asserts.TraceAssert import io.opentelemetry.instrumentation.test.base.HttpServerTest import org.glassfish.embeddable.BootstrapProperties import org.glassfish.embeddable.Deployer @@ -63,4 +68,22 @@ class GlassFishServerTest extends HttpServerTest { boolean redirectHasBody() { true } + + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT || endpoint == ERROR || endpoint == NOT_FOUND + } + + @Override + void responseSpan(TraceAssert trace, int index, Object parent, String method, ServerEndpoint endpoint) { + switch (endpoint) { + case REDIRECT: + redirectSpan(trace, index, parent) + break + case ERROR: + case NOT_FOUND: + sendErrorSpan(trace, index, parent) + break + } + } } diff --git a/instrumentation/servlet/servlet-2.2/javaagent/servlet-2.2-javaagent.gradle b/instrumentation/servlet/servlet-2.2/javaagent/servlet-2.2-javaagent.gradle index 4d902b8e61..2184c396cb 100644 --- a/instrumentation/servlet/servlet-2.2/javaagent/servlet-2.2-javaagent.gradle +++ b/instrumentation/servlet/servlet-2.2/javaagent/servlet-2.2-javaagent.gradle @@ -19,6 +19,8 @@ dependencies { compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.2' api(project(':instrumentation-core:servlet-2.2')) + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') + testImplementation(project(':testing-common')) { exclude group: 'org.eclipse.jetty', module: 'jetty-server' } diff --git a/instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java b/instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java index 5496e2ab6e..06758e3513 100644 --- a/instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java +++ b/instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java @@ -37,7 +37,11 @@ public class Servlet2Advice { Context serverContext = tracer().getServerContext(httpServletRequest); if (serverContext != null) { - tracer().updateServerSpanNameOnce(serverContext, httpServletRequest); + Context updatedContext = tracer().runOnceUnderAppServer(serverContext, httpServletRequest); + if (updatedContext != serverContext) { + // runOnceUnderAppServer updated context, need to re-scope + scope = updatedContext.makeCurrent(); + } return; } diff --git a/instrumentation/servlet/servlet-2.2/javaagent/src/test/groovy/JettyServlet2Test.groovy b/instrumentation/servlet/servlet-2.2/javaagent/src/test/groovy/JettyServlet2Test.groovy index e33f70ce91..b7620e706b 100644 --- a/instrumentation/servlet/servlet-2.2/javaagent/src/test/groovy/JettyServlet2Test.groovy +++ b/instrumentation/servlet/servlet-2.2/javaagent/src/test/groovy/JettyServlet2Test.groovy @@ -70,10 +70,15 @@ class JettyServlet2Test extends HttpServerTest { false } + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT || endpoint == ERROR + } + @Override void responseSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { trace.span(index) { - name endpoint == REDIRECT ? "HttpServletResponse.sendRedirect" : "HttpServletResponse.sendError" + name endpoint == REDIRECT ? "Response.sendRedirect" : "Response.sendError" kind INTERNAL errored false childOf((SpanData) parent) diff --git a/instrumentation/servlet/servlet-3.0/javaagent/servlet-3.0-javaagent.gradle b/instrumentation/servlet/servlet-3.0/javaagent/servlet-3.0-javaagent.gradle index 487b75aa54..fbb6e8ef50 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/servlet-3.0-javaagent.gradle +++ b/instrumentation/servlet/servlet-3.0/javaagent/servlet-3.0-javaagent.gradle @@ -19,6 +19,7 @@ dependencies { api(project(':instrumentation-core:servlet-2.2')) testInstrumentation project(':instrumentation:jetty-8.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') testImplementation(project(':testing-common')) { exclude group: 'org.eclipse.jetty', module: 'jetty-server' 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 c871350cd7..2bb50a8e19 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 @@ -36,19 +36,35 @@ public class Servlet3Advice { Context attachedContext = tracer().getServerContext(httpServletRequest); if (attachedContext != null) { + // We are inside nested servlet/filter/app-server span, don't create new span if (Servlet3HttpServerTracer.needsRescoping(attachedContext)) { + attachedContext = tracer().runOnceUnderAppServer(attachedContext, httpServletRequest); scope = attachedContext.makeCurrent(); + return; } - tracer().updateServerSpanNameOnce(attachedContext, httpServletRequest); - // We are inside nested servlet/filter/app-server span, don't create new span + // We already have attached context to request but this could have been done by app server + // instrumentation, if needed update span with info from current request. + Context currentContext = Java8BytecodeBridge.currentContext(); + Context updatedContext = tracer().runOnceUnderAppServer(currentContext, httpServletRequest); + if (updatedContext != currentContext) { + // runOnceUnderAppServer updated context, need to re-scope + scope = updatedContext.makeCurrent(); + } return; } - Context parentContext = Java8BytecodeBridge.currentContext(); - if (parentContext != null && Java8BytecodeBridge.spanFromContext(parentContext).isRecording()) { - tracer().updateServerSpanNameOnce(parentContext, httpServletRequest); - // We are inside nested servlet/filter/app-server span, don't create new span + Context currentContext = Java8BytecodeBridge.currentContext(); + if (currentContext != null + && Java8BytecodeBridge.spanFromContext(currentContext).isRecording()) { + // We already have a span but it was not created by servlet instrumentation. + // In case it was created by app server integration we need to update it with info from + // current request. + Context updatedContext = tracer().runOnceUnderAppServer(currentContext, httpServletRequest); + if (currentContext != updatedContext) { + // runOnceUnderAppServer updated context, need to re-scope + scope = updatedContext.makeCurrent(); + } return; } diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy index 87a755157f..a6e0313085 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy @@ -10,6 +10,7 @@ import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEn 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.asserts.TraceAssert import io.opentelemetry.instrumentation.test.base.HttpServerTest import javax.servlet.Servlet import okhttp3.Request @@ -49,4 +50,25 @@ abstract class AbstractServlet3Test extends HttpServerTest ServletException } + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == NOT_FOUND || super.hasResponseSpan(endpoint) + } + + @Override + void responseSpan(TraceAssert trace, int index, Object parent, String method, ServerEndpoint endpoint) { + switch (endpoint) { + case NOT_FOUND: + sendErrorSpan(trace, index, parent) + break + } + super.responseSpan(trace, index, parent, method, endpoint) + } + @Shared def accessLogValue = new TestAccessLogValve() @@ -122,10 +139,26 @@ abstract class TomcatServlet3Test extends AbstractServlet3Test def loggedTraces = accessLogValue.loggedIds*.first def loggedSpans = accessLogValue.loggedIds*.second + def expectedCount = 2 + if (hasIncludeSpan()) { + expectedCount++ + } + if (hasForwardSpan()) { + expectedCount++ + } (0..count - 1).each { - trace(it, 2) { + trace(it, expectedCount) { serverSpan(it, 0, null, null, "GET", SUCCESS.body.length()) - controllerSpan(it, 1, span(0)) + def controllerIndex = 1 + if (hasIncludeSpan()) { + includeSpan(it, 1, span(0)) + controllerIndex++ + } + if (hasForwardSpan()) { + forwardSpan(it, 1, span(0)) + controllerIndex++ + } + controllerSpan(it, controllerIndex, span(controllerIndex - 1)) } assert loggedTraces.contains(traces[it][0].traceId) @@ -150,10 +183,27 @@ abstract class TomcatServlet3Test extends AbstractServlet3Test response.body().string() == ERROR.body and: + def spanCount = 2 + if (errorEndpointUsesSendError()) { + spanCount++ + } + if (hasForwardSpan()) { + spanCount++ + } assertTraces(1) { - trace(0, 2) { + trace(0, spanCount) { serverSpan(it, 0, null, null, method, response.body().contentLength(), ERROR) - controllerSpan(it, 1, span(0)) + def spanIndex = 1 + if (hasForwardSpan()) { + forwardSpan(it, spanIndex, span(spanIndex - 1)) + spanIndex++ + } + controllerSpan(it, spanIndex, span(spanIndex - 1)) + spanIndex++ + if (errorEndpointUsesSendError()) { + sendErrorSpan(it, spanIndex, span(spanIndex - 1)) + spanIndex++ + } } def (String traceId, String spanId) = accessLogValue.loggedIds[0] @@ -256,6 +306,11 @@ class TomcatServlet3TestAsync extends TomcatServlet3Test { TestServlet3.Async } + @Override + boolean errorEndpointUsesSendError() { + false + } + @Override boolean testException() { // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807 @@ -288,6 +343,11 @@ class TomcatServlet3TestForward extends TomcatDispatchTest { false } + @Override + boolean hasForwardSpan() { + true + } + @Override protected void setupServlets(Context context) { super.setupServlets(context) @@ -322,6 +382,11 @@ class TomcatServlet3TestInclude extends TomcatDispatchTest { false } + @Override + boolean hasIncludeSpan() { + true + } + @Override protected void setupServlets(Context context) { super.setupServlets(context) @@ -379,6 +444,11 @@ class TomcatServlet3TestDispatchAsync extends TomcatDispatchTest { addServlet(context, "/dispatch/recursive", TestServlet3.DispatchRecursive) } + @Override + boolean errorEndpointUsesSendError() { + false + } + @Override boolean testException() { // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807 diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/spring-webmvc-3.1-javaagent.gradle b/instrumentation/spring/spring-webmvc-3.1/javaagent/spring-webmvc-3.1-javaagent.gradle index 3ac5e1bd7b..d5fce2dbb8 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/spring-webmvc-3.1-javaagent.gradle +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/spring-webmvc-3.1-javaagent.gradle @@ -33,6 +33,8 @@ dependencies { // Include servlet instrumentation for verifying the tomcat requests testInstrumentation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') + testInstrumentation project(':instrumentation:tomcat-7.0:javaagent') testImplementation group: 'javax.validation', name: 'validation-api', version: '1.1.0.Final' testImplementation group: 'org.hibernate', name: 'hibernate-validator', version: '5.4.2.Final' diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy index d8db857de3..bed62e2d49 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -58,6 +58,11 @@ class SpringBootBasedTest extends HttpServerTest endpoint == REDIRECT } + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT + } + @Override boolean testNotFound() { // FIXME: the instrumentation adds an extra controller span which is not consistent. @@ -84,9 +89,11 @@ class SpringBootBasedTest extends HttpServerTest and: assertTraces(1) { - trace(0, 2) { + trace(0, 4) { serverSpan(it, 0, null, null, "GET", null, AUTH_ERROR) - errorPageSpans(it, 1, null) + sendErrorSpan(it, 1, span(0)) + forwardSpan(it, 2, span(0)) + errorPageSpans(it, 3, null) } } } @@ -111,8 +118,9 @@ class SpringBootBasedTest extends HttpServerTest and: assertTraces(1) { - trace(0, 1) { + trace(0, 2) { serverSpan(it, 0, null, null, "POST", response.body()?.contentLength(), LOGIN) + redirectSpan(it, 1, span(0)) } } @@ -134,7 +142,7 @@ class SpringBootBasedTest extends HttpServerTest @Override void responseSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { trace.span(index) { - name "HttpServletResponse.sendRedirect" + name "OnCommittedResponseWrapper.sendRedirect" kind INTERNAL errored false attributes { @@ -173,7 +181,13 @@ class SpringBootBasedTest extends HttpServerTest void serverSpan(TraceAssert trace, int index, String traceID = null, String parentID = null, String method = "GET", Long responseContentLength = null, ServerEndpoint endpoint = SUCCESS) { trace.span(index) { - name endpoint == PATH_PARAM ? getContextPath() + "/path/{id}/param" : endpoint.resolvePath(address).path + if (endpoint == PATH_PARAM) { + name getContextPath() + "/path/{id}/param" + } else if (endpoint == AUTH_ERROR) { + name "/error" + } else { + name endpoint.resolvePath(address).path + } kind SERVER errored endpoint.errored if (parentID != null) { @@ -186,7 +200,7 @@ class SpringBootBasedTest extends HttpServerTest errorEvent(Exception, EXCEPTION.body) } attributes { - "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it == "127.0.0.1" } // Optional + "${SemanticAttributes.NET_PEER_IP.key}" "127.0.0.1" "${SemanticAttributes.NET_PEER_PORT.key}" Long "${SemanticAttributes.HTTP_URL.key}" { it == "${endpoint.resolve(address)}" || it == "${endpoint.resolveWithoutFragment(address)}" } "${SemanticAttributes.HTTP_METHOD.key}" method diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy index 40809810ee..ddc889b7ab 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/filter/ServletFilterTest.groovy @@ -9,6 +9,7 @@ import static io.opentelemetry.api.trace.SpanKind.INTERNAL 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.PATH_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.asserts.TraceAssert @@ -43,6 +44,28 @@ class ServletFilterTest extends HttpServerTest { endpoint == ERROR || endpoint == EXCEPTION } + @Override + int getErrorPageSpansCount(ServerEndpoint endpoint) { + 2 + } + + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT || endpoint == ERROR + } + + @Override + void responseSpan(TraceAssert trace, int index, Object parent, String method, ServerEndpoint endpoint) { + switch (endpoint) { + case REDIRECT: + redirectSpan(trace, index, parent) + break + case ERROR: + sendErrorSpan(trace, index, parent) + break + } + } + @Override boolean testPathParam() { true @@ -75,18 +98,31 @@ class ServletFilterTest extends HttpServerTest { @Override String expectedServerSpanName(ServerEndpoint endpoint) { - return endpoint == PATH_PARAM ? "/path/{id}/param" : endpoint.resolvePath(address).path + if (endpoint == PATH_PARAM) { + return "/path/{id}/param" + } else if (endpoint == ERROR || endpoint == EXCEPTION) { + return "/error" + } + return endpoint.resolvePath(address).path } @Override void errorPageSpans(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { trace.span(index) { - name "BasicErrorController.error" + name "ApplicationDispatcher.forward" kind INTERNAL errored false childOf((SpanData) parent) attributes { } } + trace.span(index + 1) { + name "BasicErrorController.error" + kind INTERNAL + errored false + childOf(trace.span(index)) + attributes { + } + } } } diff --git a/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy b/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy index 8a7c388d07..5ca6b0e000 100644 --- a/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy +++ b/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy @@ -3,8 +3,10 @@ * SPDX-License-Identifier: Apache-2.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.PATH_PARAM +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT import io.opentelemetry.api.trace.SpanKind import io.opentelemetry.instrumentation.test.asserts.TraceAssert @@ -14,6 +16,7 @@ import io.opentelemetry.semconv.trace.attributes.SemanticAttributes import javax.servlet.DispatcherType import org.apache.struts2.dispatcher.ng.filter.StrutsPrepareAndExecuteFilter import org.eclipse.jetty.server.Server +import org.eclipse.jetty.server.handler.HandlerCollection import org.eclipse.jetty.servlet.DefaultServlet import org.eclipse.jetty.servlet.ServletContextHandler import org.eclipse.jetty.util.resource.FileResource @@ -45,6 +48,24 @@ class Struts2ActionSpanTest extends HttpServerTest { return true } + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT || endpoint == ERROR || endpoint == EXCEPTION + } + + @Override + void responseSpan(TraceAssert trace, int index, Object controllerSpan, Object handlerSpan, String method, ServerEndpoint endpoint) { + switch (endpoint) { + case REDIRECT: + redirectSpan(trace, index, handlerSpan) + break + case ERROR: + case EXCEPTION: + sendErrorSpan(trace, index, handlerSpan) + break + } + } + String expectedServerSpanName(ServerEndpoint endpoint) { return endpoint == PATH_PARAM ? getContextPath() + "/path/{id}/param" : endpoint.resolvePath(address).path } @@ -79,7 +100,11 @@ class Struts2ActionSpanTest extends HttpServerTest { context.setContextPath(getContextPath()) def resource = new FileResource(getClass().getResource("/")) context.setBaseResource(resource) - server.setHandler(context) + // jetty integration is disabled for some handler classes, using HandlerCollection here + // enables jetty integration + HandlerCollection handlerCollection = new HandlerCollection() + handlerCollection.addHandler(context) + server.setHandler(handlerCollection) context.addServlet(DefaultServlet, "/") context.addFilter(StrutsPrepareAndExecuteFilter, "/*", EnumSet.of(DispatcherType.REQUEST)) diff --git a/instrumentation/struts-2.3/javaagent/struts-2.3-javaagent.gradle b/instrumentation/struts-2.3/javaagent/struts-2.3-javaagent.gradle index 77588abffe..51b12f456d 100644 --- a/instrumentation/struts-2.3/javaagent/struts-2.3-javaagent.gradle +++ b/instrumentation/struts-2.3/javaagent/struts-2.3-javaagent.gradle @@ -24,6 +24,7 @@ dependencies { testRuntimeOnly group: 'javax.servlet', name: 'jsp-api', version: '2.0' testInstrumentation project(":instrumentation:servlet:servlet-3.0:javaagent") + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') testInstrumentation project(':instrumentation:jetty-8.0:javaagent') } diff --git a/instrumentation/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatTracer.java b/instrumentation/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatTracer.java index 9e3127a4be..8539004cf0 100644 --- a/instrumentation/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatTracer.java +++ b/instrumentation/tomcat-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatTracer.java @@ -12,6 +12,7 @@ import io.opentelemetry.instrumentation.api.tracer.HttpServerTracer; import java.lang.reflect.Method; import java.net.URI; import java.util.Collections; +import org.apache.coyote.ActionCode; import org.apache.coyote.Request; import org.apache.coyote.Response; import org.apache.tomcat.util.buf.MessageBytes; @@ -51,12 +52,14 @@ public class TomcatTracer extends HttpServerTracer { + HttpServerTest.controller( + serverEndpoint, + () -> { + if (serverEndpoint == HttpServerTest.ServerEndpoint.EXCEPTION) { throw new Exception(serverEndpoint.getBody()); - }); - } else { - resp.getWriter().print(HttpServerTest.controller(serverEndpoint, serverEndpoint::getBody)); - } - - if (serverEndpoint == HttpServerTest.ServerEndpoint.REDIRECT) { - resp.sendRedirect(serverEndpoint.getBody()); - } else { - resp.setStatus(serverEndpoint.getStatus()); - } + } + resp.getWriter().print(serverEndpoint.getBody()); + if (serverEndpoint == HttpServerTest.ServerEndpoint.REDIRECT) { + resp.sendRedirect(serverEndpoint.getBody()); + } else if (serverEndpoint == HttpServerTest.ServerEndpoint.ERROR) { + resp.sendError(serverEndpoint.getStatus()); + } else { + resp.setStatus(serverEndpoint.getStatus()); + } + return null; + }); } else if ("/errorPage".equals(path)) { resp.getWriter().print(HttpServerTest.ServerEndpoint.EXCEPTION.getBody()); resp.setStatus(500); diff --git a/instrumentation/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatHandlerTest.groovy b/instrumentation/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatHandlerTest.groovy index c4f4bc8d17..1a4cab6cf6 100644 --- a/instrumentation/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatHandlerTest.groovy +++ b/instrumentation/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat7/TomcatHandlerTest.groovy @@ -5,7 +5,15 @@ package io.opentelemetry.javaagent.instrumentation.tomcat7 +import static io.opentelemetry.api.trace.SpanKind.INTERNAL +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.REDIRECT +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +import io.opentelemetry.instrumentation.test.asserts.TraceAssert import io.opentelemetry.instrumentation.test.base.HttpServerTest +import io.opentelemetry.sdk.trace.data.SpanData import org.apache.catalina.Context import org.apache.catalina.startup.Tomcat import org.apache.tomcat.util.descriptor.web.ErrorPage @@ -53,4 +61,42 @@ class TomcatHandlerTest extends HttpServerTest { tomcat.getServer().stop() } + @Override + boolean testExceptionBody() { + false + } + + @Override + boolean hasErrorPageSpans(ServerEndpoint endpoint) { + endpoint == ERROR || endpoint == EXCEPTION + } + + @Override + void errorPageSpans(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + name "ApplicationDispatcher.forward" + kind INTERNAL + errored false + childOf((SpanData) parent) + attributes { + } + } + } + + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT || endpoint == ERROR + } + + @Override + void responseSpan(TraceAssert trace, int index, Object parent, String method, ServerEndpoint endpoint) { + switch (endpoint) { + case REDIRECT: + redirectSpan(trace, index, parent) + break + case ERROR: + sendErrorSpan(trace, index, parent) + break + } + } } diff --git a/instrumentation/tomcat-7.0/javaagent/tomcat-7.0-javaagent.gradle b/instrumentation/tomcat-7.0/javaagent/tomcat-7.0-javaagent.gradle index b58672ddba..f8a32b71f1 100644 --- a/instrumentation/tomcat-7.0/javaagent/tomcat-7.0-javaagent.gradle +++ b/instrumentation/tomcat-7.0/javaagent/tomcat-7.0-javaagent.gradle @@ -13,6 +13,7 @@ muzzle { dependencies { library group: 'org.apache.tomcat.embed', name: 'tomcat-embed-core', version: '7.0.4' implementation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') // Tests need at least version 9 to have necessary classes to configure the embedded tomcat... // ... but not newer that version 10, because its servlet 5. diff --git a/instrumentation/wicket-8.0/javaagent/wicket-8.0-javaagent.gradle b/instrumentation/wicket-8.0/javaagent/wicket-8.0-javaagent.gradle index eda7aa3eea..773afebafc 100644 --- a/instrumentation/wicket-8.0/javaagent/wicket-8.0-javaagent.gradle +++ b/instrumentation/wicket-8.0/javaagent/wicket-8.0-javaagent.gradle @@ -18,4 +18,5 @@ dependencies { testImplementation group: 'org.eclipse.jetty', name: 'jetty-servlet', version: '8.0.0.v20110901' testInstrumentation project(":instrumentation:servlet:servlet-3.0:javaagent") + testInstrumentation project(":instrumentation:servlet:servlet-common:javaagent") } diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy index a21f9ded79..6b445047c3 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy @@ -51,6 +51,18 @@ abstract class HttpServerTest extends AgentInstrumentationSpecification false } + boolean hasForwardSpan() { + false + } + + boolean hasIncludeSpan() { + false + } + + int getErrorPageSpansCount(ServerEndpoint endpoint) { + 1 + } + boolean hasErrorPageSpans(ServerEndpoint endpoint) { false } @@ -341,16 +353,22 @@ abstract class HttpServerTest extends AgentInstrumentationSpecification if (hasHandlerSpan()) { spanCount++ } + if (hasResponseSpan(endpoint)) { + spanCount++ + } if (endpoint != NOT_FOUND) { spanCount++ // controller span if (hasRenderSpan(endpoint)) { spanCount++ } - if (hasResponseSpan(endpoint)) { + if (hasForwardSpan()) { + spanCount++ + } + if (hasIncludeSpan()) { spanCount++ } if (hasErrorPageSpans(endpoint)) { - spanCount++ + spanCount += getErrorPageSpansCount(endpoint) } } assertTraces(size) { @@ -362,21 +380,31 @@ abstract class HttpServerTest extends AgentInstrumentationSpecification handlerSpan(it, spanIndex++, span(0), method, endpoint) } if (endpoint != NOT_FOUND) { + def controllerSpanIndex = 0 if (hasHandlerSpan()) { - controllerSpan(it, spanIndex++, span(1), errorMessage, expectedExceptionClass()) - } else { - controllerSpan(it, spanIndex++, span(0), errorMessage, expectedExceptionClass()) + controllerSpanIndex++ } + if (hasForwardSpan()) { + forwardSpan(it, spanIndex++, span(0), errorMessage, expectedExceptionClass()) + controllerSpanIndex++ + } + if (hasIncludeSpan()) { + includeSpan(it, spanIndex++, span(0), errorMessage, expectedExceptionClass()) + controllerSpanIndex++ + } + controllerSpan(it, spanIndex++, span(controllerSpanIndex), errorMessage, expectedExceptionClass()) if (hasRenderSpan(endpoint)) { renderSpan(it, spanIndex++, span(0), method, endpoint) } if (hasResponseSpan(endpoint)) { - responseSpan(it, spanIndex, span(spanIndex - 1), method, endpoint) + responseSpan(it, spanIndex, span(spanIndex - 1), span(0), method, endpoint) spanIndex++ } if (hasErrorPageSpans(endpoint)) { errorPageSpans(it, spanIndex, span(0), method, endpoint) } + } else if (hasResponseSpan(endpoint)) { + responseSpan(it, 1, span(0), span(0), method, endpoint) } } } @@ -402,6 +430,10 @@ abstract class HttpServerTest extends AgentInstrumentationSpecification throw new UnsupportedOperationException("renderSpan not implemented in " + getClass().name) } + void responseSpan(TraceAssert trace, int index, Object controllerSpan, Object handlerSpan, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + responseSpan(trace, index, controllerSpan, method, endpoint) + } + void responseSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { throw new UnsupportedOperationException("responseSpan not implemented in " + getClass().name) } @@ -410,6 +442,46 @@ abstract class HttpServerTest extends AgentInstrumentationSpecification throw new UnsupportedOperationException("errorPageSpans not implemented in " + getClass().name) } + void redirectSpan(TraceAssert trace, int index, Object parent) { + trace.span(index) { + name ~/\.sendRedirect$/ + kind SpanKind.INTERNAL + childOf((SpanData) parent) + } + } + + void sendErrorSpan(TraceAssert trace, int index, Object parent) { + trace.span(index) { + name ~/\.sendError$/ + kind SpanKind.INTERNAL + childOf((SpanData) parent) + } + } + + void forwardSpan(TraceAssert trace, int index, Object parent, String errorMessage = null, Class exceptionClass = Exception) { + trace.span(index) { + name ~/\.forward$/ + kind SpanKind.INTERNAL + errored errorMessage != null + if (errorMessage) { + errorEvent(exceptionClass, errorMessage) + } + childOf((SpanData) parent) + } + } + + void includeSpan(TraceAssert trace, int index, Object parent, String errorMessage = null, Class exceptionClass = Exception) { + trace.span(index) { + name ~/\.include$/ + kind SpanKind.INTERNAL + errored errorMessage != null + if (errorMessage) { + errorEvent(exceptionClass, errorMessage) + } + childOf((SpanData) parent) + } + } + // parent span must be cast otherwise it breaks debugging classloading (junit loads it early) void serverSpan(TraceAssert trace, int index, String traceID = null, String parentID = null, String method = "GET", Long responseContentLength = null, ServerEndpoint endpoint = SUCCESS) { trace.span(index) {