diff --git a/dd-java-agent/instrumentation/spring-web/src/main/java/datadog/trace/instrumentation/springweb/SpringWebInstrumentation.java b/dd-java-agent/instrumentation/spring-web/src/main/java/datadog/trace/instrumentation/springweb/SpringWebInstrumentation.java index 688016051a..f14edf8148 100644 --- a/dd-java-agent/instrumentation/spring-web/src/main/java/datadog/trace/instrumentation/springweb/SpringWebInstrumentation.java +++ b/dd-java-agent/instrumentation/spring-web/src/main/java/datadog/trace/instrumentation/springweb/SpringWebInstrumentation.java @@ -2,6 +2,7 @@ package datadog.trace.instrumentation.springweb; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasClassWithField; +import static io.opentracing.log.Fields.ERROR_OBJECT; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; @@ -9,20 +10,29 @@ import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.not; 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.api.DDSpanTypes; import datadog.trace.api.DDTags; import io.opentracing.Scope; +import io.opentracing.Span; +import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; +import java.lang.reflect.Method; +import java.util.Collections; import java.util.HashMap; import java.util.Map; +import javax.servlet.Servlet; import javax.servlet.http.HttpServletRequest; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; +import org.springframework.web.HttpRequestHandler; +import org.springframework.web.method.HandlerMethod; import org.springframework.web.servlet.HandlerMapping; +import org.springframework.web.servlet.mvc.Controller; @AutoService(Instrumenter.class) public final class SpringWebInstrumentation extends Instrumenter.Default { @@ -50,26 +60,88 @@ public final class SpringWebInstrumentation extends Instrumenter.Default { isMethod() .and(isPublic()) .and(nameStartsWith("handle")) - .and(takesArgument(0, named("javax.servlet.http.HttpServletRequest"))), - SpringWebNamingAdvice.class.getName()); + .and(takesArgument(0, named("javax.servlet.http.HttpServletRequest"))) + .and(takesArguments(3)), + ControllerAdvice.class.getName()); return transformers; } - public static class SpringWebNamingAdvice { + public static class ControllerAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static void nameResource(@Advice.Argument(0) final HttpServletRequest request) { - final Scope scope = GlobalTracer.get().scopeManager().active(); - if (scope != null && request != null) { + public static Scope nameResourceAndStartSpan( + @Advice.Argument(0) final HttpServletRequest request, + @Advice.Argument(2) final Object handler) { + // Name the parent span based on the matching pattern + // This is likely the servlet.request span. + final Scope parentScope = GlobalTracer.get().scopeManager().active(); + if (parentScope != null && request != null) { final String method = request.getMethod(); final Object bestMatchingPattern = request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); if (method != null && bestMatchingPattern != null) { final String resourceName = method + " " + bestMatchingPattern; - scope.span().setTag(DDTags.RESOURCE_NAME, resourceName); - scope.span().setTag(DDTags.SPAN_TYPE, DDSpanTypes.WEB_SERVLET); + parentScope.span().setTag(DDTags.RESOURCE_NAME, resourceName); + parentScope.span().setTag(DDTags.SPAN_TYPE, DDSpanTypes.WEB_SERVLET); } } + + // Now create a span for controller execution. + + final Class clazz; + final String methodName; + + if (handler instanceof HandlerMethod) { + // name span based on the class and method name defined in the handler + final Method method = ((HandlerMethod) handler).getMethod(); + clazz = method.getDeclaringClass(); + methodName = method.getName(); + } else if (handler instanceof HttpRequestHandler) { + // org.springframework.web.servlet.mvc.HttpRequestHandlerAdapter + clazz = handler.getClass(); + methodName = "handleRequest"; + } else if (handler instanceof Controller) { + // org.springframework.web.servlet.mvc.SimpleControllerHandlerAdapter + clazz = handler.getClass(); + methodName = "handleRequest"; + } else if (handler instanceof Servlet) { + // org.springframework.web.servlet.handler.SimpleServletHandlerAdapter + clazz = handler.getClass(); + methodName = "service"; + } else { + // perhaps org.springframework.web.servlet.mvc.annotation.AnnotationMethodHandlerAdapter + clazz = handler.getClass(); + methodName = ""; + } + + String className = clazz.getSimpleName(); + if (className.isEmpty()) { + className = clazz.getName(); + if (clazz.getPackage() != null) { + final String pkgName = clazz.getPackage().getName(); + if (!pkgName.isEmpty()) { + className = clazz.getName().replace(pkgName, "").substring(1); + } + } + } + + final String operationName = className + "." + methodName; + + return GlobalTracer.get() + .buildSpan(operationName) + .withTag(Tags.COMPONENT.getKey(), "spring-web-controller") + .startActive(true); + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void stopSpan( + @Advice.Enter final Scope scope, @Advice.Thrown final Throwable throwable) { + if (throwable != null) { + final Span span = scope.span(); + Tags.ERROR.set(span, true); + span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); + } + scope.close(); } } } diff --git a/dd-java-agent/instrumentation/spring-web/src/test/groovy/test/SpringBootBasedTest.groovy b/dd-java-agent/instrumentation/spring-web/src/test/groovy/test/SpringBootBasedTest.groovy index d6eee233c8..ef3efc9242 100644 --- a/dd-java-agent/instrumentation/spring-web/src/test/groovy/test/SpringBootBasedTest.groovy +++ b/dd-java-agent/instrumentation/spring-web/src/test/groovy/test/SpringBootBasedTest.groovy @@ -1,7 +1,10 @@ package test import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.agent.test.TraceAssert import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags +import io.opentracing.tag.Tags import org.springframework.beans.factory.annotation.Autowired import org.springframework.boot.context.embedded.LocalServerPort import org.springframework.boot.test.context.SpringBootTest @@ -36,7 +39,7 @@ class SpringBootBasedTest extends AgentTestRunner { restTemplate.getForObject("http://localhost:$port/param/asdf1234/", String) == "Hello asdf1234" assertTraces(TEST_WRITER, 1) { - trace(0, 1) { + trace(0, 2) { span(0) { operationName "servlet.request" resourceName "GET /param/{parameter}/" @@ -53,6 +56,7 @@ class SpringBootBasedTest extends AgentTestRunner { defaultTags() } } + controllerSpan(it, 1, "TestController.withParam") } } } @@ -66,7 +70,7 @@ class SpringBootBasedTest extends AgentTestRunner { response.get("error") == "Not Found" assertTraces(TEST_WRITER, 2) { - trace(0, 1) { + trace(0, 2) { span(0) { operationName "servlet.request" resourceName "404" @@ -83,8 +87,9 @@ class SpringBootBasedTest extends AgentTestRunner { defaultTags() } } + controllerSpan(it, 1, "ResourceHttpRequestHandler.handleRequest") } - trace(1, 1) { + trace(1, 2) { span(0) { operationName "servlet.request" resourceName "404" @@ -101,6 +106,7 @@ class SpringBootBasedTest extends AgentTestRunner { defaultTags() } } + controllerSpan(it, 1, "BasicErrorController.error") } } } @@ -108,7 +114,7 @@ class SpringBootBasedTest extends AgentTestRunner { def "generates error spans"() { setup: def response = restTemplate.getForObject("http://localhost:$port/error/qwerty/", Map) - + expect: response.get("status") == 500 response.get("error") == "Internal Server Error" @@ -116,7 +122,7 @@ class SpringBootBasedTest extends AgentTestRunner { response.get("message") == "qwerty" assertTraces(TEST_WRITER, 2) { - trace(0, 1) { + trace(0, 2) { span(0) { operationName "servlet.request" resourceName "GET /error/{parameter}/" @@ -134,8 +140,9 @@ class SpringBootBasedTest extends AgentTestRunner { defaultTags() } } + controllerSpan(it, 1, "TestController.withError", RuntimeException) } - trace(1, 1) { + trace(1, 2) { span(0) { operationName "servlet.request" resourceName "GET /error" @@ -153,6 +160,7 @@ class SpringBootBasedTest extends AgentTestRunner { defaultTags() } } + controllerSpan(it, 1, "BasicErrorController.error") } } } @@ -162,7 +170,7 @@ class SpringBootBasedTest extends AgentTestRunner { restTemplate.postForObject("http://localhost:$port/validated", new TestForm("bob", 20), String) == "Hello bob Person(Name: bob, Age: 20)" assertTraces(TEST_WRITER, 1) { - trace(0, 1) { + trace(0, 2) { span(0) { operationName "servlet.request" resourceName "POST /validated" @@ -179,6 +187,7 @@ class SpringBootBasedTest extends AgentTestRunner { defaultTags() } } + controllerSpan(it, 1, "TestController.withValidation") } } } @@ -194,7 +203,7 @@ class SpringBootBasedTest extends AgentTestRunner { response.get("message") == "Validation failed for object='testForm'. Error count: 1" assertTraces(TEST_WRITER, 2) { - trace(0, 1) { + trace(0, 2) { span(0) { operationName "servlet.request" resourceName "POST /validated" @@ -215,8 +224,9 @@ class SpringBootBasedTest extends AgentTestRunner { defaultTags() } } + controllerSpan(it, 1, "TestController.withValidation", MethodArgumentNotValidException) } - trace(1, 1) { + trace(1, 2) { span(0) { operationName "servlet.request" resourceName "POST /error" @@ -233,6 +243,26 @@ class SpringBootBasedTest extends AgentTestRunner { defaultTags() } } + controllerSpan(it, 1, "BasicErrorController.error") + } + } + } + + def controllerSpan(TraceAssert trace, int index, String name, Class errorType = null) { + trace.span(index) { + serviceName "unnamed-java-app" + operationName name + resourceName name + childOf(trace.span(0)) + errored errorType != null + tags { + "$DDTags.SPAN_TYPE" DDSpanTypes.WEB_SERVLET + "$Tags.COMPONENT.key" "spring-web-controller" + if (errorType) { + "error.msg" String + errorTags(errorType) + } + defaultTags() } } }