Avoid creation of empty CopyOnWriteArrayList for span links (#10822)

Avoid creation of empty CopyOnWriteArrayList for span links

This change shares an "EMPTY" list between span when there are no span links.

This avoids the construction of a CopyOnWriteArrayList, empty Object[], and internal lock Object for the common case where there are no links.

This elimination of the CopyOnWriteArrayList in the empty case, reduced allocation by 5% and GC time by 7% in a span creation stress test.

Merge branch 'master' into dougqh/span-links-allocation

Introducing WritableSpanLinks & SpanLinkAccessor

Introduced the new interface WritableSpanLinks & SpanLinkAccessor that are used to provide limited access to DDSpan

Updated TagProcessors to use the new interfaces

Updated tests as needed

Merge branch 'dougqh/span-links-allocation' of github.com:DataDog/dd-trace-java into dougqh/span-links-allocation

spotless

Addressing review comments

- removing implied visibility from new interfaces
- added some javadoc to new interfaces
- renamed WritableSpanLinks -> AppendableSpanLinks

Remove vestigial processTags(Map, DDSpanContext, List) method from TagsPostProcessor

Removed vestigial processTags(Map, DDSpanContext, List) method from TagsPostProcessor

This method was a hold over from before TagMap was introduced.  It was kept to make porting tests easier while TagMap was still in experimental phase.

The method has outlived its usefulness; however, removing the method did require updating a lot of tests.

spotless

codeNarcTest - fixing typo

Merge branch 'master' into dougqh/span-links-allocation

Restricting API access

Removed SpanLinkAccessor interface
SpanLinkAccessor was intended to limit how the span is modified in DDSpanContext#processTagsAndBaggage, but the trade-off was introducing a public getLinks method on DDSpan.

Reduced visibility of processTagsAndBaggage methods on DDSpanContext
Since processTagsAndBaggage is really just a helper routine of DDSpan.processTagsAndBaggage and the usage is a bit sensitive, I decided to reduce the visibility

spotless

Merge branch 'master' into dougqh/span-links-allocation

Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>

Author: dougqh

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

@@ -116,7 +116,8 @@ static DDSpan create(
    */
   private volatile int longRunningVersion = 0;
 
-  protected final List<AgentSpanLink> links;
+  private static final List<AgentSpanLink> EMPTY = Collections.emptyList();
+  protected volatile List<AgentSpanLink> links;
 
   /**
    * Spans should be constructed using the builder, not by calling the constructor directly.
@@ -144,7 +145,7 @@ private DDSpan(
       context.getTraceCollector().touch(); // external clock: explicitly update lastReferenced
     }
 
-    this.links = links == null ? new CopyOnWriteArrayList<>() : new CopyOnWriteArrayList<>(links);
+    this.links = links == null || links.isEmpty() ? EMPTY : new CopyOnWriteArrayList<>(links);
   }
 
   public boolean isFinished() {
@@ -764,12 +765,12 @@ public CharSequence getType() {
 
   @Override
   public void processServiceTags() {
-    context.earlyProcessTags(links);
+    context.earlyProcessTags(this);
   }
 
   @Override
   public void processTagsAndBaggage(final MetadataConsumer consumer) {
-    context.processTagsAndBaggage(consumer, longRunningVersion, links);
+    context.processTagsAndBaggage(consumer, longRunningVersion, this);
   }
 
   @Override
@@ -883,10 +884,44 @@ public TraceConfig traceConfig() {
     return context.getTraceCollector().getTraceConfig();
   }
 
+  List<? extends AgentSpanLink> getLinks() {
+    return this.links;
+  }
+
   @Override
   public void addLink(AgentSpanLink link) {
-    if (link != null) {
-      this.links.add(link);
+    if (link == null) {
+      return;
+    }
+
+    // If links are initially null / empty, then the shared placeholder List EMPTY is used.
+    // Bacause EMPTY is shared, EMPTY is safe for reading, but not for writing.
+    // On write - if links is the EMPTY placeholder, then need to create a CopyOnWriteArrayList
+    // owned by this DDSpan
+
+    // Creation of the CopyOnWriteArrayList is done via double checking locking using volatile &
+    // synchronized
+
+    // If before or inside the synchronized block, links no longer points to EMPTY,
+    // then this thread or another thread has already handled the list construction,
+    // so just add to the list
+
+    // If links still points to EMPTY inside the synchronized block, then construct a new
+    // CopyOnWriteArrayList containing the newly added link
+
+    List<AgentSpanLink> links = this.links;
+    if (links != EMPTY) {
+      links.add(link);
+      return;
+    }
+
+    synchronized (this) {
+      links = this.links;
+      if (links != EMPTY) {
+        links.add(link);
+      } else {
+        this.links = new CopyOnWriteArrayList<>(Collections.singletonList(link));
+      }
     }
   }
 

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

@@ -24,7 +24,7 @@
 import datadog.trace.api.sampling.PrioritySampling;
 import datadog.trace.api.sampling.SamplingMechanism;
 import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext;
-import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink;
+import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks;
 import datadog.trace.bootstrap.instrumentation.api.Baggage;
 import datadog.trace.bootstrap.instrumentation.api.ProfilerContext;
 import datadog.trace.bootstrap.instrumentation.api.ProfilingContextIntegration;
@@ -1087,19 +1087,24 @@ public <T> void setMetaStruct(final String field, final T value) {
     }
   }
 
-  public void earlyProcessTags(List<AgentSpanLink> links) {
+  void earlyProcessTags(AppendableSpanLinks links) {
     synchronized (unsafeTags) {
       TagsPostProcessorFactory.eagerProcessor().processTags(unsafeTags, this, links);
     }
   }
 
-  public void processTagsAndBaggage(
-      final MetadataConsumer consumer, int longRunningVersion, List<AgentSpanLink> links) {
+  void processTagsAndBaggage(
+      final MetadataConsumer consumer, int longRunningVersion, DDSpan restrictedSpan) {
+    // NOTE: The span is passed for the sole purpose of allowing updating & reading of the span
+    // links
+    // This is a compromise to avoid...
+    // - creating an extra wrapper object that would create significant allocation
+    // - implementing an interface to read the spans that require making the read method public
     synchronized (unsafeTags) {
       // Tags
-      TagsPostProcessorFactory.lazyProcessor().processTags(unsafeTags, this, links);
+      TagsPostProcessorFactory.lazyProcessor().processTags(unsafeTags, this, restrictedSpan);
 
-      String linksTag = DDSpanLink.toTag(links);
+      String linksTag = DDSpanLink.toTag(restrictedSpan.getLinks());
       if (linksTag != null) {
         unsafeTags.put(SPAN_LINKS, linksTag);
       }

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

@@ -69,7 +69,7 @@ public static SpanLink from(ExtractedContext context, SpanAttributes attributes)
    * @param links The span link collection to encode.
    * @return The encoded tag value, {@code null} if no links.
    */
-  public static String toTag(List<AgentSpanLink> links) {
+  public static String toTag(List<? extends AgentSpanLink> links) {
     if (links == null || links.isEmpty()) {
       return null;
     }

dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/HttpEndpointPostProcessor.java

@@ -6,9 +6,8 @@
 
 import datadog.trace.api.TagMap;
 import datadog.trace.api.endpoint.EndpointResolver;
-import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink;
+import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks;
 import datadog.trace.core.DDSpanContext;
-import java.util.List;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -56,7 +55,7 @@ public HttpEndpointPostProcessor() {
 
   @Override
   public void processTags(
-      TagMap unsafeTags, DDSpanContext spanContext, List<AgentSpanLink> spanLinks) {
+      TagMap unsafeTags, DDSpanContext spanContext, AppendableSpanLinks spanLinks) {
     if (!endpointResolver.isEnabled()) {
       log.debug("EndpointResolver is not enabled, skipping HTTP endpoint post processing");
       return;

dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/IntegrationAdder.java

@@ -3,14 +3,13 @@
 import static datadog.trace.api.DDTags.DD_INTEGRATION;
 
 import datadog.trace.api.TagMap;
-import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink;
+import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks;
 import datadog.trace.core.DDSpanContext;
-import java.util.List;
 
 public class IntegrationAdder extends TagsPostProcessor {
   @Override
   public void processTags(
-      TagMap unsafeTags, DDSpanContext spanContext, List<AgentSpanLink> spanLinks) {
+      TagMap unsafeTags, DDSpanContext spanContext, AppendableSpanLinks spanLinks) {
     final CharSequence instrumentationName = spanContext.getIntegrationName();
     if (instrumentationName != null) {
       unsafeTags.set(DD_INTEGRATION, instrumentationName);

dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/InternalTagsAdder.java

@@ -4,10 +4,9 @@
 
 import datadog.trace.api.DDTags;
 import datadog.trace.api.TagMap;
-import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink;
+import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks;
 import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString;
 import datadog.trace.core.DDSpanContext;
-import java.util.List;
 import javax.annotation.Nullable;
 
 public final class InternalTagsAdder extends TagsPostProcessor {
@@ -21,7 +20,7 @@ public InternalTagsAdder(@Nullable final String ddService, @Nullable final Strin
 
   @Override
   public void processTags(
-      TagMap unsafeTags, DDSpanContext spanContext, List<AgentSpanLink> spanLinks) {
+      TagMap unsafeTags, DDSpanContext spanContext, AppendableSpanLinks spanLinks) {
     if (spanContext == null || ddService == null) {
       return;
     }

dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/PayloadTagsProcessor.java

@@ -7,7 +7,7 @@
 import datadog.trace.api.ConfigDefaults;
 import datadog.trace.api.TagMap;
 import datadog.trace.api.telemetry.LogCollector;
-import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink;
+import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks;
 import datadog.trace.core.DDSpanContext;
 import datadog.trace.core.util.JsonStreamParser;
 import datadog.trace.payloadtags.PayloadTagsData;
@@ -72,7 +72,7 @@ public static PayloadTagsProcessor create(Config config) {
 
   @Override
   public void processTags(
-      TagMap unsafeTags, DDSpanContext spanContext, List<AgentSpanLink> spanLinks) {
+      TagMap unsafeTags, DDSpanContext spanContext, AppendableSpanLinks spanLinks) {
     int spanMaxTags = maxTags + unsafeTags.size();
     for (Map.Entry<String, RedactionRules> tagPrefixRedactionRules :
         redactionRulesByTagPrefix.entrySet()) {

dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/PeerServiceCalculator.java

@@ -5,10 +5,9 @@
 import datadog.trace.api.TagMap;
 import datadog.trace.api.naming.NamingSchema;
 import datadog.trace.api.naming.SpanNaming;
-import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink;
+import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks;
 import datadog.trace.bootstrap.instrumentation.api.Tags;
 import datadog.trace.core.DDSpanContext;
-import java.util.List;
 import java.util.Map;
 import javax.annotation.Nonnull;
 
@@ -34,7 +33,7 @@ public PeerServiceCalculator() {
 
   @Override
   public void processTags(
-      TagMap unsafeTags, DDSpanContext spanContext, List<AgentSpanLink> spanLinks) {
+      TagMap unsafeTags, DDSpanContext spanContext, AppendableSpanLinks spanLinks) {
     Object peerService = unsafeTags.getObject(Tags.PEER_SERVICE);
     // the user set it
     if (peerService != null) {

dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/PostProcessorChain.java

@@ -1,9 +1,8 @@
 package datadog.trace.core.tagprocessor;
 
 import datadog.trace.api.TagMap;
-import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink;
+import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks;
 import datadog.trace.core.DDSpanContext;
-import java.util.List;
 import java.util.Objects;
 import javax.annotation.Nonnull;
 
@@ -16,7 +15,7 @@ public PostProcessorChain(@Nonnull final TagsPostProcessor... processors) {
 
   @Override
   public void processTags(
-      TagMap unsafeTags, DDSpanContext spanContext, List<AgentSpanLink> spanLinks) {
+      TagMap unsafeTags, DDSpanContext spanContext, AppendableSpanLinks spanLinks) {
     for (final TagsPostProcessor tagsPostProcessor : chain) {
       tagsPostProcessor.processTags(unsafeTags, spanContext, spanLinks);
     }

dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/QueryObfuscator.java

@@ -5,11 +5,10 @@
 import com.google.re2j.PatternSyntaxException;
 import datadog.trace.api.DDTags;
 import datadog.trace.api.TagMap;
-import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink;
+import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks;
 import datadog.trace.bootstrap.instrumentation.api.Tags;
 import datadog.trace.core.DDSpanContext;
 import datadog.trace.util.Strings;
-import java.util.List;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -59,7 +58,7 @@ private String obfuscate(String query) {
 
   @Override
   public void processTags(
-      TagMap unsafeTags, DDSpanContext spanContext, List<AgentSpanLink> spanLinks) {
+      TagMap unsafeTags, DDSpanContext spanContext, AppendableSpanLinks spanLinks) {
     Object query = unsafeTags.getObject(DDTags.HTTP_QUERY);
     if (query instanceof CharSequence) {
       query = obfuscate(query.toString());