Don't populate a map to populate the tags map

Author: bantonsson

dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java

@@ -455,7 +455,7 @@ public class CoreSpanBuilder implements AgentTracer.SpanBuilder {
     private final String operationName;
 
     // Builder attributes
-    private final Map<String, Object> tags = new LinkedHashMap<String, Object>(defaultSpanTags);
+    private Map<String, Object> tags;
     private long timestampMicro;
     private Object parent;
     private String serviceName;
@@ -542,10 +542,14 @@ public CoreSpanBuilder asChildOf(final AgentSpan agentSpan) {
 
     @Override
     public CoreSpanBuilder withTag(final String tag, final Object value) {
+      Map<String, Object> tagMap = this.tags;
+      if (tagMap == null) {
+        tags = tagMap = new LinkedHashMap<>(); // Insertion order is important
+      }
       if (value == null || (value instanceof String && ((String) value).isEmpty())) {
-        tags.remove(tag);
+        tagMap.remove(tag);
       } else {
-        tags.put(tag, value);
+        tagMap.put(tag, value);
       }
       return this;
     }
@@ -564,6 +568,8 @@ private DDSpanContext buildSpanContext() {
       final PendingTrace parentTrace;
       final int samplingPriority;
       final String origin;
+      final Map<String, String> coreTags;
+      final Map<String, String> rootSpanTags;
 
       final DDSpanContext context;
 
@@ -589,6 +595,8 @@ private DDSpanContext buildSpanContext() {
         parentTrace = ddsc.getTrace();
         samplingPriority = PrioritySampling.UNSET;
         origin = null;
+        coreTags = null;
+        rootSpanTags = null;
         if (serviceName == null) {
           serviceName = ddsc.getServiceName();
         }
@@ -611,13 +619,14 @@ private DDSpanContext buildSpanContext() {
 
         // Get header tags and set origin whether propagating or not.
         if (parentContext instanceof TagContext) {
-          tags.putAll(((TagContext) parentContext).getTags());
+          coreTags = ((TagContext) parentContext).getTags();
           origin = ((TagContext) parentContext).getOrigin();
         } else {
+          coreTags = null;
           origin = null;
         }
 
-        tags.putAll(localRootSpanTags);
+        rootSpanTags = localRootSpanTags;
 
         parentTrace = PendingTrace.create(CoreTracer.this, traceId);
       }
@@ -628,6 +637,11 @@ private DDSpanContext buildSpanContext() {
 
       final String operationName = this.operationName != null ? this.operationName : resourceName;
 
+      int tagsSize =
+          (null == tags ? 0 : tags.size())
+              + defaultSpanTags.size()
+              + (null == coreTags ? 0 : coreTags.size())
+              + (null == rootSpanTags ? 0 : rootSpanTags.size());
       // some attributes are inherited from the parent
       context =
           new DDSpanContext(
@@ -642,40 +656,18 @@ private DDSpanContext buildSpanContext() {
               baggage,
               errorFlag,
               spanType,
-              tags,
+              tagsSize,
               parentTrace,
               CoreTracer.this,
               serviceNameMappings);
 
-      // Apply Decorators to handle any tags that may have been set via the builder.
-      for (final Map.Entry<String, Object> tag : tags.entrySet()) {
-        if (tag.getValue() == null) {
-          context.setTag(tag.getKey(), null);
-          continue;
-        }
-
-        boolean addTag = true;
-
-        // Call interceptors
-        final List<AbstractTagInterceptor> interceptors = getSpanTagInterceptors(tag.getKey());
-        if (interceptors != null) {
-          for (final AbstractTagInterceptor interceptor : interceptors) {
-            try {
-              addTag &= interceptor.shouldSetTag(context, tag.getKey(), tag.getValue());
-            } catch (final Throwable ex) {
-              log.debug(
-                  "Could not intercept the span interceptor={}: {}",
-                  interceptor.getClass().getSimpleName(),
-                  ex.getMessage());
-            }
-          }
-        }
-
-        if (!addTag) {
-          context.setTag(tag.getKey(), null);
-        }
-      }
-
+      // By setting the tags on the context we apply decorators to any tags that have been set via
+      // the builder. This is the order that the tags were added previously, but maybe the `tags`
+      // set in the builder should come last, so that they override other tags.
+      context.setAllTags(defaultSpanTags);
+      context.setAllTags(tags);
+      context.setAllTags(coreTags);
+      context.setAllTags(rootSpanTags);
       return context;
     }
   }

dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java

@@ -233,12 +233,12 @@ public AgentSpan removeTag(final String tag) {
   }
 
   public Object getAndRemoveTag(final String tag) {
-    return context.getTags().remove(tag);
+    return context.getAndRemoveTag(tag);
   }
 
   @Override
   public Object getTag(final String tag) {
-    return context.getTags().get(tag);
+    return context.getTag(tag);
   }
 
   @Override

dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java

@@ -45,7 +45,7 @@ public class DDSpanContext implements AgentSpan.Context {
   private final DDId parentId;
 
   /** Tags are associated to the current span, they will not propagate to the children span */
-  private final Map<String, Object> tags = new ConcurrentHashMap<>();
+  private final Map<String, Object> tags;
 
   /** The service name is required, otherwise the span are dropped by the agent */
   private volatile String serviceName;
@@ -69,10 +69,6 @@ public class DDSpanContext implements AgentSpan.Context {
   /** Metrics on the span */
   private final AtomicReference<Map<String, Number>> metrics = new AtomicReference<>();
 
-  // Additional Metadata
-  private final String threadName = Thread.currentThread().getName();
-  private final long threadId = Thread.currentThread().getId();
-
   private final Map<String, String> serviceNameMappings;
 
   public DDSpanContext(
@@ -87,7 +83,7 @@ public DDSpanContext(
       final Map<String, String> baggageItems,
       final boolean errorFlag,
       final String spanType,
-      final Map<String, Object> tags,
+      final int tagsSize,
       final PendingTrace trace,
       final CoreTracer tracer,
       final Map<String, String> serviceNameMappings) {
@@ -110,9 +106,10 @@ public DDSpanContext(
       this.baggageItems = new ConcurrentHashMap<>(baggageItems);
     }
 
-    if (tags != null) {
-      this.tags.putAll(tags);
-    }
+    // Three is the magic number from the tags below that we set at the end,
+    // and "* 4 / 3" is to make sure that we don't resize immediately
+    int capacity = ((tagsSize <= 0 ? 3 : tagsSize + 3) * 4) / 3;
+    this.tags = new ConcurrentHashMap<>(capacity);
 
     this.serviceNameMappings = serviceNameMappings;
     setServiceName(serviceName);
@@ -129,8 +126,10 @@ public DDSpanContext(
     if (origin != null) {
       this.tags.put(ORIGIN_KEY, origin);
     }
-    this.tags.put(DDTags.THREAD_NAME, threadName);
-    this.tags.put(DDTags.THREAD_ID, threadId);
+    // Additional Metadata
+    Thread current = Thread.currentThread();
+    this.tags.put(DDTags.THREAD_NAME, current.getName());
+    this.tags.put(DDTags.THREAD_ID, current.getId());
   }
 
   @Override
@@ -322,7 +321,25 @@ public void setMetric(final String key, final Number value) {
    * @param tag the tag-name
    * @param value the value of the tag. tags with null values are ignored.
    */
-  public synchronized void setTag(final String tag, final Object value) {
+  public void setTag(final String tag, final Object value) {
+    synchronized (tags) {
+      internalSetTag(tag, value);
+    }
+  }
+
+  void setAllTags(final Map<String, ? extends Object> map) {
+    if (map == null) {
+      return;
+    }
+
+    synchronized (tags) {
+      for (final Map.Entry<String, ? extends Object> tag : map.entrySet()) {
+        internalSetTag(tag.getKey(), tag.getValue());
+      }
+    }
+  }
+
+  private void internalSetTag(final String tag, final Object value) {
     if (value == null || (value instanceof String && ((String) value).isEmpty())) {
       tags.remove(tag);
       return;
@@ -350,6 +367,14 @@ public synchronized void setTag(final String tag, final Object value) {
     }
   }
 
+  Object getTag(final String key) {
+    return tags.get(key);
+  }
+
+  Object getAndRemoveTag(final String tag) {
+    return tags.remove(tag);
+  }
+
   public Map<String, Object> getTags() {
     return tags;
   }

dd-trace-core/src/test/groovy/datadog/trace/api/writer/DDAgentWriterTest.groovy

@@ -203,7 +203,7 @@ class DDAgentWriterTest extends DDSpecification {
       [:],
       false,
       "",
-      [:],
+      0,
       Mock(PendingTrace),
       Mock(CoreTracer),
       [:])
@@ -283,7 +283,7 @@ class DDAgentWriterTest extends DDSpecification {
       [:],
       false,
       "",
-      [:],
+      0,
       Mock(PendingTrace),
       Mock(CoreTracer),
       [:])

dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanSerializationTest.groovy

@@ -61,10 +61,11 @@ class DDSpanSerializationTest extends DDSpecification {
         ["a-baggage": "value"],
         false,
         spanType,
-        ["k1": "v1"],
+        1,
         PendingTrace.create(tracer, DDId.ONE),
         tracer,
         [:])
+    context.setAllTags(["k1": "v1"])
 
     DDSpan span = DDSpan.create(100L, context)
 
@@ -97,7 +98,7 @@ class DDSpanSerializationTest extends DDSpecification {
       Collections.emptyMap(),
       false,
       spanType,
-      Collections.emptyMap(),
+      0,
       PendingTrace.create(tracer, DDId.ONE),
       tracer,
       [:])

dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanTest.groovy

@@ -34,7 +34,7 @@ class DDSpanTest extends DDSpecification {
         Collections.<String, String> emptyMap(),
         false,
         "fakeType",
-        null,
+        0,
         PendingTrace.create(tracer, DDId.ONE),
         tracer,
         [:])

dd-trace-core/src/test/groovy/datadog/trace/core/SpanFactory.groovy

@@ -23,7 +23,7 @@ class SpanFactory {
       Collections.emptyMap(),
       false,
       "fakeType",
-      Collections.emptyMap(),
+      0,
       PendingTrace.create(tracer, DDId.ONE),
       tracer, [:])
     Thread.currentThread().setName(currentThreadName)
@@ -43,7 +43,7 @@ class SpanFactory {
       Collections.emptyMap(),
       false,
       "fakeType",
-      Collections.emptyMap(),
+      0,
       PendingTrace.create(tracer, DDId.ONE),
       tracer, [:])
     return DDSpan.create(1, context)
@@ -62,7 +62,7 @@ class SpanFactory {
       Collections.emptyMap(),
       false,
       "fakeType",
-      Collections.emptyMap(),
+      0,
       trace,
       trace.tracer, [:])
     return DDSpan.create(1, context)
@@ -83,7 +83,7 @@ class SpanFactory {
       Collections.emptyMap(),
       false,
       "fakeType",
-      Collections.emptyMap(),
+      0,
       PendingTrace.create(tracer, DDId.ONE),
       tracer,
       [:])

dd-trace-core/src/test/groovy/datadog/trace/core/propagation/B3HttpInjectorTest.groovy

@@ -39,7 +39,7 @@ class B3HttpInjectorTest extends DDSpecification {
         },
         false,
         "fakeType",
-        null,
+        0,
         new PendingTrace(tracer, DDId.ONE),
         tracer,
         [:])

dd-trace-core/src/test/groovy/datadog/trace/core/propagation/DatadogHttpInjectorTest.groovy

@@ -41,7 +41,7 @@ class DatadogHttpInjectorTest extends DDSpecification {
         },
         false,
         "fakeType",
-        null,
+        0,
         new PendingTrace(tracer, DDId.ONE),
         tracer,
         [:])

dd-trace-core/src/test/groovy/datadog/trace/core/propagation/HaystackHttpInjectorTest.groovy

@@ -39,7 +39,7 @@ class HaystackHttpInjectorTest extends DDSpecification {
         },
         false,
         "fakeType",
-        null,
+        0,
         new PendingTrace(tracer, DDId.ONE),
         tracer,
         [:])