diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/DispatcherServletInstrumentation.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/DispatcherServletInstrumentation.java index 2c2874bee35..bfe0f775159 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/DispatcherServletInstrumentation.java +++ b/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() diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/ResourceNameFilterMappingInstrumentation.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/ResourceNameFilterMappingInstrumentation.java new file mode 100644 index 00000000000..9b1b73a4f62 --- /dev/null +++ b/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"); + } +} diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/ServletPathRequestFilterInstrumentation.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/ServletPathRequestFilterInstrumentation.java index 5c2db70ba85..784da331cf0 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/ServletPathRequestFilterInstrumentation.java +++ b/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 classLoaderMatcher() { return hasClassNamed("org.springframework.web.filter.ServletRequestPathFilter") diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/WebApplicationContextInstrumentation.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/WebApplicationContextInstrumentation.java index 672570f94bf..a7c54f26e93 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java/datadog/trace/instrumentation/springweb6/WebApplicationContextInstrumentation.java +++ b/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"; diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/ControllerAdvice.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/ControllerAdvice.java index 5a675262cad..b5eaeb2b9bd 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/ControllerAdvice.java +++ b/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; diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandleMatchAdvice.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandleMatchAdvice.java index e299ac0f91c..426aff5ffcb 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandleMatchAdvice.java +++ b/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")) { diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandlerMappingAdvice.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandlerMappingAdvice.java index ffabc4115be..08783e50a96 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/HandlerMappingAdvice.java +++ b/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. + * + *

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 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()); } } } diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/ResourceNameFilterMappingAdvice.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/main/java17/datadog/trace/instrumentation/springweb6/ResourceNameFilterMappingAdvice.java new file mode 100644 index 00000000000..7794aa0ac15 --- /dev/null +++ b/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 handlerMappings) { + if (handlerMappings != null && springCtx.containsBean("ddDispatcherFilter")) { + final HandlerMappingResourceNameFilter filter = + (HandlerMappingResourceNameFilter) springCtx.getBean("ddDispatcherFilter"); + filter.setHandlerMappings(handlerMappings); + } + } +} diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/boot/SpringBootBasedTest.groovy b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/boot/SpringBootBasedTest.groovy index 3cd4ff3efd9..7bf01c8905f 100644 --- a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/groovy/datadog/trace/instrumentation/springweb6/boot/SpringBootBasedTest.groovy +++ b/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 Map 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 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") + } +} diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/java/datadog/trace/instrumentation/springweb6/boot/FailOnHeaderConfig.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/java/datadog/trace/instrumentation/springweb6/boot/FailOnHeaderConfig.java new file mode 100644 index 00000000000..654893ffd40 --- /dev/null +++ b/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()); + } +} diff --git a/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/java/datadog/trace/instrumentation/springweb6/boot/FailOnHeaderInterceptor.java b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/java/datadog/trace/instrumentation/springweb6/boot/FailOnHeaderInterceptor.java new file mode 100644 index 00000000000..03728a89e4e --- /dev/null +++ b/dd-java-agent/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/src/test/java/datadog/trace/instrumentation/springweb6/boot/FailOnHeaderInterceptor.java @@ -0,0 +1,21 @@ +package datadog.trace.instrumentation.springweb6.boot; + +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.springframework.web.servlet.HandlerInterceptor; + +/** + * Aborts the request from {@code preHandle} when the {@code fail} header is set, so the controller + * is never invoked. Test to verify the route is still named on the server span in that case (see + * {@code HandlerMappingAdvice}). + */ +public class FailOnHeaderInterceptor implements HandlerInterceptor { + @Override + public boolean preHandle( + final HttpServletRequest request, final HttpServletResponse response, final Object handler) { + if ("true".equalsIgnoreCase(request.getHeader("fail"))) { + throw new RuntimeException("Stop here"); + } + return true; + } +}