Null or empty tags should remove previously set values.

Add context to servlet2 test.

Clean up some declared tracers that mess up the classpath.

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!"

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,23 +17,15 @@ 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() {
+  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.
+      TEST_WRITER.waitForTraces(1)
       return response
     }
   })
@@ -47,19 +35,11 @@ class JettyServlet2Test extends AgentTestRunner {
   private Server jettyServer
   private ServletContextHandler servletContext
 
-  ListWriter writer = new ListWriter() {
-    @Override
-    void write(final List<DDSpan> 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()
           }
         }

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,23 +17,15 @@ 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() {
+  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.
+      TEST_WRITER.waitForTraces(1)
       return response
     }
   })
@@ -47,15 +35,6 @@ class JettyServlet3Test extends AgentTestRunner {
   private Server jettyServer
   private ServletContextHandler servletContext
 
-  ListWriter writer = new ListWriter() {
-    @Override
-    void write(final List<DDSpan> 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()

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"

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
   }
 }

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 (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);
+      }
       return this;
     }
 

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