diff --git a/dd-java-agent/instrumentation/jsp-2.3/src/main/java/datadog/trace/instrumentation/jsp/JSPInstrumentation.java b/dd-java-agent/instrumentation/jsp-2.3/src/main/java/datadog/trace/instrumentation/jsp/JSPInstrumentation.java index 767aecbcd0..b93a63743d 100644 --- a/dd-java-agent/instrumentation/jsp-2.3/src/main/java/datadog/trace/instrumentation/jsp/JSPInstrumentation.java +++ b/dd-java-agent/instrumentation/jsp-2.3/src/main/java/datadog/trace/instrumentation/jsp/JSPInstrumentation.java @@ -16,15 +16,19 @@ import io.opentracing.Scope; import io.opentracing.Span; import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; +import java.net.URI; +import java.net.URISyntaxException; import java.util.Collections; import java.util.HashMap; import java.util.Map; import javax.servlet.RequestDispatcher; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import javax.servlet.jsp.HttpJspPage; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; +import org.slf4j.LoggerFactory; @AutoService(Instrumenter.class) public final class JSPInstrumentation extends Instrumenter.Default { @@ -68,19 +72,28 @@ public final class JSPInstrumentation extends Instrumenter.Default { // get the JSP file name being rendered in an include action final Object includeServletPath = req.getAttribute(RequestDispatcher.INCLUDE_SERVLET_PATH); String resourceName = req.getServletPath(); - if (includeServletPath != null && includeServletPath instanceof String) { + if (includeServletPath instanceof String) { resourceName = includeServletPath.toString(); } span.setTag(DDTags.RESOURCE_NAME, resourceName); final Object forwardOrigin = req.getAttribute(RequestDispatcher.FORWARD_SERVLET_PATH); - if (forwardOrigin != null && forwardOrigin instanceof String) { + if (forwardOrigin instanceof String) { span.setTag("jsp.forwardOrigin", forwardOrigin.toString()); } // add the request URL as a tag to provide better context when looking at spans produced by - // actions - span.setTag("jsp.requestURL", req.getRequestURL().toString()); + // actions. Tomcat 9 has relative path symbols in the value returned from + // HttpServletRequest#getRequestURL(), + // normalizing the URL should remove those symbols for readability and consistency + try { + span.setTag( + "jsp.requestURL", (new URI(req.getRequestURL().toString())).normalize().toString()); + } catch (final URISyntaxException uriSE) { + LoggerFactory.getLogger(HttpJspPage.class) + .warn("Failed to get and normalize request URL: " + uriSE.getMessage()); + } + Tags.COMPONENT.set(span, "jsp-http-servlet"); return scope; diff --git a/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy b/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy index 05c8dd6b0d..0ce5e85558 100644 --- a/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy +++ b/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy @@ -45,17 +45,22 @@ class JSPInstrumentationBasicTests extends AgentTestRunner { OkHttpClient client = OkHttpUtils.client() def setupSpec() { - port = TestUtils.randomOpenPort() - tomcatServer = new Tomcat() - tomcatServer.setPort(port) - // comment to debug - tomcatServer.setSilent(true) - baseDir = Files.createTempDir() baseDir.deleteOnExit() expectedJspClassFilesDir = baseDir.getCanonicalFile().getAbsolutePath() + expectedJspClassFilesDir - baseUrl = "http://localhost:$port/$jspWebappContext" + + port = TestUtils.randomOpenPort() + + tomcatServer = new Tomcat() tomcatServer.setBaseDir(baseDir.getAbsolutePath()) + tomcatServer.setPort(port) + // comment to debug + tomcatServer.setSilent(true) + // this is needed in tomcat 9, this triggers the creation of a connector, will not + // affect tomcat 7 and 8 + // https://stackoverflow.com/questions/48998387/code-works-with-embedded-apache-tomcat-8-but-not-with-9-whats-changed + tomcatServer.getConnector() + baseUrl = "http://localhost:$port/$jspWebappContext" appContext = tomcatServer.addWebapp("/$jspWebappContext", JSPInstrumentationBasicTests.getResource("/webapps/jsptest").getPath()) @@ -344,7 +349,14 @@ class JSPInstrumentationBasicTests extends AgentTestRunner { "span.origin.type" jspClassName "servlet.context" "/$jspWebappContext" "jsp.requestURL" reqUrl - errorTags(exceptionClass, errorMessage) + "error" true + "error.type" { String tagExceptionType -> + return tagExceptionType == exceptionClass.getName() || tagExceptionType.contains(exceptionClass.getSimpleName()) + } + "error.msg" { String tagErrorMsg -> + return errorMessageOptional || tagErrorMsg instanceof String + } + "error.stack" String defaultTags() } } @@ -373,10 +385,10 @@ class JSPInstrumentationBasicTests extends AgentTestRunner { res.close() where: - test | jspFileName | jspClassName | exceptionClass | errorMessage - "java runtime error" | "runtimeError.jsp" | "runtimeError_jsp" | ArithmeticException | String - "invalid write" | "invalidWrite.jsp" | "invalidWrite_jsp" | StringIndexOutOfBoundsException | String - "missing query gives null" | "getQuery.jsp" | "getQuery_jsp" | NullPointerException | null + test | jspFileName | jspClassName | exceptionClass | errorMessageOptional + "java runtime error" | "runtimeError.jsp" | "runtimeError_jsp" | ArithmeticException | false + "invalid write" | "invalidWrite.jsp" | "invalidWrite_jsp" | IndexOutOfBoundsException | true + "missing query gives null" | "getQuery.jsp" | "getQuery_jsp" | NullPointerException | true } def "non-erroneous include plain HTML GET"() { diff --git a/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationForwardTests.groovy b/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationForwardTests.groovy index 71abf2066d..3a888113f3 100644 --- a/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationForwardTests.groovy +++ b/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationForwardTests.groovy @@ -43,17 +43,23 @@ class JSPInstrumentationForwardTests extends AgentTestRunner { OkHttpClient client = OkHttpUtils.client() def setupSpec() { - port = TestUtils.randomOpenPort() - tomcatServer = new Tomcat() - tomcatServer.setPort(port) - // comment to debug - tomcatServer.setSilent(true) - baseDir = Files.createTempDir() baseDir.deleteOnExit() expectedJspClassFilesDir = baseDir.getCanonicalFile().getAbsolutePath() + expectedJspClassFilesDir - baseUrl = "http://localhost:$port/$jspWebappContext" + + port = TestUtils.randomOpenPort() + + tomcatServer = new Tomcat() tomcatServer.setBaseDir(baseDir.getAbsolutePath()) + tomcatServer.setPort(port) + // comment to debug + tomcatServer.setSilent(true) + // this is needed in tomcat 9, this triggers the creation of a connector, will not + // affect tomcat 7 and 8 + // https://stackoverflow.com/questions/48998387/code-works-with-embedded-apache-tomcat-8-but-not-with-9-whats-changed + tomcatServer.getConnector() + + baseUrl = "http://localhost:$port/$jspWebappContext" appContext = tomcatServer.addWebapp("/$jspWebappContext", JSPInstrumentationForwardTests.getResource("/webapps/jsptest").getPath())