Tighten AggregateEntry surface — drop one-line factory, doc the conventions

dougqh · claude · dougqh · commit ccb4a4b0ccf9 · 2026-05-27T12:37:25.000-04:00
Five small cleanups surfaced by the design re-review:

- Drop AggregateEntry.forSnapshot(SpanSnapshot, long). It wrapped the
  private constructor for no reason; make the constructor package-
  private and have AggregateTable.findOrInsert and
  AggregateEntryTestUtils.forSnapshot call it directly.

- Class-level Javadoc now documents the required-vs-optional field
  absence convention: required fields canonicalize null -&gt; EMPTY,
  optional fields stay null so the serializer's `!= null` check works.
  Previously a reader had to infer it from the constructor body.

- Field Javadocs on `synthetic` (synthetic-monitoring origin tag) and
  `traceRoot` (parentId == 0). Both make it onto the wire; neither was
  obvious to a fresh reader.

- Tighten the `peerTagNames` / `peerTagValues` field comment. The
  previous wording implied package-private was for "test-only" access;
  in fact production matches() reads them from within the class and the
  test helper is just one consumer.

- Add a `canonicalizeOptional` helper that mirrors `canonicalize` but
  returns null (not EMPTY) for null input. Folds the four optional-
  field assignments in the constructor from three-line ternaries into
  one-liners. Keeps the `instanceof UTF8BytesString` short-circuit
  consistent across all label fields -- dead code for the String-typed
  optionals (httpMethod/Endpoint/grpcStatusCode), live for the
  CharSequence-typed serviceNameSource.

Co-Authored-By: Claude Opus 4.7 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java
@@ -37,6 +37,15 @@
  * key. The class is wider than its predecessors as a result, but that's the trade we explicitly
  * chose.
  *
+ * <p><b>Required vs optional field absence.</b> Required label fields ({@code resource}, {@code
+ * service}, {@code operationName}, {@code type}, {@code spanKind}) canonicalize a {@code null}
+ * snapshot value into {@link UTF8BytesString#EMPTY} via {@link #canonicalize} -- they are never
+ * {@code null} on a constructed entry. Optional label fields ({@code serviceSource}, {@code
+ * httpMethod}, {@code httpEndpoint}, {@code grpcStatusCode}) stay {@code null} on the entry when
+ * the snapshot value was {@code null}; the serializer uses {@code != null} to decide whether to
+ * emit them on the wire. {@link #contentEquals} treats {@code null} and length-0 as equivalent so
+ * {@link #matches} works against either form.
+ *
  * <p><b>Not thread-safe.</b> Counter and histogram updates are performed by the single aggregator
  * thread; producer threads tag durations via {@link #ERROR_TAG} / {@link #TOP_LEVEL_TAG} bits and
  * hand them off through the snapshot inbox.
@@ -98,16 +107,20 @@ final class AggregateEntry extends Hashtable.Entry {
   @Nullable private final UTF8BytesString httpEndpoint;
   @Nullable private final UTF8BytesString grpcStatusCode;
   private final short httpStatusCode;
+
+  /** Whether the root span carried the {@code synthetics} origin tag (synthetic-monitoring run). */
   private final boolean synthetic;
+
+  /** Whether this span is the trace root ({@code parentId == 0}). */
   private final boolean traceRoot;
 
   // Peer tags carried in two forms: parallel String[] arrays mirroring the snapshot's (schema +
   // values) shape for matches(), and pre-encoded List<UTF8BytesString> ("name:value") for the
   // serializer. peerTagNames is the schema's names array (shared by-reference when the schema
   // hasn't been replaced); peerTagValues is the per-span String[] parallel to it.
   //
-  // Package-private rather than private so test-only helpers (e.g. argument-matcher classes in
-  // the same package) can compare them without going through the encoded list.
+  // Package-private so the in-package test helper (AggregateEntryTestUtils) can compare entries
+  // by raw layout; production access comes from this class's own matches() + constructor.
   @Nullable final String[] peerTagNames;
   @Nullable final String[] peerTagValues;
   private final List<UTF8BytesString> peerTags;
@@ -121,29 +134,17 @@ final class AggregateEntry extends Hashtable.Entry {
   private long duration;
 
   /** Hot-path constructor for the producer/consumer flow. Builds UTF8 fields via the caches. */
-  private AggregateEntry(SpanSnapshot s, long keyHash) {
+  AggregateEntry(SpanSnapshot s, long keyHash) {
     super(keyHash);
     this.resource = canonicalize(RESOURCE_CACHE, s.resourceName);
     this.service = canonicalize(SERVICE_CACHE, s.serviceName);
     this.operationName = canonicalize(OPERATION_CACHE, s.operationName);
-    this.serviceSource =
-        s.serviceNameSource == null
-            ? null
-            : canonicalize(SERVICE_SOURCE_CACHE, s.serviceNameSource);
+    this.serviceSource = canonicalizeOptional(SERVICE_SOURCE_CACHE, s.serviceNameSource);
     this.type = canonicalize(TYPE_CACHE, s.spanType);
     this.spanKind = canonicalize(SPAN_KIND_CACHE, s.spanKind);
-    this.httpMethod =
-        s.httpMethod == null
-            ? null
-            : HTTP_METHOD_CACHE.computeIfAbsent(s.httpMethod, UTF8BytesString::create);
-    this.httpEndpoint =
-        s.httpEndpoint == null
-            ? null
-            : HTTP_ENDPOINT_CACHE.computeIfAbsent(s.httpEndpoint, UTF8BytesString::create);
-    this.grpcStatusCode =
-        s.grpcStatusCode == null
-            ? null
-            : GRPC_STATUS_CODE_CACHE.computeIfAbsent(s.grpcStatusCode, UTF8BytesString::create);
+    this.httpMethod = canonicalizeOptional(HTTP_METHOD_CACHE, s.httpMethod);
+    this.httpEndpoint = canonicalizeOptional(HTTP_ENDPOINT_CACHE, s.httpEndpoint);
+    this.grpcStatusCode = canonicalizeOptional(GRPC_STATUS_CODE_CACHE, s.grpcStatusCode);
     this.httpStatusCode = s.httpStatusCode;
     this.synthetic = s.synthetic;
     this.traceRoot = s.traceRoot;
@@ -152,15 +153,6 @@ private AggregateEntry(SpanSnapshot s, long keyHash) {
     this.peerTags = materializePeerTags(this.peerTagNames, this.peerTagValues);
   }
 
-  /**
-   * Construct from a snapshot at consumer-thread miss time, using the {@code keyHash} the caller
-   * (typically {@link AggregateTable#findOrInsert}) already computed for the lookup. Avoids a
-   * second pass over the snapshot's fields just to re-hash them.
-   */
-  static AggregateEntry forSnapshot(SpanSnapshot s, long keyHash) {
-    return new AggregateEntry(s, keyHash);
-  }
-
   /**
    * Records a single hit. {@code tagAndDuration} carries the duration nanos with optional {@link
    * #ERROR_TAG} / {@link #TOP_LEVEL_TAG} bits OR-ed in.
@@ -358,6 +350,28 @@ private static UTF8BytesString canonicalize(
     return cache.computeIfAbsent(charSeq.toString(), UTF8BytesString::create);
   }
 
+  /**
+   * Like {@link #canonicalize} but returns {@code null} for a {@code null} input (rather than
+   * {@link UTF8BytesString#EMPTY}). Used for the four optional fields so the serializer can
+   * distinguish "absent" via a {@code != null} check and elide the field on the wire.
+   *
+   * <p>The {@code instanceof UTF8BytesString} short-circuit is dead code for {@link
+   * SpanSnapshot#httpMethod}/{@code httpEndpoint}/{@code grpcStatusCode} (statically {@code
+   * String}) but live for {@link SpanSnapshot#serviceNameSource} ({@link CharSequence}); keeping a
+   * single helper keeps the constructor consistent.
+   */
+  @Nullable
+  private static UTF8BytesString canonicalizeOptional(
+      DDCache<String, UTF8BytesString> cache, @Nullable CharSequence charSeq) {
+    if (charSeq == null) {
+      return null;
+    }
+    if (charSeq instanceof UTF8BytesString) {
+      return (UTF8BytesString) charSeq;
+    }
+    return cache.computeIfAbsent(charSeq.toString(), UTF8BytesString::create);
+  }
+
   /**
    * UTF8 vs raw CharSequence content-equality, no allocation in the common (String) case.
    *
diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java
@@ -66,7 +66,7 @@ AggregateEntry findOrInsert(SpanSnapshot snapshot) {
     if (size >= maxAggregates && !evictOneStale()) {
       return null;
     }
-    AggregateEntry entry = AggregateEntry.forSnapshot(snapshot, keyHash);
+    AggregateEntry entry = new AggregateEntry(snapshot, keyHash);
     Support.insertHeadEntry(buckets, keyHash, entry);
     size++;
     return entry;
diff --git a/dd-trace-core/src/test/java/datadog/trace/common/metrics/AggregateEntryTestUtils.java b/dd-trace-core/src/test/java/datadog/trace/common/metrics/AggregateEntryTestUtils.java
@@ -90,13 +90,12 @@ public static AggregateEntry of(
 
   /**
    * Builds an {@link AggregateEntry} from {@code s} by computing its lookup hash via {@link
-   * AggregateEntry#hashOf(SpanSnapshot)} and delegating to {@link
-   * AggregateEntry#forSnapshot(SpanSnapshot, long)}. Production callers route through {@link
-   * AggregateTable#findOrInsert} which already has the {@code keyHash} on hand; tests rarely do, so
-   * this helper hides the second argument.
+   * AggregateEntry#hashOf(SpanSnapshot)} and calling the package-private constructor directly.
+   * Production callers route through {@link AggregateTable#findOrInsert} which already has the
+   * {@code keyHash} on hand; tests rarely do, so this helper hides the second argument.
    */
   public static AggregateEntry forSnapshot(SpanSnapshot s) {
-    return AggregateEntry.forSnapshot(s, AggregateEntry.hashOf(s));
+    return new AggregateEntry(s, AggregateEntry.hashOf(s));
   }
 
   /**
diff --git a/dd-trace-core/src/traceAgentTest/groovy/MetricsIntegrationTest.groovy b/dd-trace-core/src/traceAgentTest/groovy/MetricsIntegrationTest.groovy
@@ -46,13 +46,13 @@ class MetricsIntegrationTest extends AbstractTraceAgentTest {
     SpanSnapshot snap1 = new SpanSnapshot(
       "resource1", "service1", "operation1", null, "sql", (short) 0,
       false, true, "xyzzy", schema, ["quux"] as String[], null, null, null, 0L)
-    def entry1 = AggregateEntry.forSnapshot(snap1, AggregateEntry.hashOf(snap1))
+    def entry1 = new AggregateEntry(snap1, AggregateEntry.hashOf(snap1))
     [2, 1, 2, 250, 4].each { entry1.recordOneDuration(it as long) }
     writer.add(entry1)
     SpanSnapshot snap2 = new SpanSnapshot(
       "resource2", "service2", "operation2", null, "web", (short) 200,
       false, true, "xyzzy", schema, ["quux"] as String[], null, null, null, 0L)
-    def entry2 = AggregateEntry.forSnapshot(snap2, AggregateEntry.hashOf(snap2))
+    def entry2 = new AggregateEntry(snap2, AggregateEntry.hashOf(snap2))
     [1, 1, 200, 2, 3, 4, 5, 6, 7, 8].each { entry2.recordOneDuration(it as long) }
     writer.add(entry2)
     writer.finishBucket()

Original file line number	Diff line number	Diff line change
`@@ -66,7 +66,7 @@ AggregateEntry findOrInsert(SpanSnapshot snapshot) {`
`66`	`66`	`if (size >= maxAggregates && !evictOneStale()) {`
`67`	`67`	`return null;`
`68`	`68`	`}`
`69`		`- AggregateEntry entry = AggregateEntry.forSnapshot(snapshot, keyHash);`
	`69`	`+ AggregateEntry entry = new AggregateEntry(snapshot, keyHash);`
`70`	`70`	`Support.insertHeadEntry(buckets, keyHash, entry);`
`71`	`71`	`size++;`
`72`	`72`	`return entry;`