Tighten AggregateEntry / PeerTagSchema surface area

dougqh · claude · dougqh · commit acf2ffaec73e · 2026-05-27T12:29:03.000-04:00
Three small cleanups that the recent design review surfaced:

- Move test-only AggregateEntry.forSnapshot(SpanSnapshot) to
  AggregateEntryTestUtils. Production callers (AggregateTable.findOrInsert)
  already use the two-arg forSnapshot(snap, keyHash); the no-keyHash
  overload existed for tests. AggregateEntryTest now goes through the test
  helper. MetricsIntegrationTest can't see src/test, so it inlines
  forSnapshot(snap, hashOf(snap)) using the production API directly.

- Change AggregateEntry.recordOneDuration to return void. Returned `this`
  for fluent-style chaining but the only caller (Aggregator.accept)
  discards the return.

- Remove PeerTagSchema.hashCode/equals + cachedHashCode field. Used only
  by AggregateEntry.hashOf, which now inlines Arrays.hashCode(schema.names)
  with an explicit null guard. Drops 42 lines from PeerTagSchema and three
  now-redundant equals tests from PeerTagSchemaTest -- the schema's
  identity contract is enforced by the hash function and hasSameTagsAs
  rather than the Object#equals contract.

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
@@ -161,20 +161,11 @@ static AggregateEntry forSnapshot(SpanSnapshot s, long keyHash) {
     return new AggregateEntry(s, keyHash);
   }
 
-  /**
-   * Convenience overload that computes the hash itself. For test callers that don't have a
-   * precomputed hash on hand; the production path goes through {@link #forSnapshot(SpanSnapshot,
-   * long)} from {@link AggregateTable#findOrInsert}.
-   */
-  static AggregateEntry forSnapshot(SpanSnapshot s) {
-    return new AggregateEntry(s, hashOf(s));
-  }
-
   /**
    * Records a single hit. {@code tagAndDuration} carries the duration nanos with optional {@link
    * #ERROR_TAG} / {@link #TOP_LEVEL_TAG} bits OR-ed in.
    */
-  AggregateEntry recordOneDuration(long tagAndDuration) {
+  void recordOneDuration(long tagAndDuration) {
     ++hitCount;
     if ((tagAndDuration & TOP_LEVEL_TAG) == TOP_LEVEL_TAG) {
       tagAndDuration ^= TOP_LEVEL_TAG;
@@ -188,7 +179,6 @@ AggregateEntry recordOneDuration(long tagAndDuration) {
       okLatencies.accept(tagAndDuration);
     }
     duration += tagAndDuration;
-    return this;
   }
 
   int getErrorCount() {
@@ -279,11 +269,13 @@ static long hashOf(SpanSnapshot s) {
     h = LongHashingUtils.addToHash(h, s.traceRoot);
     h = LongHashingUtils.addToHash(h, s.spanKind);
     // Always mix in both the schema's content hash and the values' content hash, unconditionally
-    // (no null-skip). PeerTagSchema overrides hashCode() to be content-based on names; we use
-    // Arrays.hashCode for the String[] values since the default Object[].hashCode is identity-
-    // based, not content-based. Null inputs hash to 0 for both, distinct from any real schema's
-    // hash or any non-empty values array.
-    h = LongHashingUtils.addToHash(h, s.peerTagSchema);
+    // (no null-skip). Arrays.hashCode is content-based for both String[]s; the default
+    // Object[].hashCode is identity-based, which would let two snapshots with content-equal but
+    // distinct PeerTagSchema instances hash to different buckets. Null inputs hash to 0 here,
+    // distinct from {@code Arrays.hashCode(empty)} = 1 or any non-empty array.
+    h =
+        LongHashingUtils.addToHash(
+            h, s.peerTagSchema == null ? 0 : Arrays.hashCode(s.peerTagSchema.names));
     h = LongHashingUtils.addToHash(h, Arrays.hashCode(s.peerTagValues));
     h = LongHashingUtils.addToHash(h, s.httpMethod);
     h = LongHashingUtils.addToHash(h, s.httpEndpoint);
diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/PeerTagSchema.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/PeerTagSchema.java
@@ -3,7 +3,6 @@
 import static datadog.trace.api.DDTags.BASE_SERVICE;
 
 import datadog.communication.ddagent.DDAgentFeaturesDiscovery;
-import java.util.Arrays;
 import java.util.Set;
 
 /**
@@ -53,15 +52,6 @@ final class PeerTagSchema {
    */
   String state;
 
-  /**
-   * Lazily computed content hash of {@link #names}, used as the bucket-distinguishing contribution
-   * when {@link AggregateEntry#hashOf} hashes a snapshot's peer-tag schema. Benign race pattern: a
-   * concurrent first-time read may recompute the value, but {@link Arrays#hashCode(Object[])} on
-   * the same content array is deterministic so the recomputed value matches. {@code int} writes are
-   * atomic per JLS.
-   */
-  private int cachedHashCode;
-
   private PeerTagSchema(String[] names, String state) {
     this.names = names;
     this.state = state;
@@ -101,36 +91,4 @@ boolean hasSameTagsAs(Set<String> other) {
   int size() {
     return names.length;
   }
-
-  /**
-   * Content-based hash of {@link #names}. Used by {@link AggregateEntry#hashOf} to incorporate the
-   * schema identity into a snapshot's lookup hash. Distinct schemas with the same names hash to the
-   * same value so an entry built under one schema instance still matches a snapshot pinned to a
-   * content-equal replacement (e.g. after reconcile rebuilds the schema).
-   */
-  @Override
-  public int hashCode() {
-    int h = cachedHashCode;
-    if (h == 0) {
-      h = Arrays.hashCode(names);
-      cachedHashCode = h;
-    }
-    return h;
-  }
-
-  /**
-   * Content equality on {@link #names}. {@link #state} is intentionally excluded: it is a
-   * reconcile-bookkeeping field, not part of the schema's identity. Two schemas built from the same
-   * tag list at different discovery snapshots represent the same schema.
-   */
-  @Override
-  public boolean equals(Object o) {
-    if (this == o) {
-      return true;
-    }
-    if (!(o instanceof PeerTagSchema)) {
-      return false;
-    }
-    return Arrays.equals(names, ((PeerTagSchema) o).names);
-  }
 }
diff --git a/dd-trace-core/src/test/java/datadog/trace/common/metrics/AggregateEntryTest.java b/dd-trace-core/src/test/java/datadog/trace/common/metrics/AggregateEntryTest.java
@@ -95,10 +95,10 @@ void testUtilsEqualsIsConsistentWithHashCodeAcrossDifferentSchemaLayouts() {
     //   A: schema ["a","b"], values [null,"x"] -> encoded ["b:x"]
     //   B: schema ["b","c"], values ["x",null] -> encoded ["b:x"]
     AggregateEntry a =
-        AggregateEntry.forSnapshot(
+        AggregateEntryTestUtils.forSnapshot(
             snapshotWithPeerTags(new String[] {"a", "b"}, new String[] {null, "x"}));
     AggregateEntry b =
-        AggregateEntry.forSnapshot(
+        AggregateEntryTestUtils.forSnapshot(
             snapshotWithPeerTags(new String[] {"b", "c"}, new String[] {"x", null}));
 
     // Sanity: same encoded peer tags, despite different raw layout.
@@ -113,10 +113,10 @@ void testUtilsEqualsIsConsistentWithHashCodeAcrossDifferentSchemaLayouts() {
   @Test
   void testUtilsEqualEntriesHaveEqualHashCodes() {
     AggregateEntry a =
-        AggregateEntry.forSnapshot(
+        AggregateEntryTestUtils.forSnapshot(
             snapshotWithPeerTags(new String[] {"a", "b"}, new String[] {null, "x"}));
     AggregateEntry b =
-        AggregateEntry.forSnapshot(
+        AggregateEntryTestUtils.forSnapshot(
             snapshotWithPeerTags(new String[] {"a", "b"}, new String[] {null, "x"}));
 
     assertTrue(AggregateEntryTestUtils.equals(a, b));
@@ -160,6 +160,6 @@ private static AggregateEntry newEntry() {
             null,
             null,
             0L);
-    return AggregateEntry.forSnapshot(snapshot);
+    return AggregateEntryTestUtils.forSnapshot(snapshot);
   }
 }
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
@@ -36,8 +36,8 @@ private AggregateEntryTestUtils() {}
    * <p><b>Test-only.</b> The split is at the <em>first</em> {@code ':'}, so peer-tag values
    * containing a colon (URLs, IPv6 addresses, {@code service:env} patterns) will be silently
    * misparsed and the recovered (name, value) pair will be wrong. Keep test data colon-free in
-   * peer-tag values, or wire a production-style snapshot through {@link
-   * AggregateEntry#forSnapshot(SpanSnapshot)} directly instead.
+   * peer-tag values, or wire a production-style snapshot through {@link #forSnapshot(SpanSnapshot)}
+   * directly instead.
    */
   public static AggregateEntry of(
       CharSequence resource,
@@ -85,7 +85,18 @@ public static AggregateEntry of(
             httpEndpoint == null ? null : httpEndpoint.toString(),
             grpcStatusCode == null ? null : grpcStatusCode.toString(),
             0L);
-    return AggregateEntry.forSnapshot(syntheticSnapshot);
+    return forSnapshot(syntheticSnapshot);
+  }
+
+  /**
+   * 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.
+   */
+  public static AggregateEntry forSnapshot(SpanSnapshot s) {
+    return AggregateEntry.forSnapshot(s, AggregateEntry.hashOf(s));
   }
 
   /**
diff --git a/dd-trace-core/src/test/java/datadog/trace/common/metrics/PeerTagSchemaTest.java b/dd-trace-core/src/test/java/datadog/trace/common/metrics/PeerTagSchemaTest.java
@@ -3,7 +3,6 @@
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -91,44 +90,4 @@ void hasSameTagsAsHandlesEmpty() {
     assertTrue(empty.hasSameTagsAs(Collections.<String>emptySet()));
     assertFalse(empty.hasSameTagsAs(Collections.<String>singleton("peer.hostname")));
   }
-
-  @Test
-  void equalsIsContentBasedOnNames() {
-    PeerTagSchema a = PeerTagSchema.testSchema(new String[] {"peer.hostname", "peer.service"});
-    PeerTagSchema b = PeerTagSchema.testSchema(new String[] {"peer.hostname", "peer.service"});
-
-    assertEquals(a, b);
-    assertEquals(b, a);
-    assertEquals(a.hashCode(), b.hashCode());
-  }
-
-  @Test
-  void equalsIgnoresState() {
-    // state is a reconcile-bookkeeping field, not part of schema identity.
-    PeerTagSchema early =
-        PeerTagSchema.of(Collections.<String>singleton("peer.hostname"), "state-1");
-    PeerTagSchema late =
-        PeerTagSchema.of(Collections.<String>singleton("peer.hostname"), "state-2");
-
-    assertEquals(early, late);
-    assertEquals(early.hashCode(), late.hashCode());
-  }
-
-  @Test
-  void equalsDistinguishesByOrder() {
-    // names is positional -- the array index pairs with SpanSnapshot.peerTagValues. Schemas with
-    // the same tags in different positions are NOT interchangeable.
-    PeerTagSchema ab = PeerTagSchema.testSchema(new String[] {"a", "b"});
-    PeerTagSchema ba = PeerTagSchema.testSchema(new String[] {"b", "a"});
-
-    assertNotEquals(ab, ba);
-  }
-
-  @Test
-  void equalsHandlesNullAndOtherTypes() {
-    PeerTagSchema schema = PeerTagSchema.testSchema(new String[] {"peer.hostname"});
-
-    assertNotEquals(schema, null);
-    assertNotEquals(schema, "peer.hostname");
-  }
 }
diff --git a/dd-trace-core/src/traceAgentTest/groovy/MetricsIntegrationTest.groovy b/dd-trace-core/src/traceAgentTest/groovy/MetricsIntegrationTest.groovy
@@ -39,18 +39,20 @@ class MetricsIntegrationTest extends AbstractTraceAgentTest {
       sink
       )
     writer.startBucket(2, System.nanoTime(), SECONDS.toNanos(10))
-    // Build entries via SpanSnapshot directly: the test factory lives in src/test/java but this
-    // is the separate traceAgentTest source set, so we can't see it. Both entries use one peer
-    // tag (grault:quux) -> schema names=["grault"], values=["quux"].
+    // Build entries via the production AggregateEntry.forSnapshot(snap, keyHash) path -- same
+    // construction as AggregateTable.findOrInsert. Both entries use one peer tag (grault:quux)
+    // -> schema names=["grault"], values=["quux"].
     PeerTagSchema schema = PeerTagSchema.testSchema(["grault"] as String[])
-    def entry1 = AggregateEntry.forSnapshot(new SpanSnapshot(
+    SpanSnapshot snap1 = new SpanSnapshot(
       "resource1", "service1", "operation1", null, "sql", (short) 0,
-      false, true, "xyzzy", schema, ["quux"] as String[], null, null, null, 0L))
+      false, true, "xyzzy", schema, ["quux"] as String[], null, null, null, 0L)
+    def entry1 = AggregateEntry.forSnapshot(snap1, AggregateEntry.hashOf(snap1))
     [2, 1, 2, 250, 4].each { entry1.recordOneDuration(it as long) }
     writer.add(entry1)
-    def entry2 = AggregateEntry.forSnapshot(new SpanSnapshot(
+    SpanSnapshot snap2 = new SpanSnapshot(
       "resource2", "service2", "operation2", null, "web", (short) 200,
-      false, true, "xyzzy", schema, ["quux"] as String[], null, null, null, 0L))
+      false, true, "xyzzy", schema, ["quux"] as String[], null, null, null, 0L)
+    def entry2 = AggregateEntry.forSnapshot(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()
