Drop the duplicate handler resolution in Spring Web 6 instrumentation (#11533)

Drop the duplicate handler resolution in Spring Web 6 instrumentation

reintroduce the old instrumentation and make it opt-out

Co-authored-by: andrea.marziali <andrea.marziali@datadoghq.com>

Author: amarziali

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/DispatcherServletInstrumentation.java

@@ -4,6 +4,7 @@
 import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.isMethod;
 import static net.bytebuddy.matcher.ElementMatchers.isProtected;
+import static net.bytebuddy.matcher.ElementMatchers.returns;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
 
@@ -27,10 +28,7 @@ public String instrumentedType() {
   @Override
   public String[] helperClassNames() {
     return new String[] {
-      packageName + ".SpringWebHttpServerDecorator",
-      packageName + ".ServletRequestURIAdapter",
-      packageName + ".HandlerMappingResourceNameFilter",
-      packageName + ".PathMatchingHttpServletRequestWrapper",
+      packageName + ".SpringWebHttpServerDecorator", packageName + ".ServletRequestURIAdapter",
     };
   }
 
@@ -39,9 +37,10 @@ public void methodAdvice(MethodTransformer transformer) {
     transformer.applyAdvice(
         isMethod()
             .and(isProtected())
-            .and(named("onRefresh"))
-            .and(takesArgument(0, named("org.springframework.context.ApplicationContext")))
-            .and(takesArguments(1)),
+            .and(named("getHandler"))
+            .and(takesArgument(0, named("jakarta.servlet.http.HttpServletRequest")))
+            .and(takesArguments(1))
+            .and(returns(named("org.springframework.web.servlet.HandlerExecutionChain"))),
         packageName + ".HandlerMappingAdvice");
     transformer.applyAdvice(
         isMethod()

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/ResourceNameFilterMappingInstrumentation.java

@@ -0,0 +1,58 @@
+package datadog.trace.instrumentation.springweb6;
+
+import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
+import static net.bytebuddy.matcher.ElementMatchers.isMethod;
+import static net.bytebuddy.matcher.ElementMatchers.isProtected;
+import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
+import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
+
+import com.google.auto.service.AutoService;
+import datadog.trace.agent.tooling.Instrumenter;
+import datadog.trace.agent.tooling.InstrumenterModule;
+
+/**
+ * When the opt-in {@code spring-path-filter} integration is enabled, wires the {@code
+ * DispatcherServlet} handler mappings into {@link HandlerMappingResourceNameFilter} on context
+ * refresh. Disabled by default; the default route naming is handled by {@link HandlerMappingAdvice}
+ * ({@code getHandler}) and {@link ControllerAdvice}.
+ */
+@AutoService(InstrumenterModule.class)
+public final class ResourceNameFilterMappingInstrumentation extends InstrumenterModule.Tracing
+    implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice {
+
+  public ResourceNameFilterMappingInstrumentation() {
+    super("spring-web", "spring-path-filter");
+  }
+
+  @Override
+  protected boolean defaultEnabled() {
+    return false;
+  }
+
+  @Override
+  public String instrumentedType() {
+    return "org.springframework.web.servlet.DispatcherServlet";
+  }
+
+  @Override
+  public String[] helperClassNames() {
+    return new String[] {
+      packageName + ".SpringWebHttpServerDecorator",
+      packageName + ".ServletRequestURIAdapter",
+      packageName + ".HandlerMappingResourceNameFilter",
+      packageName + ".HandlerMappingResourceNameFilter$BeanDefinition",
+      packageName + ".PathMatchingHttpServletRequestWrapper",
+    };
+  }
+
+  @Override
+  public void methodAdvice(MethodTransformer transformer) {
+    transformer.applyAdvice(
+        isMethod()
+            .and(isProtected())
+            .and(named("onRefresh"))
+            .and(takesArgument(0, named("org.springframework.context.ApplicationContext")))
+            .and(takesArguments(1)),
+        packageName + ".ResourceNameFilterMappingAdvice");
+  }
+}

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/ServletPathRequestFilterInstrumentation.java

@@ -14,8 +14,10 @@
 import net.bytebuddy.matcher.ElementMatcher;
 
 /**
- * This instrumentation adds the ServletPathRequestFilter definition to the spring context When the
- * context is created, the filter will be added to the beginning of the filter chain
+ * Companion to {@link WebApplicationContextInstrumentation}: when the opt-in {@code
+ * spring-path-filter} integration is enabled, registers Spring's {@code ServletRequestPathFilter}
+ * first in the chain so the request path is parsed before {@link HandlerMappingResourceNameFilter}
+ * resolves the handler. Disabled by default.
  */
 @AutoService(InstrumenterModule.class)
 public class ServletPathRequestFilterInstrumentation extends InstrumenterModule.Tracing
@@ -24,6 +26,11 @@ public ServletPathRequestFilterInstrumentation() {
     super("spring-web", "spring-path-filter");
   }
 
+  @Override
+  protected boolean defaultEnabled() {
+    return false;
+  }
+
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
     return hasClassNamed("org.springframework.web.filter.ServletRequestPathFilter")

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/WebApplicationContextInstrumentation.java

@@ -13,8 +13,10 @@
 import net.bytebuddy.matcher.ElementMatcher;
 
 /**
- * This instrumentation adds the HandlerMappingResourceNameFilter definition to the spring context
- * When the context is created, the filter will be added to the beginning of the filter chain
+ * Opt-in alternative to the default {@code getHandler}/{@code ControllerAdvice} route naming. Adds
+ * the {@link HandlerMappingResourceNameFilter} bean to the Spring context, which names the route at
+ * the start of the filter chain by resolving the handler itself. This is the legacy approach and is
+ * disabled by default; enable it with {@code dd.integration.spring-path-filter.enabled=true}.
  */
 @AutoService(InstrumenterModule.class)
 public class WebApplicationContextInstrumentation extends InstrumenterModule.Tracing
@@ -23,6 +25,11 @@ public WebApplicationContextInstrumentation() {
     super("spring-web", "spring-path-filter");
   }
 
+  @Override
+  protected boolean defaultEnabled() {
+    return false;
+  }
+
   @Override
   public String hierarchyMarkerType() {
     return "org.springframework.web.context.WebApplicationContext";

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/ControllerAdvice.java

@@ -25,7 +25,11 @@ public static ContextScope nameResourceAndStartSpan(
       @Advice.Argument(2) final Object handler,
       @Advice.Local("handlerSpanKey") String handlerSpanKey) {
     handlerSpanKey = "";
-    // Name the parent span based on the matching pattern
+
+    /*
+    By the time HandlerAdapter.handle runs, every handler mapping kind (annotated and SimpleUrlHandlerMapping via its
+    PathExposingHandlerInterceptor) has populated BEST_MATCHING_PATTERN_ATTRIBUTE.
+    */
     Object contextObj = request.getAttribute(DD_CONTEXT_ATTRIBUTE);
     if (contextObj instanceof Context) {
       Context context = (Context) contextObj;

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandleMatchAdvice.java

@@ -37,7 +37,8 @@ public static void after(
       return;
     }
 
-    // hacky, but APM instrumentation causes the instrumented method to be called twice
+    // When the opt-in spring-path-filter integration is enabled, the filter resolves the handler
+    // against a PathMatchingHttpServletRequestWrapper, which triggers handleMatch a second time.
     if (req.getClass()
         .getName()
         .equals("datadog.trace.instrumentation.springweb6.PathMatchingHttpServletRequestWrapper")) {

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandlerMappingAdvice.java

@@ -1,26 +1,39 @@
 package datadog.trace.instrumentation.springweb6;
 
-import java.util.List;
+import static datadog.context.Context.root;
+import static datadog.trace.bootstrap.instrumentation.api.Java8BytecodeBridge.spanFromContext;
+import static datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator.DD_CONTEXT_ATTRIBUTE;
+import static datadog.trace.instrumentation.springweb6.SpringWebHttpServerDecorator.DECORATE;
+
+import datadog.context.Context;
+import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
+import jakarta.servlet.http.HttpServletRequest;
 import net.bytebuddy.asm.Advice;
-import org.springframework.context.ApplicationContext;
-import org.springframework.web.servlet.HandlerMapping;
+import org.springframework.web.servlet.HandlerExecutionChain;
 
 /**
- * This advice creates a filter that has reference to the handlerMappings from DispatcherServlet
- * which allows the mappings to be evaluated at the beginning of the filter chain. This evaluation
- * is done inside the Servlet3Decorator.onContext method.
+ * Names the server span route as soon as {@code DispatcherServlet} has resolved the handler.
+ *
+ * <p>For {@code RequestMappingInfoHandlerMapping}, {@code handleMatch} sets {@link
+ * org.springframework.web.servlet.HandlerMapping#BEST_MATCHING_PATTERN_ATTRIBUTE} synchronously
+ * inside {@code getHandler}, before interceptors run. Naming the route here means it survives a
+ * {@code HandlerInterceptor.preHandle} that aborts the request before the controller executes.
  */
 public class HandlerMappingAdvice {
 
-  @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
-  public static void afterRefresh(
-      @Advice.Argument(0) final ApplicationContext springCtx,
-      @Advice.FieldValue("handlerMappings") final List<HandlerMapping> handlerMappings) {
-    if (springCtx.containsBean("ddDispatcherFilter")) {
-      final datadog.trace.instrumentation.springweb6.HandlerMappingResourceNameFilter filter =
-          (HandlerMappingResourceNameFilter) springCtx.getBean("ddDispatcherFilter");
-      if (handlerMappings != null && filter != null) {
-        filter.setHandlerMappings(handlerMappings);
+  @Advice.OnMethodExit(suppress = Throwable.class)
+  public static void onExit(
+      @Advice.Argument(0) final HttpServletRequest request,
+      @Advice.Return final HandlerExecutionChain chain) {
+    if (chain == null) {
+      // No handler matched (e.g. 404): leave the resource name untouched.
+      return;
+    }
+    final Object contextObj = request.getAttribute(DD_CONTEXT_ATTRIBUTE);
+    if (contextObj instanceof Context) {
+      final AgentSpan parentSpan = spanFromContext((Context) contextObj);
+      if (parentSpan != null) {
+        DECORATE.onRequest(parentSpan, request, request, root());
       }
     }
   }

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/ResourceNameFilterMappingAdvice.java

@@ -0,0 +1,26 @@
+package datadog.trace.instrumentation.springweb6;
+
+import java.util.List;
+import net.bytebuddy.asm.Advice;
+import org.springframework.context.ApplicationContext;
+import org.springframework.web.servlet.HandlerMapping;
+
+/**
+ * Wires the {@code DispatcherServlet} handler mappings into the (opt-in) {@link
+ * HandlerMappingResourceNameFilter} after the context refreshes, so the filter can evaluate them at
+ * the start of the filter chain. Only active when the {@code spring-path-filter} integration is
+ * enabled (see {@code ResourceNameFilterMappingInstrumentation}).
+ */
+public class ResourceNameFilterMappingAdvice {
+
+  @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
+  public static void afterRefresh(
+      @Advice.Argument(0) final ApplicationContext springCtx,
+      @Advice.FieldValue("handlerMappings") final List<HandlerMapping> handlerMappings) {
+    if (handlerMappings != null && springCtx.containsBean("ddDispatcherFilter")) {
+      final HandlerMappingResourceNameFilter filter =
+          (HandlerMappingResourceNameFilter) springCtx.getBean("ddDispatcherFilter");
+      filter.setHandlerMappings(handlerMappings);
+    }
+  }
+}

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/boot/SpringBootBasedTest.groovy

@@ -16,8 +16,6 @@ import datadog.trace.bootstrap.instrumentation.api.Tags
 import datadog.trace.core.DDSpan
 import datadog.trace.instrumentation.springweb6.SetupSpecHelper
 import datadog.trace.instrumentation.springweb6.SpringWebHttpServerDecorator
-import jakarta.servlet.http.HttpServletRequest
-import jakarta.servlet.http.HttpServletResponse
 import okhttp3.Credentials
 import okhttp3.FormBody
 import okhttp3.Request
@@ -28,7 +26,6 @@ import org.springframework.boot.SpringApplication
 import org.springframework.boot.web.servlet.context.ServletWebServerApplicationContext
 import org.springframework.context.ConfigurableApplicationContext
 import org.springframework.http.MediaType
-import org.springframework.web.servlet.HandlerInterceptor
 import org.springframework.web.socket.BinaryMessage
 import org.springframework.web.socket.TextMessage
 import org.springframework.web.socket.WebSocketSession
@@ -59,7 +56,7 @@ class SpringBootBasedTest extends HttpServerTest<ConfigurableApplicationContext>
   Map<String, String> extraServerTags = [:]
 
   SpringApplication application() {
-    return new SpringApplication(SecurityConfig, TestController, AppConfig, WebsocketConfig)
+    return new SpringApplication(SecurityConfig, TestController, AppConfig, WebsocketConfig, FailOnHeaderConfig)
   }
 
   class SpringBootServer implements WebsocketServer {
@@ -451,26 +448,20 @@ class SpringBootBasedTest extends HttpServerTest<ConfigurableApplicationContext>
     InstrumentationBridge.clearIastModules()
   }
 
-  def 'path is extract when preHandle fails'() {
+  def 'path is extracted when preHandle fails'() {
     setup:
     def request = request(PATH_PARAM, 'GET', null).header("fail", "true").build()
-    context.getBeanFactory().registerSingleton("testHandler", new HandlerInterceptor() {
-        @Override
-        boolean preHandle(HttpServletRequest req, HttpServletResponse response, Object handler) throws Exception {
-          if ("true".equalsIgnoreCase(req.getHeader("fail"))) {
-            throw new RuntimeException("Stop here")
-          }
-          return true
-        }
-      })
 
     when:
-    client.newCall(request).execute()
+    def response = client.newCall(request).execute()
+    response.close()
     TEST_WRITER.waitForTraces(1)
     DDSpan span = TEST_WRITER.flatten().find { "servlet.request".contentEquals(it.operationName) }
 
     then:
+    response.code() == 500
     span.getResourceName().toString() == "GET " + testPathParam()
+    span.isError()
   }
 
   boolean hasResponseSpan(ServerEndpoint endpoint) {
@@ -526,3 +517,16 @@ class SpringBootRumInjectionForkedTest extends SpringBootBasedTest {
     true
   }
 }
+
+/**
+ * Runs the full suite with the opt-in legacy route-naming filter enabled, to verify that path still
+ * produces the same traces (including APMS-8174). The default path (getHandler + ControllerAdvice)
+ * stays active; the DD_FILTERED_SPRING_ROUTE_ALREADY_APPLIED guard prevents double naming.
+ */
+class SpringBootResourceNameFilterForkedTest extends SpringBootBasedTest {
+  @Override
+  protected void configurePreAgent() {
+    super.configurePreAgent()
+    injectSysConfig("dd.integration.spring-path-filter.enabled", "true")
+  }
+}

dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/java/datadog/trace/instrumentation/springweb6/boot/FailOnHeaderConfig.java

@@ -0,0 +1,13 @@
+package datadog.trace.instrumentation.springweb6.boot;
+
+import org.springframework.context.annotation.Configuration;
+import org.springframework.web.servlet.config.annotation.InterceptorRegistry;
+import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;
+
+@Configuration
+public class FailOnHeaderConfig implements WebMvcConfigurer {
+  @Override
+  public void addInterceptors(final InterceptorRegistry registry) {
+    registry.addInterceptor(new FailOnHeaderInterceptor());
+  }
+}