From 7013376030ac67352e5f2c72d162a2a04cce3691 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Tue, 9 Mar 2021 08:56:33 +0200 Subject: [PATCH] Add support for Grails (#2512) * Add support for Grails * exclude bad version from muzzle * Review fixes * review fixes * rebase * Trigger Build --- docs/supported-libraries.md | 1 + .../javaagent/grails-3.0-javaagent.gradle | 44 +++++ ...tGrailsControllerClassInstrumentation.java | 72 ++++++++ .../grails/GrailsInstrumentationModule.java | 27 +++ .../instrumentation/grails/GrailsTracer.java | 40 +++++ ...ingsInfoHandlerAdapterInstrumentation.java | 59 +++++++ .../test/groovy/test/ErrorController.groovy | 17 ++ .../src/test/groovy/test/GrailsTest.groovy | 156 ++++++++++++++++++ .../test/groovy/test/TestController.groovy | 67 ++++++++ .../src/test/groovy/test/UrlMappings.groovy | 20 +++ .../src/test/resources/application.yml | 8 + .../HandlerAdapterInstrumentation.java | 4 +- .../springwebmvc/SpringWebMvcTracer.java | 12 +- .../test/boot/SpringBootBasedTest.groovy | 87 +++++----- .../AdditionalLibraryIgnoresMatcher.java | 2 + settings.gradle | 1 + .../test/base/HttpServerTest.groovy | 28 ++-- 17 files changed, 579 insertions(+), 66 deletions(-) create mode 100644 instrumentation/grails-3.0/javaagent/grails-3.0-javaagent.gradle create mode 100644 instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/DefaultGrailsControllerClassInstrumentation.java create mode 100644 instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsInstrumentationModule.java create mode 100644 instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java create mode 100644 instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/UrlMappingsInfoHandlerAdapterInstrumentation.java create mode 100644 instrumentation/grails-3.0/javaagent/src/test/groovy/test/ErrorController.groovy create mode 100644 instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy create mode 100644 instrumentation/grails-3.0/javaagent/src/test/groovy/test/TestController.groovy create mode 100644 instrumentation/grails-3.0/javaagent/src/test/groovy/test/UrlMappings.groovy create mode 100644 instrumentation/grails-3.0/javaagent/src/test/resources/application.yml diff --git a/docs/supported-libraries.md b/docs/supported-libraries.md index ae09b7951561..aa001b427a93 100644 --- a/docs/supported-libraries.md +++ b/docs/supported-libraries.md @@ -36,6 +36,7 @@ These are the supported libraries and frameworks: | [Elasticsearch REST Client](https://www.elastic.co/guide/en/elasticsearch/client/java-rest/current/index.html) | 5.0+ | | [Finatra](https://github.com/twitter/finatra) | 2.9+ | | [Geode Client](https://geode.apache.org/) | 1.4+ | +| [Grails](https://grails.org/) | 3.0+ | | [Google HTTP Client](https://github.com/googleapis/google-http-java-client) | 1.19+ | | [Grizzly](https://javaee.github.io/grizzly/httpserverframework.html) | 2.0+ (disabled by default) | | [gRPC](https://github.com/grpc/grpc-java) | 1.5+ | diff --git a/instrumentation/grails-3.0/javaagent/grails-3.0-javaagent.gradle b/instrumentation/grails-3.0/javaagent/grails-3.0-javaagent.gradle new file mode 100644 index 000000000000..902b0983e7e9 --- /dev/null +++ b/instrumentation/grails-3.0/javaagent/grails-3.0-javaagent.gradle @@ -0,0 +1,44 @@ +apply from: "$rootDir/gradle/instrumentation.gradle" + +muzzle { + pass { + group = "org.grails" + module = "grails-web-url-mappings" + versions = "[3.0,)" + skipVersions += ['3.1.15'] + assertInverse = true + } +} + +repositories { + maven { + url "https://repo.grails.org/grails/core" + mavenContent { + releasesOnly() + } + } +} + +// first version where our tests work +def grailsVersion = '3.0.6' +def springBootVersion = '1.2.5.RELEASE' + +dependencies { + library ("org.grails:grails-plugin-url-mappings:$grailsVersion") + + testInstrumentation project(':instrumentation:servlet:servlet-3.0:javaagent') + testInstrumentation project(':instrumentation:servlet:servlet-common:javaagent') + testInstrumentation project(':instrumentation:tomcat-7.0:javaagent') + testInstrumentation project(':instrumentation:spring:spring-webmvc-3.1:javaagent') + + testImplementation "org.springframework.boot:spring-boot-autoconfigure:$springBootVersion" + testImplementation "org.springframework.boot:spring-boot-starter-tomcat:$springBootVersion" + + testImplementation(project(':testing-common')) { + exclude group: 'org.eclipse.jetty', module: 'jetty-server' + } + + latestDepTestLibrary ("org.grails:grails-plugin-url-mappings:4.0.+") + latestDepTestLibrary "org.springframework.boot:spring-boot-autoconfigure:2.1.17.RELEASE" + latestDepTestLibrary "org.springframework.boot:spring-boot-starter-tomcat:2.1.17.RELEASE" +} \ No newline at end of file diff --git a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/DefaultGrailsControllerClassInstrumentation.java b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/DefaultGrailsControllerClassInstrumentation.java new file mode 100644 index 000000000000..fa444ec2c7d0 --- /dev/null +++ b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/DefaultGrailsControllerClassInstrumentation.java @@ -0,0 +1,72 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.grails; + +import static io.opentelemetry.javaagent.instrumentation.grails.GrailsTracer.tracer; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isPublic; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; + +import io.opentelemetry.context.Context; +import io.opentelemetry.context.Scope; +import io.opentelemetry.javaagent.tooling.TypeInstrumentation; +import java.util.Map; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +public class DefaultGrailsControllerClassInstrumentation implements TypeInstrumentation { + @Override + public ElementMatcher typeMatcher() { + return named("org.grails.core.DefaultGrailsControllerClass"); + } + + @Override + public Map, String> transformers() { + return singletonMap( + isMethod() + .and(isPublic()) + .and(named("invoke")) + .and(takesArgument(0, named(Object.class.getName()))) + .and(takesArgument(1, named(String.class.getName()))) + .and(takesArguments(2)), + DefaultGrailsControllerClassInstrumentation.class.getName() + "$ControllerAdvice"); + } + + public static class ControllerAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void startSpan( + @Advice.Argument(0) Object controller, + @Advice.Argument(1) String action, + @Advice.FieldValue("defaultActionName") String defaultActionName, + @Advice.Local("otelContext") Context context, + @Advice.Local("otelScope") Scope scope) { + + context = tracer().startSpan(controller, action != null ? action : defaultActionName); + scope = context.makeCurrent(); + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void stopSpan( + @Advice.Thrown Throwable throwable, + @Advice.Local("otelContext") Context context, + @Advice.Local("otelScope") Scope scope) { + if (scope == null) { + return; + } + scope.close(); + if (throwable == null) { + tracer().end(context); + } else { + tracer().endExceptionally(context, throwable); + } + } + } +} diff --git a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsInstrumentationModule.java b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsInstrumentationModule.java new file mode 100644 index 000000000000..47f2869dc5cc --- /dev/null +++ b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsInstrumentationModule.java @@ -0,0 +1,27 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.grails; + +import static java.util.Arrays.asList; + +import com.google.auto.service.AutoService; +import io.opentelemetry.javaagent.tooling.InstrumentationModule; +import io.opentelemetry.javaagent.tooling.TypeInstrumentation; +import java.util.List; + +@AutoService(InstrumentationModule.class) +public class GrailsInstrumentationModule extends InstrumentationModule { + public GrailsInstrumentationModule() { + super("grails", "grails-3.0"); + } + + @Override + public List typeInstrumentations() { + return asList( + new DefaultGrailsControllerClassInstrumentation(), + new UrlMappingsInfoHandlerAdapterInstrumentation()); + } +} diff --git a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java new file mode 100644 index 000000000000..093f511bc777 --- /dev/null +++ b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java @@ -0,0 +1,40 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.grails; + +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; +import io.opentelemetry.instrumentation.api.tracer.BaseTracer; +import org.grails.web.mapping.mvc.GrailsControllerUrlMappingInfo; + +public class GrailsTracer extends BaseTracer { + + private static final GrailsTracer TRACER = new GrailsTracer(); + + public static GrailsTracer tracer() { + return TRACER; + } + + public Context startSpan(Object controller, String action) { + return startSpan(spanNameForClass(controller.getClass()) + "." + action); + } + + public void nameServerSpan( + Context context, Span serverSpan, GrailsControllerUrlMappingInfo info) { + String action = + info.getActionName() != null + ? info.getActionName() + : info.getControllerClass().getDefaultAction(); + serverSpan.updateName( + ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action)); + } + + @Override + protected String getInstrumentationName() { + return "io.opentelemetry.javaagent.grails-3.0"; + } +} diff --git a/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/UrlMappingsInfoHandlerAdapterInstrumentation.java b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/UrlMappingsInfoHandlerAdapterInstrumentation.java new file mode 100644 index 000000000000..6bc2d72a5a8a --- /dev/null +++ b/instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/UrlMappingsInfoHandlerAdapterInstrumentation.java @@ -0,0 +1,59 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.grails; + +import static io.opentelemetry.javaagent.instrumentation.grails.GrailsTracer.tracer; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isPublic; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; + +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.tracer.ServerSpan; +import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; +import io.opentelemetry.javaagent.tooling.TypeInstrumentation; +import java.util.Map; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import org.grails.web.mapping.mvc.GrailsControllerUrlMappingInfo; + +public class UrlMappingsInfoHandlerAdapterInstrumentation implements TypeInstrumentation { + @Override + public ElementMatcher typeMatcher() { + return named("org.grails.web.mapping.mvc.UrlMappingsInfoHandlerAdapter"); + } + + @Override + public Map, String> transformers() { + return singletonMap( + isMethod() + .and(isPublic()) + .and(named("handle")) + .and(takesArgument(2, named(Object.class.getName()))) + .and(takesArguments(3)), + UrlMappingsInfoHandlerAdapterInstrumentation.class.getName() + "$ServerSpanNameAdvice"); + } + + public static class ServerSpanNameAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void nameSpan(@Advice.Argument(2) Object handler) { + + if (handler instanceof GrailsControllerUrlMappingInfo) { + Context parentContext = Java8BytecodeBridge.currentContext(); + Span serverSpan = ServerSpan.fromContextOrNull(parentContext); + if (serverSpan != null) { + tracer() + .nameServerSpan(parentContext, serverSpan, (GrailsControllerUrlMappingInfo) handler); + } + } + } + } +} diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/ErrorController.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/ErrorController.groovy new file mode 100644 index 000000000000..b196b0ba3881 --- /dev/null +++ b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/ErrorController.groovy @@ -0,0 +1,17 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package test + +import grails.artefact.Controller +import grails.web.Action + +class ErrorController implements Controller { + + @Action + def index() { + render "Error" + } +} diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy new file mode 100644 index 000000000000..91eb10664eba --- /dev/null +++ b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy @@ -0,0 +1,156 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package test + +import static io.opentelemetry.api.trace.SpanKind.INTERNAL +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT + +import grails.boot.GrailsApp +import grails.boot.config.GrailsAutoConfiguration +import groovy.transform.CompileStatic +import io.opentelemetry.instrumentation.test.AgentTestTrait +import io.opentelemetry.instrumentation.test.asserts.TraceAssert +import io.opentelemetry.instrumentation.test.base.HttpServerTest +import io.opentelemetry.sdk.trace.data.SpanData +import org.springframework.boot.autoconfigure.SpringBootApplication +import org.springframework.boot.autoconfigure.web.ServerProperties +import org.springframework.context.ConfigurableApplicationContext + +class GrailsTest extends HttpServerTest implements AgentTestTrait { + + @CompileStatic + @SpringBootApplication + static class TestApplication extends GrailsAutoConfiguration { + static ConfigurableApplicationContext start(int port, String contextPath) { + GrailsApp grailsApp = new GrailsApp(TestApplication) + // context path configuration property name changes between spring boot versions + def contextPathKey = "server.context-path" + try { + ServerProperties.getDeclaredMethod("getServlet") + contextPathKey = "server.servlet.contextPath" + } catch (NoSuchMethodException ignore) {} + Map properties = new HashMap<>() + properties.put("server.port", port) + properties.put(contextPathKey, contextPath) + grailsApp.setDefaultProperties(properties) + return grailsApp.run() + } + + @Override + Collection classes() { + return Arrays.asList(TestController, ErrorController, UrlMappings) + } + } + + @Override + String expectedServerSpanName(ServerEndpoint endpoint) { + if (endpoint == PATH_PARAM) { + return getContextPath() + "/test/path" + } else if (endpoint == QUERY_PARAM) { + return getContextPath() + "/test/query" + } else if (endpoint == ERROR || endpoint == EXCEPTION) { + return getContextPath() + "/error/index" + } else if (endpoint == NOT_FOUND) { + return getContextPath() + "/error" + } + return getContextPath() + "/test" + endpoint.path + } + + @Override + ConfigurableApplicationContext startServer(int port) { + return TestApplication.start(port, getContextPath()) + } + + @Override + void stopServer(ConfigurableApplicationContext ctx) { + ctx.close() + } + + @Override + String getContextPath() { + return "/xyz" + } + + @Override + boolean hasHandlerSpan() { + true + } + + @Override + boolean hasExceptionOnServerSpan() { + true + } + + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT || endpoint == ERROR || endpoint == NOT_FOUND + } + + @Override + boolean hasErrorPageSpans(ServerEndpoint endpoint) { + endpoint == ERROR || endpoint == EXCEPTION || endpoint == NOT_FOUND + } + + @Override + int getErrorPageSpansCount(ServerEndpoint endpoint) { + 2 + } + + @Override + boolean testPathParam() { + true + } + + @Override + void errorPageSpans(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint) { + forwardSpan(trace, index, trace.span(0)) + def errorSpanName = endpoint == NOT_FOUND ? "BasicErrorController.error" : "ErrorController.index" + trace.span(index + 1) { + name errorSpanName + kind INTERNAL + errored false + attributes { + } + } + } + + @Override + void responseSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint) { + trace.span(index) { + name endpoint == REDIRECT ? ~/\.sendRedirect$/ : ~/\.sendError$/ + kind INTERNAL + errored false + attributes { + } + } + } + + @Override + void handlerSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint) { + trace.span(index) { + if (endpoint == QUERY_PARAM) { + name "TestController.query" + } else if (endpoint == PATH_PARAM) { + name "TestController.path" + } else if (endpoint == NOT_FOUND) { + name "ResourceHttpRequestHandler.handleRequest" + } else { + name "TestController.${endpoint.name().toLowerCase()}" + } + kind INTERNAL + errored endpoint == EXCEPTION + if (endpoint == EXCEPTION) { + errorEvent(Exception, EXCEPTION.body) + } + childOf((SpanData) parent) + } + } +} diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/TestController.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/TestController.groovy new file mode 100644 index 000000000000..243bf4d7c421 --- /dev/null +++ b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/TestController.groovy @@ -0,0 +1,67 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package test + +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +import grails.artefact.Controller +import grails.web.Action +import io.opentelemetry.instrumentation.test.base.HttpServerTest + +class TestController implements Controller { + + @Action + def index() { + render "Hello World!" + } + + @Action + def success() { + HttpServerTest.controller(SUCCESS) { + render SUCCESS.body + } + } + + @Action + def query() { + HttpServerTest.controller(QUERY_PARAM) { + render "some=${params.some}" + } + } + + @Action + def redirect() { + HttpServerTest.controller(REDIRECT) { + response.sendRedirect(REDIRECT.body) + } + } + + @Action + def error() { + HttpServerTest.controller(ERROR) { + response.sendError(ERROR.status, ERROR.body) + } + } + + @Action + def exception() { + HttpServerTest.controller(EXCEPTION) { + throw new Exception(EXCEPTION.body) + } + } + + @Action + def path() { + HttpServerTest.controller(PATH_PARAM) { + render params.id + } + } +} diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/UrlMappings.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/UrlMappings.groovy new file mode 100644 index 000000000000..9b2c7ec95349 --- /dev/null +++ b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/UrlMappings.groovy @@ -0,0 +1,20 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package test + +class UrlMappings { + + static mappings = { + "/success"(controller: 'test', action: 'success') + "/query"(controller: 'test', action: 'query') + "/redirect"(controller: 'test', action: 'redirect') + "/error-status"(controller: 'test', action: 'error') + "/exception"(controller: 'test', action: 'exception') + "/path/$id/param"(controller: 'test', action: 'path') + + "500"(controller: 'error') + } +} diff --git a/instrumentation/grails-3.0/javaagent/src/test/resources/application.yml b/instrumentation/grails-3.0/javaagent/src/test/resources/application.yml new file mode 100644 index 000000000000..730dce4bbd12 --- /dev/null +++ b/instrumentation/grails-3.0/javaagent/src/test/resources/application.yml @@ -0,0 +1,8 @@ +--- +grails: + profile: web + env: production +spring: + groovy: + template: + check-template-location: false \ No newline at end of file diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java index 2baefc83ecef..c314b490b466 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java @@ -66,7 +66,9 @@ public static void nameResourceAndStartSpan( tracer().onRequest(parentContext, serverSpan, request); // Now create a span for handler/controller execution. context = tracer().startHandlerSpan(parentContext, handler); - scope = context.makeCurrent(); + if (context != null) { + scope = context.makeCurrent(); + } } } diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java index 7a7c53443e56..b2cadf485493 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java @@ -16,6 +16,7 @@ import java.lang.reflect.Method; import javax.servlet.Servlet; import javax.servlet.http.HttpServletRequest; +import org.checkerframework.checker.nullness.qual.Nullable; import org.springframework.web.HttpRequestHandler; import org.springframework.web.method.HandlerMethod; import org.springframework.web.servlet.HandlerMapping; @@ -36,8 +37,12 @@ public static SpringWebMvcTracer tracer() { return TRACER; } - public Context startHandlerSpan(Context parentContext, Object handler) { - return startSpan(parentContext, spanNameOnHandle(handler), INTERNAL); + public @Nullable Context startHandlerSpan(Context parentContext, Object handler) { + String spanName = spanNameOnHandle(handler); + if (spanName != null) { + return startSpan(parentContext, spanName, INTERNAL); + } + return null; } public Context startSpan(ModelAndView mv) { @@ -78,6 +83,9 @@ private String spanNameOnHandle(Object handler) { // org.springframework.web.servlet.handler.SimpleServletHandlerAdapter clazz = handler.getClass(); methodName = "service"; + } else if (handler.getClass().getName().startsWith("org.grails.")) { + // skip creating handler span for grails, grails instrumentation will take care of it + return null; } else { // perhaps org.springframework.web.servlet.mvc.annotation.AnnotationMethodHandlerAdapter clazz = handler.getClass(); diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy index 942e7e6aa0dd..ad9f8433eccc 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -6,10 +6,10 @@ package test.boot import static io.opentelemetry.api.trace.SpanKind.INTERNAL -import static io.opentelemetry.api.trace.SpanKind.SERVER import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.AUTH_ERROR import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.LOGIN +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -18,7 +18,6 @@ import io.opentelemetry.instrumentation.test.AgentTestTrait import io.opentelemetry.instrumentation.test.asserts.TraceAssert import io.opentelemetry.instrumentation.test.base.HttpServerTest import io.opentelemetry.sdk.trace.data.SpanData -import io.opentelemetry.semconv.trace.attributes.SemanticAttributes import okhttp3.FormBody import okhttp3.RequestBody import org.springframework.boot.SpringApplication @@ -55,20 +54,18 @@ class SpringBootBasedTest extends HttpServerTest } @Override - boolean hasRenderSpan(ServerEndpoint endpoint) { - endpoint == REDIRECT + boolean hasExceptionOnServerSpan() { + true } @Override - boolean hasResponseSpan(ServerEndpoint endpoint) { + boolean hasRenderSpan(ServerEndpoint endpoint) { endpoint == REDIRECT } @Override - boolean testNotFound() { - // FIXME: the instrumentation adds an extra controller span which is not consistent. - // Fix tests or remove extra span. - false + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT || endpoint == NOT_FOUND } @Override @@ -76,6 +73,26 @@ class SpringBootBasedTest extends HttpServerTest true } + boolean hasErrorPageSpans(ServerEndpoint endpoint) { + endpoint == NOT_FOUND + } + + int getErrorPageSpansCount(ServerEndpoint endpoint) { + 2 + } + + @Override + String expectedServerSpanName(ServerEndpoint endpoint) { + if (endpoint == PATH_PARAM) { + return getContextPath() + "/path/{id}/param" + } else if (endpoint == AUTH_ERROR || endpoint == NOT_FOUND) { + return getContextPath() + "/error" + } else if (endpoint == LOGIN) { + return "HTTP POST" + } + return super.expectedServerSpanName(endpoint) + } + def "test spans with auth error"() { setup: def authProvider = server.getBean(SavingAuthenticationProvider) @@ -131,6 +148,10 @@ class SpringBootBasedTest extends HttpServerTest @Override void errorPageSpans(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + if (endpoint == NOT_FOUND) { + forwardSpan(trace, index, trace.span(0)) + index++ + } trace.span(index) { name "BasicErrorController.error" kind INTERNAL @@ -142,8 +163,9 @@ class SpringBootBasedTest extends HttpServerTest @Override void responseSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + def responseSpanName = endpoint == NOT_FOUND ? "OnCommittedResponseWrapper.sendError" : "OnCommittedResponseWrapper.sendRedirect" trace.span(index) { - name "OnCommittedResponseWrapper.sendRedirect" + name responseSpanName kind INTERNAL errored false attributes { @@ -165,8 +187,12 @@ class SpringBootBasedTest extends HttpServerTest @Override void handlerSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + def handlerSpanName = "TestController.${endpoint.name().toLowerCase()}" + if (endpoint == NOT_FOUND) { + handlerSpanName = "ResourceHttpRequestHandler.handleRequest" + } trace.span(index) { - name "TestController.${endpoint.name().toLowerCase()}" + name handlerSpanName kind INTERNAL errored endpoint == EXCEPTION if (endpoint == EXCEPTION) { @@ -175,43 +201,4 @@ class SpringBootBasedTest extends HttpServerTest childOf((SpanData) parent) } } - - // this override is needed because the exception is propagated up from the handler span - // to the server span, which is different from the the expectation of the super method - @Override - void serverSpan(TraceAssert trace, int index, String traceID = null, String parentID = null, String method = "GET", Long responseContentLength = null, ServerEndpoint endpoint = SUCCESS) { - - trace.span(index) { - if (endpoint == PATH_PARAM) { - name getContextPath() + "/path/{id}/param" - } else if (endpoint == AUTH_ERROR) { - name getContextPath() + "/error" - } else if (endpoint == LOGIN) { - name 'HTTP POST' - } else { - name endpoint.resolvePath(address).path - } - kind SERVER - errored endpoint.errored - if (parentID != null) { - traceId traceID - parentSpanId parentID - } else { - hasNoParent() - } - if (endpoint == EXCEPTION) { - errorEvent(Exception, EXCEPTION.body) - } - attributes { - "${SemanticAttributes.NET_PEER_IP.key}" "127.0.0.1" - "${SemanticAttributes.NET_PEER_PORT.key}" Long - "${SemanticAttributes.HTTP_URL.key}" { it == "${endpoint.resolve(address)}" || it == "${endpoint.resolveWithoutFragment(address)}" } - "${SemanticAttributes.HTTP_METHOD.key}" method - "${SemanticAttributes.HTTP_STATUS_CODE.key}" endpoint.status - "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1" - "${SemanticAttributes.HTTP_USER_AGENT.key}" TEST_USER_AGENT - "${SemanticAttributes.HTTP_CLIENT_IP.key}" TEST_CLIENT_IP - } - } - } } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/AdditionalLibraryIgnoresMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/AdditionalLibraryIgnoresMatcher.java index 8c15b0b0866a..2c2fe4de2eb4 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/matcher/AdditionalLibraryIgnoresMatcher.java @@ -107,6 +107,7 @@ public boolean matches(T target) { if (name.startsWith("org.springframework.boot.")) { return !instrumentedSpringBootClasses(name) + && !name.startsWith("org.springframework.boot.context.web.") && !name.startsWith("org.springframework.boot.web.filter.") && !name.startsWith("org.springframework.boot.web.servlet."); } @@ -338,6 +339,7 @@ public boolean matches(T target) { instrumented.add( "org.springframework.boot.web.embedded.tomcat.TomcatEmbeddedWebappClassLoader"); instrumented.add("org.springframework.boot.web.servlet.DelegatingFilterProxyRegistrationBean$"); + instrumented.add("org.springframework.boot.StartupInfoLogger$"); INSTRUMENTED_SPRING_BOOT_CLASSES = Collections.unmodifiableSet(instrumented); } diff --git a/settings.gradle b/settings.gradle index dd9c0527b24b..c51f026ebb8c 100644 --- a/settings.gradle +++ b/settings.gradle @@ -106,6 +106,7 @@ include ':instrumentation:external-annotations:javaagent-unittests' include ':instrumentation:finatra-2.9:javaagent' include ':instrumentation:geode-1.4:javaagent' include ':instrumentation:google-http-client-1.19:javaagent' +include ':instrumentation:grails-3.0:javaagent' include ':instrumentation:grizzly-2.0:javaagent' include ':instrumentation:grpc-1.5:javaagent' include ':instrumentation:grpc-1.5:library' diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy index a19d896add7a..8b75c381ccc4 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy @@ -43,6 +43,10 @@ abstract class HttpServerTest extends InstrumentationSpecification imple false } + boolean hasExceptionOnServerSpan() { + !hasHandlerSpan() + } + boolean hasRenderSpan(ServerEndpoint endpoint) { false } @@ -355,9 +359,9 @@ abstract class HttpServerTest extends InstrumentationSpecification imple if (hasIncludeSpan()) { spanCount++ } - if (hasErrorPageSpans(endpoint)) { - spanCount += getErrorPageSpansCount(endpoint) - } + } + if (hasErrorPageSpans(endpoint)) { + spanCount += getErrorPageSpansCount(endpoint) } assertTraces(size) { (0..size - 1).each { @@ -384,15 +388,13 @@ abstract class HttpServerTest extends InstrumentationSpecification imple if (hasRenderSpan(endpoint)) { renderSpan(it, spanIndex++, span(0), method, endpoint) } - if (hasResponseSpan(endpoint)) { - responseSpan(it, spanIndex, span(spanIndex - 1), span(0), method, endpoint) - spanIndex++ - } - if (hasErrorPageSpans(endpoint)) { - errorPageSpans(it, spanIndex, span(0), method, endpoint) - } - } else if (hasResponseSpan(endpoint)) { - responseSpan(it, 1, span(0), span(0), method, endpoint) + } + if (hasResponseSpan(endpoint)) { + responseSpan(it, spanIndex, span(spanIndex - 1), span(0), method, endpoint) + spanIndex++ + } + if (hasErrorPageSpans(endpoint)) { + errorPageSpans(it, spanIndex, span(0), method, endpoint) } } } @@ -482,7 +484,7 @@ abstract class HttpServerTest extends InstrumentationSpecification imple } else { hasNoParent() } - if (endpoint == EXCEPTION && !hasHandlerSpan()) { + if (endpoint == EXCEPTION && hasExceptionOnServerSpan()) { event(0) { eventName(SemanticAttributes.EXCEPTION_EVENT_NAME) attributes {