Reuse TagMap.Entry objects in BaseDecorator (#10501)

Updating tests to account for API changes

Adding tagIterator and valueIterator

Adding checks for EntryIterator

Removing EntryIterator and EntryChangeIterator

EntryIterator and EntryChangeIterator are arguably redundant

spotless

Merge branch 'master' into dougqh/tagmap-entryreader

Refining handling of primitive types

Fixed bug TagValueConversions.toBoolean
Could cause LegacyTagMap.EntryReader to produce incorrect answers to some queries

For simplicity, now treating Byte and Short as Integer.  That will make calling code doing primitive handling simpler.

Fleshing out tests -- more tests to come

Refining comment in toBoolean

Adding more TagValueConversionTest-s

Coverage for byte, short, float, and double

Adding Entry tests for byte and short boxes

spotless

Merge branch 'master' into dougqh/tagmap-entryreader

Merge branch 'master' into dougqh/tagmap-entryreader

Direct TagMap.Entry support in AgentSpan / DDSpan

Adding methods to AgentSpan / DDSpan that take TagMap.Entry/Reader objects directly

This will enable TagMap.Entry reuse which can reduce memory allocation/GC pressure

Adding public create methods to TagMap.Entry

Methods are intended to be used to create TagMap.Entry objects for repeatedly used values

Overloads are provided for all the supported types to be easier for developers not familiar with TagMap internals.  Internally, TagMap still uses the more explicit new<X>Entry methods.

spotless

Merge branch 'master' into dougqh/fdirect-apis-for-tagmap-entry

Fixed merge

Removing statics that were previously moved to TagValueConversions

Clarifying comments

Adding tests for TagMap.Entry.create

- tests exposed missing TagMap.Entry.create for boolean
- added explanatory strings to some asserts

spotless

Comments

Merge branch 'master' into dougqh/fdirect-apis-for-tagmap-entry

Adding test for TagMap.Entry setters

Merge branch 'dougqh/fdirect-apis-for-tagmap-entry' of github.com:DataDog/dd-trace-java into dougqh/fdirect-apis-for-tagmap-entry

Adding missing import

spotless

Groovy codeNarc

Merge branch 'master' into dougqh/fdirect-apis-for-tagmap-entry

Reusing entries for analytics sample rate & component

spotless

For clarity, adding an explicit initial set to null

Reduced visibility of cachedComponentEntry

Doesn't need to be visible to children
If any child needs the componentEntry, they can call the method instead

Merge branch 'master' into dougqh/basedecorator-entry-reuse

Taking advantage of null handling in setMetric to simplify code

Merge branch 'dougqh/basedecorator-entry-reuse' of github.com:DataDog/dd-trace-java into dougqh/basedecorator-entry-reuse

Clarifying comment & spotless

Simplifying approach - just use final field in constructor

Merge branch 'master' into dougqh/basedecorator-entry-reuse

Merge branch 'master' into dougqh/basedecorator-entry-reuse

spotless

Merge branch 'dougqh/basedecorator-entry-reuse' of github.com:DataDog/dd-trace-java into dougqh/basedecorator-entry-reuse

Merge branch 'master' into dougqh/basedecorator-entry-reuse

Fixing tests

Code is now calling setTag(TagMap.Entry) or setMetric(TagMap.Entry)

Since these tests use mocks rather than checking span contents, I had to update them to account for the change

Merge branch 'dougqh/basedecorator-entry-reuse' of github.com:DataDog/dd-trace-java into dougqh/basedecorator-entry-reuse

Fixing ordering issue introduced by moving setting into constructor

spotless

Reverting to the lazy caching approach

Adding comment to explain the approach taken

Reverting field order

This file no longer needs to change with the caching approach, and this serves as a useful test that the behavior hasn't been altered

Merge branch 'master' into dougqh/basedecorator-entry-reuse

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

Author: dougqh

dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/BaseDecorator.java

@@ -7,6 +7,7 @@
 import datadog.trace.api.Config;
 import datadog.trace.api.DDTags;
 import datadog.trace.api.Functions;
+import datadog.trace.api.TagMap;
 import datadog.trace.api.cache.QualifiedClassNameCache;
 import datadog.trace.bootstrap.instrumentation.api.AgentScope;
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
@@ -40,17 +41,29 @@ public String apply(Class<?> clazz) {
           Functions.PrefixJoin.of("."));
 
   protected final boolean traceAnalyticsEnabled;
-  protected final Double traceAnalyticsSampleRate;
+  protected final double traceAnalyticsSampleRate;
+
+  private final TagMap.Entry traceAnalyticsEntry;
+
+  // Deliberately not volatile, reading null and repeating the calculation is safe
+  private TagMap.Entry cachedComponentEntry = null;
 
   protected BaseDecorator() {
     final Config config = Config.get();
     final String[] instrumentationNames = instrumentationNames();
+
     this.traceAnalyticsEnabled =
         instrumentationNames.length > 0
             && config.isTraceAnalyticsIntegrationEnabled(
                 traceAnalyticsDefault(), instrumentationNames);
+
     this.traceAnalyticsSampleRate =
         (double) config.getInstrumentationAnalyticsSampleRate(instrumentationNames);
+
+    this.traceAnalyticsEntry =
+        this.traceAnalyticsEnabled
+            ? TagMap.Entry.create(DDTags.ANALYTICS_SAMPLE_RATE, traceAnalyticsSampleRate)
+            : null;
   }
 
   protected abstract String[] instrumentationNames();
@@ -59,6 +72,20 @@ protected BaseDecorator() {
 
   protected abstract CharSequence component();
 
+  /** Caches the component TagMap.Entry, so it isn't recreated for every trace */
+  protected final TagMap.Entry componentEntry() {
+    // DQH = Tried calling component() in the constructor, but that had issues with static
+    // field ordering.  That was caught be an integration test, but I didn't want to risk
+    // breaking other integrations where the test is not as thorough.
+
+    // This approach while more complicated doesn't have any field initialization ordering issues.
+    TagMap.Entry componentEntry = cachedComponentEntry;
+    if (componentEntry == null) {
+      cachedComponentEntry = componentEntry = TagMap.Entry.create(Tags.COMPONENT, component());
+    }
+    return componentEntry;
+  }
+
   protected boolean traceAnalyticsDefault() {
     return false;
   }
@@ -67,12 +94,17 @@ public AgentSpan afterStart(final AgentSpan span) {
     if (spanType() != null) {
       span.setSpanType(spanType());
     }
+
+    span.setTag(componentEntry());
+
+    // DQH - Could retrieve the value from componentEntry and cast to avoid the virtual call,
+    // unclear which option is better here
     final CharSequence component = component();
-    span.setTag(Tags.COMPONENT, component);
     span.context().setIntegrationName(component);
-    if (traceAnalyticsEnabled) {
-      span.setMetric(DDTags.ANALYTICS_SAMPLE_RATE, traceAnalyticsSampleRate);
-    }
+
+    // null handled by setMetric
+    span.setMetric(traceAnalyticsEntry);
+
     return span;
   }
 

dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/BaseDecoratorTest.groovy

@@ -1,6 +1,6 @@
 package datadog.trace.bootstrap.instrumentation.decorator
 
-
+import datadog.trace.api.TagMap
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan
 import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext
 import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities
@@ -25,11 +25,13 @@ class BaseDecoratorTest extends DDSpecification {
 
     then:
     1 * span.setSpanType(decorator.spanType())
-    1 * span.setTag(Tags.COMPONENT, "test-component")
+    1 * span.setTag(TagMap.Entry.create(Tags.COMPONENT, "test-component"))
     1 * span.context() >> spanContext
     1 * spanContext.setIntegrationName("test-component")
+    _ * span.setTag(_)
     _ * span.setTag(_, _) // Want to allow other calls from child implementations.
     _ * span.setMeasured(true)
+    _ * span.setMetric(_)
     _ * span.setMetric(_, _)
     _ * span.setServiceName(_, _)
     _ * span.setOperationName(_)

dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/ClientDecoratorTest.groovy

@@ -1,6 +1,7 @@
 package datadog.trace.bootstrap.instrumentation.decorator
 
 import datadog.trace.api.DDTags
+import datadog.trace.api.TagMap
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan
 import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext
 import datadog.trace.bootstrap.instrumentation.api.Tags
@@ -22,12 +23,13 @@ class ClientDecoratorTest extends BaseDecoratorTest {
       1 * span.setServiceName(serviceName, "test-component")
     }
     1 * span.setMeasured(true)
-    1 * span.setTag(Tags.COMPONENT, "test-component")
+    1 * span.setTag(TagMap.Entry.create(Tags.COMPONENT, "test-component"))
     1 * span.context() >> spanContext
     1 * spanContext.setIntegrationName("test-component")
     1 * span.setTag(Tags.SPAN_KIND, "client")
     1 * span.setSpanType(decorator.spanType())
-    1 * span.setMetric(DDTags.ANALYTICS_SAMPLE_RATE, 1.0)
+    1 * span.setMetric(TagMap.Entry.create(DDTags.ANALYTICS_SAMPLE_RATE, 1.0))
+    _ * span.setTag(_)
     _ * span.setTag(_, _) // Want to allow other calls from child implementations.
     _ * span.setServiceName(_)
     _ * span.setOperationName(_)

dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/DatabaseClientDecoratorTest.groovy

@@ -1,6 +1,7 @@
 package datadog.trace.bootstrap.instrumentation.decorator
 
 import datadog.trace.api.DDTags
+import datadog.trace.api.TagMap
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan
 import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext
 import datadog.trace.bootstrap.instrumentation.api.Tags
@@ -26,12 +27,12 @@ class DatabaseClientDecoratorTest extends ClientDecoratorTest {
       1 * span.setServiceName(serviceName, "test-component")
     }
     1 * span.setMeasured(true)
-    1 * span.setTag(Tags.COMPONENT, "test-component")
+    1 * span.setTag(TagMap.Entry.create(Tags.COMPONENT, "test-component"))
     1 * span.context() >> spanContext
     1 * spanContext.setIntegrationName("test-component")
     1 * span.setTag(Tags.SPAN_KIND, "client")
     1 * span.setSpanType("test-type")
-    1 * span.setMetric(DDTags.ANALYTICS_SAMPLE_RATE, 1.0)
+    1 * span.setMetric(TagMap.Entry.create(DDTags.ANALYTICS_SAMPLE_RATE, 1.0))
     0 * _
 
     where:

dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/ServerDecoratorTest.groovy

@@ -1,6 +1,6 @@
 package datadog.trace.bootstrap.instrumentation.decorator
 
-
+import datadog.trace.api.TagMap
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan
 import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext
 
@@ -23,13 +23,15 @@ class ServerDecoratorTest extends BaseDecoratorTest {
 
     then:
     1 * span.setTag(LANGUAGE_TAG_KEY, LANGUAGE_TAG_VALUE)
-    1 * span.setTag(COMPONENT, "test-component")
+    1 * span.setTag(TagMap.Entry.create(COMPONENT, "test-component"))
     1 * span.context() >> spanContext
     1 * spanContext.setIntegrationName("test-component")
     1 * span.setTag(SPAN_KIND, "server")
     1 * span.setSpanType(decorator.spanType())
     if (decorator.traceAnalyticsEnabled) {
-      1 * span.setMetric(ANALYTICS_SAMPLE_RATE, 1.0)
+      1 * span.setMetric(TagMap.Entry.create(ANALYTICS_SAMPLE_RATE, 1.0))
+    } else {
+      1 * span.setMetric(null)
     }
     0 * _
   }

internal-api/src/test/java/datadog/trace/api/TagMapEntryTest.java

@@ -155,7 +155,7 @@ public void createBoolean(boolean value) {
   @ValueSource(booleans = {false, true})
   public void newBooleanEntry(boolean value) {
     test(
-        () -> TagMap.Entry.newBooleanEntry("foo", value),
+        () -> TagMap.Entry.create("foo", value),
         TagMap.Entry.BOOLEAN,
         (entry) ->
             multiCheck(
@@ -498,6 +498,21 @@ public void createDouble(double value) {
 
   @ParameterizedTest
   @DisplayName("newDoubleEntry: double")
+  @ValueSource(
+      doubles = {Double.MIN_VALUE, Float.MIN_VALUE, -1D, 0D, 1D, Math.E, Math.PI, Double.MAX_VALUE})
+  public void doubleEntry_via_create(double value) {
+    test(
+        () -> TagMap.Entry.create("foo", value),
+        TagMap.Entry.DOUBLE,
+        (entry) ->
+            multiCheck(
+                checkKey("foo", entry),
+                checkValue(value, entry),
+                checkIsNumericPrimitive(entry),
+                checkType(TagMap.Entry.DOUBLE, entry)));
+  }
+
+  @ParameterizedTest
   @ValueSource(
       doubles = {Double.MIN_VALUE, Float.MIN_VALUE, -1D, 0D, 1D, Math.E, Math.PI, Double.MAX_VALUE})
   public void doubleEntry(double value) {