Merge pull request #2197 from DataDog/tyler/404-spring

Avoid grouping Spring Web routed requests into 404 bucket

Author: tylerbenson

dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/HandlerAdapterInstrumentation.java

@@ -22,7 +22,6 @@
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
 import java.util.Map;
 import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.method.MethodDescription;
 import net.bytebuddy.description.type.TypeDescription;
@@ -71,10 +70,9 @@ public static class ControllerAdvice {
     @Advice.OnMethodEnter(suppress = Throwable.class)
     public static AgentScope nameResourceAndStartSpan(
         @Advice.Argument(0) final HttpServletRequest request,
-        @Advice.Argument(2) final Object handler,
-        @Advice.Local("_parentSpan") Object parentSpan) {
+        @Advice.Argument(2) final Object handler) {
       // Name the parent span based on the matching pattern
-      parentSpan = request.getAttribute(DD_SPAN_ATTRIBUTE);
+      Object parentSpan = request.getAttribute(DD_SPAN_ATTRIBUTE);
       if (parentSpan instanceof AgentSpan) {
         DECORATE.onRequest((AgentSpan) parentSpan, request);
       }
@@ -97,16 +95,10 @@ public static AgentScope nameResourceAndStartSpan(
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
     public static void stopSpan(
-        @Advice.Argument(1) HttpServletResponse response,
-        @Advice.Local("_parentSpan") Object parentSpan,
-        @Advice.Enter final AgentScope scope,
-        @Advice.Thrown final Throwable throwable) {
+        @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) {
       if (scope == null) {
         return;
       }
-      if (parentSpan instanceof AgentSpan) {
-        DECORATE.onResponse((AgentSpan) parentSpan, response);
-      }
 
       DECORATE.onError(scope, throwable);
       DECORATE.beforeFinish(scope);

dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecorator.java

@@ -96,7 +96,8 @@ public AgentSpan onRequest(final AgentSpan span, final HttpServletRequest reques
       final String method = request.getMethod();
       final Object bestMatchingPattern =
           request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE);
-      if (method != null && bestMatchingPattern != null) {
+      if (method != null && bestMatchingPattern != null && !bestMatchingPattern.equals("/**")) {
+        // universal matching is not helpful (and prevents 404 renaming in URLAsResourceNameRule).
         final CharSequence resourceName =
             RESOURCE_NAME_CACHE.computeIfAbsent(
                 Pair.of(method, bestMatchingPattern), RESOURCE_NAME_JOINER);

dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy

@@ -1,6 +1,5 @@
 package test.boot
 
-
 import datadog.trace.agent.test.asserts.TraceAssert
 import datadog.trace.agent.test.base.HttpServerTest
 import datadog.trace.api.DDSpanTypes
@@ -17,6 +16,7 @@ import org.springframework.web.servlet.view.RedirectView
 import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
 import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.LOGIN
 import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_HERE
 import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT
 import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS
 import static java.util.Collections.singletonMap
@@ -94,6 +94,29 @@ class SpringBootBasedTest extends HttpServerTest<ConfigurableApplicationContext>
     testPassword << ["password", "dfsdföääöüüä", "🤓"]
   }
 
+  def "test not-here"() {
+    setup:
+    def request = request(NOT_HERE, method, body).build()
+    def response = client.newCall(request).execute()
+
+    expect:
+    response.code() == NOT_HERE.status
+
+    and:
+    cleanAndAssertTraces(1) {
+      trace(3) {
+        sortSpansByStart()
+        serverSpan(it, null, null, method, NOT_HERE)
+        handlerSpan(it, NOT_HERE)
+        controllerSpan(it)
+      }
+    }
+
+    where:
+    method = "GET"
+    body = null
+  }
+
   boolean hasResponseSpan(ServerEndpoint endpoint) {
     return endpoint == REDIRECT || endpoint == NOT_FOUND || endpoint == LOGIN
   }

dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/TestController.groovy

@@ -13,6 +13,7 @@ import org.springframework.web.servlet.view.RedirectView
 
 import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR
 import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
+import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_HERE
 import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
 import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
 import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT
@@ -53,6 +54,13 @@ class TestController {
     }
   }
 
+  @RequestMapping("/not-here")
+  ResponseEntity not_here() {
+    HttpServerTest.controller(NOT_HERE) {
+      new ResponseEntity(NOT_HERE.body, HttpStatus.valueOf(NOT_HERE.status))
+    }
+  }
+
   @RequestMapping("/error-status")
   ResponseEntity error() {
     HttpServerTest.controller(ERROR) {

dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy

@@ -18,6 +18,7 @@ import org.slf4j.Logger
 import org.slf4j.LoggerFactory
 import spock.lang.Shared
 import spock.lang.Unroll
+
 import java.util.concurrent.atomic.AtomicBoolean
 
 import static datadog.trace.agent.test.asserts.TraceAssert.assertTrace
@@ -115,7 +116,8 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     REDIRECT("redirect", 302, "/redirected"),
     ERROR("error-status", 500, "controller error"), // "error" is a special path for some frameworks
     EXCEPTION("exception", 500, "controller exception"),
-    NOT_FOUND("notFound", 404, "not found"),
+    NOT_FOUND("not-found", 404, "not found"),
+    NOT_HERE("not-here", 404, "not here"), // Explicitly returned 404 from a valid controller
 
     TIMEOUT("timeout", -1, null),
     TIMEOUT_ERROR("timeout_error", -1, null),
@@ -162,7 +164,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
     }
 
     String resource(String method, URI address, String pathParam) {
-      return status == 404 ? "404" : "$method ${hasPathParam ? pathParam : resolve(address).path}"
+      return path == "not-found" ? "404" : "$method ${hasPathParam ? pathParam : resolve(address).path}"
     }
 
     private static final Map<String, ServerEndpoint> PATH_MAP = values().collectEntries { [it.path, it] }