From a2b86e6b961084e455b8d99a1e44076c057ff028 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Mon, 16 Dec 2019 08:40:58 -0800 Subject: [PATCH 01/22] Pin kafka latestDepTest version to 2.3.x 2.4.0 introduces test failures: ``` Caused by: java.lang.NoClassDefFoundError: org.I0Itec.zkclient.ZkClient ``` when executing `compileLatestDepTestGroovy`. --- .../kafka-clients-0.11/kafka-clients-0.11.gradle | 6 ++++-- .../kafka-streams-0.11/kafka-streams-0.11.gradle | 8 +++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/instrumentation/kafka-clients-0.11/kafka-clients-0.11.gradle b/dd-java-agent/instrumentation/kafka-clients-0.11/kafka-clients-0.11.gradle index 0da1d632d6..d356f9b135 100644 --- a/dd-java-agent/instrumentation/kafka-clients-0.11/kafka-clients-0.11.gradle +++ b/dd-java-agent/instrumentation/kafka-clients-0.11/kafka-clients-0.11.gradle @@ -29,8 +29,10 @@ dependencies { // Include latest version of kafka itself along with latest version of client libs. // This seems to help with jar compatibility hell. - latestDepTestCompile group: 'org.apache.kafka', name: 'kafka_2.11', version: '+' - latestDepTestCompile group: 'org.apache.kafka', name: 'kafka-clients', version: '+' + latestDepTestCompile group: 'org.apache.kafka', name: 'kafka_2.11', version: '2.3.+' + // (Pinning to 2.3.x: 2.4.0 introduces an error when executing compileLatestDepTestGroovy) + // Caused by: java.lang.NoClassDefFoundError: org.I0Itec.zkclient.ZkClient + latestDepTestCompile group: 'org.apache.kafka', name: 'kafka-clients', version: '2.3.+' latestDepTestCompile group: 'org.springframework.kafka', name: 'spring-kafka', version: '2.2.+' latestDepTestCompile group: 'org.springframework.kafka', name: 'spring-kafka-test', version: '2.2.+' latestDepTestCompile group: 'org.assertj', name: 'assertj-core', version: '3.+' diff --git a/dd-java-agent/instrumentation/kafka-streams-0.11/kafka-streams-0.11.gradle b/dd-java-agent/instrumentation/kafka-streams-0.11/kafka-streams-0.11.gradle index f032e2a947..98fb5f17ee 100644 --- a/dd-java-agent/instrumentation/kafka-streams-0.11/kafka-streams-0.11.gradle +++ b/dd-java-agent/instrumentation/kafka-streams-0.11/kafka-streams-0.11.gradle @@ -33,9 +33,11 @@ dependencies { // Include latest version of kafka itself along with latest version of client libs. // This seems to help with jar compatibility hell. - latestDepTestCompile group: 'org.apache.kafka', name: 'kafka_2.11', version: '+' - latestDepTestCompile group: 'org.apache.kafka', name: 'kafka-clients', version: '+' - latestDepTestCompile group: 'org.apache.kafka', name: 'kafka-streams', version: '+' + latestDepTestCompile group: 'org.apache.kafka', name: 'kafka_2.11', version: '2.3.+' + // (Pinning to 2.3.x: 2.4.0 introduces an error when executing compileLatestDepTestGroovy) + // Caused by: java.lang.NoClassDefFoundError: org.I0Itec.zkclient.ZkClient + latestDepTestCompile group: 'org.apache.kafka', name: 'kafka-clients', version: '2.3.+' + latestDepTestCompile group: 'org.apache.kafka', name: 'kafka-streams', version: '2.3.+' latestDepTestCompile group: 'org.springframework.kafka', name: 'spring-kafka', version: '2.2.+' latestDepTestCompile group: 'org.springframework.kafka', name: 'spring-kafka-test', version: '2.2.+' latestDepTestCompile group: 'org.assertj', name: 'assertj-core', version: '3.+' From 99992df99849abc40796c18ca610a50cd2395577 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 13 Dec 2019 14:14:25 -0800 Subject: [PATCH 02/22] Fix tagging query string for additional servers Add test to common test suite and adapt each test. --- .../AkkaHttpServerInstrumentationTest.groovy | 4 + .../scala/AkkaHttpTestAsyncWebServer.scala | 1 + .../scala/AkkaHttpTestSyncWebServer.scala | 1 + .../test/scala/AkkaHttpTestWebServer.scala | 2 + .../test/groovy/DropwizardAsyncTest.groovy | 10 ++ .../src/test/groovy/DropwizardTest.groovy | 17 ++- .../test/groovy/GlassFishServerTest.groovy | 4 + .../src/test/groovy/TestServlets.java | 19 ++++ .../grizzly/GrizzlyDecorator.java | 9 +- .../src/test/groovy/GrizzlyAsyncTest.groovy | 20 +++- .../src/test/groovy/GrizzlyTest.groovy | 21 +++- .../jetty8/JettyDecorator.java | 9 +- .../src/test/groovy/JettyHandlerTest.groovy | 18 ++- .../src/test/groovy/Netty40ServerTest.groovy | 8 +- .../src/test/groovy/Netty41ServerTest.groovy | 8 +- .../groovy/server/PlayAsyncServerTest.groovy | 8 ++ .../test/groovy/server/PlayServerTest.groovy | 10 ++ .../groovy/server/PlayAsyncServerTest.groovy | 8 ++ .../test/groovy/server/PlayServerTest.groovy | 13 +++ .../ratpack/RatpackServerDecorator.java | 5 +- .../server/RatpackAsyncHttpServerTest.groovy | 12 ++ .../server/RatpackForkedHttpServerTest.groovy | 12 ++ .../server/RatpackHttpServerTest.groovy | 12 ++ .../src/test/groovy/JettyServlet2Test.groovy | 11 +- .../src/test/groovy/TestServlet2.groovy | 6 +- .../test/groovy/AbstractServlet3Test.groovy | 9 +- .../src/test/groovy/JettyServlet3Test.groovy | 104 ++++++++++-------- .../src/test/groovy/TestServlet3.groovy | 27 ++++- .../src/test/groovy/TomcatServlet3Test.groovy | 102 +++++++++-------- .../SpringWebHttpServerDecorator.java | 9 +- .../groovy/test/SpringBootBasedTest.groovy | 4 + .../test/groovy/test/TestController.groovy | 10 ++ .../groovy/server/VertxHttpServerTest.groovy | 6 + ...VertxRxCircuitBreakerHttpServerTest.groovy | 14 +++ .../server/VertxRxHttpServerTest.groovy | 6 + .../agent/test/base/HttpServerTest.groovy | 66 +++++++++-- 36 files changed, 472 insertions(+), 133 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy index fb7c250e5b..d88e5bde18 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy @@ -1,6 +1,7 @@ import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags import datadog.trace.instrumentation.akkahttp.AkkaHttpServerDecorator import datadog.trace.instrumentation.api.Tags @@ -60,6 +61,9 @@ abstract class AkkaHttpServerInstrumentationTest extends HttpServerTest resp.withEntity(endpoint.getBody) + case QUERY_PARAM => resp.withEntity(uri.queryString().orNull) case REDIRECT => resp.withHeaders(headers.Location(endpoint.getBody)) case ERROR => resp.withEntity(endpoint.getBody) case EXCEPTION => throw new Exception(endpoint.getBody) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/test/scala/AkkaHttpTestSyncWebServer.scala b/dd-java-agent/instrumentation/akka-http-10.0/src/test/scala/AkkaHttpTestSyncWebServer.scala index 11db37268e..fb60cf6d97 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/test/scala/AkkaHttpTestSyncWebServer.scala +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/test/scala/AkkaHttpTestSyncWebServer.scala @@ -23,6 +23,7 @@ object AkkaHttpTestSyncWebServer { val resp = HttpResponse(status = endpoint.getStatus) endpoint match { case SUCCESS => resp.withEntity(endpoint.getBody) + case QUERY_PARAM => resp.withEntity(uri.queryString().orNull) case REDIRECT => resp.withHeaders(headers.Location(endpoint.getBody)) case ERROR => resp.withEntity(endpoint.getBody) case EXCEPTION => throw new Exception(endpoint.getBody) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/test/scala/AkkaHttpTestWebServer.scala b/dd-java-agent/instrumentation/akka-http-10.0/src/test/scala/AkkaHttpTestWebServer.scala index caeb2379cc..6afb6fd788 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/test/scala/AkkaHttpTestWebServer.scala +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/test/scala/AkkaHttpTestWebServer.scala @@ -23,6 +23,8 @@ object AkkaHttpTestWebServer { val route = { //handleExceptions(exceptionHandler) { path(SUCCESS.rawPath) { complete(HttpResponse(status = SUCCESS.getStatus).withEntity(SUCCESS.getBody)) + } ~ path(QUERY_PARAM.rawPath) { + complete(HttpResponse(status = QUERY_PARAM.getStatus).withEntity(SUCCESS.getBody)) } ~ path(REDIRECT.rawPath) { redirect(Uri(REDIRECT.getBody), StatusCodes.Found) } ~ path(ERROR.rawPath) { diff --git a/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardAsyncTest.groovy b/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardAsyncTest.groovy index 29261d0f62..f2d7e3b192 100644 --- a/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardAsyncTest.groovy +++ b/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardAsyncTest.groovy @@ -4,6 +4,7 @@ import io.dropwizard.setup.Bootstrap import io.dropwizard.setup.Environment import javax.ws.rs.GET import javax.ws.rs.Path +import javax.ws.rs.QueryParam import javax.ws.rs.container.AsyncResponse import javax.ws.rs.container.Suspended import javax.ws.rs.core.Response @@ -12,6 +13,7 @@ import java.util.concurrent.Executors 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -56,6 +58,14 @@ class DropwizardAsyncTest extends DropwizardTest { } } + @GET + @Path("query") + Response query_param(@QueryParam("some") String param) { + controller(QUERY_PARAM) { + Response.status(QUERY_PARAM.status).entity("some=$param".toString()).build() + } + } + @GET @Path("redirect") void redirect(@Suspended final AsyncResponse asyncResponse) { diff --git a/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardTest.groovy b/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardTest.groovy index 8d591db596..d6b762f773 100644 --- a/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardTest.groovy +++ b/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardTest.groovy @@ -2,6 +2,7 @@ import datadog.opentracing.DDSpan import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags import datadog.trace.instrumentation.api.Tags import datadog.trace.instrumentation.jaxrs2.JaxRsAnnotationsDecorator import datadog.trace.instrumentation.servlet3.Servlet3Decorator @@ -11,14 +12,15 @@ import io.dropwizard.setup.Bootstrap import io.dropwizard.setup.Environment import io.dropwizard.testing.ConfigOverride import io.dropwizard.testing.DropwizardTestSupport -import org.eclipse.jetty.servlet.ServletHandler - import javax.ws.rs.GET import javax.ws.rs.Path +import javax.ws.rs.QueryParam import javax.ws.rs.core.Response +import org.eclipse.jetty.servlet.ServletHandler 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -124,6 +126,9 @@ class DropwizardTest extends HttpServerTest { "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } defaultTags(true) } } diff --git a/dd-java-agent/instrumentation/glassfish/src/test/groovy/TestServlets.java b/dd-java-agent/instrumentation/glassfish/src/test/groovy/TestServlets.java index 74cf401108..5b2ef3904d 100644 --- a/dd-java-agent/instrumentation/glassfish/src/test/groovy/TestServlets.java +++ b/dd-java-agent/instrumentation/glassfish/src/test/groovy/TestServlets.java @@ -26,6 +26,25 @@ public class TestServlets { } } + @WebServlet("/query") + public static class Query extends HttpServlet { + @Override + protected void service(final HttpServletRequest req, final HttpServletResponse resp) { + final HttpServerTest.ServerEndpoint endpoint = + HttpServerTest.ServerEndpoint.forPath(req.getServletPath()); + HttpServerTest.controller( + endpoint, + new Closure(null) { + public Object doCall() throws Exception { + resp.setContentType("text/plain"); + resp.setStatus(endpoint.getStatus()); + resp.getWriter().print(req.getQueryString()); + return null; + } + }); + } + } + @WebServlet("/redirect") public static class Redirect extends HttpServlet { @Override diff --git a/dd-java-agent/instrumentation/grizzly-2/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyDecorator.java b/dd-java-agent/instrumentation/grizzly-2/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyDecorator.java index 3c6a601d72..e5f6cca8dd 100644 --- a/dd-java-agent/instrumentation/grizzly-2/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyDecorator.java +++ b/dd-java-agent/instrumentation/grizzly-2/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyDecorator.java @@ -16,7 +16,14 @@ public class GrizzlyDecorator extends HttpServerDecorator { } } + @GET + @Path("query") + Response query_param(@QueryParam("some") String param) { + controller(QUERY_PARAM) { + Response.status(QUERY_PARAM.status).entity("some=$param".toString()).build() + } + } + @GET @Path("redirect") Response redirect() { diff --git a/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/JettyDecorator.java b/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/JettyDecorator.java index 16101b2fbb..7eaa7bb48a 100644 --- a/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/JettyDecorator.java +++ b/dd-java-agent/instrumentation/jetty-8/src/main/java/datadog/trace/instrumentation/jetty8/JettyDecorator.java @@ -28,7 +28,14 @@ public class JettyDecorator @Override protected URI url(final HttpServletRequest httpServletRequest) throws URISyntaxException { - return new URI(httpServletRequest.getRequestURL().toString()); + return new URI( + httpServletRequest.getScheme(), + null, + httpServletRequest.getServerName(), + httpServletRequest.getServerPort(), + httpServletRequest.getRequestURI(), + httpServletRequest.getQueryString(), + null); } @Override diff --git a/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy b/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy index ecdc0fc833..ef844036af 100644 --- a/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy +++ b/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy @@ -1,21 +1,22 @@ import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags import datadog.trace.instrumentation.api.Tags import datadog.trace.instrumentation.jetty8.JettyDecorator +import javax.servlet.DispatcherType +import javax.servlet.ServletException +import javax.servlet.http.HttpServletRequest +import javax.servlet.http.HttpServletResponse import org.eclipse.jetty.server.Request import org.eclipse.jetty.server.Server import org.eclipse.jetty.server.handler.AbstractHandler import org.eclipse.jetty.server.handler.ErrorHandler -import javax.servlet.DispatcherType -import javax.servlet.ServletException -import javax.servlet.http.HttpServletRequest -import javax.servlet.http.HttpServletResponse - 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_FOUND +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -78,6 +79,10 @@ class JettyHandlerTest extends HttpServerTest { response.status = endpoint.status response.writer.print(endpoint.body) break + case QUERY_PARAM: + response.status = endpoint.status + response.writer.print(request.queryString) + break case REDIRECT: response.sendRedirect(endpoint.body) break @@ -139,6 +144,9 @@ class JettyHandlerTest extends HttpServerTest { "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } defaultTags(true) } } diff --git a/dd-java-agent/instrumentation/netty-4.0/src/test/groovy/Netty40ServerTest.groovy b/dd-java-agent/instrumentation/netty-4.0/src/test/groovy/Netty40ServerTest.groovy index 601d07580f..bd2b041204 100644 --- a/dd-java-agent/instrumentation/netty-4.0/src/test/groovy/Netty40ServerTest.groovy +++ b/dd-java-agent/instrumentation/netty-4.0/src/test/groovy/Netty40ServerTest.groovy @@ -24,6 +24,7 @@ import io.netty.util.CharsetUtil 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_FOUND +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS import static io.netty.handler.codec.http.HttpHeaders.Names.CONTENT_LENGTH @@ -48,7 +49,8 @@ class Netty40ServerTest extends HttpServerTest if (msg instanceof HttpRequest) { - ServerEndpoint endpoint = ServerEndpoint.forPath((msg as HttpRequest).uri) + def uri = URI.create((msg as HttpRequest).uri) + ServerEndpoint endpoint = ServerEndpoint.forPath(uri.path) ctx.write controller(endpoint) { ByteBuf content = null FullHttpResponse response = null @@ -58,6 +60,10 @@ class Netty40ServerTest extends HttpServerTest if (msg instanceof HttpRequest) { - ServerEndpoint endpoint = ServerEndpoint.forPath((msg as HttpRequest).uri) + def uri = URI.create((msg as HttpRequest).uri) + ServerEndpoint endpoint = ServerEndpoint.forPath(uri.path) ctx.write controller(endpoint) { ByteBuf content = null FullHttpResponse response = null @@ -57,6 +59,10 @@ class Netty41ServerTest extends HttpServerTest { controller(SUCCESS) { Results.status(SUCCESS.getStatus(), SUCCESS.getBody()) } + } as Supplier) + .GET(QUERY_PARAM.getPath()).routeTo({ + controller(QUERY_PARAM) { + Results.status(QUERY_PARAM.getStatus(), QUERY_PARAM.getBody()) + } } as Supplier) .GET(REDIRECT.getPath()).routeTo({ controller(REDIRECT) { @@ -98,6 +105,9 @@ class PlayServerTest extends HttpServerTest { } else if (endpoint == EXCEPTION) { errorTags(Exception, EXCEPTION.body) } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } defaultTags() } } diff --git a/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayAsyncServerTest.groovy b/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayAsyncServerTest.groovy index 1695438450..90f5f83ac3 100644 --- a/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayAsyncServerTest.groovy +++ b/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayAsyncServerTest.groovy @@ -14,6 +14,7 @@ import java.util.function.Supplier 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -36,6 +37,13 @@ class PlayAsyncServerTest extends PlayServerTest { Results.status(SUCCESS.getStatus(), SUCCESS.getBody()) } }, execContext) + } as Supplier) + .GET(QUERY_PARAM.getPath()).routeAsync({ + CompletableFuture.supplyAsync({ + controller(QUERY_PARAM) { + Results.status(QUERY_PARAM.getStatus(), QUERY_PARAM.getBody()) + } + }, execContext) } as Supplier) .GET(REDIRECT.getPath()).routeAsync({ CompletableFuture.supplyAsync({ diff --git a/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayServerTest.groovy index 7e2f4f7350..11daedd7f1 100644 --- a/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayServerTest.groovy +++ b/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayServerTest.groovy @@ -4,6 +4,7 @@ import datadog.opentracing.DDSpan import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags import datadog.trace.instrumentation.akkahttp.AkkaHttpServerDecorator import datadog.trace.instrumentation.api.Tags import datadog.trace.instrumentation.play26.PlayHttpServerDecorator @@ -17,6 +18,7 @@ import java.util.function.Supplier 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -29,6 +31,11 @@ class PlayServerTest extends HttpServerTest { controller(SUCCESS) { Results.status(SUCCESS.getStatus(), SUCCESS.getBody()) } + } as Supplier) + .GET(QUERY_PARAM.getPath()).routeTo({ + controller(QUERY_PARAM) { + Results.status(QUERY_PARAM.getStatus(), QUERY_PARAM.getBody()) + } } as Supplier) .GET(REDIRECT.getPath()).routeTo({ controller(REDIRECT) { @@ -100,6 +107,9 @@ class PlayServerTest extends HttpServerTest { } else if (endpoint == EXCEPTION) { errorTags(Exception, EXCEPTION.body) } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } defaultTags() } } @@ -130,6 +140,9 @@ class PlayServerTest extends HttpServerTest { "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } defaultTags(true) } } diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/RatpackServerDecorator.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/RatpackServerDecorator.java index f72d002c8d..6216cd2819 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/RatpackServerDecorator.java +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java8/datadog/trace/instrumentation/ratpack/RatpackServerDecorator.java @@ -7,6 +7,7 @@ import datadog.trace.instrumentation.api.AgentSpan; import java.net.URI; import lombok.extern.slf4j.Slf4j; import ratpack.handling.Context; +import ratpack.http.HttpUrlBuilder; import ratpack.http.Request; import ratpack.http.Response; import ratpack.http.Status; @@ -42,7 +43,9 @@ public class RatpackServerDecorator extends HttpServerDecorator + controller(endpoint) { + context.response.status(endpoint.status).send(request.query) + } + } + } + } prefix(REDIRECT.rawPath()) { all { Promise.sync { diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackForkedHttpServerTest.groovy b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackForkedHttpServerTest.groovy index 39b1547535..255a7ab286 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackForkedHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackForkedHttpServerTest.groovy @@ -6,6 +6,7 @@ import ratpack.test.embed.EmbeddedApp 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -32,6 +33,17 @@ class RatpackForkedHttpServerTest extends RatpackHttpServerTest { } } } + prefix(QUERY_PARAM.rawPath()) { + all { + Promise.sync { + QUERY_PARAM + }.fork().then { ServerEndpoint endpoint -> + controller(endpoint) { + context.response.status(endpoint.status).send(request.query) + } + } + } + } prefix(REDIRECT.rawPath()) { all { Promise.sync { diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy index 5b59647d6b..e03df69987 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy @@ -4,6 +4,7 @@ import datadog.opentracing.DDSpan import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags import datadog.trace.instrumentation.api.Tags import datadog.trace.instrumentation.netty41.server.NettyHttpServerDecorator import datadog.trace.instrumentation.ratpack.RatpackServerDecorator @@ -14,6 +15,7 @@ import ratpack.test.embed.EmbeddedApp 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -36,6 +38,13 @@ class RatpackHttpServerTest extends HttpServerTest { // servletContext.setSecurityHandler(security) servletContext.addServlet(TestServlet2.Sync, SUCCESS.path) + servletContext.addServlet(TestServlet2.Sync, QUERY_PARAM.path) + servletContext.addServlet(TestServlet2.Sync, REDIRECT.path) servletContext.addServlet(TestServlet2.Sync, ERROR.path) servletContext.addServlet(TestServlet2.Sync, EXCEPTION.path) - servletContext.addServlet(TestServlet2.Sync, REDIRECT.path) servletContext.addServlet(TestServlet2.Sync, AUTH_REQUIRED.path) jettyServer.setHandler(servletContext) @@ -110,6 +112,9 @@ class JettyServlet2Test extends HttpServerTest { "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } defaultTags(true) } } diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/TestServlet2.groovy b/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/TestServlet2.groovy index ce0ec922f6..124c711cd8 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/TestServlet2.groovy +++ b/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/TestServlet2.groovy @@ -1,11 +1,11 @@ import datadog.trace.agent.test.base.HttpServerTest import groovy.servlet.AbstractHttpServlet - import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -23,6 +23,10 @@ class TestServlet2 { resp.status = endpoint.status resp.writer.print(endpoint.body) break + case QUERY_PARAM: + resp.status = endpoint.status + resp.writer.print(req.queryString) + break case REDIRECT: resp.sendRedirect(endpoint.body) break diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/AbstractServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/AbstractServlet3Test.groovy index b5af4875ff..d63bfc248d 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/AbstractServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/AbstractServlet3Test.groovy @@ -1,16 +1,17 @@ import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags import datadog.trace.instrumentation.api.Tags import datadog.trace.instrumentation.servlet3.Servlet3Decorator +import javax.servlet.Servlet import okhttp3.Request import org.apache.catalina.core.ApplicationFilterChain -import javax.servlet.Servlet - import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -53,6 +54,7 @@ abstract class AbstractServlet3Test extends HttpServerTest extends HttpServerTest servlet() { - TestServlet3.Sync // dispatch to sync servlet - } +// FIXME: not working right now... +//class JettyServlet3TestForward extends JettyDispatchTest { +// @Override +// Class servlet() { +// TestServlet3.Sync // dispatch to sync servlet +// } +// +// @Override +// boolean testNotFound() { +// false +// } +// +// @Override +// protected void setupServlets(ServletContextHandler context) { +// super.setupServlets(context) +// +// addServlet(context, "/dispatch" + SUCCESS.path, RequestDispatcherServlet.Forward) +// addServlet(context, "/dispatch" + QUERY_PARAM.path, RequestDispatcherServlet.Forward) +// addServlet(context, "/dispatch" + REDIRECT.path, RequestDispatcherServlet.Forward) +// addServlet(context, "/dispatch" + ERROR.path, RequestDispatcherServlet.Forward) +// addServlet(context, "/dispatch" + EXCEPTION.path, RequestDispatcherServlet.Forward) +// addServlet(context, "/dispatch" + AUTH_REQUIRED.path, RequestDispatcherServlet.Forward) +// } +//} - @Override - boolean testNotFound() { - false - } - - @Override - protected void setupServlets(ServletContextHandler context) { - super.setupServlets(context) - - addServlet(context, "/dispatch" + SUCCESS.path, RequestDispatcherServlet.Forward) - addServlet(context, "/dispatch" + ERROR.path, RequestDispatcherServlet.Forward) - addServlet(context, "/dispatch" + EXCEPTION.path, RequestDispatcherServlet.Forward) - addServlet(context, "/dispatch" + REDIRECT.path, RequestDispatcherServlet.Forward) - addServlet(context, "/dispatch" + AUTH_REQUIRED.path, RequestDispatcherServlet.Forward) - } -} - -class JettyServlet3TestInclude extends JettyServlet3Test { - @Override - Class servlet() { - TestServlet3.Sync // dispatch to sync servlet - } - - @Override - boolean testNotFound() { - false - } - - @Override - protected void setupServlets(ServletContextHandler context) { - super.setupServlets(context) - - addServlet(context, "/dispatch" + SUCCESS.path, RequestDispatcherServlet.Include) - addServlet(context, "/dispatch" + ERROR.path, RequestDispatcherServlet.Include) - addServlet(context, "/dispatch" + EXCEPTION.path, RequestDispatcherServlet.Include) - addServlet(context, "/dispatch" + REDIRECT.path, RequestDispatcherServlet.Include) - addServlet(context, "/dispatch" + AUTH_REQUIRED.path, RequestDispatcherServlet.Include) - } -} +// FIXME: not working right now... +//class JettyServlet3TestInclude extends JettyDispatchTest { +// @Override +// Class servlet() { +// TestServlet3.Sync // dispatch to sync servlet +// } +// +// @Override +// boolean testNotFound() { +// false +// } +// +// @Override +// protected void setupServlets(ServletContextHandler context) { +// super.setupServlets(context) +// +// addServlet(context, "/dispatch" + SUCCESS.path, RequestDispatcherServlet.Include) +// addServlet(context, "/dispatch" + QUERY_PARAM.path, RequestDispatcherServlet.Include) +// addServlet(context, "/dispatch" + REDIRECT.path, RequestDispatcherServlet.Include) +// addServlet(context, "/dispatch" + ERROR.path, RequestDispatcherServlet.Include) +// addServlet(context, "/dispatch" + EXCEPTION.path, RequestDispatcherServlet.Include) +// addServlet(context, "/dispatch" + AUTH_REQUIRED.path, RequestDispatcherServlet.Include) +// } +//} class JettyServlet3TestDispatchImmediate extends JettyDispatchTest { @@ -174,6 +179,7 @@ class JettyServlet3TestDispatchImmediate extends JettyDispatchTest { super.setupServlets(context) addServlet(context, "/dispatch" + SUCCESS.path, TestServlet3.DispatchImmediate) + addServlet(context, "/dispatch" + QUERY_PARAM.path, TestServlet3.DispatchImmediate) addServlet(context, "/dispatch" + ERROR.path, TestServlet3.DispatchImmediate) addServlet(context, "/dispatch" + EXCEPTION.path, TestServlet3.DispatchImmediate) addServlet(context, "/dispatch" + REDIRECT.path, TestServlet3.DispatchImmediate) @@ -193,6 +199,7 @@ class JettyServlet3TestDispatchAsync extends JettyDispatchTest { super.setupServlets(context) addServlet(context, "/dispatch" + SUCCESS.path, TestServlet3.DispatchAsync) + addServlet(context, "/dispatch" + QUERY_PARAM.path, TestServlet3.DispatchAsync) addServlet(context, "/dispatch" + ERROR.path, TestServlet3.DispatchAsync) addServlet(context, "/dispatch" + EXCEPTION.path, TestServlet3.DispatchAsync) addServlet(context, "/dispatch" + REDIRECT.path, TestServlet3.DispatchAsync) @@ -264,6 +271,9 @@ abstract class JettyDispatchTest extends JettyServlet3Test { "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } defaultTags(true) } } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TestServlet3.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TestServlet3.groovy index d5178a47f8..24844321de 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TestServlet3.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TestServlet3.groovy @@ -1,13 +1,14 @@ import datadog.trace.agent.test.base.HttpServerTest import groovy.servlet.AbstractHttpServlet - import javax.servlet.annotation.WebServlet import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse + import java.util.concurrent.Phaser 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -25,6 +26,10 @@ class TestServlet3 { resp.status = endpoint.status resp.writer.print(endpoint.body) break + case QUERY_PARAM: + resp.status = endpoint.status + resp.writer.print(req.queryString) + break case REDIRECT: resp.sendRedirect(endpoint.body) break @@ -52,15 +57,25 @@ class TestServlet3 { resp.contentType = "text/plain" switch (endpoint) { case SUCCESS: - case ERROR: resp.status = endpoint.status resp.writer.print(endpoint.body) context.complete() break + case QUERY_PARAM: + resp.status = endpoint.status + resp.writer.print(req.queryString) + context.complete() + break case REDIRECT: resp.sendRedirect(endpoint.body) context.complete() break + case ERROR: + resp.status = endpoint.status + resp.writer.print(endpoint.body) +// resp.sendError(endpoint.status, endpoint.body) + context.complete() + break case EXCEPTION: resp.status = endpoint.status resp.writer.print(endpoint.body) @@ -88,13 +103,19 @@ class TestServlet3 { resp.contentType = "text/plain" switch (endpoint) { case SUCCESS: - case ERROR: resp.status = endpoint.status resp.writer.print(endpoint.body) break + case QUERY_PARAM: + resp.status = endpoint.status + resp.writer.print(req.queryString) + break case REDIRECT: resp.sendRedirect(endpoint.body) break + case ERROR: + resp.sendError(endpoint.status, endpoint.body) + break case EXCEPTION: throw new Exception(endpoint.body) } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy index ec04c3b514..36dfb634df 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy @@ -2,9 +2,11 @@ import com.google.common.io.Files import datadog.opentracing.DDSpan import datadog.trace.agent.test.asserts.ListWriterAssert import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags import datadog.trace.instrumentation.api.Tags import groovy.transform.stc.ClosureParams import groovy.transform.stc.SimpleType +import javax.servlet.Servlet import org.apache.catalina.Context import org.apache.catalina.connector.Request import org.apache.catalina.connector.Response @@ -15,13 +17,12 @@ import org.apache.catalina.valves.ErrorReportValve import org.apache.tomcat.JarScanFilter import org.apache.tomcat.JarScanType -import javax.servlet.Servlet - import static datadog.trace.agent.test.asserts.TraceAssert.assertTrace import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED 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_FOUND +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS import static datadog.trace.agent.test.utils.TraceUtils.basicSpan @@ -153,51 +154,55 @@ class TomcatServlet3TestFakeAsync extends TomcatServlet3Test { } } -class TomcatServlet3TestForward extends TomcatServlet3Test { - @Override - Class servlet() { - TestServlet3.Sync // dispatch to sync servlet - } +// FIXME: not working right now... +//class TomcatServlet3TestForward extends TomcatDispatchTest { +// @Override +// Class servlet() { +// TestServlet3.Sync // dispatch to sync servlet +// } +// +// @Override +// boolean testNotFound() { +// false +// } +// +// @Override +// protected void setupServlets(Context context) { +// super.setupServlets(context) +// +// addServlet(context, "/dispatch" + SUCCESS.path, RequestDispatcherServlet.Forward) +// addServlet(context, "/dispatch" + QUERY_PARAM.path, RequestDispatcherServlet.Forward) +// addServlet(context, "/dispatch" + REDIRECT.path, RequestDispatcherServlet.Forward) +// addServlet(context, "/dispatch" + ERROR.path, RequestDispatcherServlet.Forward) +// addServlet(context, "/dispatch" + EXCEPTION.path, RequestDispatcherServlet.Forward) +// addServlet(context, "/dispatch" + AUTH_REQUIRED.path, RequestDispatcherServlet.Forward) +// } +//} - @Override - boolean testNotFound() { - false - } - - @Override - protected void setupServlets(Context context) { - super.setupServlets(context) - - addServlet(context, "/dispatch" + SUCCESS.path, RequestDispatcherServlet.Forward) - addServlet(context, "/dispatch" + ERROR.path, RequestDispatcherServlet.Forward) - addServlet(context, "/dispatch" + EXCEPTION.path, RequestDispatcherServlet.Forward) - addServlet(context, "/dispatch" + REDIRECT.path, RequestDispatcherServlet.Forward) - addServlet(context, "/dispatch" + AUTH_REQUIRED.path, RequestDispatcherServlet.Forward) - } -} - -class TomcatServlet3TestInclude extends TomcatServlet3Test { - @Override - Class servlet() { - TestServlet3.Sync // dispatch to sync servlet - } - - @Override - boolean testNotFound() { - false - } - - @Override - protected void setupServlets(Context context) { - super.setupServlets(context) - - addServlet(context, "/dispatch" + SUCCESS.path, RequestDispatcherServlet.Include) - addServlet(context, "/dispatch" + ERROR.path, RequestDispatcherServlet.Include) - addServlet(context, "/dispatch" + EXCEPTION.path, RequestDispatcherServlet.Include) - addServlet(context, "/dispatch" + REDIRECT.path, RequestDispatcherServlet.Include) - addServlet(context, "/dispatch" + AUTH_REQUIRED.path, RequestDispatcherServlet.Include) - } -} +// FIXME: not working right now... +//class TomcatServlet3TestInclude extends TomcatDispatchTest { +// @Override +// Class servlet() { +// TestServlet3.Sync // dispatch to sync servlet +// } +// +// @Override +// boolean testNotFound() { +// false +// } +// +// @Override +// protected void setupServlets(Context context) { +// super.setupServlets(context) +// +// addServlet(context, "/dispatch" + SUCCESS.path, RequestDispatcherServlet.Include) +// addServlet(context, "/dispatch" + QUERY_PARAM.path, RequestDispatcherServlet.Include) +// addServlet(context, "/dispatch" + REDIRECT.path, RequestDispatcherServlet.Include) +// addServlet(context, "/dispatch" + ERROR.path, RequestDispatcherServlet.Include) +// addServlet(context, "/dispatch" + EXCEPTION.path, RequestDispatcherServlet.Include) +// addServlet(context, "/dispatch" + AUTH_REQUIRED.path, RequestDispatcherServlet.Include) +// } +//} class TomcatServlet3TestDispatchImmediate extends TomcatDispatchTest { @Override @@ -215,6 +220,7 @@ class TomcatServlet3TestDispatchImmediate extends TomcatDispatchTest { super.setupServlets(context) addServlet(context, "/dispatch" + SUCCESS.path, TestServlet3.DispatchImmediate) + addServlet(context, "/dispatch" + QUERY_PARAM.path, TestServlet3.DispatchImmediate) addServlet(context, "/dispatch" + ERROR.path, TestServlet3.DispatchImmediate) addServlet(context, "/dispatch" + EXCEPTION.path, TestServlet3.DispatchImmediate) addServlet(context, "/dispatch" + REDIRECT.path, TestServlet3.DispatchImmediate) @@ -234,6 +240,7 @@ class TomcatServlet3TestDispatchAsync extends TomcatDispatchTest { super.setupServlets(context) addServlet(context, "/dispatch" + SUCCESS.path, TestServlet3.DispatchAsync) + addServlet(context, "/dispatch" + QUERY_PARAM.path, TestServlet3.DispatchAsync) addServlet(context, "/dispatch" + ERROR.path, TestServlet3.DispatchAsync) addServlet(context, "/dispatch" + EXCEPTION.path, TestServlet3.DispatchAsync) addServlet(context, "/dispatch" + REDIRECT.path, TestServlet3.DispatchAsync) @@ -315,6 +322,9 @@ abstract class TomcatDispatchTest extends TomcatServlet3Test { "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } defaultTags(true) } } diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecorator.java b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecorator.java index 6d540b9424..33a3c04be3 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecorator.java +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/SpringWebHttpServerDecorator.java @@ -50,7 +50,14 @@ public class SpringWebHttpServerDecorator @Override protected URI url(final HttpServletRequest httpServletRequest) throws URISyntaxException { - return new URI(httpServletRequest.getRequestURL().toString()); + return new URI( + httpServletRequest.getScheme(), + null, + httpServletRequest.getServerName(), + httpServletRequest.getServerPort(), + httpServletRequest.getRequestURI(), + httpServletRequest.getQueryString(), + null); } @Override diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/SpringBootBasedTest.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/SpringBootBasedTest.groovy index e95fee7294..f207cebeea 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/SpringBootBasedTest.groovy +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/SpringBootBasedTest.groovy @@ -6,6 +6,7 @@ import datadog.trace.agent.test.asserts.SpanAssert import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServerTest import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags import datadog.trace.instrumentation.api.Tags import datadog.trace.instrumentation.servlet3.Servlet3Decorator import datadog.trace.instrumentation.springweb.SpringWebHttpServerDecorator @@ -141,6 +142,9 @@ class SpringBootBasedTest extends HttpServerTest + controller(QUERY_PARAM) { + ctx.response().setStatusCode(QUERY_PARAM.status).end(ctx.request().query()) + } + } router.route(REDIRECT.path).handler { ctx -> controller(REDIRECT) { ctx.response().setStatusCode(REDIRECT.status).putHeader("location", REDIRECT.body).end() diff --git a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxCircuitBreakerHttpServerTest.groovy b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxCircuitBreakerHttpServerTest.groovy index d9c38f6140..5db78a337d 100644 --- a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxCircuitBreakerHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxCircuitBreakerHttpServerTest.groovy @@ -8,6 +8,7 @@ import io.vertx.reactivex.ext.web.Router 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -45,6 +46,19 @@ class VertxRxCircuitBreakerHttpServerTest extends VertxHttpServerTest { } }) } + router.route(QUERY_PARAM.path).handler { ctx -> + breaker.executeCommand({ future -> + future.complete(QUERY_PARAM) + }, { it -> + if (it.failed()) { + throw it.cause() + } + HttpServerTest.ServerEndpoint endpoint = it.result() + controller(endpoint) { + ctx.response().setStatusCode(endpoint.status).end(ctx.request().query()) + } + }) + } router.route(REDIRECT.path).handler { ctx -> breaker.executeCommand({ future -> future.complete(REDIRECT) diff --git a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxHttpServerTest.groovy b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxHttpServerTest.groovy index e2e7637f28..7516173c3a 100644 --- a/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/vertx/src/test/groovy/server/VertxRxHttpServerTest.groovy @@ -7,6 +7,7 @@ import io.vertx.reactivex.ext.web.Router 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.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -29,6 +30,11 @@ class VertxRxHttpServerTest extends VertxHttpServerTest { ctx.response().setStatusCode(SUCCESS.status).end(SUCCESS.body) } } + router.route(QUERY_PARAM.path).handler { ctx -> + controller(QUERY_PARAM) { + ctx.response().setStatusCode(QUERY_PARAM.status).end(ctx.request().query()) + } + } router.route(REDIRECT.path).handler { ctx -> controller(REDIRECT) { ctx.response().setStatusCode(REDIRECT.status).putHeader("location", REDIRECT.body).end() diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy index 4b5ec23c46..e35134d075 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -8,6 +8,7 @@ import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.utils.OkHttpUtils import datadog.trace.agent.test.utils.PortUtils import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags import datadog.trace.instrumentation.api.Tags import groovy.transform.stc.ClosureParams import groovy.transform.stc.SimpleType @@ -24,8 +25,10 @@ import static datadog.trace.agent.test.asserts.TraceAssert.assertTrace 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_FOUND +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS +import static datadog.trace.agent.test.utils.ConfigUtils.withConfigOverride import static datadog.trace.agent.test.utils.TraceUtils.basicSpan import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace import static datadog.trace.instrumentation.api.AgentTracer.activeScope @@ -111,16 +114,25 @@ abstract class HttpServerTest ext NOT_FOUND("notFound", 404, "not found"), // TODO: add tests for the following cases: + QUERY_PARAM("query?some=query", 200, "some=query"), + // OkHttp never sends the fragment in the request, so these cases don't work. +// FRAGMENT_PARAM("fragment#some-fragment", 200, "some-fragment"), +// QUERY_FRAGMENT_PARAM("query/fragment?some=query#some-fragment", 200, "some=query#some-fragment"), PATH_PARAM("path/123/param", 200, "123"), AUTH_REQUIRED("authRequired", 200, null), private final String path + final String query + final String fragment final int status final String body final Boolean errored - ServerEndpoint(String path, int status, String body) { - this.path = path + ServerEndpoint(String uri, int status, String body) { + def uriObj = URI.create(uri) + this.path = uriObj.path + this.query = uriObj.query + this.fragment = uriObj.fragment this.status = status this.body = body this.errored = status >= 500 @@ -146,8 +158,12 @@ abstract class HttpServerTest ext } Request.Builder request(ServerEndpoint uri, String method, String body) { + def url = HttpUrl.get(uri.resolve(address)).newBuilder() + .query(uri.query) + .fragment(uri.fragment) + .build() return new Request.Builder() - .url(HttpUrl.get(uri.resolve(address))) + .url(url) .method(method, body) } @@ -232,6 +248,39 @@ abstract class HttpServerTest ext body = null } + def "test tag query string for #endpoint"() { + setup: + def request = request(endpoint, method, body).build() + Response response = withConfigOverride("http.server.tag.query-string", "true") { + client.newCall(request).execute() + } + + expect: + response.code() == endpoint.status + response.body().string() == endpoint.body + + and: + cleanAndAssertTraces(1) { + if (hasHandlerSpan()) { + trace(0, 3) { + serverSpan(it, 0, null, null, "GET", endpoint) + handlerSpan(it, 1, span(0), endpoint) + controllerSpan(it, 2, span(1)) + } + } else { + trace(0, 2) { + serverSpan(it, 0, null, null, "GET", endpoint) + controllerSpan(it, 1, span(0)) + } + } + } + + where: + method = "GET" + body = null + endpoint << [SUCCESS, QUERY_PARAM] + } + def "test redirect"() { setup: def request = request(REDIRECT, method, body).build() @@ -447,13 +496,16 @@ abstract class HttpServerTest ext "$Tags.HTTP_URL" "${endpoint.resolve(address)}" "$Tags.HTTP_METHOD" method "$Tags.HTTP_STATUS" endpoint.status -// if (tagQueryString) { -// "$DDTags.HTTP_QUERY" uri.query -// "$DDTags.HTTP_FRAGMENT" { it == null || it == uri.fragment } // Optional -// } if (endpoint.errored) { "$Tags.ERROR" endpoint.errored } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } + // OkHttp never sends the fragment in the request. +// if (endpoint.fragment) { +// "$DDTags.HTTP_FRAGMENT" endpoint.fragment +// } defaultTags(true) } } From 96edbe8b8a1d60a384fcb350d32bffb3eba634b2 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 17 Dec 2019 13:09:59 -0800 Subject: [PATCH 03/22] Add better exception handling in Agent initialization An invalid statsd hostname causes an exception for jmxfetch. This should be an error, but not prevent the app from starting. Also improved log variable name consistency. --- .../java/datadog/trace/bootstrap/Agent.java | 91 ++++++++++--------- .../trace/bootstrap/AgentBootstrap.java | 28 ++++-- .../agent/AgentLoadedIntoBootstrapTest.groovy | 2 +- .../datadog/trace/agent/JMXFetchTest.groovy | 11 +++ 4 files changed, 79 insertions(+), 53 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 2b2eec5f5d..4cfd50a828 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -16,6 +16,7 @@ import org.slf4j.LoggerFactory; *

The intention is for this class to be loaded by bootstrap classloader to make sure we have * unimpeded access to the rest of Datadog's agent parts. */ +// We cannot use lombok here because we need to configure logger first public class Agent { private static final String SIMPLE_LOGGER_SHOW_DATE_TIME_PROPERTY = @@ -28,19 +29,19 @@ public class Agent { "datadog.slf4j.simpleLogger.defaultLogLevel"; // We cannot use lombok here because we need to configure logger first - private static final Logger LOGGER; + private static final Logger log; static { // We can configure logger here because datadog.trace.agent.AgentBootstrap doesn't touch it. configureLogger(); - LOGGER = LoggerFactory.getLogger(Agent.class); + log = LoggerFactory.getLogger(Agent.class); } // fields must be managed under class lock private static ClassLoader AGENT_CLASSLOADER = null; private static ClassLoader JMXFETCH_CLASSLOADER = null; - public static void start(final Instrumentation inst, final URL bootstrapURL) throws Exception { + public static void start(final Instrumentation inst, final URL bootstrapURL) { startDatadogAgent(inst, bootstrapURL); final boolean appUsingCustomLogManager = isAppUsingCustomLogManager(); @@ -59,10 +60,10 @@ public class Agent { * the global log manager and jmxfetch won't be able to touch it due to classloader locking. */ if (appUsingCustomLogManager) { - LOGGER.debug("Custom logger detected. Delaying JMXFetch initialization."); - registerLogManagerCallback(new StartJmxFetchCallback(inst, bootstrapURL)); + log.debug("Custom logger detected. Delaying JMXFetch initialization."); + registerLogManagerCallback(new StartJmxFetchCallback(bootstrapURL)); } else { - startJmxFetch(inst, bootstrapURL); + startJmxFetch(bootstrapURL); } /* @@ -71,28 +72,30 @@ public class Agent { * logging facility. */ if (isJavaBefore9WithJFR() && appUsingCustomLogManager) { - LOGGER.debug("Custom logger detected. Delaying Datadog Tracer initialization."); - registerLogManagerCallback(new InstallDatadogTracerCallback(inst, bootstrapURL)); + log.debug("Custom logger detected. Delaying Datadog Tracer initialization."); + registerLogManagerCallback(new InstallDatadogTracerCallback(bootstrapURL)); } else { - installDatadogTracer(inst, bootstrapURL); + installDatadogTracer(); } } - private static void registerLogManagerCallback(final Runnable callback) throws Exception { - final Class agentInstallerClass = - AGENT_CLASSLOADER.loadClass("datadog.trace.agent.tooling.AgentInstaller"); - final Method registerCallbackMethod = - agentInstallerClass.getMethod("registerClassLoadCallback", String.class, Runnable.class); - registerCallbackMethod.invoke(null, "java.util.logging.LogManager", callback); + private static void registerLogManagerCallback(final ClassLoadCallBack callback) { + try { + final Class agentInstallerClass = + AGENT_CLASSLOADER.loadClass("datadog.trace.agent.tooling.AgentInstaller"); + final Method registerCallbackMethod = + agentInstallerClass.getMethod("registerClassLoadCallback", String.class, Runnable.class); + registerCallbackMethod.invoke(null, "java.util.logging.LogManager", callback); + } catch (final Exception ex) { + log.error("Error registering callback for " + callback.getName(), ex); + } } protected abstract static class ClassLoadCallBack implements Runnable { - final Instrumentation inst; final URL bootstrapURL; - ClassLoadCallBack(final Instrumentation inst, final URL bootstrapURL) { - this.inst = inst; + ClassLoadCallBack(final URL bootstrapURL) { this.bootstrapURL = bootstrapURL; } @@ -111,7 +114,7 @@ public class Agent { try { execute(); } catch (final Exception e) { - LOGGER.error("Failed to run class loader callback {}", getName(), e); + log.error("Failed to run class loader callback {}", getName(), e); } } }); @@ -122,12 +125,12 @@ public class Agent { public abstract String getName(); - public abstract void execute() throws Exception; + public abstract void execute(); } protected static class StartJmxFetchCallback extends ClassLoadCallBack { - StartJmxFetchCallback(final Instrumentation inst, final URL bootstrapURL) { - super(inst, bootstrapURL); + StartJmxFetchCallback(final URL bootstrapURL) { + super(bootstrapURL); } @Override @@ -136,14 +139,14 @@ public class Agent { } @Override - public void execute() throws Exception { - startJmxFetch(inst, bootstrapURL); + public void execute() { + startJmxFetch(bootstrapURL); } } protected static class InstallDatadogTracerCallback extends ClassLoadCallBack { - InstallDatadogTracerCallback(final Instrumentation inst, final URL bootstrapURL) { - super(inst, bootstrapURL); + InstallDatadogTracerCallback(final URL bootstrapURL) { + super(bootstrapURL); } @Override @@ -152,18 +155,18 @@ public class Agent { } @Override - public void execute() throws Exception { - installDatadogTracer(inst, bootstrapURL); + public void execute() { + installDatadogTracer(); } } private static synchronized void startDatadogAgent( - final Instrumentation inst, final URL bootstrapURL) throws Exception { + final Instrumentation inst, final URL bootstrapURL) { if (AGENT_CLASSLOADER == null) { - final ClassLoader agentClassLoader = - createDatadogClassLoader("agent-tooling-and-instrumentation.isolated", bootstrapURL); final ClassLoader contextLoader = Thread.currentThread().getContextClassLoader(); try { + final ClassLoader agentClassLoader = + createDatadogClassLoader("agent-tooling-and-instrumentation.isolated", bootstrapURL); Thread.currentThread().setContextClassLoader(agentClassLoader); final Class agentInstallerClass = agentClassLoader.loadClass("datadog.trace.agent.tooling.AgentInstaller"); @@ -171,14 +174,15 @@ public class Agent { agentInstallerClass.getMethod("installBytebuddyAgent", Instrumentation.class); agentInstallerMethod.invoke(null, inst); AGENT_CLASSLOADER = agentClassLoader; + } catch (final Throwable ex) { + log.error("Throwable thrown while installing the Datadog Agent", ex); } finally { Thread.currentThread().setContextClassLoader(contextLoader); } } } - private static synchronized void installDatadogTracer( - final Instrumentation inst, final URL bootstrapURL) throws Exception { + private static synchronized void installDatadogTracer() { if (AGENT_CLASSLOADER == null) { throw new IllegalStateException("Datadog agent should have been started already"); } @@ -194,24 +198,27 @@ public class Agent { tracerInstallerMethod.invoke(null); final Method logVersionInfoMethod = tracerInstallerClass.getMethod("logVersionInfo"); logVersionInfoMethod.invoke(null); + } catch (final Throwable ex) { + log.error("Throwable thrown while installing the Datadog Tracer", ex); } finally { Thread.currentThread().setContextClassLoader(contextLoader); } } - private static synchronized void startJmxFetch(final Instrumentation inst, final URL bootstrapURL) - throws Exception { + private static synchronized void startJmxFetch(final URL bootstrapURL) { if (JMXFETCH_CLASSLOADER == null) { - final ClassLoader jmxFetchClassLoader = - createDatadogClassLoader("agent-jmxfetch.isolated", bootstrapURL); final ClassLoader contextLoader = Thread.currentThread().getContextClassLoader(); try { + final ClassLoader jmxFetchClassLoader = + createDatadogClassLoader("agent-jmxfetch.isolated", bootstrapURL); Thread.currentThread().setContextClassLoader(jmxFetchClassLoader); final Class jmxFetchAgentClass = jmxFetchClassLoader.loadClass("datadog.trace.agent.jmxfetch.JMXFetch"); final Method jmxFetchInstallerMethod = jmxFetchAgentClass.getMethod("run"); jmxFetchInstallerMethod.invoke(null); JMXFETCH_CLASSLOADER = jmxFetchClassLoader; + } catch (final Throwable ex) { + log.error("Throwable thrown while starting JmxFetch", ex); } finally { Thread.currentThread().setContextClassLoader(contextLoader); } @@ -305,8 +312,8 @@ public class Agent { System.getenv(tracerCustomLogManSysprop.replace('.', '_').toUpperCase()); if (customLogManagerProp != null || customLogManagerEnv != null) { - LOGGER.debug("Prop - customlogmanager: " + customLogManagerProp); - LOGGER.debug("Env - customlogmanager: " + customLogManagerEnv); + log.debug("Prop - customlogmanager: " + customLogManagerProp); + log.debug("Env - customlogmanager: " + customLogManagerEnv); // Allow setting to skip these automatic checks: return Boolean.parseBoolean(customLogManagerProp) || Boolean.parseBoolean(customLogManagerEnv); @@ -314,7 +321,7 @@ public class Agent { final String jbossHome = System.getenv("JBOSS_HOME"); if (jbossHome != null) { - LOGGER.debug("Env - jboss: " + jbossHome); + log.debug("Env - jboss: " + jbossHome); // JBoss/Wildfly is known to set a custom log manager after startup. // Originally we were checking for the presence of a jboss class, // but it seems some non-jboss applications have jboss classes on the classpath. @@ -327,8 +334,8 @@ public class Agent { if (logManagerProp != null) { final boolean onSysClasspath = ClassLoader.getSystemResource(logManagerProp.replaceAll("\\.", "/") + ".class") != null; - LOGGER.debug("Prop - logging.manager: " + logManagerProp); - LOGGER.debug("logging.manager on system classpath: " + onSysClasspath); + log.debug("Prop - logging.manager: " + logManagerProp); + log.debug("logging.manager on system classpath: " + onSysClasspath); // Some applications set java.util.logging.manager but never actually initialize the logger. // Check to see if the configured manager is on the system classpath. // If so, it should be safe to initialize jmxfetch which will setup the log manager. diff --git a/dd-java-agent/src/main/java/datadog/trace/bootstrap/AgentBootstrap.java b/dd-java-agent/src/main/java/datadog/trace/bootstrap/AgentBootstrap.java index ed19b12962..9d12306fa5 100644 --- a/dd-java-agent/src/main/java/datadog/trace/bootstrap/AgentBootstrap.java +++ b/dd-java-agent/src/main/java/datadog/trace/bootstrap/AgentBootstrap.java @@ -41,19 +41,23 @@ import java.util.regex.Pattern; */ public class AgentBootstrap { - public static void premain(final String agentArgs, final Instrumentation inst) throws Exception { + public static void premain(final String agentArgs, final Instrumentation inst) { agentmain(agentArgs, inst); } - public static void agentmain(final String agentArgs, final Instrumentation inst) - throws Exception { + public static void agentmain(final String agentArgs, final Instrumentation inst) { + try { - final URL bootstrapURL = installBootstrapJar(inst); + final URL bootstrapURL = installBootstrapJar(inst); - final Class agentClass = - ClassLoader.getSystemClassLoader().loadClass("datadog.trace.bootstrap.Agent"); - final Method startMethod = agentClass.getMethod("start", Instrumentation.class, URL.class); - startMethod.invoke(null, inst, bootstrapURL); + final Class agentClass = + ClassLoader.getSystemClassLoader().loadClass("datadog.trace.bootstrap.Agent"); + final Method startMethod = agentClass.getMethod("start", Instrumentation.class, URL.class); + startMethod.invoke(null, inst, bootstrapURL); + } catch (final Throwable ex) { + // Don't rethrow. We don't have a log manager here, so just print. + ex.printStackTrace(); + } } private static synchronized URL installBootstrapJar(final Instrumentation inst) @@ -106,8 +110,12 @@ public class AgentBootstrap { throw new RuntimeException("Unable to parse javaagent parameter: " + agentArgument); } - bootstrapURL = new URL("file:" + matcher.group(1)); - inst.appendToBootstrapClassLoaderSearch(new JarFile(new File(bootstrapURL.toURI()))); + final File javaagentFile = new File(matcher.group(1)); + if (!(javaagentFile.exists() || javaagentFile.isFile())) { + throw new RuntimeException("Unable to find javaagent file: " + javaagentFile); + } + bootstrapURL = javaagentFile.toURI().toURL(); + inst.appendToBootstrapClassLoaderSearch(new JarFile(javaagentFile)); return bootstrapURL; } diff --git a/dd-java-agent/src/test/groovy/datadog/trace/agent/AgentLoadedIntoBootstrapTest.groovy b/dd-java-agent/src/test/groovy/datadog/trace/agent/AgentLoadedIntoBootstrapTest.groovy index 5afe435c67..a3d8d48181 100644 --- a/dd-java-agent/src/test/groovy/datadog/trace/agent/AgentLoadedIntoBootstrapTest.groovy +++ b/dd-java-agent/src/test/groovy/datadog/trace/agent/AgentLoadedIntoBootstrapTest.groovy @@ -5,7 +5,7 @@ import jvmbootstraptest.AgentLoadedChecker import spock.lang.Specification class AgentLoadedIntoBootstrapTest extends Specification { - + def "Agent loads in when separate jvm is launched"() { expect: IntegrationTestUtils.runOnSeparateJvm(AgentLoadedChecker.getName() diff --git a/dd-java-agent/src/test/groovy/datadog/trace/agent/JMXFetchTest.groovy b/dd-java-agent/src/test/groovy/datadog/trace/agent/JMXFetchTest.groovy index 1b70815081..de5ac7369c 100644 --- a/dd-java-agent/src/test/groovy/datadog/trace/agent/JMXFetchTest.groovy +++ b/dd-java-agent/src/test/groovy/datadog/trace/agent/JMXFetchTest.groovy @@ -2,6 +2,7 @@ package datadog.trace.agent import datadog.trace.agent.test.IntegrationTestUtils import datadog.trace.api.Config +import jvmbootstraptest.AgentLoadedChecker import org.junit.Rule import org.junit.contrib.java.lang.system.RestoreSystemProperties import spock.lang.Specification @@ -49,6 +50,16 @@ class JMXFetchTest extends Specification { Thread.currentThread().setContextClassLoader(currentContextLoader) } + def "Agent loads when JmxFetch is misconfigured"() { + // verify the agent starts up correctly with a bogus address. + expect: + IntegrationTestUtils.runOnSeparateJvm(AgentLoadedChecker.getName() + , ["-Ddd.jmxfetch.enabled=true", "-Ddd.jmxfetch.statsd.host=example.local"] as String[] + , "" as String[] + , [:] + , true) == 0 + } + def "test jmxfetch config"() { setup: names.each { From 7095ea34260641c3f256bc111639b673d815b862 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 18 Dec 2019 10:40:54 -0800 Subject: [PATCH 04/22] Add async propagation flags for kafka consumer. --- .../trace/instrumentation/kafka_clients/TracingIterator.java | 1 + .../kafka_streams/KafkaStreamsProcessorInstrumentation.java | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TracingIterator.java b/dd-java-agent/instrumentation/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TracingIterator.java index 87723f863c..ae1dbc6d8f 100644 --- a/dd-java-agent/instrumentation/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TracingIterator.java +++ b/dd-java-agent/instrumentation/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TracingIterator.java @@ -59,6 +59,7 @@ public class TracingIterator implements Iterator { decorator.afterStart(span); decorator.onConsume(span, next); currentScope = activateSpan(span, true); + currentScope.setAsyncPropagation(true); } } catch (final Exception e) { log.debug("Error during decoration", e); diff --git a/dd-java-agent/instrumentation/kafka-streams-0.11/src/main/java/datadog/trace/instrumentation/kafka_streams/KafkaStreamsProcessorInstrumentation.java b/dd-java-agent/instrumentation/kafka-streams-0.11/src/main/java/datadog/trace/instrumentation/kafka_streams/KafkaStreamsProcessorInstrumentation.java index 88f0fba8b4..3d156892b9 100644 --- a/dd-java-agent/instrumentation/kafka-streams-0.11/src/main/java/datadog/trace/instrumentation/kafka_streams/KafkaStreamsProcessorInstrumentation.java +++ b/dd-java-agent/instrumentation/kafka-streams-0.11/src/main/java/datadog/trace/instrumentation/kafka_streams/KafkaStreamsProcessorInstrumentation.java @@ -81,7 +81,7 @@ public class KafkaStreamsProcessorInstrumentation { CONSUMER_DECORATE.afterStart(span); CONSUMER_DECORATE.onConsume(span, record); - activateSpan(span, true); + activateSpan(span, true).setAsyncPropagation(true); } } } @@ -119,6 +119,7 @@ public class KafkaStreamsProcessorInstrumentation { @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan(@Advice.Thrown final Throwable throwable) { + // This is dangerous... we assume the span/scope is the one we expect, but it may not be. final AgentSpan span = activeSpan(); if (span != null) { CONSUMER_DECORATE.onError(span, throwable); From ded28674d36329dcd546c1f8f068fec62c30a312 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 18 Dec 2019 13:36:01 -0800 Subject: [PATCH 05/22] Add option for muzzle validation on the specific JDK version This is still useful to validate various aspects of the integrations even if it doesn't need to check against maven. --- buildSrc/src/main/groovy/MuzzlePlugin.groovy | 59 +++++++++++++------ .../http-url-connection.gradle | 6 ++ .../java-concurrent/java-concurrent.gradle | 7 +++ .../instrumentation/jdbc/jdbc.gradle | 6 ++ 4 files changed, 61 insertions(+), 17 deletions(-) diff --git a/buildSrc/src/main/groovy/MuzzlePlugin.groovy b/buildSrc/src/main/groovy/MuzzlePlugin.groovy index 7b8643cab1..2f6256ed3f 100644 --- a/buildSrc/src/main/groovy/MuzzlePlugin.groovy +++ b/buildSrc/src/main/groovy/MuzzlePlugin.groovy @@ -103,15 +103,19 @@ class MuzzlePlugin implements Plugin { Task runAfter = project.tasks.muzzle for (MuzzleDirective muzzleDirective : project.muzzle.directives) { - project.getLogger().info("configured ${muzzleDirective.assertPass ? 'pass' : 'fail'} directive: ${muzzleDirective.group}:${muzzleDirective.module}:${muzzleDirective.versions}") + project.getLogger().info("configured $muzzleDirective") - muzzleDirectiveToArtifacts(muzzleDirective, system, session).collect() { Artifact singleVersion -> - runAfter = addMuzzleTask(muzzleDirective, singleVersion, project, runAfter, bootstrapProject, toolingProject) - } - if (muzzleDirective.assertInverse) { - inverseOf(muzzleDirective, system, session).collect() { MuzzleDirective inverseDirective -> - muzzleDirectiveToArtifacts(inverseDirective, system, session).collect() { Artifact singleVersion -> - runAfter = addMuzzleTask(inverseDirective, singleVersion, project, runAfter, bootstrapProject, toolingProject) + if (muzzleDirective.coreJdk) { + runAfter = addMuzzleTask(muzzleDirective, null, project, runAfter, bootstrapProject, toolingProject) + } else { + muzzleDirectiveToArtifacts(muzzleDirective, system, session).collect() { Artifact singleVersion -> + runAfter = addMuzzleTask(muzzleDirective, singleVersion, project, runAfter, bootstrapProject, toolingProject) + } + if (muzzleDirective.assertInverse) { + inverseOf(muzzleDirective, system, session).collect() { MuzzleDirective inverseDirective -> + muzzleDirectiveToArtifacts(inverseDirective, system, session).collect() { Artifact singleVersion -> + runAfter = addMuzzleTask(inverseDirective, singleVersion, project, runAfter, bootstrapProject, toolingProject) + } } } } @@ -258,17 +262,25 @@ class MuzzlePlugin implements Plugin { * @return The created muzzle task. */ private static Task addMuzzleTask(MuzzleDirective muzzleDirective, Artifact versionArtifact, Project instrumentationProject, Task runAfter, Project bootstrapProject, Project toolingProject) { - def taskName = "muzzle-Assert${muzzleDirective.assertPass ? "Pass" : "Fail"}-$versionArtifact.groupId-$versionArtifact.artifactId-$versionArtifact.version${muzzleDirective.name ? "-${muzzleDirective.getNameSlug()}" : ""}" - def config = instrumentationProject.configurations.create(taskName) - def dep = instrumentationProject.dependencies.create("$versionArtifact.groupId:$versionArtifact.artifactId:$versionArtifact.version") { - transitive = true + def taskName + if (muzzleDirective.coreJdk) { + taskName = "muzzle-Assert$muzzleDirective" + } else { + taskName = "muzzle-Assert${muzzleDirective.assertPass ? "Pass" : "Fail"}-$versionArtifact.groupId-$versionArtifact.artifactId-$versionArtifact.version${muzzleDirective.name ? "-${muzzleDirective.getNameSlug()}" : ""}" } - // The following optional transitive dependencies are brought in by some legacy module such as log4j 1.x but are no - // longer bundled with the JVM and have to be excluded for the muzzle tests to be able to run. - dep.exclude group: 'com.sun.jdmk', module: 'jmxtools' - dep.exclude group: 'com.sun.jmx', module: 'jmxri' + def config = instrumentationProject.configurations.create(taskName) - config.dependencies.add(dep) + if (!muzzleDirective.coreJdk) { + def dep = instrumentationProject.dependencies.create("$versionArtifact.groupId:$versionArtifact.artifactId:$versionArtifact.version") { + transitive = true + } + // The following optional transitive dependencies are brought in by some legacy module such as log4j 1.x but are no + // longer bundled with the JVM and have to be excluded for the muzzle tests to be able to run. + dep.exclude group: 'com.sun.jdmk', module: 'jmxtools' + dep.exclude group: 'com.sun.jmx', module: 'jmxri' + + config.dependencies.add(dep) + } for (String additionalDependency : muzzleDirective.additionalDependencies) { config.dependencies.add(instrumentationProject.dependencies.create(additionalDependency) { transitive = true @@ -369,6 +381,11 @@ class MuzzleDirective { List additionalDependencies = new ArrayList<>() boolean assertPass boolean assertInverse = false + boolean coreJdk = false + + void coreJdk() { + coreJdk = true + } /** * Adds extra dependencies to the current muzzle test. @@ -391,6 +408,14 @@ class MuzzleDirective { return name.trim().replaceAll("[^a-zA-Z0-9]+", "-") } + + String toString() { + if (coreJdk) { + return "${assertPass ? 'Pass' : 'Fail'}-core-jdk" + } else { + return "${assertPass ? 'pass' : 'fail'} $group:$module:$versions" + } + } } /** diff --git a/dd-java-agent/instrumentation/http-url-connection/http-url-connection.gradle b/dd-java-agent/instrumentation/http-url-connection/http-url-connection.gradle index 38851b24a8..075d73deed 100644 --- a/dd-java-agent/instrumentation/http-url-connection/http-url-connection.gradle +++ b/dd-java-agent/instrumentation/http-url-connection/http-url-connection.gradle @@ -1,3 +1,9 @@ +muzzle { + pass { + coreJdk() + } +} + apply from: "${rootDir}/gradle/java.gradle" dependencies { diff --git a/dd-java-agent/instrumentation/java-concurrent/java-concurrent.gradle b/dd-java-agent/instrumentation/java-concurrent/java-concurrent.gradle index 4ff2d984ab..77de36fba9 100644 --- a/dd-java-agent/instrumentation/java-concurrent/java-concurrent.gradle +++ b/dd-java-agent/instrumentation/java-concurrent/java-concurrent.gradle @@ -1,3 +1,10 @@ +// This won't work until the akka and scala integrations are split into separate projects. +//muzzle { +// pass { +// coreJdk() +// } +//} + apply from: "${rootDir}/gradle/java.gradle" apply from: "${rootDir}/gradle/test-with-scala.gradle" apply from: "${rootDir}/gradle/test-with-kotlin.gradle" diff --git a/dd-java-agent/instrumentation/jdbc/jdbc.gradle b/dd-java-agent/instrumentation/jdbc/jdbc.gradle index 66e4080b7b..b775fae9d9 100644 --- a/dd-java-agent/instrumentation/jdbc/jdbc.gradle +++ b/dd-java-agent/instrumentation/jdbc/jdbc.gradle @@ -1,3 +1,9 @@ +muzzle { + pass { + coreJdk() + } +} + apply from: "${rootDir}/gradle/java.gradle" apply plugin: 'org.unbroken-dome.test-sets' From 31b77cbd7bb48fccf14e7e9d4dbdd6408db8d5ec Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 18 Dec 2019 10:42:02 -0800 Subject: [PATCH 06/22] Capture error message even if no exception object provided. This is currently only used by the GoogleHttpClient instrumentation, but may be used by customers too. --- .../src/test/groovy/AbstractGoogleHttpClientTest.groovy | 2 ++ dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/dd-java-agent/instrumentation/google-http-client/src/test/groovy/AbstractGoogleHttpClientTest.groovy b/dd-java-agent/instrumentation/google-http-client/src/test/groovy/AbstractGoogleHttpClientTest.groovy index de5bd5f38c..1a7c6dd02b 100644 --- a/dd-java-agent/instrumentation/google-http-client/src/test/groovy/AbstractGoogleHttpClientTest.groovy +++ b/dd-java-agent/instrumentation/google-http-client/src/test/groovy/AbstractGoogleHttpClientTest.groovy @@ -4,6 +4,7 @@ import com.google.api.client.http.HttpResponse import com.google.api.client.http.javanet.NetHttpTransport import datadog.trace.agent.test.base.HttpClientTest import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags import datadog.trace.instrumentation.api.Tags import datadog.trace.instrumentation.googlehttpclient.GoogleHttpClientDecorator import spock.lang.Shared @@ -69,6 +70,7 @@ abstract class AbstractGoogleHttpClientTest extends HttpClientTest Date: Wed, 18 Dec 2019 14:23:46 -0800 Subject: [PATCH 07/22] Allow flush to return if executor already shut down Otherwise it could block shutdown if executor is stopped before disruptor. --- .../trace/common/writer/DDAgentWriter.java | 109 ++++++++++-------- .../trace/api/writer/DDAgentWriterTest.groovy | 19 +++ 2 files changed, 81 insertions(+), 47 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java index c88394717e..a98baabf56 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java @@ -22,6 +22,7 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.Executors; import java.util.concurrent.Phaser; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.Semaphore; @@ -317,7 +318,7 @@ public class DDAgentWriter implements Writer { } if (event.shouldFlush || payloadSize >= FLUSH_PAYLOAD_BYTES) { - boolean early = (payloadSize >= FLUSH_PAYLOAD_BYTES); + final boolean early = (payloadSize >= FLUSH_PAYLOAD_BYTES); reportTraces(early); event.shouldFlush = false; @@ -333,6 +334,12 @@ public class DDAgentWriter implements Writer { return; // scheduleFlush called in finally block. } + if (scheduledWriterExecutor.isShutdown()) { + monitor.onFailedSend( + DDAgentWriter.this, traceCount.get(), payloadSize, DDApi.Response.failed(-1)); + apiPhaser.arrive(); // Allow flush to return + return; + } final List toSend = serializedTraces; serializedTraces = new ArrayList<>(toSend.size()); // ^ Initialize with similar size to reduce arraycopy churn. @@ -340,60 +347,67 @@ public class DDAgentWriter implements Writer { final int representativeCount = traceCount.getAndSet(0); final int sizeInBytes = payloadSize; - monitor.onFlush(DDAgentWriter.this, early); - - // Run the actual IO task on a different thread to avoid blocking the consumer. try { - senderSemaphore.acquire(); - } catch (final InterruptedException e) { - monitor.onFailedSend( - DDAgentWriter.this, representativeCount, sizeInBytes, DDApi.Response.failed(e)); + monitor.onFlush(DDAgentWriter.this, early); - // Finally, we'll schedule another flush - // Any threads awaiting the flush will continue to wait - return; - } - scheduledWriterExecutor.execute( - new Runnable() { - @Override - public void run() { - senderSemaphore.release(); + // Run the actual IO task on a different thread to avoid blocking the consumer. + try { + senderSemaphore.acquire(); + } catch (final InterruptedException e) { + monitor.onFailedSend( + DDAgentWriter.this, representativeCount, sizeInBytes, DDApi.Response.failed(e)); - try { - final DDApi.Response response = - api.sendSerializedTraces(representativeCount, sizeInBytes, toSend); + // Finally, we'll schedule another flush + // Any threads awaiting the flush will continue to wait + return; + } + scheduledWriterExecutor.execute( + new Runnable() { + @Override + public void run() { + senderSemaphore.release(); - if (response.success()) { - log.debug("Successfully sent {} traces to the API", toSend.size()); + try { + final DDApi.Response response = + api.sendSerializedTraces(representativeCount, sizeInBytes, toSend); - monitor.onSend(DDAgentWriter.this, representativeCount, sizeInBytes, response); - } else { - log.debug( - "Failed to send {} traces (representing {}) of size {} bytes to the API", - toSend.size(), - representativeCount, - sizeInBytes); + if (response.success()) { + log.debug("Successfully sent {} traces to the API", toSend.size()); + monitor.onSend( + DDAgentWriter.this, representativeCount, sizeInBytes, response); + } else { + log.debug( + "Failed to send {} traces (representing {}) of size {} bytes to the API", + toSend.size(), + representativeCount, + sizeInBytes); + + monitor.onFailedSend( + DDAgentWriter.this, representativeCount, sizeInBytes, response); + } + } catch (final Throwable e) { + log.debug("Failed to send traces to the API: {}", e.getMessage()); + + // DQH - 10/2019 - DDApi should wrap most exceptions itself, so this really + // shouldn't occur. + // However, just to be safe to start, create a failed Response to handle any + // spurious Throwable-s. monitor.onFailedSend( - DDAgentWriter.this, representativeCount, sizeInBytes, response); + DDAgentWriter.this, + representativeCount, + sizeInBytes, + DDApi.Response.failed(e)); + } finally { + apiPhaser.arrive(); // Flush completed. } - } catch (final Throwable e) { - log.debug("Failed to send traces to the API: {}", e.getMessage()); - - // DQH - 10/2019 - DDApi should wrap most exceptions itself, so this really - // shouldn't occur. - // However, just to be safe to start, create a failed Response to handle any - // spurious Throwable-s. - monitor.onFailedSend( - DDAgentWriter.this, - representativeCount, - sizeInBytes, - DDApi.Response.failed(e)); - } finally { - apiPhaser.arrive(); // Flush completed. } - } - }); + }); + } catch (final RejectedExecutionException ex) { + monitor.onFailedSend( + DDAgentWriter.this, representativeCount, sizeInBytes, DDApi.Response.failed(ex)); + apiPhaser.arrive(); // Allow flush to return + } } finally { payloadSize = 0; scheduleFlush(); @@ -541,7 +555,7 @@ public class DDAgentWriter implements Writer { }; } - private static final String tag(final String tagPrefix, final String tagValue) { + private static String tag(final String tagPrefix, final String tagValue) { return tagPrefix + ":" + tagValue; } @@ -626,6 +640,7 @@ public class DDAgentWriter implements Writer { } } + @Override public String toString() { if (hostInfo == null) { return "StatsD"; diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy index 6c65b34de6..0868e56ba5 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy @@ -198,6 +198,25 @@ class DDAgentWriterTest extends DDSpecification { writer.traceCount.get() == 0 } + def "check shutdown if executor stopped first"() { + setup: + def writer = new DDAgentWriter(api) + writer.start() + writer.scheduledWriterExecutor.shutdown() + + when: + writer.write([]) + writer.flush() + + then: + 1 * api.serializeTrace(_) >> { trace -> callRealMethod() } + 0 * _ + writer.traceCount.get() == 1 + + cleanup: + writer.close() + } + def createMinimalTrace() { def minimalContext = new DDSpanContext( 1G, From dc0cbeb95d66ee1ba13d0d4d7b7426166f1c636a Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Wed, 4 Dec 2019 15:42:53 -0500 Subject: [PATCH 08/22] apply comma split logic to servlet request extract adapter --- .../servlet2/HttpServletRequestExtractAdapter.java | 4 +++- .../servlet3/HttpServletRequestExtractAdapter.java | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/HttpServletRequestExtractAdapter.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/HttpServletRequestExtractAdapter.java index 054fb9bae1..bcaf31e1da 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/HttpServletRequestExtractAdapter.java +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/HttpServletRequestExtractAdapter.java @@ -29,6 +29,8 @@ public class HttpServletRequestExtractAdapter if (attribute instanceof String) { return (String) attribute; } - return carrier.getHeader(key); + String s = carrier.getHeader(key); + if (s == null) return null; + return s.split(",")[0].trim(); } } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java index e8b1fff393..889482e06c 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java @@ -29,6 +29,8 @@ public class HttpServletRequestExtractAdapter if (attribute instanceof String) { return (String) attribute; } - return carrier.getHeader(key); + String s = carrier.getHeader(key); + if (s == null) return null; + return s.split(",")[0].trim(); } } From a5e693bb92a8dd8a4b37197fa17e3815ef82bdcb Mon Sep 17 00:00:00 2001 From: kfujita Date: Tue, 10 Dec 2019 12:19:34 +0900 Subject: [PATCH 09/22] assert fix same as JettyServlet3Test --- .../servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy index 36dfb634df..384a15f516 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy @@ -304,7 +304,7 @@ abstract class TomcatDispatchTest extends TomcatServlet3Test { tags { "$Tags.COMPONENT" serverDecorator.component() "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER - "$Tags.PEER_HOSTNAME" "localhost" + "$Tags.PEER_HOSTNAME" { it == "localhost" || it == "127.0.0.1" } "$Tags.PEER_HOST_IPV4" { it == null || it == "127.0.0.1" } // Optional "$Tags.PEER_PORT" Integer "$Tags.HTTP_URL" "${endpoint.resolve(address)}" From 0f3125498c68e7f5a67d356a60ccbefdc267480a Mon Sep 17 00:00:00 2001 From: kfujita Date: Tue, 10 Dec 2019 13:28:00 +0900 Subject: [PATCH 10/22] add test case --- .../agent/test/base/HttpServerTest.groovy | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy index e35134d075..7f6bb435f1 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -281,6 +281,42 @@ abstract class HttpServerTest ext endpoint << [SUCCESS, QUERY_PARAM] } + def "test success with multiple header attached parent"() { + setup: + def traceId = 123G + def parentId = 456G + def request = request(SUCCESS, method, body) + .header("x-datadog-trace-id", traceId.toString() + ", " + traceId.toString()) + .header("x-datadog-parent-id", parentId.toString() + ", " + parentId.toString()) + .header("x-datadog-sampling-priority", "1, 1") + .build() + def response = client.newCall(request).execute() + + expect: + response.code() == SUCCESS.status + response.body().string() == SUCCESS.body + + and: + cleanAndAssertTraces(1) { + if (hasHandlerSpan()) { + trace(0, 3) { + serverSpan(it, 0, traceId, parentId) + handlerSpan(it, 1, span(0)) + controllerSpan(it, 2, span(1)) + } + } else { + trace(0, 2) { + serverSpan(it, 0, traceId, parentId) + controllerSpan(it, 1, span(0)) + } + } + } + + where: + method = "GET" + body = null + } + def "test redirect"() { setup: def request = request(REDIRECT, method, body).build() From b707a3aab9792d0179e702463ecbdf4cddcb6a8f Mon Sep 17 00:00:00 2001 From: kfujita Date: Tue, 10 Dec 2019 14:03:17 +0900 Subject: [PATCH 11/22] move multi-value aware logic to Extractor. --- .../java/datadog/trace/agent/tooling/OpenTracing32.java | 6 +++++- .../servlet2/HttpServletRequestExtractAdapter.java | 4 +--- .../servlet3/HttpServletRequestExtractAdapter.java | 4 +--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java index 8ba9739ade..8143dadc6d 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java @@ -298,7 +298,11 @@ public final class OpenTracing32 implements TracerAPI { private Extractor(final C carrier, final Getter getter) { extracted = new HashMap<>(); for (final String key : getter.keys(carrier)) { - extracted.put(key, getter.get(carrier, key)); + // extracted header value + String s = getter.get(carrier, key); + // in case of multiple values in the header, need to parse + if (s != null) s = s.split(",")[0].trim(); + extracted.put(key, s); } } diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/HttpServletRequestExtractAdapter.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/HttpServletRequestExtractAdapter.java index bcaf31e1da..054fb9bae1 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/HttpServletRequestExtractAdapter.java +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/HttpServletRequestExtractAdapter.java @@ -29,8 +29,6 @@ public class HttpServletRequestExtractAdapter if (attribute instanceof String) { return (String) attribute; } - String s = carrier.getHeader(key); - if (s == null) return null; - return s.split(",")[0].trim(); + return carrier.getHeader(key); } } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java index 889482e06c..e8b1fff393 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java @@ -29,8 +29,6 @@ public class HttpServletRequestExtractAdapter if (attribute instanceof String) { return (String) attribute; } - String s = carrier.getHeader(key); - if (s == null) return null; - return s.split(",")[0].trim(); + return carrier.getHeader(key); } } From 0ee80a0b956a675fd0930c04ff5795d932140135 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 5 Dec 2019 12:43:45 +0100 Subject: [PATCH 12/22] [rmi] Instrumentation for RMI 1.2 and later includes instrumentation of client and server endpoints currently missing passing of execution context from client to server --- .../trace/agent/tooling/AgentInstaller.java | 2 + dd-java-agent/instrumentation/rmi/rmi.gradle | 12 ++ .../instrumentation/rmi/ClientAdvice.java | 41 +++++ .../rmi/RmiClientInstrumentation.java | 39 ++++ .../rmi/RmiServerInstrumentation.java | 37 ++++ .../instrumentation/rmi/ServerAdvice.java | 35 ++++ .../rmi/src/test/groovy/RmiTest.groovy | 174 ++++++++++++++++++ .../rmi/src/test/java/rmi/app/Greeter.java | 10 + .../rmi/src/test/java/rmi/app/Server.java | 24 +++ .../src/test/java/rmi/app/ServerLegacy.java | 24 +++ settings.gradle | 3 +- 11 files changed, 400 insertions(+), 1 deletion(-) create mode 100644 dd-java-agent/instrumentation/rmi/rmi.gradle create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ClientAdvice.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiClientInstrumentation.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ServerAdvice.java create mode 100644 dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy create mode 100644 dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Greeter.java create mode 100644 dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Server.java create mode 100644 dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/ServerLegacy.java diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java index a7b425e261..ce0a0a1c79 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java @@ -92,6 +92,7 @@ public class AgentInstaller { not( named("java.net.URL") .or(named("java.net.HttpURLConnection")) + .or(nameStartsWith("java.rmi.")) .or(nameStartsWith("java.util.concurrent.")) .or( nameStartsWith("java.util.logging.") @@ -111,6 +112,7 @@ public class AgentInstaller { .and( not( nameStartsWith("sun.net.www.protocol.") + .or(nameStartsWith("sun.rmi.server.")) .or(named("sun.net.www.http.HttpClient"))))) .or(nameStartsWith("jdk.")) .or(nameStartsWith("org.aspectj.")) diff --git a/dd-java-agent/instrumentation/rmi/rmi.gradle b/dd-java-agent/instrumentation/rmi/rmi.gradle new file mode 100644 index 0000000000..446dcca50e --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/rmi.gradle @@ -0,0 +1,12 @@ +muzzle { +} + +apply from: "${rootDir}/gradle/java.gradle" + +task "rmic", dependsOn: testClasses { + def clazz = 'rmi.app.ServerLegacy' + String command = """rmic -g -keep -classpath ${sourceSets.test.output.classesDirs.asPath} -d ${buildDir}/classes/java/test ${clazz}""" + command.execute().text +} + +test.dependsOn "rmic" diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ClientAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ClientAdvice.java new file mode 100644 index 0000000000..ec5e03d3df --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ClientAdvice.java @@ -0,0 +1,41 @@ +package datadog.trace.instrumentation.rmi; + +import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; +import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; +import static datadog.trace.instrumentation.api.AgentTracer.startSpan; + +import datadog.trace.api.DDTags; +import datadog.trace.instrumentation.api.AgentScope; +import datadog.trace.instrumentation.api.AgentSpan; +import java.lang.reflect.Method; +import net.bytebuddy.asm.Advice; + +public class ClientAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static AgentScope onEnter(@Advice.Argument(value = 1) final Method method) { + if (activeSpan() == null) { + return null; + } + final AgentSpan span = + startSpan("rmi.invoke") + .setTag( + DDTags.RESOURCE_NAME, + method.getDeclaringClass().getSimpleName() + "#" + method.getName()) + .setTag("span.origin.type", method.getDeclaringClass().getCanonicalName()); + return activateSpan(span, true); + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void stopSpan( + @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { + if (scope == null) { + return; + } + final AgentSpan span = scope.span(); + if (throwable != null) { + span.setError(true); + span.addThrowable(throwable); + } + scope.close(); + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiClientInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiClientInstrumentation.java new file mode 100644 index 0000000000..ca95bcf335 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiClientInstrumentation.java @@ -0,0 +1,39 @@ +package datadog.trace.instrumentation.rmi; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isInterface; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import java.util.Map; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +@AutoService(Instrumenter.class) +public final class RmiClientInstrumentation extends Instrumenter.Default { + + public RmiClientInstrumentation() { + super("rmi", "rmi-client"); + } + + @Override + public ElementMatcher typeMatcher() { + return not(isInterface()).and(safeHasSuperType(named("sun.rmi.server.UnicastRef"))); + } + + @Override + public Map, String> transformers() { + return singletonMap( + isMethod() + .and(named("invoke")) + .and(takesArgument(0, named("java.rmi.Remote"))) + .and(takesArgument(1, named("java.lang.reflect.Method"))), + "datadog.trace.instrumentation.rmi.ClientAdvice"); + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java new file mode 100644 index 0000000000..8531b50a28 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java @@ -0,0 +1,37 @@ +package datadog.trace.instrumentation.rmi; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isInterface; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isPublic; +import static net.bytebuddy.matcher.ElementMatchers.isStatic; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import java.util.Map; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +@AutoService(Instrumenter.class) +public final class RmiServerInstrumentation extends Instrumenter.Default { + + public RmiServerInstrumentation() { + super("rmi", "rmi-server"); + } + + @Override + public ElementMatcher typeMatcher() { + return not(isInterface()).and(safeHasSuperType(named("java.rmi.server.RemoteServer"))); + } + + @Override + public Map, String> transformers() { + return singletonMap( + isMethod().and(isPublic()).and(not(isStatic())), + "datadog.trace.instrumentation.rmi.ServerAdvice"); + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ServerAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ServerAdvice.java new file mode 100644 index 0000000000..052fe851e5 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ServerAdvice.java @@ -0,0 +1,35 @@ +package datadog.trace.instrumentation.rmi; + +import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; +import static datadog.trace.instrumentation.api.AgentTracer.startSpan; + +import datadog.trace.api.DDTags; +import datadog.trace.instrumentation.api.AgentScope; +import datadog.trace.instrumentation.api.AgentSpan; +import net.bytebuddy.asm.Advice; + +public class ServerAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static AgentScope onEnter( + @Advice.This final Object thiz, @Advice.Origin(value = "#m") final String method) { + final AgentSpan span = + startSpan("rmi.request") + .setTag(DDTags.RESOURCE_NAME, thiz.getClass().getSimpleName() + "#" + method) + .setTag("span.origin.type", thiz.getClass().getCanonicalName()); + return activateSpan(span, true); + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void stopSpan( + @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { + if (scope == null) { + return; + } + final AgentSpan span = scope.span(); + if (throwable != null) { + span.setError(true); + span.addThrowable(throwable); + } + scope.close(); + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy b/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy new file mode 100644 index 0000000000..efd1349701 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy @@ -0,0 +1,174 @@ +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.agent.test.utils.PortUtils +import rmi.app.Greeter +import rmi.app.Server +import rmi.app.ServerLegacy + +import java.rmi.registry.LocateRegistry +import java.rmi.server.UnicastRemoteObject + +import static datadog.trace.agent.test.asserts.ListWriterAssert.assertTraces +import static datadog.trace.agent.test.utils.TraceUtils.basicSpan +import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace + +class RmiTest extends AgentTestRunner { + def registryPort = PortUtils.randomOpenPort() + def serverRegistry = LocateRegistry.createRegistry(registryPort) + def clientRegistry = LocateRegistry.getRegistry("localhost", registryPort) + + def cleanup() { + UnicastRemoteObject.unexportObject(serverRegistry, true) + } + + def "Client call creates spans"() { + setup: + def server = new Server() + serverRegistry.rebind(Server.RMI_ID, server) + + when: + def response = runUnderTrace("parent") { + def client = (Greeter) clientRegistry.lookup(Server.RMI_ID) + return client.hello("you") + } + + then: + response.contains("Hello you") + assertTraces(TEST_WRITER, 2) { + trace(1, 2) { + basicSpan(it, 0, "parent") + span(1) { + resourceName "Greeter#hello" + operationName "rmi.invoke" + childOf span(0) + tags { + "span.origin.type" Greeter.canonicalName + defaultTags() + } + } + } + trace(0, 1) { + span(0) { + resourceName "Server#hello" + operationName "rmi.request" + tags { + "span.origin.type" server.class.canonicalName + defaultTags() + } + } + } + } + + cleanup: + serverRegistry.unbind("Server") + } + + def "Calling server builtin methods doesn't create server spans"() { + setup: + def server = new Server() + serverRegistry.rebind(Server.RMI_ID, server) + + when: + server.equals(new Server()) + server.getRef() + server.hashCode() + server.toString() + server.getClass() + + then: + assertTraces(TEST_WRITER, 0) {} + + cleanup: + serverRegistry.unbind("Server") + } + + def "Service throws exception and its propagated to spans"() { + setup: + def server = new Server() + serverRegistry.rebind(Server.RMI_ID, server) + + when: + runUnderTrace("parent") { + def client = (Greeter) clientRegistry.lookup(Server.RMI_ID) + client.exceptional() + } + + then: + def thrownException = thrown(RuntimeException) + assertTraces(TEST_WRITER, 2) { + trace(1, 2) { + basicSpan(it, 0, "parent", null, thrownException) + span(1) { + resourceName "Greeter#exceptional" + operationName "rmi.invoke" + childOf span(0) + errored true + + tags { + "span.origin.type" Greeter.canonicalName + errorTags(RuntimeException, String) + defaultTags() + } + } + } + trace(0, 1) { + span(0) { + resourceName "Server#exceptional" + operationName "rmi.request" + errored true + + tags { + "span.origin.type" server.class.canonicalName + errorTags(RuntimeException, String) + defaultTags() + } + } + } + } + + cleanup: + serverRegistry.unbind("Server") + } + + def "Client call using ServerLegacy_stub creates spans"() { + setup: + def server = new ServerLegacy() + serverRegistry.rebind(ServerLegacy.RMI_ID, server) + + + when: + def response = runUnderTrace("parent") { + def client = (Greeter) clientRegistry.lookup(ServerLegacy.RMI_ID) + return client.hello("you") + } + + then: + response.contains("Hello you") + assertTraces(TEST_WRITER, 2) { + trace(1, 2) { + basicSpan(it, 0, "parent") + span(1) { + resourceName "Greeter#hello" + operationName "rmi.invoke" + childOf span(0) + tags { + "span.origin.type" Greeter.canonicalName + defaultTags() + } + } + } + trace(0, 1) { + span(0) { + resourceName "ServerLegacy#hello" + operationName "rmi.request" + tags { + "span.origin.type" server.class.canonicalName + defaultTags() + } + } + } + } + + cleanup: + serverRegistry.unbind(ServerLegacy.RMI_ID) + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Greeter.java b/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Greeter.java new file mode 100644 index 0000000000..0e1d1d3fa8 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Greeter.java @@ -0,0 +1,10 @@ +package rmi.app; + +import java.rmi.Remote; +import java.rmi.RemoteException; + +public interface Greeter extends Remote { + String hello(String name) throws RemoteException; + + void exceptional() throws RemoteException, RuntimeException; +} diff --git a/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Server.java b/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Server.java new file mode 100644 index 0000000000..cc915d89bb --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Server.java @@ -0,0 +1,24 @@ +package rmi.app; + +import java.rmi.RemoteException; +import java.rmi.server.UnicastRemoteObject; + +public class Server extends UnicastRemoteObject implements Greeter { + static String RMI_ID = Server.class.getSimpleName(); + + private static final long serialVersionUID = 1L; + + public Server() throws RemoteException { + super(); + } + + @Override + public String hello(final String name) { + return "Hello " + name; + } + + @Override + public void exceptional() throws RuntimeException { + throw new RuntimeException("expected"); + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/ServerLegacy.java b/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/ServerLegacy.java new file mode 100644 index 0000000000..7635ccd5b6 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/ServerLegacy.java @@ -0,0 +1,24 @@ +package rmi.app; + +import java.rmi.RemoteException; +import java.rmi.server.UnicastRemoteObject; + +public class ServerLegacy extends UnicastRemoteObject implements Greeter { + static String RMI_ID = ServerLegacy.class.getSimpleName(); + + private static final long serialVersionUID = 1L; + + public ServerLegacy() throws RemoteException { + super(); + } + + @Override + public String hello(final String name) { + return "Hello " + name; + } + + @Override + public void exceptional() throws RuntimeException { + throw new RuntimeException("expected"); + } +} diff --git a/settings.gradle b/settings.gradle index 9bfffbd444..8a982d5967 100644 --- a/settings.gradle +++ b/settings.gradle @@ -110,8 +110,9 @@ include ':dd-java-agent:instrumentation:play-ws-2' include ':dd-java-agent:instrumentation:play-ws-2.1' include ':dd-java-agent:instrumentation:rabbitmq-amqp-2.7' include ':dd-java-agent:instrumentation:ratpack-1.4' -include ':dd-java-agent:instrumentation:rxjava-1' include ':dd-java-agent:instrumentation:reactor-core-3.1' +include ':dd-java-agent:instrumentation:rmi' +include ':dd-java-agent:instrumentation:rxjava-1' include ':dd-java-agent:instrumentation:servlet' include ':dd-java-agent:instrumentation:servlet:request-2' include ':dd-java-agent:instrumentation:servlet:request-3' From 832605a01a904e70cda1883b5470c9548f154ba6 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Mon, 9 Dec 2019 18:01:26 +0100 Subject: [PATCH 13/22] [rmi] Add distributed context propagation The flow for context propagation is as follows. *

We inject into StreamRemoteCall constructor used for invoking remote tasks and performs a * backwards compatible check to ensure if the other side is prepared to receive context propagation * messages then if successful sends a context propagation message * *

Context propagation consist of a Serialized HashMap with all data set by usual context * injection, which includes things like sampling priority, trace and parent id * *

As well as optional baggage items * *

On the other side of the communication a special Dispatcher is created when a message with * DD_CONTEXT_CALL_ID is received. * *

If the server is not instrumented first call will gracefully fail just like any other unknown * call. With small caveat that this first call needs to *not* have any parameters, since those will * not be read from connection and instead will be interpreted as another remote instruction, but * that instruction will essentially be garbage data and will cause the parsing loop to throw exception * and shutdown the connection which we do not want --- .../trace/agent/tooling/AgentInstaller.java | 3 +- .../rmi/RmiContextInstrumentation.java | 163 ++++++++++++++++++ .../rmi/RmiServerInstrumentation.java | 59 ++++++- .../instrumentation/rmi/ServerAdvice.java | 35 ---- .../rmi/context/ContextPayload.java | 36 ++++ .../StreamRemoteCallConstructorAdvice.java | 128 ++++++++++++++ .../rmi/src/test/groovy/RmiTest.groovy | 10 +- 7 files changed, 393 insertions(+), 41 deletions(-) create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiContextInstrumentation.java delete mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ServerAdvice.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java index ce0a0a1c79..8af04df97e 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java @@ -112,7 +112,8 @@ public class AgentInstaller { .and( not( nameStartsWith("sun.net.www.protocol.") - .or(nameStartsWith("sun.rmi.server.")) + .or(nameStartsWith("sun.rmi.server")) + .or(nameStartsWith("sun.rmi.transport")) .or(named("sun.net.www.http.HttpClient"))))) .or(nameStartsWith("jdk.")) .or(nameStartsWith("org.aspectj.")) diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiContextInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiContextInstrumentation.java new file mode 100644 index 0000000000..90ad6cc154 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiContextInstrumentation.java @@ -0,0 +1,163 @@ +package datadog.trace.instrumentation.rmi; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.api.AgentTracer.propagate; +import static datadog.trace.instrumentation.rmi.context.ContextPayload.GETTER; +import static datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstructorAdvice.DD_CONTEXT_CALL_ID; +import static net.bytebuddy.matcher.ElementMatchers.isConstructor; +import static net.bytebuddy.matcher.ElementMatchers.isInterface; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isStatic; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.bootstrap.ContextStore; +import datadog.trace.bootstrap.InstrumentationContext; +import datadog.trace.instrumentation.api.AgentSpan; +import datadog.trace.instrumentation.rmi.context.ContextPayload; +import datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstructorAdvice; +import java.io.IOException; +import java.io.ObjectInput; +import java.lang.reflect.Field; +import java.rmi.Remote; +import java.rmi.server.ObjID; +import java.rmi.server.RemoteCall; +import java.util.Collections; +import java.util.HashMap; +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 sun.rmi.server.Dispatcher; +import sun.rmi.transport.ObjectTable; +import sun.rmi.transport.StreamRemoteCall; +import sun.rmi.transport.Target; + +@AutoService(Instrumenter.class) +public class RmiContextInstrumentation extends Instrumenter.Default { + // TODO clean this up + + public RmiContextInstrumentation() { + super("rmi", "rmi-context-propagator"); + } + + @Override + public ElementMatcher typeMatcher() { + return not(isInterface()) + .and( + safeHasSuperType( + named(StreamRemoteCall.class.getName()).or(named(ObjectTable.class.getName())))); + } + + @Override + public Map contextStore() { + return Collections.singletonMap(Thread.class.getName(), AgentSpan.Context.class.getName()); + } + + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstructorAdvice", + "datadog.trace.instrumentation.rmi.context.ContextPayload", + "datadog.trace.instrumentation.rmi.context.ContextPayload$InjectAdapter", + "datadog.trace.instrumentation.rmi.context.ContextPayload$ExtractAdapter", + "datadog.trace.instrumentation.rmi.RmiContextInstrumentation$ObjectTableAdvice", + "datadog.trace.instrumentation.rmi.RmiContextInstrumentation$ContextDispatcher", + "datadog.trace.instrumentation.rmi.RmiContextInstrumentation$ObjectTableAdvice$DummyRemote" + }; + } + + @Override + public Map, String> transformers() { + final Map, String> transformers = new HashMap<>(); + // TODO make this more specific + transformers.put( + isConstructor() + .and(takesArgument(0, named("sun.rmi.transport.Connection"))) + .and(takesArgument(1, named("java.rmi.server.ObjID"))), + "datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstructorAdvice"); + + transformers.put( + isMethod().and(isStatic()).and(named("getTarget")), + getClass().getName() + "$ObjectTableAdvice"); + return transformers; + } + + public static class ObjectTableAdvice { + public static final DummyRemote DUMMY_REMOTE = new DummyRemote(); + + @Advice.OnMethodExit(suppress = Throwable.class) + public static void methodExit( + @Advice.Argument(0) final Object oe, @Advice.Return(readOnly = false) Target result) { + final ObjID objID = GET_OBJ_ID(oe); + if (!DD_CONTEXT_CALL_ID.equals(objID)) { + return; + } + final ContextStore callableContextStore = + InstrumentationContext.get(Thread.class, AgentSpan.Context.class); + + result = + new Target( + DUMMY_REMOTE, + new ContextDispatcher(callableContextStore), + DUMMY_REMOTE, + objID, + false); + } + + public static ObjID GET_OBJ_ID(final Object oe) { + try { + final Class clazz = oe.getClass(); + // sun.rmi.transport.ObjectEndpoint is protected and field "id" is private + final Field id = clazz.getDeclaredField("id"); + id.setAccessible(true); + return (ObjID) id.get(oe); + } catch (final ReflectiveOperationException e) { + // TODO: log it + } + return null; + } + + public static class DummyRemote implements Remote {} + } + + public static class ContextDispatcher implements Dispatcher { + + private final ContextStore callableContextStore; + + public ContextDispatcher(final ContextStore callableContextStore) { + this.callableContextStore = callableContextStore; + } + + @Override + public void dispatch(final Remote obj, final RemoteCall call) throws IOException { + final ObjectInput in = call.getInputStream(); + final int operationId = in.readInt(); + in.readLong(); // skip 8 bytes + + if (operationId == StreamRemoteCallConstructorAdvice.CONTEXT_PASS_OPERATION_ID) { + try { + final Object payload = in.readObject(); + if (payload instanceof ContextPayload) { + final AgentSpan.Context context = propagate().extract((ContextPayload) payload, GETTER); + callableContextStore.put(Thread.currentThread(), context); + } + } catch (final ClassNotFoundException e) { + // TODO log e.printStackTrace(); + } + } + + // send result stream the client is expecting + call.getResultStream(true); + + // release held streams to allow next call to continue + call.releaseInputStream(); + call.releaseOutputStream(); + call.done(); + } + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java index 8531b50a28..fc6de537a0 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java @@ -1,6 +1,8 @@ package datadog.trace.instrumentation.rmi; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; +import static datadog.trace.instrumentation.api.AgentTracer.startSpan; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; @@ -11,7 +13,14 @@ import static net.bytebuddy.matcher.ElementMatchers.not; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.api.DDTags; +import datadog.trace.bootstrap.ContextStore; +import datadog.trace.bootstrap.InstrumentationContext; +import datadog.trace.instrumentation.api.AgentScope; +import datadog.trace.instrumentation.api.AgentSpan; +import java.util.Collections; 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; @@ -23,6 +32,20 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { super("rmi", "rmi-server"); } + @Override + public Map contextStore() { + return Collections.singletonMap(Thread.class.getName(), AgentSpan.Context.class.getName()); + } + + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.instrumentation.rmi.context.ContextPayload$InjectAdapter", + "datadog.trace.instrumentation.rmi.context.ContextPayload$ExtractAdapter", + "datadog.trace.instrumentation.rmi.context.ContextPayload", + }; + } + @Override public ElementMatcher typeMatcher() { return not(isInterface()).and(safeHasSuperType(named("java.rmi.server.RemoteServer"))); @@ -32,6 +55,40 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { public Map, String> transformers() { return singletonMap( isMethod().and(isPublic()).and(not(isStatic())), - "datadog.trace.instrumentation.rmi.ServerAdvice"); + "datadog.trace.instrumentation.rmi.RmiServerInstrumentation$ServerAdvice"); + } + + public static class ServerAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class, inline = true) + public static AgentScope onEnter( + @Advice.This final Object thiz, @Advice.Origin(value = "#m") final String method) { + final ContextStore callableContextStore = + InstrumentationContext.get(Thread.class, AgentSpan.Context.class); + final AgentSpan.Context context = callableContextStore.get(Thread.currentThread()); + + final AgentSpan span; + if (context == null) { + span = startSpan("rmi.request"); + } else { + span = startSpan("rmi.request", context); + } + span.setTag(DDTags.RESOURCE_NAME, thiz.getClass().getSimpleName() + "#" + method); + span.setTag("span.origin.type", thiz.getClass().getCanonicalName()); + return activateSpan(span, true); + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void stopSpan( + @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { + if (scope == null) { + return; + } + final AgentSpan span = scope.span(); + if (throwable != null) { + span.setError(true); + span.addThrowable(throwable); + } + scope.close(); + } } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ServerAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ServerAdvice.java deleted file mode 100644 index 052fe851e5..0000000000 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ServerAdvice.java +++ /dev/null @@ -1,35 +0,0 @@ -package datadog.trace.instrumentation.rmi; - -import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; -import static datadog.trace.instrumentation.api.AgentTracer.startSpan; - -import datadog.trace.api.DDTags; -import datadog.trace.instrumentation.api.AgentScope; -import datadog.trace.instrumentation.api.AgentSpan; -import net.bytebuddy.asm.Advice; - -public class ServerAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) - public static AgentScope onEnter( - @Advice.This final Object thiz, @Advice.Origin(value = "#m") final String method) { - final AgentSpan span = - startSpan("rmi.request") - .setTag(DDTags.RESOURCE_NAME, thiz.getClass().getSimpleName() + "#" + method) - .setTag("span.origin.type", thiz.getClass().getCanonicalName()); - return activateSpan(span, true); - } - - @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - public static void stopSpan( - @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { - if (scope == null) { - return; - } - final AgentSpan span = scope.span(); - if (throwable != null) { - span.setError(true); - span.addThrowable(throwable); - } - scope.close(); - } -} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java new file mode 100644 index 0000000000..68dde3963d --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java @@ -0,0 +1,36 @@ +package datadog.trace.instrumentation.rmi.context; + +import datadog.trace.instrumentation.api.AgentPropagation; +import java.io.Serializable; +import java.util.HashMap; +import java.util.Map; +import lombok.Getter; + +public class ContextPayload implements Serializable { + @Getter private final Map context; + public static final ExtractAdapter GETTER = new ExtractAdapter(); + public static final InjectAdapter SETTER = new InjectAdapter(); + + public ContextPayload() { + context = new HashMap<>(); + } + + public static class ExtractAdapter implements AgentPropagation.Getter { + @Override + public Iterable keys(final ContextPayload carrier) { + return carrier.getContext().keySet(); + } + + @Override + public String get(final ContextPayload carrier, final String key) { + return carrier.getContext().get(key); + } + } + + public static class InjectAdapter implements AgentPropagation.Setter { + @Override + public void set(final ContextPayload carrier, final String key, final String value) { + carrier.getContext().put(key, value); + } + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java new file mode 100644 index 0000000000..5904f49263 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java @@ -0,0 +1,128 @@ +package datadog.trace.instrumentation.rmi.context; + +import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; +import static datadog.trace.instrumentation.api.AgentTracer.propagate; +import static datadog.trace.instrumentation.rmi.context.ContextPayload.SETTER; + +import datadog.trace.instrumentation.api.AgentSpan; +import java.io.IOException; +import java.io.ObjectOutput; +import java.rmi.NoSuchObjectException; +import java.rmi.server.ObjID; +import net.bytebuddy.asm.Advice; +import sun.rmi.transport.Connection; +import sun.rmi.transport.StreamRemoteCall; +import sun.rmi.transport.TransportConstants; + +/** + * Main entry point for transferring context between RMI service. + * + *

It injects into StreamRemoteCall constructor used for invoking remote tasks and performs a + * backwards compatible check to ensure if the other side is prepared to receive context propagation + * messages then if successful sends a context propagation message + * + *

Context propagation consist of a Serialized HashMap with all data set by usual context + * injection, which includes things like sampling priority, trace and parent id + * + *

As well as optional baggage items + * + *

On the other side of the communication a special Dispatcher is created when a message with + * DD_CONTEXT_CALL_ID is received. + * + *

If the server is not instrumented first call will gracefully fail just like any other unknown + * call. With small caveat that this first call needs to *not* have any parameters, since those will + * not be read from connection and instead will be interpreted as another remote instruction, but + * that instruction will essentially be garbage data and will cause the parsing loop to throw + * exception and shutdown the connection which we do not want + */ +public class StreamRemoteCallConstructorAdvice { + public static final ObjID ACTIVATOR_ID = new ObjID(ObjID.ACTIVATOR_ID); + public static final ObjID DGC_ID = new ObjID(ObjID.DGC_ID); + public static final ObjID REGISTRY_ID = new ObjID(ObjID.REGISTRY_ID); + public static final ObjID DD_CONTEXT_CALL_ID = new ObjID("Datadog.context_call".hashCode()); + public static final int CONTEXT_CHECK_CALL_OP_ID = -1; + public static final int CONTEXT_PASS_OPERATION_ID = -2; + public static ThreadLocal internalCall = new ThreadLocal<>(); + + static { + internalCall.set(false); + } + + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void onEnter( + @Advice.Argument(value = 0) final Connection c, @Advice.Argument(value = 1) final ObjID id) { + if (!c.isReusable()) { + return; + } + if (isRMIInternalObject(id)) { + return; + } + + attemptToPropagateContext(c); + } + + public static boolean isRMIInternalObject(final ObjID id) { + return ACTIVATOR_ID.equals(id) || DGC_ID.equals(id) || REGISTRY_ID.equals(id); + } + + public static void attemptToPropagateContext(final Connection c) { + final AgentSpan span = activeSpan(); + if (span == null) { + return; + } + + if (checkIfContextCanBePassed(c)) { + final ContextPayload payload = new ContextPayload(); + propagate().inject(span, payload, SETTER); + syntheticCall(c, payload, CONTEXT_PASS_OPERATION_ID); + } + } + + private static boolean checkIfContextCanBePassed(final Connection c) { + // TODO memorize this per connection to avoid unnecessary overhead + return syntheticCall(c, null, CONTEXT_CHECK_CALL_OP_ID); + } + + private static boolean syntheticCall( + final Connection c, final Object payload, final int operationId) { + final StreamRemoteCall shareContextCall = new StreamRemoteCall(c); + try { + c.getOutputStream().write(TransportConstants.Call); + + final ObjectOutput out = shareContextCall.getOutputStream(); + + DD_CONTEXT_CALL_ID.write(out); + + // call header, part 2 (read by Dispatcher) + out.writeInt(operationId); // method number (operation index) + out.writeLong(operationId); // stub/skeleton hash + + if (payload != null) { + out.writeObject(payload); + } + + try { + shareContextCall.executeCall(); + } catch (final Exception e) { + final Exception ex = shareContextCall.getServerException(); + if (ex != null) { + if (ex instanceof NoSuchObjectException) { + return false; + } else { + // TODO: log ex.printStackTrace(); + } + } else { + // TODO: log ex.printStackTrace(); + } + return false; + } finally { + shareContextCall.done(); + } + + } catch (final IOException e) { + // TODO: log ex.printStackTrace(); + return false; + } + return true; + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy b/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy index efd1349701..a0945354e2 100644 --- a/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy +++ b/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy @@ -52,7 +52,7 @@ class RmiTest extends AgentTestRunner { operationName "rmi.request" tags { "span.origin.type" server.class.canonicalName - defaultTags() + defaultTags(true) } } } @@ -119,7 +119,7 @@ class RmiTest extends AgentTestRunner { tags { "span.origin.type" server.class.canonicalName errorTags(RuntimeException, String) - defaultTags() + defaultTags(true) } } } @@ -134,7 +134,6 @@ class RmiTest extends AgentTestRunner { def server = new ServerLegacy() serverRegistry.rebind(ServerLegacy.RMI_ID, server) - when: def response = runUnderTrace("parent") { def client = (Greeter) clientRegistry.lookup(ServerLegacy.RMI_ID) @@ -144,6 +143,7 @@ class RmiTest extends AgentTestRunner { then: response.contains("Hello you") assertTraces(TEST_WRITER, 2) { + def parentSpan = TEST_WRITER[1][1] trace(1, 2) { basicSpan(it, 0, "parent") span(1) { @@ -156,13 +156,15 @@ class RmiTest extends AgentTestRunner { } } } + trace(0, 1) { span(0) { + childOf parentSpan resourceName "ServerLegacy#hello" operationName "rmi.request" tags { "span.origin.type" server.class.canonicalName - defaultTags() + defaultTags(true) } } } From bb05700806d4d2c8ed7e97986054fb8166d93f29 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Fri, 13 Dec 2019 10:41:11 +0100 Subject: [PATCH 14/22] [rmi] Improve connection overhead and add additional metadata - Cache response per connection to ensure as little communication overhead as possible is needed - change context payload serialization to use plain hashmap + add more logging - only set parent context on root entry point + use decorator to create spans - Use Instrumentation context to avoid reflection - separate integration parts into packages --- dd-java-agent/instrumentation/rmi/rmi.gradle | 5 + .../rmi/RmiContextInstrumentation.java | 163 ------------------ .../rmi/{ => client}/ClientAdvice.java | 11 +- .../rmi/client/ClientDecorator.java | 23 +++ .../RmiClientInstrumentation.java | 11 +- .../rmi/context/ContextDispatcher.java | 45 +++++ .../rmi/context/ContextPayload.java | 26 +++ .../ObjectEndpointConstructorAdvice.java | 15 ++ .../rmi/context/ObjectTableAdvice.java | 50 ++++++ .../context/RmiContextInstrumentation.java | 93 ++++++++++ .../StreamRemoteCallConstructorAdvice.java | 55 +++--- .../RmiServerInstrumentation.java | 43 +++-- .../rmi/server/ServerDecorator.java | 23 +++ .../rmi/src/test/groovy/RmiTest.groovy | 28 ++- .../rmi/src/test/java/rmi/app/Server.java | 6 +- 15 files changed, 387 insertions(+), 210 deletions(-) delete mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiContextInstrumentation.java rename dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/{ => client}/ClientAdvice.java (86%) create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientDecorator.java rename dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/{ => client}/RmiClientInstrumentation.java (83%) create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextDispatcher.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectEndpointConstructorAdvice.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java rename dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/{ => server}/RmiServerInstrumentation.java (72%) create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/ServerDecorator.java diff --git a/dd-java-agent/instrumentation/rmi/rmi.gradle b/dd-java-agent/instrumentation/rmi/rmi.gradle index 446dcca50e..4c8088bea7 100644 --- a/dd-java-agent/instrumentation/rmi/rmi.gradle +++ b/dd-java-agent/instrumentation/rmi/rmi.gradle @@ -1,4 +1,9 @@ muzzle { + pass { + group = "javax.ws.rs" + module = "jsr311-api" + versions = "[0.5,)" + } } apply from: "${rootDir}/gradle/java.gradle" diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiContextInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiContextInstrumentation.java deleted file mode 100644 index 90ad6cc154..0000000000 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiContextInstrumentation.java +++ /dev/null @@ -1,163 +0,0 @@ -package datadog.trace.instrumentation.rmi; - -import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; -import static datadog.trace.instrumentation.api.AgentTracer.propagate; -import static datadog.trace.instrumentation.rmi.context.ContextPayload.GETTER; -import static datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstructorAdvice.DD_CONTEXT_CALL_ID; -import static net.bytebuddy.matcher.ElementMatchers.isConstructor; -import static net.bytebuddy.matcher.ElementMatchers.isInterface; -import static net.bytebuddy.matcher.ElementMatchers.isMethod; -import static net.bytebuddy.matcher.ElementMatchers.isStatic; -import static net.bytebuddy.matcher.ElementMatchers.named; -import static net.bytebuddy.matcher.ElementMatchers.not; -import static net.bytebuddy.matcher.ElementMatchers.takesArgument; - -import com.google.auto.service.AutoService; -import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.bootstrap.ContextStore; -import datadog.trace.bootstrap.InstrumentationContext; -import datadog.trace.instrumentation.api.AgentSpan; -import datadog.trace.instrumentation.rmi.context.ContextPayload; -import datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstructorAdvice; -import java.io.IOException; -import java.io.ObjectInput; -import java.lang.reflect.Field; -import java.rmi.Remote; -import java.rmi.server.ObjID; -import java.rmi.server.RemoteCall; -import java.util.Collections; -import java.util.HashMap; -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 sun.rmi.server.Dispatcher; -import sun.rmi.transport.ObjectTable; -import sun.rmi.transport.StreamRemoteCall; -import sun.rmi.transport.Target; - -@AutoService(Instrumenter.class) -public class RmiContextInstrumentation extends Instrumenter.Default { - // TODO clean this up - - public RmiContextInstrumentation() { - super("rmi", "rmi-context-propagator"); - } - - @Override - public ElementMatcher typeMatcher() { - return not(isInterface()) - .and( - safeHasSuperType( - named(StreamRemoteCall.class.getName()).or(named(ObjectTable.class.getName())))); - } - - @Override - public Map contextStore() { - return Collections.singletonMap(Thread.class.getName(), AgentSpan.Context.class.getName()); - } - - @Override - public String[] helperClassNames() { - return new String[] { - "datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstructorAdvice", - "datadog.trace.instrumentation.rmi.context.ContextPayload", - "datadog.trace.instrumentation.rmi.context.ContextPayload$InjectAdapter", - "datadog.trace.instrumentation.rmi.context.ContextPayload$ExtractAdapter", - "datadog.trace.instrumentation.rmi.RmiContextInstrumentation$ObjectTableAdvice", - "datadog.trace.instrumentation.rmi.RmiContextInstrumentation$ContextDispatcher", - "datadog.trace.instrumentation.rmi.RmiContextInstrumentation$ObjectTableAdvice$DummyRemote" - }; - } - - @Override - public Map, String> transformers() { - final Map, String> transformers = new HashMap<>(); - // TODO make this more specific - transformers.put( - isConstructor() - .and(takesArgument(0, named("sun.rmi.transport.Connection"))) - .and(takesArgument(1, named("java.rmi.server.ObjID"))), - "datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstructorAdvice"); - - transformers.put( - isMethod().and(isStatic()).and(named("getTarget")), - getClass().getName() + "$ObjectTableAdvice"); - return transformers; - } - - public static class ObjectTableAdvice { - public static final DummyRemote DUMMY_REMOTE = new DummyRemote(); - - @Advice.OnMethodExit(suppress = Throwable.class) - public static void methodExit( - @Advice.Argument(0) final Object oe, @Advice.Return(readOnly = false) Target result) { - final ObjID objID = GET_OBJ_ID(oe); - if (!DD_CONTEXT_CALL_ID.equals(objID)) { - return; - } - final ContextStore callableContextStore = - InstrumentationContext.get(Thread.class, AgentSpan.Context.class); - - result = - new Target( - DUMMY_REMOTE, - new ContextDispatcher(callableContextStore), - DUMMY_REMOTE, - objID, - false); - } - - public static ObjID GET_OBJ_ID(final Object oe) { - try { - final Class clazz = oe.getClass(); - // sun.rmi.transport.ObjectEndpoint is protected and field "id" is private - final Field id = clazz.getDeclaredField("id"); - id.setAccessible(true); - return (ObjID) id.get(oe); - } catch (final ReflectiveOperationException e) { - // TODO: log it - } - return null; - } - - public static class DummyRemote implements Remote {} - } - - public static class ContextDispatcher implements Dispatcher { - - private final ContextStore callableContextStore; - - public ContextDispatcher(final ContextStore callableContextStore) { - this.callableContextStore = callableContextStore; - } - - @Override - public void dispatch(final Remote obj, final RemoteCall call) throws IOException { - final ObjectInput in = call.getInputStream(); - final int operationId = in.readInt(); - in.readLong(); // skip 8 bytes - - if (operationId == StreamRemoteCallConstructorAdvice.CONTEXT_PASS_OPERATION_ID) { - try { - final Object payload = in.readObject(); - if (payload instanceof ContextPayload) { - final AgentSpan.Context context = propagate().extract((ContextPayload) payload, GETTER); - callableContextStore.put(Thread.currentThread(), context); - } - } catch (final ClassNotFoundException e) { - // TODO log e.printStackTrace(); - } - } - - // send result stream the client is expecting - call.getResultStream(true); - - // release held streams to allow next call to continue - call.releaseInputStream(); - call.releaseOutputStream(); - call.done(); - } - } -} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ClientAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientAdvice.java similarity index 86% rename from dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ClientAdvice.java rename to dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientAdvice.java index ec5e03d3df..9504bb828a 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/ClientAdvice.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientAdvice.java @@ -1,8 +1,9 @@ -package datadog.trace.instrumentation.rmi; +package datadog.trace.instrumentation.rmi.client; import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.instrumentation.api.AgentTracer.startSpan; +import static datadog.trace.instrumentation.rmi.client.ClientDecorator.DECORATE; import datadog.trace.api.DDTags; import datadog.trace.instrumentation.api.AgentScope; @@ -22,6 +23,8 @@ public class ClientAdvice { DDTags.RESOURCE_NAME, method.getDeclaringClass().getSimpleName() + "#" + method.getName()) .setTag("span.origin.type", method.getDeclaringClass().getCanonicalName()); + + DECORATE.afterStart(span); return activateSpan(span, true); } @@ -31,11 +34,7 @@ public class ClientAdvice { if (scope == null) { return; } - final AgentSpan span = scope.span(); - if (throwable != null) { - span.setError(true); - span.addThrowable(throwable); - } + DECORATE.onError(scope, throwable); scope.close(); } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientDecorator.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientDecorator.java new file mode 100644 index 0000000000..b07029e04c --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientDecorator.java @@ -0,0 +1,23 @@ +package datadog.trace.instrumentation.rmi.client; + +import datadog.trace.agent.decorator.BaseDecorator; +import datadog.trace.api.DDSpanTypes; + +public class ClientDecorator extends BaseDecorator { + public static final ClientDecorator DECORATE = new ClientDecorator(); + + @Override + protected String[] instrumentationNames() { + return new String[] {"rmi"}; + } + + @Override + protected String spanType() { + return DDSpanTypes.RPC; + } + + @Override + protected String component() { + return "rmi-client"; + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiClientInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java similarity index 83% rename from dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiClientInstrumentation.java rename to dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java index ca95bcf335..b5f6243113 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiClientInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java @@ -1,4 +1,4 @@ -package datadog.trace.instrumentation.rmi; +package datadog.trace.instrumentation.rmi.client; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; import static java.util.Collections.singletonMap; @@ -27,6 +27,13 @@ public final class RmiClientInstrumentation extends Instrumenter.Default { return not(isInterface()).and(safeHasSuperType(named("sun.rmi.server.UnicastRef"))); } + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".ClientDecorator", "datadog.trace.agent.decorator.BaseDecorator" + }; + } + @Override public Map, String> transformers() { return singletonMap( @@ -34,6 +41,6 @@ public final class RmiClientInstrumentation extends Instrumenter.Default { .and(named("invoke")) .and(takesArgument(0, named("java.rmi.Remote"))) .and(takesArgument(1, named("java.lang.reflect.Method"))), - "datadog.trace.instrumentation.rmi.ClientAdvice"); + packageName + ".ClientAdvice"); } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextDispatcher.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextDispatcher.java new file mode 100644 index 0000000000..b676ab51e6 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextDispatcher.java @@ -0,0 +1,45 @@ +package datadog.trace.instrumentation.rmi.context; + +import static datadog.trace.instrumentation.api.AgentTracer.propagate; +import static datadog.trace.instrumentation.rmi.context.ContextPayload.GETTER; + +import datadog.trace.bootstrap.ContextStore; +import datadog.trace.instrumentation.api.AgentSpan; +import java.io.IOException; +import java.io.ObjectInput; +import java.rmi.Remote; +import java.rmi.server.RemoteCall; +import sun.rmi.server.Dispatcher; + +public class ContextDispatcher implements Dispatcher { + + private final ContextStore callableContextStore; + + public ContextDispatcher(final ContextStore callableContextStore) { + this.callableContextStore = callableContextStore; + } + + @Override + public void dispatch(final Remote obj, final RemoteCall call) throws IOException { + final ObjectInput in = call.getInputStream(); + final int operationId = in.readInt(); + in.readLong(); // skip 8 bytes + + if (operationId == StreamRemoteCallConstructorAdvice.CONTEXT_PASS_OPERATION_ID) { + final ContextPayload payload = ContextPayload.read(in); + if (payload != null) { + final AgentSpan.Context context = propagate().extract(payload, GETTER); + + callableContextStore.put(Thread.currentThread(), context); + } + } + + // send result stream the client is expecting + call.getResultStream(true); + + // release held streams to allow next call to continue + call.releaseInputStream(); + call.releaseOutputStream(); + call.done(); + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java index 68dde3963d..25de0574c2 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java @@ -1,11 +1,16 @@ package datadog.trace.instrumentation.rmi.context; import datadog.trace.instrumentation.api.AgentPropagation; +import java.io.IOException; +import java.io.ObjectInput; +import java.io.ObjectOutput; import java.io.Serializable; import java.util.HashMap; import java.util.Map; import lombok.Getter; +import lombok.extern.slf4j.Slf4j; +@Slf4j public class ContextPayload implements Serializable { @Getter private final Map context; public static final ExtractAdapter GETTER = new ExtractAdapter(); @@ -15,6 +20,27 @@ public class ContextPayload implements Serializable { context = new HashMap<>(); } + public ContextPayload(final Map context) { + this.context = context; + } + + public static ContextPayload read(final ObjectInput oi) throws IOException { + try { + final Object object = oi.readObject(); + if (object instanceof Map) { + return new ContextPayload((Map) object); + } + } catch (final ClassCastException | ClassNotFoundException ex) { + log.debug("Error reading object", ex); + } + + return null; + } + + public void write(final ObjectOutput out) throws IOException { + out.writeObject(context); + } + public static class ExtractAdapter implements AgentPropagation.Getter { @Override public Iterable keys(final ContextPayload carrier) { diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectEndpointConstructorAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectEndpointConstructorAdvice.java new file mode 100644 index 0000000000..13e1ebc24c --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectEndpointConstructorAdvice.java @@ -0,0 +1,15 @@ +package datadog.trace.instrumentation.rmi.context; + +import datadog.trace.bootstrap.InstrumentationContext; +import java.rmi.server.ObjID; +import net.bytebuddy.asm.Advice; + +public class ObjectEndpointConstructorAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) + public static void onEnter( + @Advice.This final Object thiz, @Advice.Argument(value = 0) final ObjID id) { + if (id != null) { + InstrumentationContext.get(Object.class, ObjID.class).put(thiz, id); + } + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java new file mode 100644 index 0000000000..9d26b8cc48 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java @@ -0,0 +1,50 @@ +package datadog.trace.instrumentation.rmi.context; + +import static datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstructorAdvice.DD_CONTEXT_CALL_ID; + +import datadog.trace.bootstrap.ContextStore; +import datadog.trace.bootstrap.InstrumentationContext; +import datadog.trace.instrumentation.api.AgentSpan; +import java.lang.reflect.Field; +import java.rmi.Remote; +import java.rmi.server.ObjID; +import lombok.extern.slf4j.Slf4j; +import net.bytebuddy.asm.Advice; +import sun.rmi.transport.Target; + +@Slf4j +public class ObjectTableAdvice { + public static final DummyRemote DUMMY_REMOTE = new DummyRemote(); + + @Advice.OnMethodExit(suppress = Throwable.class) + public static void methodExit( + @Advice.Argument(0) final Object oe, @Advice.Return(readOnly = false) Target result) { + final ObjID objID = InstrumentationContext.get(Object.class, ObjID.class).get(oe); + + if (!DD_CONTEXT_CALL_ID.equals(objID)) { + return; + } + + final ContextStore callableContextStore = + InstrumentationContext.get(Thread.class, AgentSpan.Context.class); + + result = + new Target( + DUMMY_REMOTE, new ContextDispatcher(callableContextStore), DUMMY_REMOTE, objID, false); + } + + public static ObjID GET_OBJ_ID(final Object oe) { + try { + final Class clazz = oe.getClass(); + // sun.rmi.transport.ObjectEndpoint is protected and field "id" is private + final Field id = clazz.getDeclaredField("id"); + id.setAccessible(true); + return (ObjID) id.get(oe); + } catch (final ReflectiveOperationException e) { + log.debug("Error getting object id from: {}", oe, e); + } + return null; + } + + public static class DummyRemote implements Remote {} +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java new file mode 100644 index 0000000000..1d2d537cb8 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java @@ -0,0 +1,93 @@ +package datadog.trace.instrumentation.rmi.context; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static net.bytebuddy.matcher.ElementMatchers.isConstructor; +import static net.bytebuddy.matcher.ElementMatchers.isDeclaredBy; +import static net.bytebuddy.matcher.ElementMatchers.isInterface; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isStatic; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.instrumentation.api.AgentSpan; +import java.rmi.server.ObjID; +import java.util.HashMap; +import java.util.Map; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import sun.rmi.transport.Connection; +import sun.rmi.transport.ObjectTable; +import sun.rmi.transport.StreamRemoteCall; + +@AutoService(Instrumenter.class) +public class RmiContextInstrumentation extends Instrumenter.Default { + // TODO clean this up + + public RmiContextInstrumentation() { + super("rmi", "rmi-context-propagator"); + } + + @Override + public ElementMatcher typeMatcher() { + return not(isInterface()) + .and( + safeHasSuperType( + named(StreamRemoteCall.class.getName()) // TODO replace with string + .or(named(ObjectTable.class.getName())) + .or(named("sun.rmi.transport.ObjectEndpoint")))); + } + + @Override + public Map contextStore() { + final HashMap contextStore = new HashMap<>(); + // thread context that stores distributed context + contextStore.put(Thread.class.getName(), AgentSpan.Context.class.getName()); + + // caching if a connection can support enhanced format + contextStore.put(Connection.class.getName(), Boolean.class.getName()); + + // used to avoid reflection when instrumenting protected class ObjectEndpoint + contextStore.put(Object.class.getName(), ObjID.class.getName()); + return contextStore; + } + + @Override + public String[] helperClassNames() { + return new String[] { + packageName + ".StreamRemoteCallConstructorAdvice", + packageName + ".ContextPayload", + packageName + ".ContextPayload$InjectAdapter", + packageName + ".ContextPayload$ExtractAdapter", + packageName + ".ObjectTableAdvice", + packageName + ".ContextDispatcher", + packageName + ".ObjectEndpointConstructorAdvice", + packageName + ".ObjectTableAdvice$DummyRemote" + }; + } + + @Override + public Map, String> transformers() { + final Map, String> transformers = new HashMap<>(); + transformers.put( + isConstructor() + .and(takesArgument(0, named("sun.rmi.transport.Connection"))) + .and(takesArgument(1, named("java.rmi.server.ObjID"))), + packageName + ".StreamRemoteCallConstructorAdvice"); + + transformers.put( + isMethod() + .and(isStatic()) + .and(named("getTarget")) + .and((takesArgument(0, named("sun.rmi.transport.ObjectEndpoint")))), + packageName + ".ObjectTableAdvice"); + + transformers.put( + isConstructor().and(isDeclaredBy(named("sun.rmi.transport.ObjectEndpoint"))), + packageName + ".ObjectEndpointConstructorAdvice"); + return transformers; + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java index 5904f49263..992c147f00 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java @@ -4,11 +4,14 @@ import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.instrumentation.api.AgentTracer.propagate; import static datadog.trace.instrumentation.rmi.context.ContextPayload.SETTER; +import datadog.trace.bootstrap.ContextStore; +import datadog.trace.bootstrap.InstrumentationContext; import datadog.trace.instrumentation.api.AgentSpan; import java.io.IOException; import java.io.ObjectOutput; import java.rmi.NoSuchObjectException; import java.rmi.server.ObjID; +import lombok.extern.slf4j.Slf4j; import net.bytebuddy.asm.Advice; import sun.rmi.transport.Connection; import sun.rmi.transport.StreamRemoteCall; @@ -35,18 +38,14 @@ import sun.rmi.transport.TransportConstants; * that instruction will essentially be garbage data and will cause the parsing loop to throw * exception and shutdown the connection which we do not want */ +@Slf4j public class StreamRemoteCallConstructorAdvice { public static final ObjID ACTIVATOR_ID = new ObjID(ObjID.ACTIVATOR_ID); public static final ObjID DGC_ID = new ObjID(ObjID.DGC_ID); public static final ObjID REGISTRY_ID = new ObjID(ObjID.REGISTRY_ID); - public static final ObjID DD_CONTEXT_CALL_ID = new ObjID("Datadog.context_call".hashCode()); + public static final ObjID DD_CONTEXT_CALL_ID = new ObjID("Datadog.context_call.v2".hashCode()); public static final int CONTEXT_CHECK_CALL_OP_ID = -1; public static final int CONTEXT_PASS_OPERATION_ID = -2; - public static ThreadLocal internalCall = new ThreadLocal<>(); - - static { - internalCall.set(false); - } @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter( @@ -58,33 +57,45 @@ public class StreamRemoteCallConstructorAdvice { return; } - attemptToPropagateContext(c); + final ContextStore contextStore = + InstrumentationContext.get(Connection.class, Boolean.class); + attemptToPropagateContext(contextStore, c); } public static boolean isRMIInternalObject(final ObjID id) { return ACTIVATOR_ID.equals(id) || DGC_ID.equals(id) || REGISTRY_ID.equals(id); } - public static void attemptToPropagateContext(final Connection c) { + public static void attemptToPropagateContext( + final ContextStore contextStore, final Connection c) { final AgentSpan span = activeSpan(); if (span == null) { return; } - if (checkIfContextCanBePassed(c)) { + if (checkIfContextCanBePassed(contextStore, c)) { final ContextPayload payload = new ContextPayload(); propagate().inject(span, payload, SETTER); - syntheticCall(c, payload, CONTEXT_PASS_OPERATION_ID); + if (!syntheticCall(c, payload, CONTEXT_PASS_OPERATION_ID)) { + log.debug("Couldn't propagate context"); + } } } - private static boolean checkIfContextCanBePassed(final Connection c) { - // TODO memorize this per connection to avoid unnecessary overhead - return syntheticCall(c, null, CONTEXT_CHECK_CALL_OP_ID); + private static boolean checkIfContextCanBePassed( + final ContextStore contextStore, final Connection c) { + final Boolean storedResult = contextStore.get(c); + if (storedResult != null) { + return storedResult; + } + + final boolean result = syntheticCall(c, null, CONTEXT_CHECK_CALL_OP_ID); + contextStore.put(c, result); + return result; } private static boolean syntheticCall( - final Connection c, final Object payload, final int operationId) { + final Connection c, final ContextPayload payload, final int operationId) { final StreamRemoteCall shareContextCall = new StreamRemoteCall(c); try { c.getOutputStream().write(TransportConstants.Call); @@ -94,11 +105,15 @@ public class StreamRemoteCallConstructorAdvice { DD_CONTEXT_CALL_ID.write(out); // call header, part 2 (read by Dispatcher) - out.writeInt(operationId); // method number (operation index) - out.writeLong(operationId); // stub/skeleton hash + out.writeInt(operationId); // in normal call this is method number (operation index) + out.writeLong(operationId); // in normal RMI call this holds stub/skeleton hash + // if method is not found by uninstrumented code then writing payload will cause an exception + // in + // RMI server - as the payload will be interpreted as another call + // but it will not be parsed correctly - closing connection if (payload != null) { - out.writeObject(payload); + payload.write(out); } try { @@ -109,10 +124,10 @@ public class StreamRemoteCallConstructorAdvice { if (ex instanceof NoSuchObjectException) { return false; } else { - // TODO: log ex.printStackTrace(); + log.debug("Server error when executing synthetic call", ex); } } else { - // TODO: log ex.printStackTrace(); + log.debug("Error executing synthetic call", e); } return false; } finally { @@ -120,7 +135,7 @@ public class StreamRemoteCallConstructorAdvice { } } catch (final IOException e) { - // TODO: log ex.printStackTrace(); + log.debug("Communication error executing synthetic call", e); return false; } return true; diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java similarity index 72% rename from dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java rename to dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java index fc6de537a0..dda9a50312 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/RmiServerInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java @@ -1,8 +1,9 @@ -package datadog.trace.instrumentation.rmi; +package datadog.trace.instrumentation.rmi.server; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; -import static datadog.trace.instrumentation.api.AgentTracer.startSpan; +import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; +import static datadog.trace.instrumentation.rmi.server.ServerDecorator.DECORATE; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; @@ -18,6 +19,7 @@ import datadog.trace.bootstrap.ContextStore; import datadog.trace.bootstrap.InstrumentationContext; import datadog.trace.instrumentation.api.AgentScope; import datadog.trace.instrumentation.api.AgentSpan; +import datadog.trace.instrumentation.api.AgentTracer; import java.util.Collections; import java.util.Map; import net.bytebuddy.asm.Advice; @@ -40,9 +42,7 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { @Override public String[] helperClassNames() { return new String[] { - "datadog.trace.instrumentation.rmi.context.ContextPayload$InjectAdapter", - "datadog.trace.instrumentation.rmi.context.ContextPayload$ExtractAdapter", - "datadog.trace.instrumentation.rmi.context.ContextPayload", + packageName + ".ServerDecorator", "datadog.trace.agent.decorator.BaseDecorator" }; } @@ -55,7 +55,7 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { public Map, String> transformers() { return singletonMap( isMethod().and(isPublic()).and(not(isStatic())), - "datadog.trace.instrumentation.rmi.RmiServerInstrumentation$ServerAdvice"); + "datadog.trace.instrumentation.rmi.server.RmiServerInstrumentation$ServerAdvice"); } public static class ServerAdvice { @@ -64,17 +64,28 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { @Advice.This final Object thiz, @Advice.Origin(value = "#m") final String method) { final ContextStore callableContextStore = InstrumentationContext.get(Thread.class, AgentSpan.Context.class); + + final AgentSpan span = + startSpan(callableContextStore) + .setTag(DDTags.RESOURCE_NAME, thiz.getClass().getSimpleName() + "#" + method) + .setTag("span.origin.type", thiz.getClass().getCanonicalName()); + DECORATE.afterStart(span); + return activateSpan(span, true); + } + + public static AgentSpan startSpan( + final ContextStore callableContextStore) { + if (activeSpan() != null) { + return AgentTracer.startSpan("rmi.request"); + } + final AgentSpan.Context context = callableContextStore.get(Thread.currentThread()); - final AgentSpan span; if (context == null) { - span = startSpan("rmi.request"); + return AgentTracer.startSpan("rmi.request"); } else { - span = startSpan("rmi.request", context); + return AgentTracer.startSpan("rmi.request", context); } - span.setTag(DDTags.RESOURCE_NAME, thiz.getClass().getSimpleName() + "#" + method); - span.setTag("span.origin.type", thiz.getClass().getCanonicalName()); - return activateSpan(span, true); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) @@ -83,11 +94,9 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { if (scope == null) { return; } - final AgentSpan span = scope.span(); - if (throwable != null) { - span.setError(true); - span.addThrowable(throwable); - } + + DECORATE.onError(scope, throwable); + scope.close(); } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/ServerDecorator.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/ServerDecorator.java new file mode 100644 index 0000000000..73870eccfe --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/ServerDecorator.java @@ -0,0 +1,23 @@ +package datadog.trace.instrumentation.rmi.server; + +import datadog.trace.agent.decorator.BaseDecorator; +import datadog.trace.api.DDSpanTypes; + +public class ServerDecorator extends BaseDecorator { + public static final ServerDecorator DECORATE = new ServerDecorator(); + + @Override + protected String[] instrumentationNames() { + return new String[] {"rmi"}; + } + + @Override + protected String spanType() { + return DDSpanTypes.RPC; + } + + @Override + protected String component() { + return "rmi-server"; + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy b/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy index a0945354e2..203704fc5d 100644 --- a/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy +++ b/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy @@ -1,5 +1,7 @@ import datadog.trace.agent.test.AgentTestRunner import datadog.trace.agent.test.utils.PortUtils +import datadog.trace.api.DDSpanTypes +import datadog.trace.instrumentation.api.Tags import rmi.app.Greeter import rmi.app.Server import rmi.app.ServerLegacy @@ -40,21 +42,37 @@ class RmiTest extends AgentTestRunner { resourceName "Greeter#hello" operationName "rmi.invoke" childOf span(0) + spanType DDSpanTypes.RPC + tags { + "$Tags.COMPONENT" "rmi-client" "span.origin.type" Greeter.canonicalName defaultTags() } } } - trace(0, 1) { + trace(0, 2) { span(0) { resourceName "Server#hello" operationName "rmi.request" + spanType DDSpanTypes.RPC + tags { "span.origin.type" server.class.canonicalName + "$Tags.COMPONENT" "rmi-server" defaultTags(true) } } + span(1) { + resourceName "Server#someMethod" + operationName "rmi.request" + spanType DDSpanTypes.RPC + tags { + "$Tags.COMPONENT" "rmi-server" + "span.origin.type" server.class.canonicalName + defaultTags() + } + } } } @@ -102,8 +120,10 @@ class RmiTest extends AgentTestRunner { operationName "rmi.invoke" childOf span(0) errored true + spanType DDSpanTypes.RPC tags { + "$Tags.COMPONENT" "rmi-client" "span.origin.type" Greeter.canonicalName errorTags(RuntimeException, String) defaultTags() @@ -115,9 +135,11 @@ class RmiTest extends AgentTestRunner { resourceName "Server#exceptional" operationName "rmi.request" errored true + spanType DDSpanTypes.RPC tags { "span.origin.type" server.class.canonicalName + "$Tags.COMPONENT" "rmi-server" errorTags(RuntimeException, String) defaultTags(true) } @@ -149,8 +171,10 @@ class RmiTest extends AgentTestRunner { span(1) { resourceName "Greeter#hello" operationName "rmi.invoke" + spanType DDSpanTypes.RPC childOf span(0) tags { + "$Tags.COMPONENT" "rmi-client" "span.origin.type" Greeter.canonicalName defaultTags() } @@ -162,7 +186,9 @@ class RmiTest extends AgentTestRunner { childOf parentSpan resourceName "ServerLegacy#hello" operationName "rmi.request" + spanType DDSpanTypes.RPC tags { + "$Tags.COMPONENT" "rmi-server" "span.origin.type" server.class.canonicalName defaultTags(true) } diff --git a/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Server.java b/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Server.java index cc915d89bb..892a85f38a 100644 --- a/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Server.java +++ b/dd-java-agent/instrumentation/rmi/src/test/java/rmi/app/Server.java @@ -4,7 +4,7 @@ import java.rmi.RemoteException; import java.rmi.server.UnicastRemoteObject; public class Server extends UnicastRemoteObject implements Greeter { - static String RMI_ID = Server.class.getSimpleName(); + public static String RMI_ID = Server.class.getSimpleName(); private static final long serialVersionUID = 1L; @@ -14,6 +14,10 @@ public class Server extends UnicastRemoteObject implements Greeter { @Override public String hello(final String name) { + return someMethod(name); + } + + public String someMethod(final String name) { return "Hello " + name; } From 712a5c14830669e679a3755cb913de154a291526 Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Thu, 19 Dec 2019 13:02:59 -0500 Subject: [PATCH 15/22] Disable decorators via config --- .../main/java/datadog/trace/api/Config.java | 4 ++ .../decorators/DDDecoratorsFactory.java | 51 ++++++++------ .../decorators/SpanDecoratorTest.groovy | 68 +++++++++++++++++++ 3 files changed, 104 insertions(+), 19 deletions(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index 856516cf35..5ad4842782 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -596,6 +596,10 @@ public class Config { return jmxFetchIntegrationEnabled(integrationNames, defaultEnabled); } + public boolean isDecoratorEnabled(final String name) { + return getBooleanSettingFromEnvironment("trace." + name.toLowerCase() + ".enabled", true); + } + /** * @deprecated This method should only be used internally. Use the instance getter instead {@link * #isJmxFetchIntegrationEnabled(SortedSet, boolean)}. diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java index b46d355952..e4880cffa5 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java @@ -4,31 +4,44 @@ import datadog.trace.api.Config; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import lombok.extern.slf4j.Slf4j; /** Create DDSpanDecorators */ +@Slf4j public class DDDecoratorsFactory { public static List createBuiltinDecorators() { - final List decorators = - new ArrayList<>( - Arrays.asList( - new AnalyticsSampleRateDecorator(), - new DBStatementAsResourceName(), - new DBTypeDecorator(), - new ErrorFlag(), - new ForceManualDropDecorator(), - new ForceManualKeepDecorator(), - new OperationDecorator(), - new PeerServiceDecorator(), - new ResourceNameDecorator(), - new ServiceNameDecorator(), - new ServiceNameDecorator("service", false), - new ServletContextDecorator(), - new SpanTypeDecorator(), - new Status404Decorator(), - new Status5XXDecorator(), - new URLAsResourceName())); + final List decorators = new ArrayList<>(); + for (final AbstractDecorator decorator : + Arrays.asList( + new AnalyticsSampleRateDecorator(), + new DBStatementAsResourceName(), + new DBTypeDecorator(), + new ErrorFlag(), + new ForceManualDropDecorator(), + new ForceManualKeepDecorator(), + new OperationDecorator(), + new PeerServiceDecorator(), + new ResourceNameDecorator(), + new ServiceNameDecorator(), + new ServiceNameDecorator("service", false), + new ServletContextDecorator(), + new SpanTypeDecorator(), + new Status404Decorator(), + new Status5XXDecorator(), + new URLAsResourceName())) { + + if (Config.get().isDecoratorEnabled(decorator.getClass().getSimpleName())) { + decorators.add(decorator); + } else { + log.debug("{} disabled", decorator.getClass().getSimpleName()); + } + } + + // SplitByTags purposely does not check for ServiceNameDecorator being enabled + // This allows for ServiceNameDecorator to be disabled above while keeping SplitByTags + // SplitByTags can be disable by removing SplitByTags config for (final String splitByTag : Config.get().getSplitByTags()) { decorators.add(new ServiceNameDecorator(splitByTag, true)); } diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy index 64bffaeed7..f1386e4154 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy @@ -470,4 +470,72 @@ class SpanDecoratorTest extends DDSpecification { then: span.resourceName == "some-statement" } + + def "disable decorator via config"() { + setup: + ConfigUtils.updateConfig { + System.setProperty("dd.trace." + PeerServiceDecorator.getSimpleName().toLowerCase() + ".enabled", "false") + } + + tracer = new DDTracer( + "some-service", + new LoggingWriter(), + new AllSampler(), + "some-runtime-id", + emptyMap(), + emptyMap(), + emptyMap(), + emptyMap() + ) + + when: + def span = tracer.buildSpan("some span").withTag(Tags.PEER_SERVICE.key, "peer-service").start() + span.finish() + + then: + span.getServiceName() == "some-service" + + cleanup: + ConfigUtils.updateConfig { + System.clearProperty("dd.trace." + PeerServiceDecorator.getSimpleName().toLowerCase() + ".enabled") + } + } + + def "disabling service decorator does not disable split by tags"() { + setup: + ConfigUtils.updateConfig { + System.setProperty("dd.trace." + ServiceNameDecorator.getSimpleName().toLowerCase() + ".enabled", "false") + } + + tracer = new DDTracer( + "some-service", + new LoggingWriter(), + new AllSampler(), + "some-runtime-id", + emptyMap(), + emptyMap(), + emptyMap(), + emptyMap() + ) + + when: + def span = tracer.buildSpan("some span").withTag(tag, name).start() + span.finish() + + then: + span.getServiceName() == expected + + cleanup: + ConfigUtils.updateConfig { + System.clearProperty("dd.trace." + ServiceNameDecorator.getSimpleName().toLowerCase() + ".enabled") + } + + where: + tag | name | expected + DDTags.SERVICE_NAME | "new-service" | "some-service" + "service" | "new-service" | "some-service" + "sn.tag1" | "new-service" | "new-service" + + + } } From c3308042d31819997cd64ac2252fbc6666e27b06 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 18 Dec 2019 15:52:27 +0100 Subject: [PATCH 16/22] [rmi] use ObjectEndpoint#toString() to avoid need for reflection to be able to compare object identifiers. --- dd-java-agent/instrumentation/rmi/rmi.gradle | 4 +-- .../rmi/client/RmiClientInstrumentation.java | 4 ++- .../ObjectEndpointConstructorAdvice.java | 15 ---------- .../rmi/context/ObjectTableAdvice.java | 28 +++++++----------- .../context/RmiContextInstrumentation.java | 29 +++++-------------- .../rmi/server/RmiServerInstrumentation.java | 6 ++-- 6 files changed, 26 insertions(+), 60 deletions(-) delete mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectEndpointConstructorAdvice.java diff --git a/dd-java-agent/instrumentation/rmi/rmi.gradle b/dd-java-agent/instrumentation/rmi/rmi.gradle index 4c8088bea7..936a733f6e 100644 --- a/dd-java-agent/instrumentation/rmi/rmi.gradle +++ b/dd-java-agent/instrumentation/rmi/rmi.gradle @@ -1,8 +1,6 @@ muzzle { pass { - group = "javax.ws.rs" - module = "jsr311-api" - versions = "[0.5,)" + coreJdk() } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java index b5f6243113..9dbc044331 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java @@ -30,7 +30,9 @@ public final class RmiClientInstrumentation extends Instrumenter.Default { @Override public String[] helperClassNames() { return new String[] { - packageName + ".ClientDecorator", "datadog.trace.agent.decorator.BaseDecorator" + "datadog.trace.agent.decorator.BaseDecorator", + packageName + ".ClientDecorator", + packageName + ".ClientAdvice" }; } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectEndpointConstructorAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectEndpointConstructorAdvice.java deleted file mode 100644 index 13e1ebc24c..0000000000 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectEndpointConstructorAdvice.java +++ /dev/null @@ -1,15 +0,0 @@ -package datadog.trace.instrumentation.rmi.context; - -import datadog.trace.bootstrap.InstrumentationContext; -import java.rmi.server.ObjID; -import net.bytebuddy.asm.Advice; - -public class ObjectEndpointConstructorAdvice { - @Advice.OnMethodExit(suppress = Throwable.class) - public static void onEnter( - @Advice.This final Object thiz, @Advice.Argument(value = 0) final ObjID id) { - if (id != null) { - InstrumentationContext.get(Object.class, ObjID.class).put(thiz, id); - } - } -} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java index 9d26b8cc48..1a7a088ac7 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java @@ -5,9 +5,7 @@ import static datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstruc import datadog.trace.bootstrap.ContextStore; import datadog.trace.bootstrap.InstrumentationContext; import datadog.trace.instrumentation.api.AgentSpan; -import java.lang.reflect.Field; import java.rmi.Remote; -import java.rmi.server.ObjID; import lombok.extern.slf4j.Slf4j; import net.bytebuddy.asm.Advice; import sun.rmi.transport.Target; @@ -19,9 +17,12 @@ public class ObjectTableAdvice { @Advice.OnMethodExit(suppress = Throwable.class) public static void methodExit( @Advice.Argument(0) final Object oe, @Advice.Return(readOnly = false) Target result) { - final ObjID objID = InstrumentationContext.get(Object.class, ObjID.class).get(oe); - if (!DD_CONTEXT_CALL_ID.equals(objID)) { + // comparing toString() output allows us to avoid using reflection to be able to compare + // ObjID and ObjectEndpoint objects + // ObjectEndpoint#toString() only returns this.objId.toString() value which is exactly + // what we're interested in here. + if (!DD_CONTEXT_CALL_ID.toString().equals(oe.toString())) { return; } @@ -30,20 +31,11 @@ public class ObjectTableAdvice { result = new Target( - DUMMY_REMOTE, new ContextDispatcher(callableContextStore), DUMMY_REMOTE, objID, false); - } - - public static ObjID GET_OBJ_ID(final Object oe) { - try { - final Class clazz = oe.getClass(); - // sun.rmi.transport.ObjectEndpoint is protected and field "id" is private - final Field id = clazz.getDeclaredField("id"); - id.setAccessible(true); - return (ObjID) id.get(oe); - } catch (final ReflectiveOperationException e) { - log.debug("Error getting object id from: {}", oe, e); - } - return null; + DUMMY_REMOTE, + new ContextDispatcher(callableContextStore), + DUMMY_REMOTE, + DD_CONTEXT_CALL_ID, + false); } public static class DummyRemote implements Remote {} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java index 1d2d537cb8..05042f0ce4 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java @@ -2,7 +2,6 @@ package datadog.trace.instrumentation.rmi.context; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; import static net.bytebuddy.matcher.ElementMatchers.isConstructor; -import static net.bytebuddy.matcher.ElementMatchers.isDeclaredBy; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isStatic; @@ -12,20 +11,15 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.instrumentation.api.AgentSpan; -import java.rmi.server.ObjID; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; -import sun.rmi.transport.Connection; -import sun.rmi.transport.ObjectTable; -import sun.rmi.transport.StreamRemoteCall; @AutoService(Instrumenter.class) public class RmiContextInstrumentation extends Instrumenter.Default { - // TODO clean this up public RmiContextInstrumentation() { super("rmi", "rmi-context-propagator"); @@ -36,8 +30,8 @@ public class RmiContextInstrumentation extends Instrumenter.Default { return not(isInterface()) .and( safeHasSuperType( - named(StreamRemoteCall.class.getName()) // TODO replace with string - .or(named(ObjectTable.class.getName())) + named("sun.rmi.transport.StreamRemoteCall") + .or(named("sun.rmi.transport.ObjectTable")) .or(named("sun.rmi.transport.ObjectEndpoint")))); } @@ -45,26 +39,22 @@ public class RmiContextInstrumentation extends Instrumenter.Default { public Map contextStore() { final HashMap contextStore = new HashMap<>(); // thread context that stores distributed context - contextStore.put(Thread.class.getName(), AgentSpan.Context.class.getName()); + contextStore.put("java.lang.Thread", "datadog.trace.instrumentation.api.AgentSpan$Context"); // caching if a connection can support enhanced format - contextStore.put(Connection.class.getName(), Boolean.class.getName()); - - // used to avoid reflection when instrumenting protected class ObjectEndpoint - contextStore.put(Object.class.getName(), ObjID.class.getName()); + contextStore.put("sun.rmi.transport.Connection", "java.lang.Boolean"); return contextStore; } @Override public String[] helperClassNames() { return new String[] { - packageName + ".StreamRemoteCallConstructorAdvice", packageName + ".ContextPayload", packageName + ".ContextPayload$InjectAdapter", packageName + ".ContextPayload$ExtractAdapter", - packageName + ".ObjectTableAdvice", packageName + ".ContextDispatcher", - packageName + ".ObjectEndpointConstructorAdvice", + packageName + ".StreamRemoteCallConstructorAdvice", + packageName + ".ObjectTableAdvice", packageName + ".ObjectTableAdvice$DummyRemote" }; } @@ -85,9 +75,6 @@ public class RmiContextInstrumentation extends Instrumenter.Default { .and((takesArgument(0, named("sun.rmi.transport.ObjectEndpoint")))), packageName + ".ObjectTableAdvice"); - transformers.put( - isConstructor().and(isDeclaredBy(named("sun.rmi.transport.ObjectEndpoint"))), - packageName + ".ObjectEndpointConstructorAdvice"); - return transformers; + return Collections.unmodifiableMap(transformers); } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java index dda9a50312..97e6e53a6d 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java @@ -42,7 +42,9 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { @Override public String[] helperClassNames() { return new String[] { - packageName + ".ServerDecorator", "datadog.trace.agent.decorator.BaseDecorator" + packageName + ".ServerDecorator", + packageName + ".RmiServerInstrumentation$ServerAdvice", + "datadog.trace.agent.decorator.BaseDecorator" }; } @@ -55,7 +57,7 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { public Map, String> transformers() { return singletonMap( isMethod().and(isPublic()).and(not(isStatic())), - "datadog.trace.instrumentation.rmi.server.RmiServerInstrumentation$ServerAdvice"); + packageName + ".RmiServerInstrumentation$ServerAdvice"); } public static class ServerAdvice { From 3f21f6419d2ac038524e13c57ddca019a249a796 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Fri, 20 Dec 2019 12:29:48 +0100 Subject: [PATCH 17/22] [rmi] Split context propagation to server and client instrumentation + Separate extra code from advices + cleanup helper definitions --- ...ClientAdvice.java => RmiClientAdvice.java} | 8 +- ...Decorator.java => RmiClientDecorator.java} | 11 ++- .../rmi/client/RmiClientInstrumentation.java | 6 +- ...ctorAdvice.java => ContextPropagator.java} | 66 +++------------ .../context/RmiContextInstrumentation.java | 80 ------------------- .../RmiClientContextInstrumentation.java | 54 +++++++++++++ .../StreamRemoteCallConstructorAdvice.java | 56 +++++++++++++ .../{ => server}/ContextDispatcher.java | 6 +- .../{ => server}/ObjectTableAdvice.java | 15 ++-- .../RmiServerContextInstrumentation.java | 57 +++++++++++++ .../rmi/server/RmiServerDecorator.java | 43 ++++++++++ .../rmi/server/RmiServerInstrumentation.java | 36 +++------ .../rmi/server/ServerDecorator.java | 23 ------ .../rmi/src/test/groovy/RmiTest.groovy | 23 ++++-- 14 files changed, 274 insertions(+), 210 deletions(-) rename dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/{ClientAdvice.java => RmiClientAdvice.java} (82%) rename dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/{ClientDecorator.java => RmiClientDecorator.java} (57%) rename dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/{StreamRemoteCallConstructorAdvice.java => ContextPropagator.java} (55%) delete mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/RmiClientContextInstrumentation.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/StreamRemoteCallConstructorAdvice.java rename dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/{ => server}/ContextDispatcher.java (84%) rename dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/{ => server}/ObjectTableAdvice.java (74%) create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/RmiServerContextInstrumentation.java create mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerDecorator.java delete mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/ServerDecorator.java diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientAdvice.java similarity index 82% rename from dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientAdvice.java rename to dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientAdvice.java index 9504bb828a..2819807937 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientAdvice.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientAdvice.java @@ -3,7 +3,7 @@ package datadog.trace.instrumentation.rmi.client; import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.instrumentation.api.AgentTracer.startSpan; -import static datadog.trace.instrumentation.rmi.client.ClientDecorator.DECORATE; +import static datadog.trace.instrumentation.rmi.client.RmiClientDecorator.DECORATE; import datadog.trace.api.DDTags; import datadog.trace.instrumentation.api.AgentScope; @@ -11,7 +11,7 @@ import datadog.trace.instrumentation.api.AgentSpan; import java.lang.reflect.Method; import net.bytebuddy.asm.Advice; -public class ClientAdvice { +public class RmiClientAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static AgentScope onEnter(@Advice.Argument(value = 1) final Method method) { if (activeSpan() == null) { @@ -19,9 +19,7 @@ public class ClientAdvice { } final AgentSpan span = startSpan("rmi.invoke") - .setTag( - DDTags.RESOURCE_NAME, - method.getDeclaringClass().getSimpleName() + "#" + method.getName()) + .setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForMethod(method)) .setTag("span.origin.type", method.getDeclaringClass().getCanonicalName()); DECORATE.afterStart(span); diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientDecorator.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientDecorator.java similarity index 57% rename from dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientDecorator.java rename to dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientDecorator.java index b07029e04c..e11102c443 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/ClientDecorator.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientDecorator.java @@ -1,10 +1,10 @@ package datadog.trace.instrumentation.rmi.client; -import datadog.trace.agent.decorator.BaseDecorator; +import datadog.trace.agent.decorator.ClientDecorator; import datadog.trace.api.DDSpanTypes; -public class ClientDecorator extends BaseDecorator { - public static final ClientDecorator DECORATE = new ClientDecorator(); +public class RmiClientDecorator extends ClientDecorator { + public static final RmiClientDecorator DECORATE = new RmiClientDecorator(); @Override protected String[] instrumentationNames() { @@ -20,4 +20,9 @@ public class ClientDecorator extends BaseDecorator { protected String component() { return "rmi-client"; } + + @Override + protected String service() { + return "rmi"; + } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java index 9dbc044331..76a8e92a08 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java @@ -31,8 +31,8 @@ public final class RmiClientInstrumentation extends Instrumenter.Default { public String[] helperClassNames() { return new String[] { "datadog.trace.agent.decorator.BaseDecorator", - packageName + ".ClientDecorator", - packageName + ".ClientAdvice" + "datadog.trace.agent.decorator.ClientDecorator", + packageName + ".RmiClientDecorator" }; } @@ -43,6 +43,6 @@ public final class RmiClientInstrumentation extends Instrumenter.Default { .and(named("invoke")) .and(takesArgument(0, named("java.rmi.Remote"))) .and(takesArgument(1, named("java.lang.reflect.Method"))), - packageName + ".ClientAdvice"); + packageName + ".RmiClientAdvice"); } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPropagator.java similarity index 55% rename from dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java rename to dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPropagator.java index 992c147f00..7b8aae8548 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/StreamRemoteCallConstructorAdvice.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPropagator.java @@ -1,78 +1,38 @@ package datadog.trace.instrumentation.rmi.context; -import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.instrumentation.api.AgentTracer.propagate; import static datadog.trace.instrumentation.rmi.context.ContextPayload.SETTER; import datadog.trace.bootstrap.ContextStore; -import datadog.trace.bootstrap.InstrumentationContext; import datadog.trace.instrumentation.api.AgentSpan; import java.io.IOException; import java.io.ObjectOutput; import java.rmi.NoSuchObjectException; import java.rmi.server.ObjID; import lombok.extern.slf4j.Slf4j; -import net.bytebuddy.asm.Advice; import sun.rmi.transport.Connection; import sun.rmi.transport.StreamRemoteCall; import sun.rmi.transport.TransportConstants; -/** - * Main entry point for transferring context between RMI service. - * - *

It injects into StreamRemoteCall constructor used for invoking remote tasks and performs a - * backwards compatible check to ensure if the other side is prepared to receive context propagation - * messages then if successful sends a context propagation message - * - *

Context propagation consist of a Serialized HashMap with all data set by usual context - * injection, which includes things like sampling priority, trace and parent id - * - *

As well as optional baggage items - * - *

On the other side of the communication a special Dispatcher is created when a message with - * DD_CONTEXT_CALL_ID is received. - * - *

If the server is not instrumented first call will gracefully fail just like any other unknown - * call. With small caveat that this first call needs to *not* have any parameters, since those will - * not be read from connection and instead will be interpreted as another remote instruction, but - * that instruction will essentially be garbage data and will cause the parsing loop to throw - * exception and shutdown the connection which we do not want - */ @Slf4j -public class StreamRemoteCallConstructorAdvice { - public static final ObjID ACTIVATOR_ID = new ObjID(ObjID.ACTIVATOR_ID); - public static final ObjID DGC_ID = new ObjID(ObjID.DGC_ID); - public static final ObjID REGISTRY_ID = new ObjID(ObjID.REGISTRY_ID); - public static final ObjID DD_CONTEXT_CALL_ID = new ObjID("Datadog.context_call.v2".hashCode()); - public static final int CONTEXT_CHECK_CALL_OP_ID = -1; +public class ContextPropagator { + private static final ObjID ACTIVATOR_ID = new ObjID(ObjID.ACTIVATOR_ID); + private static final ObjID DGC_ID = new ObjID(ObjID.DGC_ID); + private static final ObjID REGISTRY_ID = new ObjID(ObjID.REGISTRY_ID); + public static final ObjID DD_CONTEXT_CALL_ID = new ObjID("Datadog.v1.context_call".hashCode()); + private static final int CONTEXT_CHECK_CALL_OP_ID = -1; public static final int CONTEXT_PASS_OPERATION_ID = -2; - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void onEnter( - @Advice.Argument(value = 0) final Connection c, @Advice.Argument(value = 1) final ObjID id) { - if (!c.isReusable()) { - return; - } - if (isRMIInternalObject(id)) { - return; - } - - final ContextStore contextStore = - InstrumentationContext.get(Connection.class, Boolean.class); - attemptToPropagateContext(contextStore, c); - } - public static boolean isRMIInternalObject(final ObjID id) { return ACTIVATOR_ID.equals(id) || DGC_ID.equals(id) || REGISTRY_ID.equals(id); } - public static void attemptToPropagateContext( - final ContextStore contextStore, final Connection c) { - final AgentSpan span = activeSpan(); - if (span == null) { - return; - } + public static final ContextPropagator PROPAGATOR = new ContextPropagator(); + public void attemptToPropagateContext( + final ContextStore contextStore, + final Connection c, + final AgentSpan span) { if (checkIfContextCanBePassed(contextStore, c)) { final ContextPayload payload = new ContextPayload(); propagate().inject(span, payload, SETTER); @@ -82,7 +42,7 @@ public class StreamRemoteCallConstructorAdvice { } } - private static boolean checkIfContextCanBePassed( + private boolean checkIfContextCanBePassed( final ContextStore contextStore, final Connection c) { final Boolean storedResult = contextStore.get(c); if (storedResult != null) { @@ -94,7 +54,7 @@ public class StreamRemoteCallConstructorAdvice { return result; } - private static boolean syntheticCall( + private boolean syntheticCall( final Connection c, final ContextPayload payload, final int operationId) { final StreamRemoteCall shareContextCall = new StreamRemoteCall(c); try { diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java deleted file mode 100644 index 05042f0ce4..0000000000 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/RmiContextInstrumentation.java +++ /dev/null @@ -1,80 +0,0 @@ -package datadog.trace.instrumentation.rmi.context; - -import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; -import static net.bytebuddy.matcher.ElementMatchers.isConstructor; -import static net.bytebuddy.matcher.ElementMatchers.isInterface; -import static net.bytebuddy.matcher.ElementMatchers.isMethod; -import static net.bytebuddy.matcher.ElementMatchers.isStatic; -import static net.bytebuddy.matcher.ElementMatchers.named; -import static net.bytebuddy.matcher.ElementMatchers.not; -import static net.bytebuddy.matcher.ElementMatchers.takesArgument; - -import com.google.auto.service.AutoService; -import datadog.trace.agent.tooling.Instrumenter; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; -import net.bytebuddy.description.method.MethodDescription; -import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.matcher.ElementMatcher; - -@AutoService(Instrumenter.class) -public class RmiContextInstrumentation extends Instrumenter.Default { - - public RmiContextInstrumentation() { - super("rmi", "rmi-context-propagator"); - } - - @Override - public ElementMatcher typeMatcher() { - return not(isInterface()) - .and( - safeHasSuperType( - named("sun.rmi.transport.StreamRemoteCall") - .or(named("sun.rmi.transport.ObjectTable")) - .or(named("sun.rmi.transport.ObjectEndpoint")))); - } - - @Override - public Map contextStore() { - final HashMap contextStore = new HashMap<>(); - // thread context that stores distributed context - contextStore.put("java.lang.Thread", "datadog.trace.instrumentation.api.AgentSpan$Context"); - - // caching if a connection can support enhanced format - contextStore.put("sun.rmi.transport.Connection", "java.lang.Boolean"); - return contextStore; - } - - @Override - public String[] helperClassNames() { - return new String[] { - packageName + ".ContextPayload", - packageName + ".ContextPayload$InjectAdapter", - packageName + ".ContextPayload$ExtractAdapter", - packageName + ".ContextDispatcher", - packageName + ".StreamRemoteCallConstructorAdvice", - packageName + ".ObjectTableAdvice", - packageName + ".ObjectTableAdvice$DummyRemote" - }; - } - - @Override - public Map, String> transformers() { - final Map, String> transformers = new HashMap<>(); - transformers.put( - isConstructor() - .and(takesArgument(0, named("sun.rmi.transport.Connection"))) - .and(takesArgument(1, named("java.rmi.server.ObjID"))), - packageName + ".StreamRemoteCallConstructorAdvice"); - - transformers.put( - isMethod() - .and(isStatic()) - .and(named("getTarget")) - .and((takesArgument(0, named("sun.rmi.transport.ObjectEndpoint")))), - packageName + ".ObjectTableAdvice"); - - return Collections.unmodifiableMap(transformers); - } -} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/RmiClientContextInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/RmiClientContextInstrumentation.java new file mode 100644 index 0000000000..818930a5e3 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/RmiClientContextInstrumentation.java @@ -0,0 +1,54 @@ +package datadog.trace.instrumentation.rmi.context.client; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isConstructor; +import static net.bytebuddy.matcher.ElementMatchers.isInterface; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import java.util.Map; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +@AutoService(Instrumenter.class) +public class RmiClientContextInstrumentation extends Instrumenter.Default { + + public RmiClientContextInstrumentation() { + super("rmi", "rmi-context-propagator", "rmi-client-context-propagator"); + } + + @Override + public ElementMatcher typeMatcher() { + return not(isInterface()).and(safeHasSuperType(named("sun.rmi.transport.StreamRemoteCall"))); + } + + @Override + public Map contextStore() { + // caching if a connection can support enhanced format + return singletonMap("sun.rmi.transport.Connection", "java.lang.Boolean"); + } + + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.instrumentation.rmi.context.ContextPayload$InjectAdapter", + "datadog.trace.instrumentation.rmi.context.ContextPayload$ExtractAdapter", + "datadog.trace.instrumentation.rmi.context.ContextPayload", + "datadog.trace.instrumentation.rmi.context.ContextPropagator" + }; + } + + @Override + public Map, String> transformers() { + return singletonMap( + isConstructor() + .and(takesArgument(0, named("sun.rmi.transport.Connection"))) + .and(takesArgument(1, named("java.rmi.server.ObjID"))), + packageName + ".StreamRemoteCallConstructorAdvice"); + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/StreamRemoteCallConstructorAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/StreamRemoteCallConstructorAdvice.java new file mode 100644 index 0000000000..dc457b979f --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/StreamRemoteCallConstructorAdvice.java @@ -0,0 +1,56 @@ +package datadog.trace.instrumentation.rmi.context.client; + +import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; +import static datadog.trace.instrumentation.rmi.context.ContextPropagator.PROPAGATOR; + +import datadog.trace.bootstrap.ContextStore; +import datadog.trace.bootstrap.InstrumentationContext; +import datadog.trace.instrumentation.api.AgentSpan; +import datadog.trace.instrumentation.rmi.context.ContextPropagator; +import java.rmi.server.ObjID; +import net.bytebuddy.asm.Advice; +import sun.rmi.transport.Connection; + +/** + * Main entry point for transferring context between RMI service. + * + *

It injects into StreamRemoteCall constructor used for invoking remote tasks and performs a + * backwards compatible check to ensure if the other side is prepared to receive context propagation + * messages then if successful sends a context propagation message + * + *

Context propagation consist of a Serialized HashMap with all data set by usual context + * injection, which includes things like sampling priority, trace and parent id + * + *

As well as optional baggage items + * + *

On the other side of the communication a special Dispatcher is created when a message with + * DD_CONTEXT_CALL_ID is received. + * + *

If the server is not instrumented first call will gracefully fail just like any other unknown + * call. With small caveat that this first call needs to *not* have any parameters, since those will + * not be read from connection and instead will be interpreted as another remote instruction, but + * that instruction will essentially be garbage data and will cause the parsing loop to throw + * exception and shutdown the connection which we do not want + */ +public class StreamRemoteCallConstructorAdvice { + + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void onEnter( + @Advice.Argument(value = 0) final Connection c, @Advice.Argument(value = 1) final ObjID id) { + if (!c.isReusable()) { + return; + } + if (ContextPropagator.isRMIInternalObject(id)) { + return; + } + final AgentSpan activeSpan = activeSpan(); + if (activeSpan == null) { + return; + } + + final ContextStore contextStore = + InstrumentationContext.get(Connection.class, Boolean.class); + + PROPAGATOR.attemptToPropagateContext(contextStore, c, activeSpan); + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextDispatcher.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ContextDispatcher.java similarity index 84% rename from dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextDispatcher.java rename to dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ContextDispatcher.java index b676ab51e6..d3a11bc177 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextDispatcher.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ContextDispatcher.java @@ -1,10 +1,12 @@ -package datadog.trace.instrumentation.rmi.context; +package datadog.trace.instrumentation.rmi.context.server; import static datadog.trace.instrumentation.api.AgentTracer.propagate; import static datadog.trace.instrumentation.rmi.context.ContextPayload.GETTER; import datadog.trace.bootstrap.ContextStore; import datadog.trace.instrumentation.api.AgentSpan; +import datadog.trace.instrumentation.rmi.context.ContextPayload; +import datadog.trace.instrumentation.rmi.context.ContextPropagator; import java.io.IOException; import java.io.ObjectInput; import java.rmi.Remote; @@ -25,7 +27,7 @@ public class ContextDispatcher implements Dispatcher { final int operationId = in.readInt(); in.readLong(); // skip 8 bytes - if (operationId == StreamRemoteCallConstructorAdvice.CONTEXT_PASS_OPERATION_ID) { + if (operationId == ContextPropagator.CONTEXT_PASS_OPERATION_ID) { final ContextPayload payload = ContextPayload.read(in); if (payload != null) { final AgentSpan.Context context = propagate().extract(payload, GETTER); diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ObjectTableAdvice.java similarity index 74% rename from dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java rename to dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ObjectTableAdvice.java index 1a7a088ac7..f187c2ef7f 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ObjectTableAdvice.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ObjectTableAdvice.java @@ -1,18 +1,15 @@ -package datadog.trace.instrumentation.rmi.context; +package datadog.trace.instrumentation.rmi.context.server; -import static datadog.trace.instrumentation.rmi.context.StreamRemoteCallConstructorAdvice.DD_CONTEXT_CALL_ID; +import static datadog.trace.instrumentation.rmi.context.ContextPropagator.DD_CONTEXT_CALL_ID; import datadog.trace.bootstrap.ContextStore; import datadog.trace.bootstrap.InstrumentationContext; import datadog.trace.instrumentation.api.AgentSpan; import java.rmi.Remote; -import lombok.extern.slf4j.Slf4j; import net.bytebuddy.asm.Advice; import sun.rmi.transport.Target; -@Slf4j public class ObjectTableAdvice { - public static final DummyRemote DUMMY_REMOTE = new DummyRemote(); @Advice.OnMethodExit(suppress = Throwable.class) public static void methodExit( @@ -31,12 +28,14 @@ public class ObjectTableAdvice { result = new Target( - DUMMY_REMOTE, + NoopRemote.NOOP_REMOTE, new ContextDispatcher(callableContextStore), - DUMMY_REMOTE, + NoopRemote.NOOP_REMOTE, DD_CONTEXT_CALL_ID, false); } - public static class DummyRemote implements Remote {} + public static class NoopRemote implements Remote { + public static final NoopRemote NOOP_REMOTE = new NoopRemote(); + } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/RmiServerContextInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/RmiServerContextInstrumentation.java new file mode 100644 index 0000000000..3eb276bb7b --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/RmiServerContextInstrumentation.java @@ -0,0 +1,57 @@ +package datadog.trace.instrumentation.rmi.context.server; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isInterface; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isStatic; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import java.util.Map; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +@AutoService(Instrumenter.class) +public class RmiServerContextInstrumentation extends Instrumenter.Default { + + public RmiServerContextInstrumentation() { + super("rmi", "rmi-context-propagator", "rmi-server-context-propagator"); + } + + @Override + public ElementMatcher typeMatcher() { + return not(isInterface()).and(safeHasSuperType(named("sun.rmi.transport.ObjectTable"))); + } + + @Override + public Map contextStore() { + return singletonMap("java.lang.Thread", "datadog.trace.instrumentation.api.AgentSpan$Context"); + } + + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.instrumentation.rmi.context.ContextPayload$InjectAdapter", + "datadog.trace.instrumentation.rmi.context.ContextPayload$ExtractAdapter", + "datadog.trace.instrumentation.rmi.context.ContextPayload", + "datadog.trace.instrumentation.rmi.context.ContextPropagator", + packageName + ".ContextDispatcher", + packageName + ".ObjectTableAdvice$NoopRemote" + }; + } + + @Override + public Map, String> transformers() { + return singletonMap( + isMethod() + .and(isStatic()) + .and(named("getTarget")) + .and((takesArgument(0, named("sun.rmi.transport.ObjectEndpoint")))), + packageName + ".ObjectTableAdvice"); + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerDecorator.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerDecorator.java new file mode 100644 index 0000000000..edd09cad90 --- /dev/null +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerDecorator.java @@ -0,0 +1,43 @@ +package datadog.trace.instrumentation.rmi.server; + +import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; +import static datadog.trace.instrumentation.api.AgentTracer.startSpan; + +import datadog.trace.agent.decorator.ServerDecorator; +import datadog.trace.api.DDSpanTypes; +import datadog.trace.bootstrap.ContextStore; +import datadog.trace.instrumentation.api.AgentSpan; + +public class RmiServerDecorator extends ServerDecorator { + public static final RmiServerDecorator DECORATE = new RmiServerDecorator(); + + public AgentSpan startSpanWithContext( + final ContextStore contextStore) { + if (activeSpan() != null) { + return startSpan("rmi.request"); + } + + final AgentSpan.Context context = contextStore.get(Thread.currentThread()); + + if (context == null) { + return startSpan("rmi.request"); + } else { + return startSpan("rmi.request", context); + } + } + + @Override + protected String[] instrumentationNames() { + return new String[] {"rmi"}; + } + + @Override + protected String spanType() { + return DDSpanTypes.RPC; + } + + @Override + protected String component() { + return "rmi-server"; + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java index 97e6e53a6d..f479120483 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java @@ -2,8 +2,7 @@ package datadog.trace.instrumentation.rmi.server; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; -import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; -import static datadog.trace.instrumentation.rmi.server.ServerDecorator.DECORATE; +import static datadog.trace.instrumentation.rmi.server.RmiServerDecorator.DECORATE; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; @@ -19,7 +18,7 @@ import datadog.trace.bootstrap.ContextStore; import datadog.trace.bootstrap.InstrumentationContext; import datadog.trace.instrumentation.api.AgentScope; import datadog.trace.instrumentation.api.AgentSpan; -import datadog.trace.instrumentation.api.AgentTracer; +import java.lang.reflect.Method; import java.util.Collections; import java.util.Map; import net.bytebuddy.asm.Advice; @@ -42,9 +41,9 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { @Override public String[] helperClassNames() { return new String[] { - packageName + ".ServerDecorator", - packageName + ".RmiServerInstrumentation$ServerAdvice", - "datadog.trace.agent.decorator.BaseDecorator" + "datadog.trace.agent.decorator.ServerDecorator", + "datadog.trace.agent.decorator.BaseDecorator", + packageName + ".RmiServerDecorator" }; } @@ -63,33 +62,20 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { public static class ServerAdvice { @Advice.OnMethodEnter(suppress = Throwable.class, inline = true) public static AgentScope onEnter( - @Advice.This final Object thiz, @Advice.Origin(value = "#m") final String method) { - final ContextStore callableContextStore = + @Advice.This final Object thiz, @Advice.Origin final Method method) { + final ContextStore threadContextStore = InstrumentationContext.get(Thread.class, AgentSpan.Context.class); final AgentSpan span = - startSpan(callableContextStore) - .setTag(DDTags.RESOURCE_NAME, thiz.getClass().getSimpleName() + "#" + method) + DECORATE + .startSpanWithContext(threadContextStore) + .setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForMethod(method)) .setTag("span.origin.type", thiz.getClass().getCanonicalName()); + DECORATE.afterStart(span); return activateSpan(span, true); } - public static AgentSpan startSpan( - final ContextStore callableContextStore) { - if (activeSpan() != null) { - return AgentTracer.startSpan("rmi.request"); - } - - final AgentSpan.Context context = callableContextStore.get(Thread.currentThread()); - - if (context == null) { - return AgentTracer.startSpan("rmi.request"); - } else { - return AgentTracer.startSpan("rmi.request", context); - } - } - @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/ServerDecorator.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/ServerDecorator.java deleted file mode 100644 index 73870eccfe..0000000000 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/ServerDecorator.java +++ /dev/null @@ -1,23 +0,0 @@ -package datadog.trace.instrumentation.rmi.server; - -import datadog.trace.agent.decorator.BaseDecorator; -import datadog.trace.api.DDSpanTypes; - -public class ServerDecorator extends BaseDecorator { - public static final ServerDecorator DECORATE = new ServerDecorator(); - - @Override - protected String[] instrumentationNames() { - return new String[] {"rmi"}; - } - - @Override - protected String spanType() { - return DDSpanTypes.RPC; - } - - @Override - protected String component() { - return "rmi-server"; - } -} diff --git a/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy b/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy index 203704fc5d..cb5a874b65 100644 --- a/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy +++ b/dd-java-agent/instrumentation/rmi/src/test/groovy/RmiTest.groovy @@ -39,12 +39,13 @@ class RmiTest extends AgentTestRunner { trace(1, 2) { basicSpan(it, 0, "parent") span(1) { - resourceName "Greeter#hello" + resourceName "Greeter.hello" operationName "rmi.invoke" childOf span(0) spanType DDSpanTypes.RPC tags { + "$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT "$Tags.COMPONENT" "rmi-client" "span.origin.type" Greeter.canonicalName defaultTags() @@ -53,21 +54,23 @@ class RmiTest extends AgentTestRunner { } trace(0, 2) { span(0) { - resourceName "Server#hello" + resourceName "Server.hello" operationName "rmi.request" spanType DDSpanTypes.RPC tags { - "span.origin.type" server.class.canonicalName + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER "$Tags.COMPONENT" "rmi-server" + "span.origin.type" server.class.canonicalName defaultTags(true) } } span(1) { - resourceName "Server#someMethod" + resourceName "Server.someMethod" operationName "rmi.request" spanType DDSpanTypes.RPC tags { + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER "$Tags.COMPONENT" "rmi-server" "span.origin.type" server.class.canonicalName defaultTags() @@ -116,13 +119,14 @@ class RmiTest extends AgentTestRunner { trace(1, 2) { basicSpan(it, 0, "parent", null, thrownException) span(1) { - resourceName "Greeter#exceptional" + resourceName "Greeter.exceptional" operationName "rmi.invoke" childOf span(0) errored true spanType DDSpanTypes.RPC tags { + "$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT "$Tags.COMPONENT" "rmi-client" "span.origin.type" Greeter.canonicalName errorTags(RuntimeException, String) @@ -132,12 +136,13 @@ class RmiTest extends AgentTestRunner { } trace(0, 1) { span(0) { - resourceName "Server#exceptional" + resourceName "Server.exceptional" operationName "rmi.request" errored true spanType DDSpanTypes.RPC tags { + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER "span.origin.type" server.class.canonicalName "$Tags.COMPONENT" "rmi-server" errorTags(RuntimeException, String) @@ -169,11 +174,12 @@ class RmiTest extends AgentTestRunner { trace(1, 2) { basicSpan(it, 0, "parent") span(1) { - resourceName "Greeter#hello" + resourceName "Greeter.hello" operationName "rmi.invoke" spanType DDSpanTypes.RPC childOf span(0) tags { + "$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT "$Tags.COMPONENT" "rmi-client" "span.origin.type" Greeter.canonicalName defaultTags() @@ -184,11 +190,12 @@ class RmiTest extends AgentTestRunner { trace(0, 1) { span(0) { childOf parentSpan - resourceName "ServerLegacy#hello" + resourceName "ServerLegacy.hello" operationName "rmi.request" spanType DDSpanTypes.RPC tags { "$Tags.COMPONENT" "rmi-server" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER "span.origin.type" server.class.canonicalName defaultTags(true) } From 7338bbdd06e8ab78bbb895c6f8757ebfda31eed8 Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Fri, 20 Dec 2019 13:15:04 -0500 Subject: [PATCH 18/22] Register on JAX-RS client instead of builder --- .../jaxrs/JaxRsClientInstrumentation.java | 10 ++-- .../groovy/JaxMultithreadedClientTest.groovy | 54 +++++++++++++++++++ 2 files changed, 60 insertions(+), 4 deletions(-) create mode 100644 dd-java-agent/instrumentation/jax-rs-client-2.0/src/test/groovy/JaxMultithreadedClientTest.groovy diff --git a/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java b/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java index 66ecce3fb1..92c81bb7ab 100644 --- a/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java @@ -8,10 +8,11 @@ import static net.bytebuddy.matcher.ElementMatchers.returns; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; import java.util.Map; -import javax.ws.rs.client.ClientBuilder; +import javax.ws.rs.client.Client; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.implementation.bytecode.assign.Assigner; import net.bytebuddy.matcher.ElementMatcher; @AutoService(Instrumenter.class) @@ -48,9 +49,10 @@ public final class JaxRsClientInstrumentation extends Instrumenter.Default { public static class ClientBuilderAdvice { - @Advice.OnMethodEnter - public static void registerFeature(@Advice.This final ClientBuilder builder) { - builder.register(ClientTracingFeature.class); + @Advice.OnMethodExit + public static void registerFeature( + @Advice.Return(typing = Assigner.Typing.DYNAMIC) final Client client) { + client.register(ClientTracingFeature.class); } } } diff --git a/dd-java-agent/instrumentation/jax-rs-client-2.0/src/test/groovy/JaxMultithreadedClientTest.groovy b/dd-java-agent/instrumentation/jax-rs-client-2.0/src/test/groovy/JaxMultithreadedClientTest.groovy new file mode 100644 index 0000000000..e7695a9082 --- /dev/null +++ b/dd-java-agent/instrumentation/jax-rs-client-2.0/src/test/groovy/JaxMultithreadedClientTest.groovy @@ -0,0 +1,54 @@ +import datadog.trace.agent.test.AgentTestRunner +import org.glassfish.jersey.client.JerseyClientBuilder +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.util.concurrent.AsyncConditions + +import javax.ws.rs.client.Client + +import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer + +class JaxMultithreadedClientTest extends AgentTestRunner { + + @AutoCleanup + @Shared + def server = httpServer { + handlers { + prefix("success") { + String msg = "Hello." + response.status(200).send(msg) + } + } + } + + def "multiple threads using the same builder works"() { + given: + def conds = new AsyncConditions(10) + def uri = server.address.resolve("/success") + def builder = new JerseyClientBuilder() + + // Start 10 threads and do 50 requests each + when: + (1..10).each { + Thread.start { + boolean hadErrors = (1..50).any { + try { + Client client = builder.build() + client.target(uri).request().get() + } catch (Exception e) { + e.printStackTrace() + return true + } + return false + } + + conds.evaluate { + assert !hadErrors + } + } + } + + then: + conds.await(10) + } +} From 97b947919fe68876efcf4d9b73d62ba7156e0589 Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Fri, 20 Dec 2019 15:27:32 -0500 Subject: [PATCH 19/22] Add a comment --- .../instrumentation/jaxrs/JaxRsClientInstrumentation.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java b/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java index 92c81bb7ab..19af4fe24e 100644 --- a/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java @@ -52,6 +52,9 @@ public final class JaxRsClientInstrumentation extends Instrumenter.Default { @Advice.OnMethodExit public static void registerFeature( @Advice.Return(typing = Assigner.Typing.DYNAMIC) final Client client) { + // Register on the generated client instead of the builder + // The build() can be called multiple times and is not thread safe + // A client is only created once client.register(ClientTracingFeature.class); } } From f71ba140168e481fb20fc4476a072ed891ad3eef Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Fri, 20 Dec 2019 18:28:46 +0100 Subject: [PATCH 20/22] [rmi] use simple thread local for propagating context + add more documentation --- .../rmi/ThreadLocalContext.java | 24 ++++++++ .../rmi/client/RmiClientAdvice.java | 38 ------------- .../rmi/client/RmiClientInstrumentation.java | 37 +++++++++++- .../rmi/context/ContextPayload.java | 13 ++++- .../rmi/context/ContextPropagator.java | 53 ++++++++++-------- .../RmiClientContextInstrumentation.java | 54 +++++++++++++++++- .../StreamRemoteCallConstructorAdvice.java | 56 ------------------- .../rmi/context/server/ContextDispatcher.java | 32 ++++++++--- .../rmi/context/server/ObjectTableAdvice.java | 41 -------------- .../RmiServerContextInstrumentation.java | 28 +++++++--- .../rmi/server/RmiServerDecorator.java | 20 ------- .../rmi/server/RmiServerInstrumentation.java | 30 +++++----- 12 files changed, 212 insertions(+), 214 deletions(-) create mode 100644 dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/rmi/ThreadLocalContext.java delete mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientAdvice.java delete mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/StreamRemoteCallConstructorAdvice.java delete mode 100644 dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ObjectTableAdvice.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/rmi/ThreadLocalContext.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/rmi/ThreadLocalContext.java new file mode 100644 index 0000000000..d4c7e6fc81 --- /dev/null +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/rmi/ThreadLocalContext.java @@ -0,0 +1,24 @@ +package datadog.trace.bootstrap.instrumentation.rmi; + +import datadog.trace.instrumentation.api.AgentSpan; + +public class ThreadLocalContext { + public static final ThreadLocalContext THREAD_LOCAL_CONTEXT = new ThreadLocalContext(); + private final ThreadLocal local; + + public ThreadLocalContext() { + local = new ThreadLocal<>(); + } + + public void set(final AgentSpan.Context context) { + local.set(context); + } + + public AgentSpan.Context getAndResetContext() { + final AgentSpan.Context context = local.get(); + if (context != null) { + local.remove(); + } + return context; + } +} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientAdvice.java deleted file mode 100644 index 2819807937..0000000000 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientAdvice.java +++ /dev/null @@ -1,38 +0,0 @@ -package datadog.trace.instrumentation.rmi.client; - -import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; -import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; -import static datadog.trace.instrumentation.api.AgentTracer.startSpan; -import static datadog.trace.instrumentation.rmi.client.RmiClientDecorator.DECORATE; - -import datadog.trace.api.DDTags; -import datadog.trace.instrumentation.api.AgentScope; -import datadog.trace.instrumentation.api.AgentSpan; -import java.lang.reflect.Method; -import net.bytebuddy.asm.Advice; - -public class RmiClientAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) - public static AgentScope onEnter(@Advice.Argument(value = 1) final Method method) { - if (activeSpan() == null) { - return null; - } - final AgentSpan span = - startSpan("rmi.invoke") - .setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForMethod(method)) - .setTag("span.origin.type", method.getDeclaringClass().getCanonicalName()); - - DECORATE.afterStart(span); - return activateSpan(span, true); - } - - @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - public static void stopSpan( - @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { - if (scope == null) { - return; - } - DECORATE.onError(scope, throwable); - scope.close(); - } -} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java index 76a8e92a08..259372ed30 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/client/RmiClientInstrumentation.java @@ -1,6 +1,10 @@ package datadog.trace.instrumentation.rmi.client; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; +import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; +import static datadog.trace.instrumentation.api.AgentTracer.startSpan; +import static datadog.trace.instrumentation.rmi.client.RmiClientDecorator.DECORATE; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; @@ -10,7 +14,12 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.api.DDTags; +import datadog.trace.instrumentation.api.AgentScope; +import datadog.trace.instrumentation.api.AgentSpan; +import java.lang.reflect.Method; 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; @@ -43,6 +52,32 @@ public final class RmiClientInstrumentation extends Instrumenter.Default { .and(named("invoke")) .and(takesArgument(0, named("java.rmi.Remote"))) .and(takesArgument(1, named("java.lang.reflect.Method"))), - packageName + ".RmiClientAdvice"); + getClass().getName() + "$RmiClientAdvice"); + } + + public static class RmiClientAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static AgentScope onEnter(@Advice.Argument(value = 1) final Method method) { + if (activeSpan() == null) { + return null; + } + final AgentSpan span = + startSpan("rmi.invoke") + .setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForMethod(method)) + .setTag("span.origin.type", method.getDeclaringClass().getCanonicalName()); + + DECORATE.afterStart(span); + return activateSpan(span, true); + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void stopSpan( + @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { + if (scope == null) { + return; + } + DECORATE.onError(scope, throwable); + scope.close(); + } } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java index 25de0574c2..a7fe9b1ff5 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPayload.java @@ -1,17 +1,20 @@ package datadog.trace.instrumentation.rmi.context; +import static datadog.trace.instrumentation.api.AgentTracer.propagate; + import datadog.trace.instrumentation.api.AgentPropagation; +import datadog.trace.instrumentation.api.AgentSpan; import java.io.IOException; import java.io.ObjectInput; import java.io.ObjectOutput; -import java.io.Serializable; import java.util.HashMap; import java.util.Map; import lombok.Getter; import lombok.extern.slf4j.Slf4j; +/** ContextPayload wraps context information shared between client and server */ @Slf4j -public class ContextPayload implements Serializable { +public class ContextPayload { @Getter private final Map context; public static final ExtractAdapter GETTER = new ExtractAdapter(); public static final InjectAdapter SETTER = new InjectAdapter(); @@ -24,6 +27,12 @@ public class ContextPayload implements Serializable { this.context = context; } + public static ContextPayload from(final AgentSpan span) { + final ContextPayload payload = new ContextPayload(); + propagate().inject(span, payload, SETTER); + return payload; + } + public static ContextPayload read(final ObjectInput oi) throws IOException { try { final Object object = oi.readObject(); diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPropagator.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPropagator.java index 7b8aae8548..2c8a829f7d 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPropagator.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/ContextPropagator.java @@ -1,8 +1,5 @@ package datadog.trace.instrumentation.rmi.context; -import static datadog.trace.instrumentation.api.AgentTracer.propagate; -import static datadog.trace.instrumentation.rmi.context.ContextPayload.SETTER; - import datadog.trace.bootstrap.ContextStore; import datadog.trace.instrumentation.api.AgentSpan; import java.io.IOException; @@ -16,44 +13,55 @@ import sun.rmi.transport.TransportConstants; @Slf4j public class ContextPropagator { + // Internal RMI object ids that we don't want to trace private static final ObjID ACTIVATOR_ID = new ObjID(ObjID.ACTIVATOR_ID); private static final ObjID DGC_ID = new ObjID(ObjID.DGC_ID); private static final ObjID REGISTRY_ID = new ObjID(ObjID.REGISTRY_ID); - public static final ObjID DD_CONTEXT_CALL_ID = new ObjID("Datadog.v1.context_call".hashCode()); - private static final int CONTEXT_CHECK_CALL_OP_ID = -1; - public static final int CONTEXT_PASS_OPERATION_ID = -2; - public static boolean isRMIInternalObject(final ObjID id) { - return ACTIVATOR_ID.equals(id) || DGC_ID.equals(id) || REGISTRY_ID.equals(id); - } + // RMI object id used to identify DataDog instrumentation + public static final ObjID DD_CONTEXT_CALL_ID = new ObjID("Datadog.v1.context_call".hashCode()); + + // Operation id used for checking context propagation is possible + // RMI expects these operations to have negative identifier, as positive ones mean legacy + // precompiled Stubs would be used instead + private static final int CONTEXT_CHECK_CALL_OPERATION_ID = -1; + // Seconds step of context propagation which contains actual payload + private static final int CONTEXT_PAYLOAD_OPERATION_ID = -2; public static final ContextPropagator PROPAGATOR = new ContextPropagator(); + public boolean isRMIInternalObject(final ObjID id) { + return ACTIVATOR_ID.equals(id) || DGC_ID.equals(id) || REGISTRY_ID.equals(id); + } + + public boolean isOperationWithPayload(final int operationId) { + return operationId == CONTEXT_PAYLOAD_OPERATION_ID; + } + public void attemptToPropagateContext( - final ContextStore contextStore, + final ContextStore knownConnections, final Connection c, final AgentSpan span) { - if (checkIfContextCanBePassed(contextStore, c)) { - final ContextPayload payload = new ContextPayload(); - propagate().inject(span, payload, SETTER); - if (!syntheticCall(c, payload, CONTEXT_PASS_OPERATION_ID)) { - log.debug("Couldn't propagate context"); + if (checkIfContextCanBePassed(knownConnections, c)) { + if (!syntheticCall(c, ContextPayload.from(span), CONTEXT_PAYLOAD_OPERATION_ID)) { + log.debug("Couldn't send context payload"); } } } private boolean checkIfContextCanBePassed( - final ContextStore contextStore, final Connection c) { - final Boolean storedResult = contextStore.get(c); + final ContextStore knownConnections, final Connection c) { + final Boolean storedResult = knownConnections.get(c); if (storedResult != null) { return storedResult; } - final boolean result = syntheticCall(c, null, CONTEXT_CHECK_CALL_OP_ID); - contextStore.put(c, result); + final boolean result = syntheticCall(c, null, CONTEXT_CHECK_CALL_OPERATION_ID); + knownConnections.put(c, result); return result; } + /** @returns true when no error happened during call */ private boolean syntheticCall( final Connection c, final ContextPayload payload, final int operationId) { final StreamRemoteCall shareContextCall = new StreamRemoteCall(c); @@ -68,9 +76,10 @@ public class ContextPropagator { out.writeInt(operationId); // in normal call this is method number (operation index) out.writeLong(operationId); // in normal RMI call this holds stub/skeleton hash - // if method is not found by uninstrumented code then writing payload will cause an exception - // in - // RMI server - as the payload will be interpreted as another call + // Payload should be sent only after we make sure we're connected to instrumented server + // + // if method is not found by un-instrumented code then writing payload will cause an exception + // in RMI server - as the payload will be interpreted as another call // but it will not be parsed correctly - closing connection if (payload != null) { payload.write(out); diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/RmiClientContextInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/RmiClientContextInstrumentation.java index 818930a5e3..1e63a346e5 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/RmiClientContextInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/RmiClientContextInstrumentation.java @@ -1,6 +1,8 @@ package datadog.trace.instrumentation.rmi.context.client; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; +import static datadog.trace.instrumentation.rmi.context.ContextPropagator.PROPAGATOR; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isConstructor; import static net.bytebuddy.matcher.ElementMatchers.isInterface; @@ -10,11 +12,38 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.bootstrap.ContextStore; +import datadog.trace.bootstrap.InstrumentationContext; +import datadog.trace.instrumentation.api.AgentSpan; +import java.rmi.server.ObjID; 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 sun.rmi.transport.Connection; +/** + * Main entry point for transferring context between RMI service. + * + *

It injects into StreamRemoteCall constructor used for invoking remote tasks and performs a + * backwards compatible check to ensure if the other side is prepared to receive context propagation + * messages then if successful sends a context propagation message + * + *

Context propagation consist of a Serialized HashMap with all data set by usual context + * injection, which includes things like sampling priority, trace and parent id + * + *

As well as optional baggage items + * + *

On the other side of the communication a special Dispatcher is created when a message with + * DD_CONTEXT_CALL_ID is received. + * + *

If the server is not instrumented first call will gracefully fail just like any other unknown + * call. With small caveat that this first call needs to *not* have any parameters, since those will + * not be read from connection and instead will be interpreted as another remote instruction, but + * that instruction will essentially be garbage data and will cause the parsing loop to throw + * exception and shutdown the connection which we do not want + */ @AutoService(Instrumenter.class) public class RmiClientContextInstrumentation extends Instrumenter.Default { @@ -49,6 +78,29 @@ public class RmiClientContextInstrumentation extends Instrumenter.Default { isConstructor() .and(takesArgument(0, named("sun.rmi.transport.Connection"))) .and(takesArgument(1, named("java.rmi.server.ObjID"))), - packageName + ".StreamRemoteCallConstructorAdvice"); + getClass().getName() + "$StreamRemoteCallConstructorAdvice"); + } + + public static class StreamRemoteCallConstructorAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void onEnter( + @Advice.Argument(value = 0) final Connection c, + @Advice.Argument(value = 1) final ObjID id) { + if (!c.isReusable()) { + return; + } + if (PROPAGATOR.isRMIInternalObject(id)) { + return; + } + final AgentSpan activeSpan = activeSpan(); + if (activeSpan == null) { + return; + } + + final ContextStore knownConnections = + InstrumentationContext.get(Connection.class, Boolean.class); + + PROPAGATOR.attemptToPropagateContext(knownConnections, c, activeSpan); + } } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/StreamRemoteCallConstructorAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/StreamRemoteCallConstructorAdvice.java deleted file mode 100644 index dc457b979f..0000000000 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/client/StreamRemoteCallConstructorAdvice.java +++ /dev/null @@ -1,56 +0,0 @@ -package datadog.trace.instrumentation.rmi.context.client; - -import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; -import static datadog.trace.instrumentation.rmi.context.ContextPropagator.PROPAGATOR; - -import datadog.trace.bootstrap.ContextStore; -import datadog.trace.bootstrap.InstrumentationContext; -import datadog.trace.instrumentation.api.AgentSpan; -import datadog.trace.instrumentation.rmi.context.ContextPropagator; -import java.rmi.server.ObjID; -import net.bytebuddy.asm.Advice; -import sun.rmi.transport.Connection; - -/** - * Main entry point for transferring context between RMI service. - * - *

It injects into StreamRemoteCall constructor used for invoking remote tasks and performs a - * backwards compatible check to ensure if the other side is prepared to receive context propagation - * messages then if successful sends a context propagation message - * - *

Context propagation consist of a Serialized HashMap with all data set by usual context - * injection, which includes things like sampling priority, trace and parent id - * - *

As well as optional baggage items - * - *

On the other side of the communication a special Dispatcher is created when a message with - * DD_CONTEXT_CALL_ID is received. - * - *

If the server is not instrumented first call will gracefully fail just like any other unknown - * call. With small caveat that this first call needs to *not* have any parameters, since those will - * not be read from connection and instead will be interpreted as another remote instruction, but - * that instruction will essentially be garbage data and will cause the parsing loop to throw - * exception and shutdown the connection which we do not want - */ -public class StreamRemoteCallConstructorAdvice { - - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void onEnter( - @Advice.Argument(value = 0) final Connection c, @Advice.Argument(value = 1) final ObjID id) { - if (!c.isReusable()) { - return; - } - if (ContextPropagator.isRMIInternalObject(id)) { - return; - } - final AgentSpan activeSpan = activeSpan(); - if (activeSpan == null) { - return; - } - - final ContextStore contextStore = - InstrumentationContext.get(Connection.class, Boolean.class); - - PROPAGATOR.attemptToPropagateContext(contextStore, c, activeSpan); - } -} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ContextDispatcher.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ContextDispatcher.java index d3a11bc177..c341331e5a 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ContextDispatcher.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ContextDispatcher.java @@ -1,24 +1,37 @@ package datadog.trace.instrumentation.rmi.context.server; +import static datadog.trace.bootstrap.instrumentation.rmi.ThreadLocalContext.THREAD_LOCAL_CONTEXT; import static datadog.trace.instrumentation.api.AgentTracer.propagate; import static datadog.trace.instrumentation.rmi.context.ContextPayload.GETTER; +import static datadog.trace.instrumentation.rmi.context.ContextPropagator.DD_CONTEXT_CALL_ID; +import static datadog.trace.instrumentation.rmi.context.ContextPropagator.PROPAGATOR; -import datadog.trace.bootstrap.ContextStore; import datadog.trace.instrumentation.api.AgentSpan; import datadog.trace.instrumentation.rmi.context.ContextPayload; -import datadog.trace.instrumentation.rmi.context.ContextPropagator; import java.io.IOException; import java.io.ObjectInput; import java.rmi.Remote; import java.rmi.server.RemoteCall; import sun.rmi.server.Dispatcher; +import sun.rmi.transport.Target; +/** + * ContextDispatcher is responsible for handling both initial context propagation check call and + * following call which carries payload + * + *

Context propagation check is only expected not to throw any exception, hinting to the client + * that its communicating with an instrumented server. Non instrumented server would've thrown + * UnknownObjectException + * + *

Because caching of the result after first call on a connection, only payload calls are + * expected + */ public class ContextDispatcher implements Dispatcher { + private static final ContextDispatcher CONTEXT_DISPATCHER = new ContextDispatcher(); + private static final NoopRemote NOOP_REMOTE = new NoopRemote(); - private final ContextStore callableContextStore; - - public ContextDispatcher(final ContextStore callableContextStore) { - this.callableContextStore = callableContextStore; + public static Target newDispatcherTarget() { + return new Target(NOOP_REMOTE, CONTEXT_DISPATCHER, NOOP_REMOTE, DD_CONTEXT_CALL_ID, false); } @Override @@ -27,12 +40,11 @@ public class ContextDispatcher implements Dispatcher { final int operationId = in.readInt(); in.readLong(); // skip 8 bytes - if (operationId == ContextPropagator.CONTEXT_PASS_OPERATION_ID) { + if (PROPAGATOR.isOperationWithPayload(operationId)) { final ContextPayload payload = ContextPayload.read(in); if (payload != null) { final AgentSpan.Context context = propagate().extract(payload, GETTER); - - callableContextStore.put(Thread.currentThread(), context); + THREAD_LOCAL_CONTEXT.set(context); } } @@ -44,4 +56,6 @@ public class ContextDispatcher implements Dispatcher { call.releaseOutputStream(); call.done(); } + + public static class NoopRemote implements Remote {} } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ObjectTableAdvice.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ObjectTableAdvice.java deleted file mode 100644 index f187c2ef7f..0000000000 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/ObjectTableAdvice.java +++ /dev/null @@ -1,41 +0,0 @@ -package datadog.trace.instrumentation.rmi.context.server; - -import static datadog.trace.instrumentation.rmi.context.ContextPropagator.DD_CONTEXT_CALL_ID; - -import datadog.trace.bootstrap.ContextStore; -import datadog.trace.bootstrap.InstrumentationContext; -import datadog.trace.instrumentation.api.AgentSpan; -import java.rmi.Remote; -import net.bytebuddy.asm.Advice; -import sun.rmi.transport.Target; - -public class ObjectTableAdvice { - - @Advice.OnMethodExit(suppress = Throwable.class) - public static void methodExit( - @Advice.Argument(0) final Object oe, @Advice.Return(readOnly = false) Target result) { - - // comparing toString() output allows us to avoid using reflection to be able to compare - // ObjID and ObjectEndpoint objects - // ObjectEndpoint#toString() only returns this.objId.toString() value which is exactly - // what we're interested in here. - if (!DD_CONTEXT_CALL_ID.toString().equals(oe.toString())) { - return; - } - - final ContextStore callableContextStore = - InstrumentationContext.get(Thread.class, AgentSpan.Context.class); - - result = - new Target( - NoopRemote.NOOP_REMOTE, - new ContextDispatcher(callableContextStore), - NoopRemote.NOOP_REMOTE, - DD_CONTEXT_CALL_ID, - false); - } - - public static class NoopRemote implements Remote { - public static final NoopRemote NOOP_REMOTE = new NoopRemote(); - } -} diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/RmiServerContextInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/RmiServerContextInstrumentation.java index 3eb276bb7b..1ce41f9310 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/RmiServerContextInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/context/server/RmiServerContextInstrumentation.java @@ -1,6 +1,7 @@ package datadog.trace.instrumentation.rmi.context.server; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.rmi.context.ContextPropagator.DD_CONTEXT_CALL_ID; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; @@ -12,9 +13,11 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; 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 sun.rmi.transport.Target; @AutoService(Instrumenter.class) public class RmiServerContextInstrumentation extends Instrumenter.Default { @@ -28,11 +31,6 @@ public class RmiServerContextInstrumentation extends Instrumenter.Default { return not(isInterface()).and(safeHasSuperType(named("sun.rmi.transport.ObjectTable"))); } - @Override - public Map contextStore() { - return singletonMap("java.lang.Thread", "datadog.trace.instrumentation.api.AgentSpan$Context"); - } - @Override public String[] helperClassNames() { return new String[] { @@ -41,7 +39,7 @@ public class RmiServerContextInstrumentation extends Instrumenter.Default { "datadog.trace.instrumentation.rmi.context.ContextPayload", "datadog.trace.instrumentation.rmi.context.ContextPropagator", packageName + ".ContextDispatcher", - packageName + ".ObjectTableAdvice$NoopRemote" + packageName + ".ContextDispatcher$NoopRemote" }; } @@ -52,6 +50,22 @@ public class RmiServerContextInstrumentation extends Instrumenter.Default { .and(isStatic()) .and(named("getTarget")) .and((takesArgument(0, named("sun.rmi.transport.ObjectEndpoint")))), - packageName + ".ObjectTableAdvice"); + getClass().getName() + "$ObjectTableAdvice"); + } + + public static class ObjectTableAdvice { + + @Advice.OnMethodExit(suppress = Throwable.class) + public static void methodExit( + @Advice.Argument(0) final Object oe, @Advice.Return(readOnly = false) Target result) { + // comparing toString() output allows us to avoid using reflection to be able to compare + // ObjID and ObjectEndpoint objects + // ObjectEndpoint#toString() only returns this.objId.toString() value which is exactly + // what we're interested in here. + if (!DD_CONTEXT_CALL_ID.toString().equals(oe.toString())) { + return; + } + result = ContextDispatcher.newDispatcherTarget(); + } } } diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerDecorator.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerDecorator.java index edd09cad90..155d64c7b2 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerDecorator.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerDecorator.java @@ -1,31 +1,11 @@ package datadog.trace.instrumentation.rmi.server; -import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; -import static datadog.trace.instrumentation.api.AgentTracer.startSpan; - import datadog.trace.agent.decorator.ServerDecorator; import datadog.trace.api.DDSpanTypes; -import datadog.trace.bootstrap.ContextStore; -import datadog.trace.instrumentation.api.AgentSpan; public class RmiServerDecorator extends ServerDecorator { public static final RmiServerDecorator DECORATE = new RmiServerDecorator(); - public AgentSpan startSpanWithContext( - final ContextStore contextStore) { - if (activeSpan() != null) { - return startSpan("rmi.request"); - } - - final AgentSpan.Context context = contextStore.get(Thread.currentThread()); - - if (context == null) { - return startSpan("rmi.request"); - } else { - return startSpan("rmi.request", context); - } - } - @Override protected String[] instrumentationNames() { return new String[] {"rmi"}; diff --git a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java index f479120483..5a96051c24 100644 --- a/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java +++ b/dd-java-agent/instrumentation/rmi/src/main/java/datadog/trace/instrumentation/rmi/server/RmiServerInstrumentation.java @@ -1,7 +1,9 @@ package datadog.trace.instrumentation.rmi.server; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.bootstrap.instrumentation.rmi.ThreadLocalContext.THREAD_LOCAL_CONTEXT; import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; +import static datadog.trace.instrumentation.api.AgentTracer.startSpan; import static datadog.trace.instrumentation.rmi.server.RmiServerDecorator.DECORATE; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isInterface; @@ -14,12 +16,9 @@ import static net.bytebuddy.matcher.ElementMatchers.not; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.api.DDTags; -import datadog.trace.bootstrap.ContextStore; -import datadog.trace.bootstrap.InstrumentationContext; import datadog.trace.instrumentation.api.AgentScope; import datadog.trace.instrumentation.api.AgentSpan; import java.lang.reflect.Method; -import java.util.Collections; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -33,11 +32,6 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { super("rmi", "rmi-server"); } - @Override - public Map contextStore() { - return Collections.singletonMap(Thread.class.getName(), AgentSpan.Context.class.getName()); - } - @Override public String[] helperClassNames() { return new String[] { @@ -55,22 +49,24 @@ public final class RmiServerInstrumentation extends Instrumenter.Default { @Override public Map, String> transformers() { return singletonMap( - isMethod().and(isPublic()).and(not(isStatic())), - packageName + ".RmiServerInstrumentation$ServerAdvice"); + isMethod().and(isPublic()).and(not(isStatic())), getClass().getName() + "$ServerAdvice"); } public static class ServerAdvice { @Advice.OnMethodEnter(suppress = Throwable.class, inline = true) public static AgentScope onEnter( @Advice.This final Object thiz, @Advice.Origin final Method method) { - final ContextStore threadContextStore = - InstrumentationContext.get(Thread.class, AgentSpan.Context.class); + final AgentSpan.Context context = THREAD_LOCAL_CONTEXT.getAndResetContext(); - final AgentSpan span = - DECORATE - .startSpanWithContext(threadContextStore) - .setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForMethod(method)) - .setTag("span.origin.type", thiz.getClass().getCanonicalName()); + final AgentSpan span; + if (context == null) { + span = startSpan("rmi.request"); + } else { + span = startSpan("rmi.request", context); + } + + span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForMethod(method)) + .setTag("span.origin.type", thiz.getClass().getCanonicalName()); DECORATE.afterStart(span); return activateSpan(span, true); From b8210ceea5dd2ca5958ee3aca68aa1377f921f40 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Mon, 23 Dec 2019 09:33:30 -0800 Subject: [PATCH 21/22] Version 0.40.0 --- dd-trace-java.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-trace-java.gradle b/dd-trace-java.gradle index 30c96b6b37..bbd845aebd 100644 --- a/dd-trace-java.gradle +++ b/dd-trace-java.gradle @@ -16,7 +16,7 @@ def isCI = System.getenv("CI") != null allprojects { group = 'com.datadoghq' - version = '0.40.0-SNAPSHOT' + version = '0.40.0' if (isCI) { buildDir = "${rootDir}/workspace/${projectDir.path.replace(rootDir.path, '')}/build/" From ab33ed4a025c4dda7e4e8c94f591e4f72822829d Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Mon, 23 Dec 2019 10:04:47 -0800 Subject: [PATCH 22/22] Begin 0.41.0 --- dd-trace-java.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-trace-java.gradle b/dd-trace-java.gradle index bbd845aebd..9bd8059084 100644 --- a/dd-trace-java.gradle +++ b/dd-trace-java.gradle @@ -16,7 +16,7 @@ def isCI = System.getenv("CI") != null allprojects { group = 'com.datadoghq' - version = '0.40.0' + version = '0.41.0-SNAPSHOT' if (isCI) { buildDir = "${rootDir}/workspace/${projectDir.path.replace(rootDir.path, '')}/build/"