Fix latestDepTest for JSP instrumentation.

Normalize jsp.requestUrl tag value because Tomcat 9 seems to add relative path symbols of jsp files.

Tomcat 9 doesn't automatically create a connector, causing failures in latestDepTest. Tomcat 9 also seems to have small changes that effect some of the tests, such as changing the exception type and also removal of the exception message in one of the tests.

Author: gary-huang

dd-java-agent/instrumentation/jsp-2.3/src/main/java/datadog/trace/instrumentation/jsp/JSPInstrumentation.java

@@ -16,15 +16,19 @@
 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 static Scope startSpan(
       // 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;

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() {
+    baseDir = Files.createTempDir()
+    baseDir.deleteOnExit()
+    expectedJspClassFilesDir = baseDir.getCanonicalFile().getAbsolutePath() + expectedJspClassFilesDir
+
     port = TestUtils.randomOpenPort()
+
     tomcatServer = new Tomcat()
+    tomcatServer.setBaseDir(baseDir.getAbsolutePath())
     tomcatServer.setPort(port)
     // comment to debug
     tomcatServer.setSilent(true)
-
-    baseDir = Files.createTempDir()
-    baseDir.deleteOnExit()
-    expectedJspClassFilesDir = baseDir.getCanonicalFile().getAbsolutePath() + expectedJspClassFilesDir
+    // 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"
-    tomcatServer.setBaseDir(baseDir.getAbsolutePath())
 
     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"() {

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() {
+    baseDir = Files.createTempDir()
+    baseDir.deleteOnExit()
+    expectedJspClassFilesDir = baseDir.getCanonicalFile().getAbsolutePath() + expectedJspClassFilesDir
+
     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()
 
-    baseDir = Files.createTempDir()
-    baseDir.deleteOnExit()
-    expectedJspClassFilesDir = baseDir.getCanonicalFile().getAbsolutePath() + expectedJspClassFilesDir
     baseUrl = "http://localhost:$port/$jspWebappContext"
-    tomcatServer.setBaseDir(baseDir.getAbsolutePath())
 
     appContext = tomcatServer.addWebapp("/$jspWebappContext",
       JSPInstrumentationForwardTests.getResource("/webapps/jsptest").getPath())