Reuse SpanKind Entry in ClientDecorator (#10503)

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

Caching & reusing spanKind entry to reduce allocation

Merge branch 'master' into dougqh/client-kind-entry-reuse

Test clean-up

Some of this was a bad merge, but I probably missed some in my other PR, too

Merge branch 'master' into dougqh/client-kind-entry-reuse

Clean-up oversights caught in review

Adding explanatory comment on reason behind the approach

Merge branch 'master' into dougqh/client-kind-entry-reuse

Fixing the tests

Merge branch 'master' into dougqh/client-kind-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/ClientDecorator.java

@@ -1,12 +1,32 @@
 package datadog.trace.bootstrap.instrumentation.decorator;
 
+import datadog.trace.api.TagMap;
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
 import datadog.trace.bootstrap.instrumentation.api.Tags;
 
 public abstract class ClientDecorator extends BaseDecorator {
+  // Deliberately not volatile, reading a stale null and creating an extra Entry is safe
+  private TagMap.Entry cachedSpanKindEntry = null;
 
   protected abstract String service();
 
+  /** Caches span kind entry to reduce allocation */
+  private final TagMap.Entry spanKindEntry() {
+    // DQH - I considered moving the creation of the TagMap.Entry into a ClientDecorator
+    // constructor, but that introduces a subtle ordering requirement.
+
+    // If the spanKind method refers to a static that isn't yet initialized,
+    // then spanKind will return null when the Decorator singleton is being constructed.
+
+    // Such an ordering problem did occur with similar changes in BaseDecorator, so I've
+    // decided to be cautious here, too.
+    TagMap.Entry kindEntry = cachedSpanKindEntry;
+    if (kindEntry == null) {
+      cachedSpanKindEntry = kindEntry = TagMap.Entry.create(Tags.SPAN_KIND, spanKind());
+    }
+    return kindEntry;
+  }
+
   protected String spanKind() {
     return Tags.SPAN_KIND_CLIENT;
   }
@@ -17,7 +37,7 @@ public AgentSpan afterStart(final AgentSpan span) {
     if (service != null) {
       span.setServiceName(service, component());
     }
-    span.setTag(Tags.SPAN_KIND, spanKind());
+    span.setTag(spanKindEntry());
 
     // Generate metrics for all client spans.
     span.setMeasured(true);

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

@@ -26,11 +26,12 @@ class ClientDecoratorTest extends BaseDecoratorTest {
     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.setTag(TagMap.Entry.create(Tags.SPAN_KIND, "client"))
     1 * span.setSpanType(decorator.spanType())
     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.setTag(_)
     _ * span.setServiceName(_)
     _ * span.setOperationName(_)
     0 * _

dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/DBTypeProcessingDatabaseClientDecoratorTest.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.Tags
 import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString
@@ -26,7 +27,7 @@ class DBTypeProcessingDatabaseClientDecoratorTest extends ClientDecoratorTest {
     1 * span.setTag(Tags.COMPONENT, "test-component")
     1 * span.context() >> spanContext
     1 * spanContext.setIntegrationName("test-component")
-    1 * span.setTag(Tags.SPAN_KIND, "client")
+    1 * span.setTag(TagMap.Entry.create(Tags.SPAN_KIND, "client"))
     1 * span.setSpanType("test-type")
     1 * span.setServiceName("test-db")
     1 * span.setOperationName(UTF8BytesString.create("test-db.query"))

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

@@ -30,7 +30,7 @@ class DatabaseClientDecoratorTest extends ClientDecoratorTest {
     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.setTag(TagMap.Entry.create(Tags.SPAN_KIND, "client"))
     1 * span.setSpanType("test-type")
     1 * span.setMetric(TagMap.Entry.create(DDTags.ANALYTICS_SAMPLE_RATE, 1.0))
     0 * _

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

@@ -104,7 +104,7 @@ public void createEmptyCharSequence() {
   }
 
   @Test
-  public void objectEntry() {
+  public void newObjectEntry() {
     test(
         () -> TagMap.Entry.newObjectEntry("foo", "bar"),
         TagMap.Entry.OBJECT,
@@ -119,7 +119,7 @@ public void objectEntry() {
   }
 
   @Test
-  @DisplayName("anyEntry: Object")
+  @DisplayName("newAnyEntry: Object")
   public void anyEntryObject() {
     test(
         () -> TagMap.Entry.newAnyEntry("foo", "bar"),
@@ -253,7 +253,7 @@ public void newIntEntryBoxed(int value) {
   @ParameterizedTest
   @DisplayName("newIntEntry: Short")
   @ValueSource(shorts = {Short.MIN_VALUE, -256, -128, -1, 0, 1, 128, 256, Short.MAX_VALUE})
-  public void intEntryBoxedShort(short value) {
+  public void newIntEntryBoxedShort(short value) {
     test(
         () -> TagMap.Entry.newIntEntry("foo", Short.valueOf(value)),
         TagMap.Entry.INT,
@@ -269,7 +269,7 @@ public void intEntryBoxedShort(short value) {
   @ParameterizedTest
   @DisplayName("newIntEntry: Byte")
   @ValueSource(bytes = {Byte.MIN_VALUE, -32, -1, 0, 1, 32, Byte.MAX_VALUE})
-  public void intEntryBoxedByte(byte value) {
+  public void newIntEntryBoxedByte(byte value) {
     test(
         () -> TagMap.Entry.newIntEntry("foo", Byte.valueOf(value)),
         TagMap.Entry.INT,
@@ -497,25 +497,9 @@ 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) {
+  public void newDoubleEntry(double value) {
     test(
         () -> TagMap.Entry.newDoubleEntry("foo", value),
         TagMap.Entry.DOUBLE,
@@ -547,7 +531,7 @@ public void newDoubleEntryBoxed(double value) {
   @DisplayName("newAnyEntry: Double")
   @ValueSource(
       doubles = {Double.MIN_VALUE, Float.MIN_VALUE, -1D, 0D, 1D, Math.E, Math.PI, Double.MAX_VALUE})
-  public void anyEntry_double(double value) {
+  public void newAnyEntryDouble(double value) {
     test(
         () -> TagMap.Entry.newAnyEntry("foo", Double.valueOf(value)),
         TagMap.Entry.ANY,