From b5759bd15c06e9d19d702a16cb15cb77fe331e74 Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Tue, 13 Mar 2018 10:01:00 -0700 Subject: [PATCH 1/3] Add decorators to servlet tests --- .../servlet-3/src/test/groovy/JettyServletTest.groovy | 10 ++++++++-- .../src/test/groovy/TomcatServletTest.groovy | 11 +++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy index 890589caa95..980a1425d87 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy @@ -1,5 +1,7 @@ import datadog.opentracing.DDSpan import datadog.opentracing.DDTracer +import datadog.opentracing.decorators.AbstractDecorator +import datadog.opentracing.decorators.DDDecoratorsFactory import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanTypes import datadog.trace.common.writer.ListWriter @@ -52,6 +54,10 @@ class JettyServletTest extends AgentTestRunner { DDTracer tracer = new DDTracer(writer) def setup() { + final List decorators = DDDecoratorsFactory.createBuiltinDecorators() + for (final AbstractDecorator decorator : decorators) { + tracer.addDecorator(decorator) + } jettyServer = new Server(PORT) servletContext = new ServletContextHandler() @@ -100,7 +106,7 @@ class JettyServletTest extends AgentTestRunner { span.context().serviceName == "unnamed-java-app" span.context().operationName == "servlet.request" - span.context().resourceName == "servlet.request" + span.context().resourceName == "GET /$path" span.context().spanType == DDSpanTypes.WEB_SERVLET !span.context().getErrorFlag() span.context().parentId != 0 // parent should be the okhttp call. @@ -137,7 +143,7 @@ class JettyServletTest extends AgentTestRunner { span.context().serviceName == "unnamed-java-app" span.context().operationName == "servlet.request" - span.context().resourceName == "servlet.request" + span.context().resourceName == "GET /$path" span.context().spanType == DDSpanTypes.WEB_SERVLET span.context().getErrorFlag() span.context().parentId != 0 // parent should be the okhttp call. diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy index 47b970e2c15..99f51442a78 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy @@ -1,5 +1,7 @@ import com.google.common.io.Files import datadog.opentracing.DDTracer +import datadog.opentracing.decorators.AbstractDecorator +import datadog.opentracing.decorators.DDDecoratorsFactory import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanTypes import datadog.trace.common.writer.ListWriter @@ -33,6 +35,11 @@ class TomcatServletTest extends AgentTestRunner { DDTracer tracer = new DDTracer(writer) def setup() { + final List decorators = DDDecoratorsFactory.createBuiltinDecorators() + for (final AbstractDecorator decorator : decorators) { + tracer.addDecorator(decorator) + } + tomcatServer = new Tomcat() tomcatServer.setPort(PORT) @@ -99,7 +106,7 @@ class TomcatServletTest extends AgentTestRunner { span.context().serviceName == "unnamed-java-app" span.context().operationName == "servlet.request" - span.context().resourceName == "servlet.request" + span.context().resourceName == "GET /$path" span.context().spanType == DDSpanTypes.WEB_SERVLET !span.context().getErrorFlag() span.context().parentId != 0 // parent should be the okhttp call. @@ -136,7 +143,7 @@ class TomcatServletTest extends AgentTestRunner { span.context().serviceName == "unnamed-java-app" span.context().operationName == "servlet.request" - span.context().resourceName == "servlet.request" + span.context().resourceName == "GET /$path" span.context().spanType == DDSpanTypes.WEB_SERVLET span.context().getErrorFlag() span.context().parentId != 0 // parent should be the okhttp call. From b5b38ee4b1a78bf4190b0f189b6c5ea37b0d7263 Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Tue, 13 Mar 2018 10:15:44 -0700 Subject: [PATCH 2/3] Mark all http status codes 5xx as an error. --- .../src/test/groovy/JettyServletTest.groovy | 40 +++++++++++++++++++ .../src/test/groovy/TestServlet.groovy | 4 ++ .../src/test/groovy/TomcatServletTest.groovy | 40 +++++++++++++++++++ .../groovy/test/SpringBootBasedTest.groovy | 4 +- .../decorators/DDDecoratorsFactory.java | 1 + .../decorators/Status5XXDecorator.java | 24 +++++++++++ 6 files changed, 111 insertions(+), 2 deletions(-) create mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status5XXDecorator.java diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy index 980a1425d87..143e0394d0d 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy @@ -166,6 +166,46 @@ class JettyServletTest extends AgentTestRunner { "sync" | "Hello Sync" } + @Unroll + def "test #path non-throwing-error servlet call"() { + setup: + def request = new Request.Builder() + .url("http://localhost:$PORT/$path?non-throwing-error=true") + .get() + .build() + def response = client.newCall(request).execute() + + expect: + response.body().string().trim() != expectedResponse + writer.size() == 2 // second (parent) trace is the okhttp call above... + def trace = writer.firstTrace() + trace.size() == 1 + def span = trace[0] + + span.context().serviceName == "unnamed-java-app" + span.context().operationName == "servlet.request" + span.context().resourceName == "GET /$path" + span.context().spanType == DDSpanTypes.WEB_SERVLET + span.context().getErrorFlag() + span.context().parentId != 0 // parent should be the okhttp call. + span.context().tags["http.url"] == "http://localhost:$PORT/$path" + span.context().tags["http.method"] == "GET" + span.context().tags["span.kind"] == "server" + span.context().tags["component"] == "java-web-servlet" + span.context().tags["http.status_code"] == 500 + span.context().tags["thread.name"] != null + span.context().tags["thread.id"] != null + span.context().tags["error"] == true + span.context().tags["error.msg"] == null + span.context().tags["error.type"] == null + span.context().tags["error.stack"] == null + span.context().tags.size() == 9 + + where: + path | expectedResponse + "sync" | "Hello Sync" + } + private static int randomOpenPort() { new ServerSocket(0).withCloseable { it.setReuseAddress(true) diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TestServlet.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TestServlet.groovy index fa25eaaa1d5..cf62ea98963 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TestServlet.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TestServlet.groovy @@ -13,6 +13,10 @@ class TestServlet { if (req.getParameter("error") != null) { throw new RuntimeException("some sync error") } + if (req.getParameter("non-throwing-error") != null) { + resp.sendError(500, "some sync error") + return + } resp.writer.print("Hello Sync") } } diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy index 99f51442a78..2ac4cc4061d 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy @@ -166,6 +166,46 @@ class TomcatServletTest extends AgentTestRunner { "sync" | "Hello Sync" } + @Unroll + def "test #path error servlet call for non-throwing error"() { + setup: + def request = new Request.Builder() + .url("http://localhost:$PORT/$path?non-throwing-error=true") + .get() + .build() + def response = client.newCall(request).execute() + + expect: + response.body().string().trim() != expectedResponse + writer.size() == 2 // second (parent) trace is the okhttp call above... + def trace = writer.firstTrace() + trace.size() == 1 + def span = trace[0] + + span.context().serviceName == "unnamed-java-app" + span.context().operationName == "servlet.request" + span.context().resourceName == "GET /$path" + span.context().spanType == DDSpanTypes.WEB_SERVLET + span.context().getErrorFlag() + span.context().parentId != 0 // parent should be the okhttp call. + span.context().tags["http.url"] == "http://localhost:$PORT/$path" + span.context().tags["http.method"] == "GET" + span.context().tags["span.kind"] == "server" + span.context().tags["component"] == "java-web-servlet" + span.context().tags["http.status_code"] == 500 + span.context().tags["thread.name"] != null + span.context().tags["thread.id"] != null + span.context().tags["error"] == true + span.context().tags["error.msg"] == null + span.context().tags["error.type"] == null + span.context().tags["error.stack"] == null + span.context().tags.size() == 9 + + where: + path | expectedResponse + "sync" | "Hello Sync" + } + private static int randomOpenPort() { new ServerSocket(0).withCloseable { it.setReuseAddress(true) diff --git a/dd-java-agent/instrumentation/spring-web/src/test/groovy/test/SpringBootBasedTest.groovy b/dd-java-agent/instrumentation/spring-web/src/test/groovy/test/SpringBootBasedTest.groovy index 93be3b64b62..38283272a52 100644 --- a/dd-java-agent/instrumentation/spring-web/src/test/groovy/test/SpringBootBasedTest.groovy +++ b/dd-java-agent/instrumentation/spring-web/src/test/groovy/test/SpringBootBasedTest.groovy @@ -148,7 +148,6 @@ class SpringBootBasedTest extends AgentTestRunner { span1.context().operationName == "servlet.request" span1.context().resourceName == "GET /error" span1.context().spanType == DDSpanTypes.WEB_SERVLET - !span1.context().getErrorFlag() span1.context().parentId == 0 span1.context().tags["http.url"] == "http://localhost:$port/error" span1.context().tags["http.method"] == "GET" @@ -156,9 +155,10 @@ class SpringBootBasedTest extends AgentTestRunner { span1.context().tags["span.type"] == "web" span1.context().tags["component"] == "java-web-servlet" span1.context().tags["http.status_code"] == 500 + span1.context().getErrorFlag() span1.context().tags["thread.name"] != null span1.context().tags["thread.id"] != null - span1.context().tags.size() == 8 + span1.context().tags.size() == 9 } def "validated form"() { 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 94962bcbffb..0786cd8c481 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 @@ -25,6 +25,7 @@ public static List createBuiltinDecorators() { builtin.add(new OperationDecorator()); builtin.add(new Status404Decorator()); builtin.add(new URLAsResourceName()); + builtin.add(new Status5XXDecorator()); return builtin; } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status5XXDecorator.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status5XXDecorator.java new file mode 100644 index 00000000000..6a094e80c46 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status5XXDecorator.java @@ -0,0 +1,24 @@ +package datadog.opentracing.decorators; + +import datadog.opentracing.DDSpanContext; +import io.opentracing.tag.Tags; + +/** Mark all 5xx status codes as an error */ +public class Status5XXDecorator extends AbstractDecorator { + public Status5XXDecorator() { + super(); + this.setMatchingTag(Tags.HTTP_STATUS.getKey()); + } + + @Override + public boolean afterSetTag(final DDSpanContext context, final String tag, final Object value) { + if (Tags.HTTP_STATUS.getKey().equals(tag)) { + final int responseCode = Integer.parseInt(value.toString()); + if (500 <= responseCode && responseCode < 600) { + context.setTag(Tags.ERROR.getKey(), true); + return true; + } + } + return false; + } +} From 10e12a666aeeb672e26183b4bf9dee916176e986 Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Tue, 13 Mar 2018 10:32:26 -0700 Subject: [PATCH 3/3] Use builtin decorators for every DDTracer --- .../src/test/groovy/JettyServletTest.groovy | 4 ++-- .../src/test/groovy/JettyServletTest.groovy | 6 ------ .../src/test/groovy/TomcatServletTest.groovy | 7 ------- .../datadog/trace/agent/test/AgentTestRunner.java | 6 ------ .../src/main/java/datadog/opentracing/DDTracer.java | 13 ++++++------- 5 files changed, 8 insertions(+), 28 deletions(-) diff --git a/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServletTest.groovy b/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServletTest.groovy index 1230b2b90df..79615fd632b 100644 --- a/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServletTest.groovy +++ b/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServletTest.groovy @@ -99,7 +99,7 @@ class JettyServletTest extends AgentTestRunner { span.context().serviceName == "unnamed-java-app" span.context().operationName == "servlet.request" - span.context().resourceName == "servlet.request" + span.context().resourceName == "GET /$path" span.context().spanType == DDSpanTypes.WEB_SERVLET !span.context().getErrorFlag() span.context().parentId != 0 // parent should be the okhttp call. @@ -134,7 +134,7 @@ class JettyServletTest extends AgentTestRunner { def span = trace[0] span.context().operationName == "servlet.request" - span.context().resourceName == "servlet.request" + span.context().resourceName == "GET /$path" span.context().spanType == DDSpanTypes.WEB_SERVLET span.context().getErrorFlag() span.context().parentId != 0 // parent should be the okhttp call. diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy index 143e0394d0d..db810dec931 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServletTest.groovy @@ -1,7 +1,5 @@ import datadog.opentracing.DDSpan import datadog.opentracing.DDTracer -import datadog.opentracing.decorators.AbstractDecorator -import datadog.opentracing.decorators.DDDecoratorsFactory import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanTypes import datadog.trace.common.writer.ListWriter @@ -54,10 +52,6 @@ class JettyServletTest extends AgentTestRunner { DDTracer tracer = new DDTracer(writer) def setup() { - final List decorators = DDDecoratorsFactory.createBuiltinDecorators() - for (final AbstractDecorator decorator : decorators) { - tracer.addDecorator(decorator) - } jettyServer = new Server(PORT) servletContext = new ServletContextHandler() diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy index 2ac4cc4061d..73377c9df8f 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServletTest.groovy @@ -1,7 +1,5 @@ import com.google.common.io.Files import datadog.opentracing.DDTracer -import datadog.opentracing.decorators.AbstractDecorator -import datadog.opentracing.decorators.DDDecoratorsFactory import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanTypes import datadog.trace.common.writer.ListWriter @@ -35,11 +33,6 @@ class TomcatServletTest extends AgentTestRunner { DDTracer tracer = new DDTracer(writer) def setup() { - final List decorators = DDDecoratorsFactory.createBuiltinDecorators() - for (final AbstractDecorator decorator : decorators) { - tracer.addDecorator(decorator) - } - tomcatServer = new Tomcat() tomcatServer.setPort(PORT) diff --git a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java index 36047957ac3..70ee48336fe 100644 --- a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java +++ b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java @@ -4,8 +4,6 @@ import ch.qos.logback.classic.Logger; import datadog.opentracing.DDSpan; import datadog.opentracing.DDTracer; -import datadog.opentracing.decorators.AbstractDecorator; -import datadog.opentracing.decorators.DDDecoratorsFactory; import datadog.trace.agent.tooling.AgentInstaller; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.common.writer.ListWriter; @@ -78,10 +76,6 @@ public boolean add(final List trace) { }; TEST_TRACER = new DDTracer(TEST_WRITER); - final List decorators = DDDecoratorsFactory.createBuiltinDecorators(); - for (final AbstractDecorator decorator : decorators) { - ((DDTracer) TEST_TRACER).addDecorator(decorator); - } ByteBuddyAgent.install(); instrumentation = ByteBuddyAgent.getInstrumentation(); } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java index 935e9e6d26f..c3ffc1d54e6 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java @@ -92,13 +92,6 @@ public DDTracer(final Properties config) { Sampler.Builder.forConfig(config), DDTraceConfig.parseMap(config.getProperty(DDTraceConfig.SPAN_TAGS))); log.debug("Using config: {}", config); - - // Create decorators from resource files - final List decorators = DDDecoratorsFactory.createBuiltinDecorators(); - for (final AbstractDecorator decorator : decorators) { - log.debug("Loading decorator: {}", decorator.getClass().getSimpleName()); - addDecorator(decorator); - } } public DDTracer(final String serviceName, final Writer writer, final Sampler sampler) { @@ -126,6 +119,12 @@ public DDTracer( registerClassLoader(ClassLoader.getSystemClassLoader()); + final List decorators = DDDecoratorsFactory.createBuiltinDecorators(); + for (final AbstractDecorator decorator : decorators) { + log.debug("Loading decorator: {}", decorator.getClass().getSimpleName()); + addDecorator(decorator); + } + log.info("New instance: {}", this); }