From 1e3634348505c5b57558fb64b97dcee430c8fe75 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 26 Jul 2018 12:20:37 +1000 Subject: [PATCH] Null or empty tags should remove previously set values. Add context to servlet2 test. Clean up some declared tracers that mess up the classpath. --- .../src/test/groovy/JerseyTest.groovy | 8 +- .../src/test/groovy/JettyServlet2Test.groovy | 85 ++++++------------- .../src/test/groovy/JettyServlet3Test.groovy | 59 +++---------- .../src/test/groovy/TomcatServlet3Test.groovy | 26 +----- .../groovy/test/SpringBootBasedTest.groovy | 24 ++---- .../java/datadog/opentracing/DDTracer.java | 12 ++- .../opentracing/DDSpanBuilderTest.groovy | 28 ++++++ 7 files changed, 92 insertions(+), 150 deletions(-) diff --git a/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JerseyTest.groovy b/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JerseyTest.groovy index a2704e5947..cd48b3b5ae 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JerseyTest.groovy +++ b/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JerseyTest.groovy @@ -3,6 +3,8 @@ import io.dropwizard.testing.junit.ResourceTestRule import org.junit.ClassRule import spock.lang.Shared +import static datadog.trace.agent.test.TestUtils.runUnderTrace + class JerseyTest extends AgentTestRunner { @Shared @@ -12,9 +14,9 @@ class JerseyTest extends AgentTestRunner { def "test resource"() { setup: // start a trace because the test doesn't go through any servlet or other instrumentation. - def scope = getTestTracer().buildSpan("test.span").startActive(true) - def response = resources.client().resource("/test/hello/bob").post(String) - scope.close() + def response = runUnderTrace("test.span") { + resources.client().resource("/test/hello/bob").post(String) + } expect: response == "Hello bob!" diff --git a/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServlet2Test.groovy b/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServlet2Test.groovy index dfd7a55a94..018bc9b9db 100644 --- a/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServlet2Test.groovy +++ b/dd-java-agent/instrumentation/servlet-2/src/test/groovy/JettyServlet2Test.groovy @@ -1,11 +1,7 @@ -import datadog.opentracing.DDSpan -import datadog.opentracing.DDTracer import datadog.trace.agent.test.AgentTestRunner import datadog.trace.agent.test.OkHttpUtils import datadog.trace.agent.test.TestUtils import datadog.trace.api.DDSpanTypes -import datadog.trace.common.writer.ListWriter -import io.opentracing.util.GlobalTracer import okhttp3.Credentials import okhttp3.Interceptor import okhttp3.OkHttpClient @@ -21,45 +17,29 @@ import org.eclipse.jetty.security.authentication.BasicAuthenticator import org.eclipse.jetty.server.Server import org.eclipse.jetty.servlet.ServletContextHandler -import java.lang.reflect.Field -import java.util.concurrent.CountDownLatch -import java.util.concurrent.TimeUnit - import static datadog.trace.agent.test.ListWriterAssert.assertTraces class JettyServlet2Test extends AgentTestRunner { - // Jetty needs this to ensure consistent ordering for async. - CountDownLatch latch = new CountDownLatch(1) - - OkHttpClient client = OkHttpUtils.clientBuilder() - .addNetworkInterceptor(new Interceptor() { - @Override - Response intercept(Interceptor.Chain chain) throws IOException { - def response = chain.proceed(chain.request()) - latch.await(30, TimeUnit.SECONDS) // don't block forever or test never fails. - return response - } - }) + OkHttpClient client = OkHttpUtils.clientBuilder().addNetworkInterceptor(new Interceptor() { + @Override + Response intercept(Interceptor.Chain chain) throws IOException { + def response = chain.proceed(chain.request()) + TEST_WRITER.waitForTraces(1) + return response + } + }) .build() int port private Server jettyServer private ServletContextHandler servletContext - ListWriter writer = new ListWriter() { - @Override - void write(final List trace) { - add(trace) - JettyServlet2Test.this.latch.countDown() - } - } - DDTracer tracer = new DDTracer(writer) - def setup() { port = TestUtils.randomOpenPort() jettyServer = new Server(port) servletContext = new ServletContextHandler() + servletContext.contextPath = "/ctx" ConstraintSecurityHandler security = setupAuthentication(jettyServer) @@ -72,17 +52,6 @@ class JettyServlet2Test extends AgentTestRunner { System.out.println( "Jetty server: http://localhost:" + port + "/") - - try { - GlobalTracer.register(tracer) - } catch (final Exception e) { - // Force it anyway using reflection - final Field field = GlobalTracer.getDeclaredField("tracer") - field.setAccessible(true) - field.set(null, tracer) - } - writer.start() - assert GlobalTracer.isRegistered() } def cleanup() { @@ -93,7 +62,7 @@ class JettyServlet2Test extends AgentTestRunner { def "test #path servlet call"() { setup: def requestBuilder = new Request.Builder() - .url("http://localhost:$port/$path") + .url("http://localhost:$port/ctx/$path") .get() if (auth) { requestBuilder.header(HttpHeaders.AUTHORIZATION, Credentials.basic("user", "password")) @@ -103,22 +72,22 @@ class JettyServlet2Test extends AgentTestRunner { expect: response.body().string().trim() == expectedResponse - assertTraces(writer, 1) { + assertTraces(TEST_WRITER, 1) { trace(0, 1) { span(0) { - serviceName "unnamed-java-app" + serviceName "ctx" operationName "servlet.request" - resourceName "GET /$path" + resourceName "GET /ctx/$path" spanType DDSpanTypes.WEB_SERVLET errored false parent() tags { - "http.url" "http://localhost:$port/$path" + "http.url" "http://localhost:$port/ctx/$path" "http.method" "GET" "span.kind" "server" "component" "java-web-servlet" "span.type" DDSpanTypes.WEB_SERVLET - "servlet.context" "" + "servlet.context" "/ctx" if (auth) { "user.principal" "user" } @@ -137,7 +106,7 @@ class JettyServlet2Test extends AgentTestRunner { def "test #path error servlet call"() { setup: def request = new Request.Builder() - .url("http://localhost:$port/$path?error=true") + .url("http://localhost:$port/ctx/$path?error=true") .get() .build() def response = client.newCall(request).execute() @@ -145,22 +114,22 @@ class JettyServlet2Test extends AgentTestRunner { expect: response.body().string().trim() != expectedResponse - assertTraces(writer, 1) { + assertTraces(TEST_WRITER, 1) { trace(0, 1) { span(0) { - serviceName "unnamed-java-app" + serviceName "ctx" operationName "servlet.request" - resourceName "GET /$path" + resourceName "GET /ctx/$path" spanType DDSpanTypes.WEB_SERVLET errored true parent() tags { - "http.url" "http://localhost:$port/$path" + "http.url" "http://localhost:$port/ctx/$path" "http.method" "GET" "span.kind" "server" "component" "java-web-servlet" "span.type" DDSpanTypes.WEB_SERVLET - "servlet.context" "" + "servlet.context" "/ctx" errorTags(RuntimeException, "some $path error") defaultTags() } @@ -177,7 +146,7 @@ class JettyServlet2Test extends AgentTestRunner { // This doesn't actually detect the error because we can't get the status code via the old servlet API. setup: def request = new Request.Builder() - .url("http://localhost:$port/$path?non-throwing-error=true") + .url("http://localhost:$port/ctx/$path?non-throwing-error=true") .get() .build() def response = client.newCall(request).execute() @@ -185,22 +154,22 @@ class JettyServlet2Test extends AgentTestRunner { expect: response.body().string().trim() != expectedResponse - assertTraces(writer, 1) { + assertTraces(TEST_WRITER, 1) { trace(0, 1) { span(0) { - serviceName "unnamed-java-app" + serviceName "ctx" operationName "servlet.request" - resourceName "GET /$path" + resourceName "GET /ctx/$path" spanType DDSpanTypes.WEB_SERVLET errored false parent() tags { - "http.url" "http://localhost:$port/$path" + "http.url" "http://localhost:$port/ctx/$path" "http.method" "GET" "span.kind" "server" "component" "java-web-servlet" "span.type" DDSpanTypes.WEB_SERVLET - "servlet.context" "" + "servlet.context" "/ctx" defaultTags() } } diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServlet3Test.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServlet3Test.groovy index 29b43a7145..99a9a3d9f9 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/JettyServlet3Test.groovy @@ -1,11 +1,7 @@ -import datadog.opentracing.DDSpan -import datadog.opentracing.DDTracer import datadog.trace.agent.test.AgentTestRunner import datadog.trace.agent.test.OkHttpUtils import datadog.trace.agent.test.TestUtils import datadog.trace.api.DDSpanTypes -import datadog.trace.common.writer.ListWriter -import io.opentracing.util.GlobalTracer import okhttp3.Credentials import okhttp3.Interceptor import okhttp3.OkHttpClient @@ -21,41 +17,24 @@ import org.eclipse.jetty.server.Server import org.eclipse.jetty.servlet.ServletContextHandler import org.eclipse.jetty.util.security.Constraint -import java.lang.reflect.Field -import java.util.concurrent.CountDownLatch -import java.util.concurrent.TimeUnit - import static datadog.trace.agent.test.ListWriterAssert.assertTraces class JettyServlet3Test extends AgentTestRunner { - // Jetty needs this to ensure consistent ordering for async. - CountDownLatch latch = new CountDownLatch(1) - - OkHttpClient client = OkHttpUtils.clientBuilder() - .addNetworkInterceptor(new Interceptor() { - @Override - Response intercept(Interceptor.Chain chain) throws IOException { - def response = chain.proceed(chain.request()) - latch.await(30, TimeUnit.SECONDS) // don't block forever or test never fails. - return response - } - }) + OkHttpClient client = OkHttpUtils.clientBuilder().addNetworkInterceptor(new Interceptor() { + @Override + Response intercept(Interceptor.Chain chain) throws IOException { + def response = chain.proceed(chain.request()) + TEST_WRITER.waitForTraces(1) + return response + } + }) .build() int port private Server jettyServer private ServletContextHandler servletContext - ListWriter writer = new ListWriter() { - @Override - void write(final List trace) { - add(trace) - JettyServlet3Test.this.latch.countDown() - } - } - DDTracer tracer = new DDTracer(writer) - def setup() { port = TestUtils.randomOpenPort() jettyServer = new Server(port) @@ -74,17 +53,6 @@ class JettyServlet3Test extends AgentTestRunner { System.out.println( "Jetty server: http://localhost:" + port + "/") - - try { - GlobalTracer.register(tracer) - } catch (final Exception e) { - // Force it anyway using reflection - final Field field = GlobalTracer.getDeclaredField("tracer") - field.setAccessible(true) - field.set(null, tracer) - } - writer.start() - assert GlobalTracer.isRegistered() } def cleanup() { @@ -105,7 +73,7 @@ class JettyServlet3Test extends AgentTestRunner { expect: response.body().string().trim() == expectedResponse - assertTraces(writer, 1) { + assertTraces(TEST_WRITER, 1) { trace(0, 1) { span(0) { serviceName "unnamed-java-app" @@ -120,7 +88,6 @@ class JettyServlet3Test extends AgentTestRunner { "span.kind" "server" "component" "java-web-servlet" "span.type" DDSpanTypes.WEB_SERVLET - "servlet.context" "" "http.status_code" 200 if (auth) { "user.principal" "user" @@ -151,7 +118,7 @@ class JettyServlet3Test extends AgentTestRunner { } expect: - assertTraces(writer, numTraces) { + assertTraces(TEST_WRITER, numTraces) { for (int i = 0; i < numTraces; ++i) { trace(i, 1) { span(0) { @@ -175,7 +142,7 @@ class JettyServlet3Test extends AgentTestRunner { expect: response.body().string().trim() != expectedResponse - assertTraces(writer, 1) { + assertTraces(TEST_WRITER, 1) { trace(0, 1) { span(0) { serviceName "unnamed-java-app" @@ -190,7 +157,6 @@ class JettyServlet3Test extends AgentTestRunner { "span.kind" "server" "component" "java-web-servlet" "span.type" DDSpanTypes.WEB_SERVLET - "servlet.context" "" "http.status_code" 500 errorTags(RuntimeException, "some $path error") defaultTags() @@ -216,7 +182,7 @@ class JettyServlet3Test extends AgentTestRunner { expect: response.body().string().trim() != expectedResponse - assertTraces(writer, 1) { + assertTraces(TEST_WRITER, 1) { trace(0, 1) { span(0) { serviceName "unnamed-java-app" @@ -231,7 +197,6 @@ class JettyServlet3Test extends AgentTestRunner { "span.kind" "server" "component" "java-web-servlet" "span.type" DDSpanTypes.WEB_SERVLET - "servlet.context" "" "http.status_code" 500 "error" true defaultTags() diff --git a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServlet3Test.groovy b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServlet3Test.groovy index 9855f97192..c4c896ee47 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet-3/src/test/groovy/TomcatServlet3Test.groovy @@ -1,11 +1,8 @@ import com.google.common.io.Files -import datadog.opentracing.DDTracer import datadog.trace.agent.test.AgentTestRunner import datadog.trace.agent.test.OkHttpUtils import datadog.trace.agent.test.TestUtils import datadog.trace.api.DDSpanTypes -import datadog.trace.common.writer.ListWriter -import io.opentracing.util.GlobalTracer import okhttp3.OkHttpClient import okhttp3.Request import org.apache.catalina.Context @@ -13,8 +10,6 @@ import org.apache.catalina.startup.Tomcat import org.apache.tomcat.JarScanFilter import org.apache.tomcat.JarScanType -import java.lang.reflect.Field - import static datadog.trace.agent.test.ListWriterAssert.assertTraces class TomcatServlet3Test extends AgentTestRunner { @@ -25,9 +20,6 @@ class TomcatServlet3Test extends AgentTestRunner { Tomcat tomcatServer Context appContext - ListWriter writer = new ListWriter() - DDTracer tracer = new DDTracer(writer) - def setup() { port = TestUtils.randomOpenPort() tomcatServer = new Tomcat() @@ -59,18 +51,6 @@ class TomcatServlet3Test extends AgentTestRunner { tomcatServer.start() System.out.println( "Tomcat server: http://" + tomcatServer.getHost().getName() + ":" + port + "/") - - - try { - GlobalTracer.register(tracer) - } catch (final Exception e) { - // Force it anyway using reflection - final Field field = GlobalTracer.getDeclaredField("tracer") - field.setAccessible(true) - field.set(null, tracer) - } - writer.start() - assert GlobalTracer.isRegistered() } def cleanup() { @@ -89,7 +69,7 @@ class TomcatServlet3Test extends AgentTestRunner { expect: response.body().string().trim() == expectedResponse - assertTraces(writer, 1) { + assertTraces(TEST_WRITER, 1) { trace(0, 1) { span(0) { serviceName "my-context" @@ -129,7 +109,7 @@ class TomcatServlet3Test extends AgentTestRunner { expect: response.body().string().trim() != expectedResponse - assertTraces(writer, 1) { + assertTraces(TEST_WRITER, 1) { trace(0, 1) { span(0) { serviceName "my-context" @@ -170,7 +150,7 @@ class TomcatServlet3Test extends AgentTestRunner { expect: response.body().string().trim() != expectedResponse - assertTraces(writer, 1) { + assertTraces(TEST_WRITER, 1) { trace(0, 1) { span(0) { serviceName "my-context" 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 bf1132d667..1a3e424c88 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 @@ -49,11 +49,10 @@ class SpringBootBasedTest extends AgentTestRunner { span.context().tags["span.kind"] == "server" span.context().tags["span.type"] == "web" span.context().tags["component"] == "java-web-servlet" - span.context().tags["servlet.context"] == "" span.context().tags["http.status_code"] == 200 span.context().tags["thread.name"] != null span.context().tags["thread.id"] != null - span.context().tags.size() == 9 + span.context().tags.size() == 8 } def "generates 404 spans"() { @@ -79,11 +78,10 @@ class SpringBootBasedTest extends AgentTestRunner { span0.context().tags["span.kind"] == "server" span0.context().tags["span.type"] == "web" span0.context().tags["component"] == "java-web-servlet" - span0.context().tags["servlet.context"] == "" span0.context().tags["http.status_code"] == 404 span0.context().tags["thread.name"] != null span0.context().tags["thread.id"] != null - span0.context().tags.size() == 9 + span0.context().tags.size() == 8 and: // trace 1 def trace1 = TEST_WRITER.get(1) @@ -100,11 +98,10 @@ class SpringBootBasedTest extends AgentTestRunner { span1.context().tags["span.kind"] == "server" span1.context().tags["span.type"] == "web" span1.context().tags["component"] == "java-web-servlet" - span1.context().tags["servlet.context"] == "" span1.context().tags["http.status_code"] == 404 span1.context().tags["thread.name"] != null span1.context().tags["thread.id"] != null - span1.context().tags.size() == 9 + span1.context().tags.size() == 8 } def "generates error spans"() { @@ -132,7 +129,6 @@ class SpringBootBasedTest extends AgentTestRunner { span0.context().tags["span.kind"] == "server" span0.context().tags["span.type"] == "web" span0.context().tags["component"] == "java-web-servlet" - span0.context().tags["servlet.context"] == "" span0.context().tags["http.status_code"] == 500 span0.context().tags["thread.name"] != null span0.context().tags["thread.id"] != null @@ -140,7 +136,7 @@ class SpringBootBasedTest extends AgentTestRunner { span0.context().tags["error.msg"] == "Request processing failed; nested exception is java.lang.RuntimeException: qwerty" span0.context().tags["error.type"] == NestedServletException.getName() span0.context().tags["error.stack"] != null - span0.context().tags.size() == 13 + span0.context().tags.size() == 12 and: // trace 1 def trace1 = TEST_WRITER.get(1) @@ -156,12 +152,11 @@ class SpringBootBasedTest extends AgentTestRunner { span1.context().tags["span.kind"] == "server" span1.context().tags["span.type"] == "web" span1.context().tags["component"] == "java-web-servlet" - span1.context().tags["servlet.context"] == "" 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() == 10 + span1.context().tags.size() == 9 } def "validated form"() { @@ -184,11 +179,10 @@ class SpringBootBasedTest extends AgentTestRunner { span.context().tags["span.kind"] == "server" span.context().tags["span.type"] == "web" span.context().tags["component"] == "java-web-servlet" - span.context().tags["servlet.context"] == "" span.context().tags["http.status_code"] == 200 span.context().tags["thread.name"] != null span.context().tags["thread.id"] != null - span.context().tags.size() == 9 + span.context().tags.size() == 8 } def "invalid form"() { @@ -216,7 +210,6 @@ class SpringBootBasedTest extends AgentTestRunner { span0.context().tags["span.kind"] == "server" span0.context().tags["span.type"] == "web" span0.context().tags["component"] == "java-web-servlet" - span0.context().tags["servlet.context"] == "" span0.context().tags["http.status_code"] == 400 span0.context().tags["thread.name"] != null span0.context().tags["thread.id"] != null @@ -224,7 +217,7 @@ class SpringBootBasedTest extends AgentTestRunner { span0.context().tags["error.msg"].toString().startsWith("Validation failed") span0.context().tags["error.type"] == MethodArgumentNotValidException.getName() span0.context().tags["error.stack"] != null - span0.context().tags.size() == 13 + span0.context().tags.size() == 12 and: // trace 1 def trace1 = TEST_WRITER.get(1) @@ -241,10 +234,9 @@ class SpringBootBasedTest extends AgentTestRunner { span1.context().tags["span.kind"] == "server" span1.context().tags["span.type"] == "web" span1.context().tags["component"] == "java-web-servlet" - span1.context().tags["servlet.context"] == "" span1.context().tags["http.status_code"] == 400 span1.context().tags["thread.name"] != null span1.context().tags["thread.id"] != null - span1.context().tags.size() == 9 + span1.context().tags.size() == 8 } } 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 26af9f2629..1cd4d19e7e 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java @@ -449,10 +449,16 @@ public class DDTracer implements io.opentracing.Tracer { // Private methods private DDSpanBuilder withTag(final String tag, final Object value) { - if (this.tags.isEmpty()) { - this.tags = new HashMap<>(); + if (value == null || (value instanceof String && ((String) value).isEmpty())) { + if (this.tags.containsKey(tag)) { + this.tags.remove(tag); + } + } else { + if (this.tags.isEmpty()) { + this.tags = new HashMap<>(); + } + this.tags.put(tag, value); } - this.tags.put(tag, value); return this; } diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy index c507671dcc..f8a1fa7554 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy @@ -82,6 +82,34 @@ class DDSpanBuilderTest extends Specification { context.tags[DDTags.THREAD_ID] == Thread.currentThread().getId() } + def "setting #name should remove"() { + setup: + final DDSpan span = tracer.buildSpan("op name") + .withTag(name, "tag value") + .withTag(name, value) + .start() + + expect: + span.tags[name] == null + + when: + span.setTag(name, "a tag") + + then: + span.tags[name] == "a tag" + + when: + span.setTag(name, (String) value) + + then: + span.tags[name] == null + + where: + name | value + "null.tag" | null + "empty.tag" | "" + } + def "should build span timestamp in nano"() { setup: // time in micro