Per-component / tag cardinality limits in client-side stats by dougqh · Pull Request #11387 · DataDog/dd-trace-java

dougqh · 2026-05-15T22:55:00Z

What Does This Do

Implements per-component / per-tag cadinality limits to improve user experience under high load.

Motivation

Previously, there was a single global cap and per-component / tag caches that helped curtail allocation and bound live objects, but this approach had a couple problems.

While the aggregate table was sized capped, failure to insert into the aggregate table would lead to silent data loss. There weren't any obvious indications to the customer when metrics were lost.

And under extreme loads, the caches would degenerate to constantly missing and allocating which would result in long GC cycles.

The per-element limiting allows us to substitute a sentinel value to indicate what was dropped and why to the trace agent / backend. Additionally, this change includes logging and metrics to indicate to the user what is happening locally.

Additional Notes

The cardinality handlers introduced in this change serve dual roles. They both track cardinality and act as caches for UTF8 encodings.

By cardinality limiting first, constant allocation from string concatenation and UTF8 encoding is avoided. And given that a cache and cardinality limiter are basically both sets of recently used values, it seemed most efficient to combine them.

The one difference between the cardinality limiter and the cache is that the cardinality limiter is regularly fully reset -- which hurts the use of the limiter as a cache. To make up for that, the cardinality limiter also holds onto the values used in the previous cycle for reuse in the new cycle.

Claude's Summary

Stack: master → #11382 → #11478 → this PR. Bounds client-stats label cardinality, reworks peer-tag handling, renames the aggregator, and adds a design doc. The lazy-errorLatencies memory win that originally lived in this PR's downstream (#11389) was extracted ahead of #11387 into #11478 during a stack resequence, so the per-entry footprint reduction lands independently of the cardinality machinery.

Cardinality control: replaces the per-field DDCaches with PropertyCardinalityHandler / TagCardinalityHandler. Each has a per-field budget; once exhausted, the sentinel-substitution behavior is gated by the new trace.stats.cardinality.limits.enabled flag (default false). With the flag on, overflow values canonicalize to a blocked_by_tracer sentinel UTF8BytesString and collapse to one bucket. With the flag off (the default), the cache size is still capped at the same budget but over-cap values get freshly-allocated UTF8BytesStrings and flow to distinct buckets — so the wire format is identical to Update client-side stats to use light weight Hashtable #11382. Handlers reset every reporting cycle in either mode.
Canonicalize before hashing: AggregateTable.findOrInsert runs every label through its handler before computing the lookup hash, so cardinality-blocked values collapse into one bucket instead of fragmenting into N entries.
Peer tags reworked: producer captures peer-tag values only (parallel String[] to a PeerTagSchema.names array). tag:value interning happens on the aggregator thread via TagCardinalityHandler. The schema is synced once per trace via PeerTagSchema.currentSyncedTo(Set) with an identity-check fast path.
Rename: ConflatingMetricsAggregator → ClientStatsAggregator.
Producer-side wins identified via JFR: use the cached span.kind byte ordinal through a new CoreSpan.getSpanKindString() (skips a tag-map lookup per metrics-eligible span); hoist schema.names out of capturePeerTagValues; avoid toString() allocation in isSynthetic.
Cleanups: fix TracerHealthMetrics.previousCounts size bug that would have silently dropped the new statsInboxFull counter; drop dead clearAggregates().
Docs: new docs/client_metrics_design.md covering the pipeline shape, the canonical-key trick, thread-safety contract, reporting cadence, failure modes, and benchmark numbers.

Benchmark

`ClientStatsAggregatorDDSpanBenchmark` — producer publish() latency

(64 client-kind DDSpans per op, real CoreTracer)

Stage	µs/op
master baseline	6.428
stack tip before this PR	2.454
+ peer-tag schema hoist	2.410
+ cached span-kind ordinal	1.995

~3.2× over master end to end on the producer side.

Aggregator bench suite vs v1.62.0 + master + #11382

Re-measured 2026-05-27 with three benches in matrix form: full adversarial (all four label dimensions vary) and two cardinality-isolation companions (only resource varies; only peer.hostname varies). Same machine state, same JMH config (8 producer threads, 2×15s warmup + 5×15s, 1 fork, throughput mode). The HighCardinality* and Adversarial benches were backported onto the v1.62.0 tag using its ConflatingMetricsAggregator constructor and HealthMetrics.NO_OP (v1.62.0 predates the inbox split so per-iteration drop counters are not directly comparable).

	v1.62.0 release	master (post-#11381)	#11382	#11387 limits OFF (default)	#11387 limits ON
`AdversarialMetricsBenchmark` (ops/s)	444,290 ± 1,616,937	14,276,351 ± 1,091,138	32,556,300 ± 4,321,490	23,480,978 ± 2,221,623	8,068,173 ± 1,754,400
vs v1.62.0	1.00×	32.1×	73.3×	52.9×	18.2×
`HighCardinalityResource` (ops/s)	4,854,335 ± 1,214,233	8,168,005 ± 3,493,716	35,739,452 ± 2,556,684	28,866,978 ± 1,251,950	25,095,814 ± 1,934,690
vs v1.62.0	1.00×	1.68×	7.36×	5.94×	5.17×
`HighCardinalityPeer` (ops/s)	6,902,209 ± 368,641	10,110,142 ± 3,380,594	37,638,634 ± 6,673,337	29,635,631 ± 5,710,512	27,408,255 ± 1,722,131
vs v1.62.0	1.00×	1.46×	5.45×	4.29×	3.97×
Adversarial `onStatsAggregateDropped`	n/a (`HealthMetrics.NO_OP`)	155,251,623	16,568,738	12,336,616	0
Resource `onStatsAggregateDropped`	n/a	188,023,595	16,557,066	9,773,903	0
Peer `onStatsAggregateDropped`	n/a	223,260,962	14,904,938	17,983,372	0

Customer headline: vs the shipping v1.62.0 release, this branch at the default flag setting (limits OFF) delivers ~50× throughput on adversarial cardinality and ~5–6× on single-axis cardinality. With the flag ON (sentinel-substitution active), ~18× / 4–5× plus zero onStatsAggregateDropped — i.e. the cardinality cap actually saves the bench from data loss. v1.62.0's Adversarial per-iteration progression shows the classic degradation curve (1.08M warmup → 277K → 199K) where the LRU cache thrashes catastrophically; this PR holds steady-state across iterations in either flag mode.

Reading the trade-off:

onStatsAggregateDropped = 0 only with limits ON. That's the safety guarantee the feature pays for. Every other config drops 10–225 M aggregate updates under adversarial cardinality because over-cap values fragment into distinct buckets and saturate tracerMetricsMaxAggregates.
Adversarial-bench limits-on cost is real. All four label dimensions exhaust their cap simultaneously, so every snapshot pays the full sentinel-substitution + blockedCounts++ + warnedCardinality bookkeeping on all four fields. Single-axis benches (HighCardinality*) show a much smaller limits-on penalty (~10%) because only one dimension is over-cap. Workloads with one runaway dimension and the rest bounded sit much closer to the limits-off throughput.
Variance collapses dramatically with limits on. ±1.72 M / ±1.75 M / ±1.93 M on limits-on vs ±5–6.67 M without. Bounded cardinality means no eviction sweeps, stable table size, no per-cycle GC churn — predictable throughput. For workloads paged on p99 latency spikes during reporting cycles, this is often more valuable than peak throughput.
Benches are adversarial. Designed to saturate every capacity bound at once; realistic workloads with smaller working sets see proportionally smaller throughput gaps between configs. The 19–66% limits-on penalty vs Update client-side stats to use light weight Hashtable #11382 is an upper bound, not a steady-state cost.

Architecture note on the limits-off cost. Limits-off matches #11382's wire format exactly, but still costs ~19% on HighCardinality* and ~28% on Adversarial. The gap comes from AggregateTable.findOrInsert canonicalizing every snapshot before lookup — required for the sentinel collapse in limits-on, but pure overhead in limits-off where the hash is content-stable across raw vs canonicalized forms. A two-path findOrInsert (hash-raw on limits-off, canonicalize-first on limits-on) would likely close most of the gap; deferred as a follow-up optimization if the default-off cost matters in practice.

Test plan

:dd-trace-core:test — metrics tests pass (existing + new AggregateTableTest cases for cardinality collapse)
JMH benchmark numbers reproduce locally
No behavior change to client-stats wire payload for traces within the cardinality budget

🤖 Generated with Claude Code

JFR profiling showed ~21% of producer CPU time spent in tag-map lookups during ClientStatsAggregator.publish. One of those lookups -- span.kind -- is redundant because DDSpanContext already caches the kind as a byte ordinal that resolves to a String via a small array. - Add CoreSpan.getSpanKindString() with a default that falls back to the tag map for non-DDSpan impls; DDSpan overrides to delegate to the context's cached resolution. - Hoist schema.names array out of the capturePeerTagValues loop. - Avoid an unnecessary toString() in isSynthetic by declaring SYNTHETICS_ORIGIN as String and using contentEquals. Benchmark (ClientStatsAggregatorDDSpanBenchmark): before: 2.410 us/op after: 1.995 us/op (~17% improvement) vs. master baseline (6.428 us/op): now ~3.2x faster. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Captures the producer/consumer split, the canonical-key trick that makes cardinality-blocking actually save space, the once-per-trace peer-tag schema sync, the role of each file in datadog.trace.common.metrics, and the rationale behind the redesign from ConflatingMetricsAggregator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

LongHashingUtilsTest (14 cases): - hashCodeX null sentinel + non-null pass-through - all primitive hash() overloads match the boxed Java hashCodes - hash(Object...) 2/3/4/5-arg overloads match the chained addToHash formula they are documented to constant-fold to - addToHash(long, primitive) overloads match the Object-version - linear-accumulation invariant (31 * h + v) holds across a sequence - iterable / deprecated int[] / deprecated Object[] variants match chained addToHash - intHash treats null as 0 (observable via hash(null, "x")) HashtableTest (24 cases across 5 nested classes): - D1: insert/get/remove/insertOrReplace/clear/forEach, in-place value mutation, null-key handling, hash-collision chaining with disambig- uating equals, remove-from-collided-chain leaves siblings intact - D2: pair-key identity, remove(pair), insertOrReplace matches on both parts, forEach - Support: capacity rounds up to a power of two, bucketIndex stays in range across a wide hash sample, clear nulls every slot - BucketIterator: walks only matching-hash entries in a chain, throws NoSuchElementException when exhausted - MutatingBucketIterator: remove from head-of-chain unlinks, replace swaps the entry while preserving chain, remove() without prior next() throws IllegalStateException Tests live in internal-api/src/test/java/datadog/trace/util and use the already-present JUnit 5 setup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Bring the new util/ files in line with google-java-format (tabs → spaces, line wrapping, javadoc list markup) so spotlessCheck passes in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@threads

Compares Hashtable.D1 and Hashtable.D2 against equivalent HashMap usage for add, update, and iterate operations. Each benchmark thread owns its own map (Scope.Thread), but @threads(8) is used so the allocation/GC pressure that Hashtable is designed to avoid surfaces in the throughput numbers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- Guard Support.sizeFor against overflow and use Integer.highestOneBit; reject capacities above 1 << 30 instead of looping forever. - Add braces around single-statement while bodies in BucketIterator. - Split HashtableBenchmark into HashtableD1Benchmark / HashtableD2Benchmark. - Add regression tests for Support.sizeFor bounds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The 5-arg Object overload was forwarding only obj0..obj3 to the int overload, silently dropping obj4. Also align LongHashingUtils.hash 3-arg signature with its 2/4/5-arg siblings (int parameters) and strengthen the 5-arg HashingUtilsTest to detect the missing-arg regression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- Split D1Tests and D2Tests into HashtableD1Test and HashtableD2Test; extract shared test entry classes into HashtableTestEntries. - Reduce visibility of LongHashingUtils.hash(int...) chaining overloads to package-private; they are internal building blocks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The iterator tests need a populated Hashtable.Entry[] to drive Support.bucketIterator / mutatingBucketIterator. Relaxing D1.buckets from private to package-private lets the same-package tests read it directly, removing the reflection helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The new reason:inbox_full reportIfChanged call advances countIndex to 51, but previousCounts was still sized for 51 counters (max index 50), so the metric never emitted and the resize warning fired every flush. Bump the array to 52 and add a regression test that exercises the flush path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The label fields and the mutable counters/histograms are 1:1 with each entry; carrying them on a separate object meant one extra allocation per unique key plus an indirection on every hot-path update. Merging them puts the counters directly on AggregateEntry, drops the entry.aggregate hop, and consolidates ERROR_TAG / TOP_LEVEL_TAG onto the same class the consumer uses to decode them. AggregateTable.findOrInsert now returns AggregateEntry. Callers in Aggregator and SerializingMetricWriter updated. Migrated AggregateMetricTest.groovy to AggregateEntryTest.java per project policy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Add a context-passing forEach(T, BiConsumer) overload to AggregateTable, mirroring TagMap's pattern. Aggregator.report now hands the writer in as context to a static BiConsumer so no fresh Consumer is allocated each report cycle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Mirrors the TagMap pattern: pairs the existing forEach(Consumer) with a forEach(T context, BiConsumer<T, TEntry>) overload so callers can hand side-band state to a non-capturing lambda and avoid the fresh-Consumer-per-call allocation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Factors the unchecked (TEntry) cast out of D1.forEach / D2.forEach (and the BiConsumer variants) into Support.forEach(buckets, ...). The cast now lives in one place, mirroring how Entry.next() handles it, and the D1/D2 methods become one-liners. Downstream higher-arity tables built on Support gain the same helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Now that Hashtable.Support exposes the parameterized forEach helpers, AggregateTable's own forEach methods can drop their duplicated loop body and the (AggregateEntry) cast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Adds Support.bucket(buckets, keyHash) which returns the bucket head already cast to the caller's concrete entry type. D1.get and D2.get now drop the raw-Entry intermediate variable and walk the chain via Entry.next() directly. The unchecked cast lives in one place, consistent with Entry.next() and Support.forEach. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- findOrInsert: walks via Support.bucket(buckets, keyHash) instead of Hashtable.Entry + intermediate cast; bucketIndex is only computed on the miss path now. - evictOneStale / expungeStaleAggregates: chain variables typed as AggregateEntry from the head down, leveraging Entry.next()'s generic inference, so the per-iteration getHitCount() checks drop their (AggregateEntry) cast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Holdover from when both lived in a shared HashtableBenchmark; redundant now that each lives in its own class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

dougqh · 2026-06-29T17:11:34Z

I think I went over everything. I insisted a bit on inline comments on cardinality handler register method, because they hold an interesting part of this change.

The claude generated javadoc is really hard to read. Maybe because I'm a non native english. But I believe the suggestions I gave significantly improves the review experience, after all this is not a small PR.

Also I wonder if some tests are missing?

Yeah, I'm trying to find a balance. I might just have to go back and rewrite all the Javadoc myself.

@VisibleForTesting

…TagCardinalityHandler Applied all bric3 review suggestions for TagCardinalityHandler: - Replace class Javadoc with self-contained description of the tag-value encoding path - Extract MAX_CARDINALITY_LIMIT = 1 << 29 constant (mirrors PropertyCardinalityHandler) - Add @VisibleForTesting on 2-arg test constructor (uses datadog.trace.api.internal) - Rewrite register() Javadoc to match PropertyCardinalityHandler's style - Add inline comments throughout register() matching the parallel structure in PropertyCardinalityHandler (probe slot, current-cycle lookup, sentinel mode, prior reuse) - Rename curKey -> existing to match PropertyCardinalityHandler naming Also applied remaining PropertyCardinalityHandler suggestion: - Add "This value is new for the current cycle." comment before capExhausted (MjO4h) Test additions: - tagRegisterOfNullDoesNotConsumeBudget (mirror of property equivalent) - propertyResetReturnsBlockedCount + tagResetReturnsBlockedCount (verify reset() return value) - PeerTagSchemaTest.resetHandlersReportsBlockedCountToHealthMetrics (verifies onTagCardinalityBlocked is called for any tag that hit its limit) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

bric3

I think this looks better. Pre-approving, but there are still a few things to tweak before.

FYI the graalvm failing jobs are infrastructure related, restarting them should work.

Caused by: org.gradle.tooling.BuildException: Could not execute build using connection to Gradle distribution '.../artifact/services.gradle.org/distributions/gradle-8.14.5-bin.zip'.

In general the claude javdoc creates some cognitive overload, as I'm reading them and looking at code to compare both. But reading them is tough, at least for me. Personnally, I would prefer you write the doc or PR description ☺️.

Suggestion to put in the user file (CLAUDE.md or AGENTS.md):

* Keep Javadoc, comments, PR description writing informative, short, concise, legible, and blog-like.

bric3 · 2026-06-29T19:40:33Z

+   * shared handler state. {@link UTF8BytesString}s are created directly. Content-equal entries from
+   * {@link Canonical#createEntry} still {@link #equals} an entry built via {@code of(...)}.
   */
  static AggregateEntry of(


todo: Did cluade forgot the suggested VisibleforTesting annotation (datadog.trace.api.internal.VisibleForTesting)

Thanks, I'll see what I can do about both of those issues.

I got rid of the test specific helper. I'm not fond of having them in a production class.

The factory was always test-only and the class Javadoc on AggregateEntryTestUtils already stated the intent: "Lives in src/test so the production class stays free of test-only API." Moving it there completes that design. To enable the move: - AggregateEntry constructor: private → package-private - AggregateEntry.createUtf8(): private static → package-private static Both are non-public internal helpers; widening to package-private exposes them only within the same source set. AggregateEntryTestUtils.of() now owns the implementation directly. All callers updated: AggregateEntryTest (Java) and ClientStatsAggregatorTest (Groovy). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…tyHandler Remove DDCache history, storage layout details, and reset mechanics from the class headers -- those belong in a design doc or the inline comments that already explain them at the call site. Keep just what's non-obvious from the class name: the cycle/budget concept and the prior-cycle reuse invariant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…alityHandler Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Skips the closed dougqh/metrics-memory-efficiency (#11389); #11387 now serves as the direct base. Conflict resolution: took HEAD (combined cardinality + additional-tags) for all conflicts. Restored @nullable annotations on populatePeerTags/populateAdditionalTags params that were lost during rebase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…torTest Both surfaced after the #11387→#11402 reconcile merge: Config.java's static-import block left TRACE_STATS_ADDITIONAL_TAGS out of alphabetical order (TRACER_* sorts before TRACE_*), and a long SimpleSpan publish line in ClientStatsAggregatorTest needed wrapping. Pure formatting; no behavior change. Restores green spotlessCheck on internal-api and dd-trace-core. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Conflict in CoreSpan.java: master made isKind() abstract (enforcing that all implementors provide an efficient implementation); our branch had made it a default that delegates to getSpanKindString(). Resolution: keep isKind() abstract per master, retain getSpanKindString() as a default since ClientStatsAggregator calls it on the CoreSpan interface. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The getSpanKindString() default method references Tags.SPAN_KIND but the import was not present after the merge conflict resolution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…rtyCardinalityHandler.java Co-authored-by: Brice Dutheil <brice.dutheil@gmail.com>

AggregateEntry importing HealthMetrics causes GraalVM native-image failures: HealthMetrics.onOrgGuardEnforce(OrgGuard.Reason) pulls OrgGuard$Reason into the reachability graph, and getDeclaringClass0() on an inner enum stored in the agent's .classdata classloader fails at native-image build time. ClientStatsAggregator already owns HealthMetrics and the reset hook; inline the per-field blocked-count reporting there, make FIELD_HANDLERS package- private for the loop, and reduce AggregateEntry.resetCardinalityHandlers() to a plain reset with no HealthMetrics dependency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…chema and fix PropertyCardinalityHandler PeerTagSchema.INTERNAL is a static field, so GraalVM's reachability analysis initializes PeerTagSchema at build time. With HealthMetrics in its method signatures, this pulls in OrgGuard$Reason (an inner enum stored in the agent's .classdata classloader), and getDeclaringClass0() fails at native-image time. Move the per-cycle block-count reporting out of PeerTagSchema.resetHandlers into a new ClientStatsAggregator.resetPeerTagSchema helper, which already owns HealthMetrics and the reset hook. PeerTagSchema and its handlers are now free of HealthMetrics. Also removes a stale class-level Javadoc block that was accidentally embedded inside PropertyCardinalityHandler.reset() instead of at the top of the file. This was causing the persistent spotless failure in CI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… conflicts - Restore OrgGuard creation in CoreTracer (decorates extractor and injector for _dd.p.opm header propagation) - Restore orgPropagationMarker field/parser/getter in DDAgentFeaturesDiscovery - Move propagationTagsFactory initialization back before OrgGuard setup - Fix PeerTagSchemaTest to not call removed resetHandlers(HealthMetrics) method Fixes spring-boot-3-native GraalVM CI failure and parametric OPM test failure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dd-octo-sts · 2026-07-01T04:09:52Z

/merge

gh-worker-devflow-routing-ef8351 · 2026-07-01T04:09:56Z

View all feedbacks in Devflow UI.

2026-07-01 04:09:56 UTC ℹ️ Start processing command /merge

2026-07-01 04:09:59 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in master is approximately 2h (p90).

2026-07-01 05:12:30 UTC ℹ️ MergeQueue: This merge request was merged

Swap MAX_RATIO numerator/denominator pair for a single float + scaled create() Replace Support.MAX_RATIO_NUMERATOR / _DENOMINATOR with a single float MAX_RATIO constant, and add a Support.create(int, float) overload that takes a scale factor. Callers now write Support.create(n, MAX_RATIO) instead of stitching together the int arithmetic at the call site. The scaled size is truncated (not ceiled) before going through sizeFor. sizeFor already rounds up to the next power of two, so truncation just absorbs float fuzz that would otherwise push a result like 12 * 4/3 = 16.0000005f past 16 and double the bucket array size for no reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Tighten Hashtable docs + rename MAX_CAPACITY to MAX_BUCKETS Five small cleanups from a design re-review pass: 1. Support javadoc: drop the stale "methods are package-private" sentence; most of them were made public in earlier commits for higher-arity callers. Also drop the "nested BucketIterator" framing (iterators are peers of Support inside Hashtable, not nested inside Support). 2. MAX_RATIO javadoc: drop the Math.ceil recommendation; create(int, float) deliberately truncates and is the canonical pathway. 3. Document the null-hash treatment on D1.Entry.hash and D2.Entry.hash so the behavior difference is explicit: D1 uses Long.MIN_VALUE as a sentinel that's collision-free against any int-valued hashCode(); D2 has no such sentinel and relies on matches() to resolve null/null vs hash-0 collisions. 4. Rename Support.MAX_CAPACITY -> MAX_BUCKETS and sizeFor's parameter to requestedSize. The cap is on the bucket-array length, not entry count; the new name reflects that. Error messages updated to match. 5. Drop the `abstract` modifier on Hashtable in favor of `final` with a private constructor. Nothing actually subclasses Hashtable -- the abstract was a namespace device that read as "intended for extension." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Dedupe chain-head splice in D1/D2 via keyHash insertHeadEntry overload - Add Support.insertHeadEntry(buckets, long keyHash, entry) overload that derives the bucket index itself. Callers that already have a hash but not the index (the common case) now avoid the redundant bucketIndex(...) hop. - D1.insert, D1.insertOrReplace, D2.insert, D2.insertOrReplace: use the new overload, drop the (thisBuckets local, bucketIndex compute, setNext, store) sequence at each call site. - D2.buckets: drop the `private` modifier to match D1.buckets. Both are package-private so iterator tests in the same package can drive Support.bucketIterator against the table's bucket array. Added a short comment on both fields documenting the rationale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Tighten Entry.next encapsulation; doc hasNext; add D1/D2 getOrCreate Three follow-ups from the design review: - Make Hashtable.Entry.next private. All same-package readers (BucketIterator) already had a next() accessor; the leftover direct field reads now route through it. Closes the "mixed encapsulation" gap where some readers used the accessor and same-package ones reached for the field. - BucketIterator and MutatingBucketIterator now document that chain-walk work happens in next() (and the constructor for the first match); hasNext() is an O(1) field read. - Add D1.getOrCreate(K, Function) and D2.getOrCreate(K1, K2, BiFunction). Both reuse the lookup hash for the insert on miss, avoiding the double-hash that "get; if null then insert" callers would otherwise pay. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Hashtable: add missing braces and detach removed/replaced entries Addresses PR #11409 review comments: - #3267164119 / #3267165525: wrap every single-line if/break body in braces (7 sites across BucketIterator, MutatingBucketIterator, and the full-table Iterator). - #3275947761 / #3275948108 (sarahchen6): null out the removed/replaced entry's next pointer after splicing it out of the chain in MutatingBucketIterator.remove / .replace. Applied the same fix to the full-table Iterator.remove for consistency. Rationale: detaching prevents accidental traversal through a removed entry via a stale reference and lets the GC reclaim a chain tail that the removed entry was the last referrer to. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Rename LongHashingUtils.hashCodeX(Object) to hash(Object) for API consistency Addresses PR #11409 review comment #3276167001. The method parallels the primitive hash(boolean) / hash(int) / hash(long) / ... family, so naming it hash(Object) -- with null collapsing to Long.MIN_VALUE as a sentinel distinct from any real hashCode -- matches the rest of the public surface. Test call sites that pass a literal null now disambiguate against hash(int[]) / hash(Object[]) / hash(Iterable) via an (Object) cast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge remote-tracking branch 'origin/master' into dougqh/optimize-metric-key Merge branch 'dougqh/util-hashtable' into dougqh/optimize-metric-key Merge branch 'dougqh/optimize-metric-key' into dougqh/control-tag-cardinality Merge remote-tracking branch 'origin/master' into dougqh/conflating-metrics-background-work Introduce slim PeerTagSchema; capture peer-tag values not pairs Addresses sarahchen6's review comment on ConflatingMetricsAggregator extractPeerTagPairs: replaces the worst-case-allocation + trim-and-copy flat-pairs layout with a parallel-array carrier. - New PeerTagSchema: minimal carrier of String[] names. Two flavors -- a static INTERNAL singleton (one entry: base.service) for internal-kind spans, and per-discovery built schemas for client/producer/consumer spans. Deliberately no cardinality limiters or per-cycle state; that layers on top in a later PR. - ConflatingMetricsAggregator: caches the peer-aggregation schema keyed on reference equality of features.peerTags() -- a single volatile read + a long compare on the steady-state producer hot path, no allocation. The producer now captures only a String[] of values parallel to the schema's names; the schema reference is carried on SpanSnapshot. The prior "build worst-case pairs then trim" code is gone. - SpanSnapshot: replaces String[] peerTagPairs with PeerTagSchema + String[] peerTagValues. Producer drops the schema reference if no values fired so the consumer short-circuits on null. - Aggregator.materializePeerTags: now reads name/value pairs at the same index from (schema.names, snapshot.peerTagValues). Counts hits once for exact-size allocation; preserves the singletonList fast path for the common one-entry case (e.g. internal-kind base.service). Producer-side cost goes from "allocate String[2n] + walk + maybe trim" to "single volatile read + walk + lazy String[n] only on first hit". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key Merge branch 'dougqh/optimize-metric-key' into dougqh/control-tag-cardinality Address PR #11381 review (round 2) - Aggregator.materializePeerTags: fold the firstHit-discovery nested if into a single guarded post-increment (amarziali, #3279243138). One body line: `if (values[i] != null && hitCount++ == 0) firstHit = i;`. - Drop redundant isKind(SpanKindFilter) overrides in both TraceGenerator.groovy files (amarziali, #3279264553 / #3279382648). CoreSpan.java:84 already supplies a default implementation that reads the same span.kind tag. - Bump TRACER_METRICS_MAX_PENDING default from 2048 -> 131072 to address the capacity regression amarziali flagged (#3279378375). Without producer-side conflation, the inbox now holds 1 SpanSnapshot per metrics-eligible span instead of 1 conflated Batch per ~64 spans; restoring effective capacity parity (~2048 * ~64 = 131072) prevents a ~64x rise in inbox-full drops at the same span rate. ~100 B per SpanSnapshot puts the worst-case heap floor at ~13 MB -- bounded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Cover inbox-full fast-path in ConflatingMetricsAggregator.publish Addresses PR #11381 review (amarziali, #3279325340 -- "Are the existing tests covering this case?"). New ConflatingMetricsAggregatorInboxFullTest constructs the aggregator with a small inbox (queueSize=8), deliberately does NOT call start() so the consumer thread never drains, then publishes enough spans to overflow the inbox. Verifies that healthMetrics.onStatsInboxFull() is called at least once -- the fast-path's `inbox.size() >= inbox.capacity()` short-circuit triggers when the producer-side queue is at capacity. Test is Java + JUnit 5 + Mockito per the project convention for new tests; uses a CoreSpan Mockito mock rather than the SimpleSpan Groovy fixture so we don't depend on Groovy-then-Java compile order from the test source set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Reconcile PeerTagSchema once per reporting cycle on the aggregator thread Addresses amarziali's review comment #3279340181 ("It would be more efficient to trigger from the other side"). The producer-side reference compare on every publish goes away; the aggregator thread reconciles the cached schema against feature discovery once per reporting cycle. - DDAgentFeaturesDiscovery: expose getLastTimeDiscovered() so callers can detect a discovery refresh without copying the peerTags Set. - PeerTagSchema: add `long lastTimeDiscovered` (plain, aggregator-only) and `hasSameTagsAs(Set)`. of(Set, long) takes the timestamp; INTERNAL uses a -1L sentinel since it's never reconciled. - ConflatingMetricsAggregator: * Drop the cachedPeerTagsSource volatile and the per-publish reference compare. * Producer fast path is now `cachedPeerTagSchema` volatile read + null-check; first publish takes the one-time synchronized bootstrap. * Add reconcilePeerTagSchema() that runs once per cycle on the aggregator thread: fast-path timestamp compare, slow-path set compare, bump-in-place when the set is unchanged. - Aggregator: new `Runnable onReportCycle` constructor parameter, run at the start of report() (before the flush, so any test awaiting writer.finishBucket() observes the schema in its post-reconcile state and so the next publish sees the new schema without a handoff). - Update "should create bucket for each set of peer tags" to drive two reporting cycles separated by a report() that triggers reconcile. The old test relied on per-publish reference detection, which the new design intentionally doesn't preserve -- the schema is now stable within a cycle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key Merge branch 'dougqh/optimize-metric-key' into dougqh/control-tag-cardinality Add bootstrap + reconcile coverage for PeerTagSchema Addresses round-3 review nice-to-haves on PR #11381. - PeerTagSchemaTest: unit coverage for hasSameTagsAs() (the predicate that drives the reconcile fast/slow path split), the of(Set, long) factory, and the INTERNAL singleton. The hasSameTagsAs cases include same-content-different-Set-reference (the case the reconcile fast path relies on after a discovery refresh) and content-mismatch in either direction. - ConflatingMetricsAggregatorBootstrapTest: integration coverage for the producer-side bootstrap + aggregator-thread reconcile flow. * bootstrapHappensOnceOnFirstPublish -- three publishes against an un-started aggregator (no consumer thread, no reconciles); verifies features.peerTags() and features.getLastTimeDiscovered() are each called exactly once. * reconcileSkipsDeepCompareWhenTimestampMatches -- two cycles with constant features.getLastTimeDiscovered(); each post-report reconcile short-circuits on the timestamp fast path, so peerTags() is called only by bootstrap (1 total). * reconcileSurvivesTimestampBumpWhenTagsUnchanged -- timestamps bump every reconcile, forcing the slow set-compare path; the tag set stays identical, so the schema is preserved and continues to flush buckets correctly across cycles. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key Use writer.finishBucket() count in bootstrap test for cascade compatibility The verify(writer).add(MetricKey, AggregateMetric) signature is unique to #11381; downstream branches use AggregateEntry. Switching to verify(writer, times(2)).finishBucket() keeps the same behavioral guarantee (both cycles flushed) across the stack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Use writer.finishBucket() count in bootstrap test for cascade compatibility The verify(writer).add(MetricKey, AggregateMetric) signature is unique to #11381; downstream branches use AggregateEntry. Switching to verify(writer, times(2)).finishBucket() keeps the same behavioral guarantee (both cycles flushed) across the stack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/optimize-metric-key' into dougqh/control-tag-cardinality Rename bootstrap test to ClientStatsAggregator + adapt PeerTagSchemaTest #11387's ClientStatsAggregator renames ConflatingMetricsAggregator; the test file's name and class refs need to match. PeerTagSchemaTest's PeerTagSchema.of() calls need the (Set, long, HealthMetrics) signature this branch introduced. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'master' into dougqh/conflating-metrics-background-work Merge branch 'dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key Preserve TRACER_METRICS_MAX_PENDING semantic + drop stale imports TRACER_METRICS_MAX_PENDING previously counted conflating Batch slots (~64 spans each). The inbox now holds 1 SpanSnapshot per slot, so multiply the configured value by LEGACY_BATCH_SIZE (64) to keep pre-existing customer overrides delivering the same effective span-throughput capacity. Default stays at 2048 logical -> 131072 snapshot slots, identical to the prior 2048 batches * 64 spans. Also drops two unused datadog.trace.core.SpanKindFilter imports left behind in TraceGenerator.groovy after the isKind() override was removed in favor of the CoreSpan default implementation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key Add AdversarialMetricsBenchmark for capacity-bound stress testing Ports the adversarial JMH benchmark from #11402 down to this branch so we can compare #11381 vs master on a high-cardinality, high-throughput workload. Adapted to use ConflatingMetricsAggregator (pre-rename) and the FixedAgentFeaturesDiscovery / NullSink helpers already in ConflatingMetricsAggregatorBenchmark. 8 producer threads hammer publish() with unique (service, operation, resource, peer.hostname) per op so the aggregate cache fills+evicts continuously and the inbox saturates. tearDown prints the drop counters (inboxFull vs aggregateDropped) so the test verifies the subsystem stayed bounded under attack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key Trim AdversarialMetricsBenchmark counters and clarify printout Drop traceComputedCalls / totalSpansCounted: under 8-way contention the volatile-long ++/+= pattern was losing ~20% of updates (296M counted vs 245M reported), and the numbers duplicate signal JMH's ops/s already provides. Switch inboxFull / aggregateDropped to LongAdder so the printed drop shape (the order-of-magnitude story the bench is built to tell) is accurate under contention. Replace the stale "both forks combined for this run" string with text that matches the actual @Fork(value=1) config and notes that counters accumulate across warmup + measurement. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key Close PeerTagSchema reconcile race + cover the swap branch buildPeerTagSchema previously read features.peerTags() before features.getLastTimeDiscovered(). DDAgentFeaturesDiscovery exposes those as two separate accessors against its volatile State -- a state-swap interleaving could leave the cached schema tagged with a NEWER timestamp than its names, after which the next reconcile short-circuits on the timestamp compare and misses the tag-set update until the next discovery refresh (~minute later). Swap the read order so timestamp is captured first. With this ordering, an interleaving leaves the schema OLDER than its names instead -- the next reconcile sees a timestamp mismatch, runs the deep compare, and self-heals on the very next cycle. Also adds reconcileSwapsSchemaWhenTagSetChanges, which closes the test gap on the slow-path swap branch (cachedPeerTagSchema = PeerTagSchema.of(...)). End-to-end check via the writer's captured MetricKeys: pre-swap snapshot carries only peer.hostname, post-swap snapshot carries both peer.hostname and peer.service. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key Adapt reconcileSwapsSchemaWhenTagSetChanges to AggregateEntry shape #11382 collapses MetricWriter.add(MetricKey, AggregateMetric) into add(AggregateEntry). Re-target the captor and accessors on this branch so the test compiles and the same end-to-end peer-tag verification holds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Clarify materializePeerTags hit-counting loop Splits the `if (values[i] != null && hitCount++ == 0)` conjunction into nested ifs. Same semantics, no codegen impact after JIT -- just visibly says what the loop is doing rather than relying on post-increment-inside-conjunction. Closes amarziali's review thread on this block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key # Conflicts: # dd-trace-core/src/main/java/datadog/trace/common/metrics/Aggregator.java Fix MetricsIntegrationTest entry recording call site AggregateEntry consolidated MetricKey + AggregateMetric so recordDurations lives directly on AggregateEntry now. The previous entry1.aggregate. recordDurations(...) form compiles under Groovy's dynamic dispatch but would throw MissingPropertyException at runtime since there is no `aggregate` property. Resolves chatgpt-codex-connector's review comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Make ConflatingMetricAggregatorTest counter checks actually verify The `1 * writer.add(value) >> { closure }` pattern treats the closure as a stubbed return value -- Spock evaluates it but discards the result, so `e.getHitCount() == X && ...` was a silent no-op across 31 occurrences. Wrapping the expression in `assert` makes Groovy's power-assert throw on mismatch, which Spock surfaces as a real failure. Resolves chatgpt-codex-connector's review comment. All 41 tests still pass, so the previously-unverified assertions happened to hold. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Drop dead recordDurations(int, AtomicLongArray) batch API This method was a vestige of master's Batch design where multiple producer threads wrote into an AtomicLongArray slot concurrently and the aggregator drained ~64 durations per Batch in one call. The new producer/consumer split publishes one SpanSnapshot per span, so production only ever calls recordOneDuration(long). Migrate the three remaining callers (AggregateEntryTest, SerializingMetricWriterTest, MetricsIntegrationTest) to a loop of recordOneDuration(long) calls, then delete the batched method and its AtomicLongArray imports. Drops the recordDurationsIgnoresTrailingZeros test -- that behavior was a specific quirk of the batched API (count parameter shorter than the array length) and doesn't apply to recordOneDuration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Warn about colon split in AggregateEntry.of test factory The factory recovers (name, value) pairs from pre-encoded "name:value" strings by splitting at the FIRST colon. Test-only, but worth being explicit so callers don't hand it a peer-tag value containing a colon (URLs, IPv6, service:env) and get a silently wrong (name, value) pair. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Add coverage for disable() -> ClearSignal threading path The bundled fix in this PR routes the agent-downgrade clear through the inbox so the aggregator thread stays the sole writer to AggregateTable. Prior to this test, there was no regression coverage for that routing. The test fires DOWNGRADED from the test thread (production-like OkHttpSink callback path), waits for the immediate no-flush window, then publishes a marker span with a distinct resource name. The subsequent report's writer.add captor must see only the marker -- if CLEAR didn't actually wipe the original entry, the original "resource" would still be present and the assertion would catch it. Cannot directly verify thread identity of the clear from inside this test (CLEAR's inbox.clear() drops any latch signal we'd queue behind it), so this is an observable-contract test rather than a strict thread-id test. Still catches both the missing-clear regression and the bucket-chain-corruption regression that the original threading race could produce. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Drop unused Tags imports flagged by codenarc Leftover from removing the isKind() override in TraceGenerator earlier in this session -- I dropped the SpanKindFilter import but missed datadog.trace.bootstrap.instrumentation.api.Tags, which is no longer referenced in either file. Resolves codenarcTest and codenarcTraceAgentTest UnusedImport violations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key Drop unused Tags imports flagged by codenarc Leftover from removing the isKind() override in TraceGenerator earlier in this session -- I dropped the SpanKindFilter import but missed datadog.trace.bootstrap.instrumentation.api.Tags, which is no longer referenced in either file. Resolves codenarcTest and codenarcTraceAgentTest UnusedImport violations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Drop unused SpanKindFilter imports flagged by codenarc Leftover from earlier cleanup of the isKind() override -- #11387 hadn't yet cascaded that part, so the import is stale here too. Resolves codenarcTest and codenarcTraceAgentTest UnusedImport violations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/optimize-metric-key' into dougqh/control-tag-cardinality Resolved conflicts: - AggregateEntry.java: kept #11387's cardinality-handler constructor shape; dropped the dead recordDurations(int, AtomicLongArray) batch API and its AtomicLongArray import (the #11382 cleanup). The of() colon-split warning from #11382 doesn't apply because #11387's of() takes peerTags as a pre-built list (no colon-splitting on test reconstruction). - ClientStatsAggregator.java: kept #11387's PeerTagSchema.of(..., healthMetrics) 3-arg signature; preserved #11382's read-order race fix (lastTimeDiscovered read before peerTags); preserved the resetCardinalityHandlers method from HEAD. - ClientStatsAggregatorTest.groovy: kept HEAD's cardinality-focused tests; applied the #11382 Spock-assert fix (wrap '>> { closure }' body expressions in assert) to all 31 sites so power-assert surfaces mismatches. - ClientStatsAggregatorBootstrapTest.java: renamed the reconcileSwapsSchemaWhenTagSetChanges test's ConflatingMetricsAggregator references to ClientStatsAggregator. - ConflatingMetricsAggregatorDisableTest.java: renamed file + references to ClientStatsAggregatorDisableTest with corresponding class-name change. - AdversarialMetricsBenchmark.java: updated ConflatingMetricsAggregator + ConflatingMetricsAggregatorBenchmark references to ClientStatsAggregator / ClientStatsAggregatorBenchmark. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Sync client_metrics_design doc with reconcile-on-aggregator-thread The doc described an old design where the producer thread per-trace read a peerTagsRevision() and rebuilt the cached PeerTagSchema under a monitor. The actual implementation (cascaded from #11381) runs reconcile once per report cycle on the aggregator thread via the onReportCycle hook, keyed on getLastTimeDiscovered(). Producers do nothing more than a volatile read of the cached schema. Updates: - Producer-side flow: drop the per-trace sync description; document the volatile-read steady state and the one-time synchronized bootstrap on first publish. - New "Aggregator-side reconcile" section under "Reporting cadence and cardinality reset" describing the timestamp fast path, the same-tags slow path that preserves warm handlers, and the read-order race fix (timestamp before names). - Memory and lifetime: replace peerTagsRevision pairing with the on-schema lastTimeDiscovered + per-aggregator-instance lifecycle. - "Why the redesign" point 6: rewritten to describe the aggregator- thread reconcile rather than the producer-side revision check. Resolves dougqh's open review thread about peerTagsRevision vs lastTimeDiscovered. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Update dd-trace-core/src/main/java/datadog/trace/common/metrics/PeerTagSchema.java Co-authored-by: Sarah Chen <sarah.chen@datadoghq.com> Spread input hash before masking in cardinality-handler probes Both PropertyCardinalityHandler and TagCardinalityHandler linear-probe on (value.hashCode() & capacityMask). Without a spreader, inputs that share a low-bit pattern (e.g. URL templates with a common prefix, or String.hashCode values clustered around 0 for short strings) collapse onto the same probe chain. With the load factor capped at 0.5 the chain length is bounded but can still grow under pathological inputs. Mixing the input hash with its upper half (h ^ (h >>> 16)) before masking spreads the high bits down, same trick HashMap.hash uses. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Apply Spotless Javadoc reflows on metrics files Pure formatting -- google-java-format reflows of Javadoc paragraph breaks and parameter wrapping. No behavior change. Picked up from a prior session's spotlessApply that wasn't bundled into the relevant commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Address sarahchen6's review pass PeerTagSchema.java: drop the duplicate Javadoc line that the GitHub UI suggestion accept inadvertently added (it added rather than replaced), collapsing back to the single intended line per sarahchen6's suggestion. Original line said "no cardinality limiters or per-cycle state" which was misleading since lastTimeDiscovered IS per-cycle state; suggestion rightly drops that clause. Config.java: wrap the TRACER_METRICS_MAX_PENDING * LEGACY_BATCH_SIZE multiplication in Math.multiplyExact to fail fast on absurd customer overrides (>= ~33M) rather than silently wrap to a negative int and explode the MPSC queue allocation with a confusing downstream error. Per sarahchen6's suggestion citing the codex bot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Clamp TRACER_METRICS_MAX_PENDING instead of throwing on overflow The previous Math.multiplyExact approach would fail the agent startup with ArithmeticException on absurd customer overrides (>= ~33M for the configured value). Clamping is gentler -- the agent starts successfully and just runs with a capped inbox. Long-promote the multiplication to a long so the product can't wrap, then clamp to MAX_SAFE_ARRAY_SIZE (Integer.MAX_VALUE - 8, the JDK's own SOFT_MAX_ARRAY_LENGTH convention for array allocations). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Fix duplicate-entry bug for null-fielded SpanSnapshots The constructor canonicalizes null fields through canonicalize() which returns UTF8BytesString.EMPTY for null inputs (or a cached UTF8BytesString("") for empty-string inputs). But matches() compared those entries against subsequent snapshots via contentEquals(...) / stringContentEquals(...), which treated non-null UTF8BytesString vs null CharSequence as inequal. Result: two snapshots with the same null-valued resource/operation/ type/serviceSource hashed to the same bucket (intHash(null) == 0 == "".hashCode()), but matches() returned false on the EMPTY-vs-null field comparison, so the second snapshot inserted a *duplicate* entry into the table. Same path for empty-string vs null. Unify the semantics: null and length-zero are treated as equivalent on either side of contentEquals/stringContentEquals. The hash already agreed (intHash(null) == "".hashCode() == 0), so this restores the matches() contract to match the existing hash contract. Adds AggregateTableTest.nullAndEmptyOptionalFieldsCollapseToOneEntry to pin the contract: two null-fielded and one empty-string-fielded snapshot must all hit the same entry. Test would have failed before the fix (a duplicate insert) but the existing 10 cases still pass. Resolves sarahchen6's review comment on AggregateEntry.java:113 and amarziali's related concern on AggregateEntry.java:114. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Clear dirty flag in ClearSignal handler After CLEAR runs the table is empty but dirty would still carry over from any prior SpanSnapshot insert. The next report() would see dirty=true, expunge no-op the empty table, find isEmpty(), and log "skipped metrics reporting because no points have changed" -- same observable outcome, but resetting dirty here keeps the invariant "dirty implies there's data to flush" honest. Resolves amarziali's review comment on Aggregator.java:121. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'master' into dougqh/conflating-metrics-background-work Drop conditional null-skip from peer-tag hashing Previously hashOf wrapped the peer-tag contribution in `if (s.peerTagSchema != null && s.peerTagValues != null)`. That meant two snapshots with different null arrangements (schema-null vs values-null) collapsed to the same hash, getting resolved only by the field-by-field matches() fallback at the bucket walk -- wasteful, and the asymmetry hurt hash quality generally. Replace with unconditional contributions: - PeerTagSchema now overrides hashCode() to be content-based on names (lazy + cached, benign-race pattern matching UTF8BytesString / utf8Bytes elsewhere). addToHash(h, schema) routes through that. - For the String[] values, pass Arrays.hashCode(values) through the int overload -- Object[].hashCode() is identity-based by default, so we have to compute content hash explicitly. Null arrays hash to 0 via Arrays.hashCode's contract. Null inputs on either side now hash to 0 distinctly from any real schema or non-empty values array, so all four null combinations are distinguishable. Same final hash for content-equal inputs across schema replacements (the reconcile path), which preserves the entry- hit invariant after the aggregator rebuilds the schema. Resolves amarziali's review comment on AggregateEntry.java:309 and dougqh's suggestion on AggregateEntry.java:310. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Delete dead Aggregator.clearAggregates() Once the ClearSignal routing replaced the direct disable()-to-table mutation, clearAggregates() lost all its call sites -- no production code, no test code. Worse, leaving it public invited future callers to bypass the ClearSignal contract and race against Drainer.accept on the aggregator thread. Drop the method outright. Update the inline comment in ConflatingMetricsAggregator.disable() to not name the deleted method. Resolves amarziali's review comment on Aggregator.java:82. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Cursor-resume eviction in AggregateTable via half-open MutatingTableIterator Previously AggregateTable.evictOneStale walked the bucket array from bucket 0 on every call. Under sustained cap pressure with mostly-hot entries clustered in low buckets, every eviction re-scanned the same hot prefix before finding a cold entry. amarziali's review concern. Add a cursor: after a successful eviction, remember the bucket where it landed. The next call resumes from there. Worst case for a single call is still O(N) when nearly every entry is hot, but a sustained eviction stream amortizes to O(1) per call -- the hot prefix is never re-scanned more than twice across N evictions. Implemented as two iterators driving [cursor, length) then [0, cursor), which required a small Hashtable.Support API addition: - New `mutatingTableIterator(buckets, startBucket, endBucket)` overload for walking a half-open bucket range. The existing zero-arg overload is kept; it now delegates to the new ctor with [0, buckets.length). - New `MutatingTableIterator.currentBucket()` accessor exposing the bucket index of the entry last returned by next() (or -1 before any next/after a remove). AggregateTable saves this as the new cursor. - The empty-range case (startBucket == endBucket) yields an immediately-exhausted iterator -- this is what makes the wrap-around pass [0, cursor) naturally produce nothing when cursor == 0, so the two-pass driver in evictOneStale needs no special case. Tests: - 4 new HashtableTest cases covering the half-open API, empty ranges, out-of-range bounds, and currentBucket() behavior before/after next. - 2 new AggregateTableTest cases: backToBackEvictionsAllSucceed (drives 3x capacity worth of cap-overrun inserts; each must succeed, which only holds if the cursor advances correctly) and clearResetsCursorForSubsequentEvictions (clear() also resets the cursor so subsequent eviction passes start from bucket 0). Resolves amarziali's review comment on AggregateTable.java:75. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Move AggregateEntry.of() test factory out of production class dougqh's review comment on AggregateEntry.java:153 asked to keep test code out of the production class. Move the factory to a new AggregateEntries helper in src/test/java/datadog/trace/common/metrics. Same package so it can call the package-private forSnapshot(); delegating to forSnapshot also means no need to widen the AggregateEntry constructor visibility. The 37 src/test/groovy call sites get a mechanical rewrite of AggregateEntry.of(...) -> AggregateEntries.of(...) (36 in ConflatingMetricAggregatorTest, 1 in SerializingMetricWriterTest). src/traceAgentTest is a separate source set without compile-time visibility into src/test, so its 2 MetricsIntegrationTest.groovy call sites can't use AggregateEntries. Migrated those to construct a SpanSnapshot inline + call AggregateEntry.forSnapshot(snapshot). Groovy's permissive package-private access makes this work from the default package the integration test currently sits in. Resolves dougqh's review comment on AggregateEntry.java:153. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Fix AggregateEntry equals/hashCode contract violation equals compared the pre-encoded peerTags List<UTF8BytesString> while hashCode (via hashOf) mixes in the raw peerTagSchema + values arrays. Two entries built from different schema layouts can collapse to the same encoded form -- e.g. tag "b" at index 1 in schema {a,b} with values {null,"x"} produces the same encoded ["b:x"] as schema {b,c} with values {"x",null}. equals returned true; hashCodes differed. Hashcode contract violated. Switch equals to compare the raw peerTagNames + peerTagValues arrays, mirroring matches(SpanSnapshot) and hashOf(SpanSnapshot). The production lookup path (AggregateTable.findOrInsert) already uses those, so this just brings equals in line with the rest of the class. Adds two regression tests on AggregateEntryTest: - equalsConsistentWithHashCodeAcrossDifferentSchemaLayouts: the failing-case shape above. Pre-fix, the encoded-list equals returned true while hashCodes differed; now equals returns false and the hashCodes differ in agreement. - equalEntriesHaveEqualHashCodes: positive case -- two entries from identical snapshots must equal and share hashCode. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Don't trample queued STOP in ClearSignal handler Prior CLEAR handler called inbox.clear() as belt-and-suspenders cleanup of in-flight snapshots. That would also erase any STOP signal queued behind CLEAR -- a real concern in disable() -> close() sequences, where the trampled STOP leaves the aggregator thread spinning until thread.join's timeout. sarahchen6 surfaced this from a Codex pass on the CLEAR logic; dougqh confirmed it's worth fixing. The CLEAR handler now clears only the aggregates table. Queued snapshots will drain naturally into the just-cleared table -- but since features.supportsMetrics() is already false by the time CLEAR was offered, producers have stopped publishing; the inbox drains and empties on its own. Worst case: one extra reporting cycle of wasted work on stale snapshots that the agent rejects, which triggers another DOWNGRADED -> disable() -> CLEAR. Self-healing, same as before. Adds ConflatingMetricsAggregatorDisableTest.clearDoesNotTrampleQueuedStopSignal: publish a snapshot, fire DOWNGRADED, call close(); the test bounds close() with its own 2s timeout and asserts the thread exits within it. Pre-fix this would have hung out THREAD_JOIN_TIMEOUT_MS; post-fix it returns in milliseconds. Resolves sarahchen6/Codex's CLEAR-trampling-STOP review comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Implement PeerTagSchema.equals symmetric with hashCode The prior commit added a content-based hashCode() but left equals falling back to Object.equals (reference identity). That violates the hashCode contract for any caller that compares two distinct schema instances built from the same tag list -- e.g. before/after a reconcile rebuilds the cached schema with an unchanged tag set. equals() now mirrors hashCode(): content-equal on names. The reconcile- timing field lastTimeDiscovered is intentionally excluded from both -- it's bookkeeping for the aggregator's discovery-version compare, not part of schema identity. Tests: - equalsIsContentBasedOnNames -- same names, two instances, equal + matching hashCode. - equalsIgnoresLastTimeDiscovered -- pins that the bookkeeping field doesn't leak into identity. - equalsDistinguishesByOrder -- names is positional (pairs with SpanSnapshot.peerTagValues by index), so reordered schemas are not interchangeable. - equalsHandlesNullAndOtherTypes -- contract corners. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Route service and spanKind through canonicalize for null-safety AggregateEntry's constructor canonicalized resource, operationName, type, and serviceSource (mapping null -> EMPTY via the canonicalize helper) but called SERVICE_CACHE.computeIfAbsent / SPAN_KIND_CACHE .computeIfAbsent directly for service and spanKind. Inputs of null would NPE on the cache call. Production paths never pass null for these -- DDSpan always supplies a service, and the producer defaults spanKind to "" via unsafeGetTag(SPAN_KIND, (CharSequence) "") -- so this is a latent- defense fix, not a live bug. But the matches/contentEquals logic already treats null and length-zero as equal on both sides, and every other label field in the constructor defends via canonicalize. Two unprotected outliers are an inconsistency that bites the next person who reaches for a new code path. Drops the Functions.UTF8_ENCODE import (its sole use was the service cache line) -- canonicalize internally creates the UTF8BytesString. Test: AggregateTableTest.nullServiceAndSpanKindDoNotNpeAndCollapseWithEmpty publishes (null, null), (null, null) again, and ("", ""); asserts a single entry results and that getService()/getSpanKind() are length-0. Without the fix, the first publish would have NPE'd at the .computeIfAbsent call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Suppress forbiddenApis for tearDown's System.err diagnostics AdversarialMetricsBenchmark.tearDown prints drop counters via System.err so a benchmark run shows how saturated each capacity bound was (inbox-full drops, aggregate-cache drops). forbiddenApisJmh disallows System.err by default to prevent excess logging in production code -- not a concern for a JMH benchmark, where stderr is the conventional channel for diagnostic output and matches the existing pattern in ExtractorBenchmark / InjectorBenchmark. Annotates tearDown with @SuppressForbidden (method-scoped, not class- scoped) so the suppression is narrowly targeted to the three println calls and any future hot-path code that lands in the benchmark stays gated by the check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'master' into dougqh/conflating-metrics-background-work Use DDAgentFeaturesDiscovery.state() hash for PeerTagSchema reconcile Addresses amarziali's review on getLastTimeDiscovered(): the existing state() accessor returns a SHA-256 of the discovery response, which is a more precise change key than the timestamp. Timestamp advances on every successful refresh regardless of content; the hash only advances when something actually changed -- so reconcile fast-path now fires only on real change, not every cycle. - PeerTagSchema: long lastTimeDiscovered -> String state. Factory signature of(Set, long) -> of(Set, String). INTERNAL carries null (it is never reconciled). - ConflatingMetricsAggregator: read features.state() first then peerTags() (same defensive ordering rationale -- if a discovery refresh interleaves, leave the schema with stale state rather than stale tags so the next reconcile re-runs the deep compare). Objects.equals for null-tolerant comparison (state can be null before discovery has produced a response). - DDAgentFeaturesDiscovery: drop the public getLastTimeDiscovered() accessor added on this branch -- the field stays private for the existing throttling logic in discoverIfOutdated(). - Tests updated to mock state() instead of getLastTimeDiscovered(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Convert TRACER_METRICS_MAX_PENDING rationale to /* */ block comment Addresses amarziali's readability nit (#3289149416) -- multi-line prose reads better as a single block comment than as a stack of // lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge remote-tracking branch 'origin/dougqh/conflating-metrics-background-work' into dougqh/conflating-metrics-background-work Add cardinality-isolation companions to AdversarialMetricsBenchmark Two new JMH benches that hold every dimension constant except one, to attribute throughput deltas to a specific axis: - HighCardinalityResourceMetricsBenchmark: ~1M distinct resource values; service/operation/peer.hostname pinned. Exercises the aggregate-cache LRU on the resource axis specifically. - HighCardinalityPeerMetricsBenchmark: ~32K distinct peer.hostname values; service/operation/resource pinned. Isolates the peer-tag encoding hot path (PEER_TAGS_CACHE lookups, UTF8 encoding, parallel-array capture in SpanSnapshot). Same shape as AdversarialMetricsBenchmark (8 threads, 2x15s warmup + 5x15s measurement, 1 fork) and reuse its CountingHealthMetrics so the inbox-full vs aggregate-dropped counters print on teardown for an apples-to-apples comparison. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge remote-tracking branch 'origin/dougqh/conflating-metrics-background-work' into dougqh/optimize-metric-key # Conflicts: # dd-trace-core/src/main/java/datadog/trace/common/metrics/PeerTagSchema.java # dd-trace-core/src/test/java/datadog/trace/common/metrics/PeerTagSchemaTest.java Merge remote-tracking branch 'origin/dougqh/optimize-metric-key' into dougqh/control-tag-cardinality # Conflicts: # dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java # dd-trace-core/src/main/java/datadog/trace/common/metrics/ClientStatsAggregator.java # dd-trace-core/src/main/java/datadog/trace/common/metrics/PeerTagSchema.java # dd-trace-core/src/test/groovy/datadog/trace/common/metrics/ClientStatsAggregatorTest.groovy # dd-trace-core/src/test/java/datadog/trace/common/metrics/AggregateEntryTest.java # dd-trace-core/src/test/java/datadog/trace/common/metrics/PeerTagSchemaTest.java Reflow reconcilePeerTagSchema Javadoc after merge Spotless tidy missed in commit b382df5e92. No semantic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Consolidate contentEquals; remove redundant stringContentEquals Addresses sarahchen6 review: - AggregateEntry.java:380 — early-return on null-or-empty `a`, then check `b` once, dropping the two split null branches and the duplicate String/UTF8BytesString instanceof checks. - AggregateEntry.java:398 — String is a CharSequence, so the general contentEquals already handles both. Migrate the five service / spanKind / httpMethod / httpEndpoint / grpcStatusCode call sites in matches() and delete the helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Rename AggregateEntries -> AggregateEntryFixtures Addresses sarahchen6 review on AggregateEntries.java:13: the prior name reads too close to the production AggregateEntry class. Pick a more test-flavored name. Touches the file itself + the 8 callers across ConflatingMetricAggregatorTest and SerializingMetricWriterTest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Tidy PR-iteration history out of test comments Addresses sarahchen6 review on AggregateTableTest:237 and ConflatingMetricsAggregatorDisableTest:143: comments narrated the prior- behavior-and-fix path that led to each test, but the test itself is self-evident -- a future reader only needs the expected behavior. Keep the behavior summary, drop the "Regression:" / "prior CLEAR handler ..." flavor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge remote-tracking branch 'origin/dougqh/optimize-metric-key' into dougqh/control-tag-cardinality # Conflicts: # dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java # dd-trace-core/src/test/groovy/datadog/trace/common/metrics/ClientStatsAggregatorTest.groovy Merge remote-tracking branch 'origin/master' into dougqh/optimize-metric-key # Conflicts: # dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateMetric.java # dd-trace-core/src/main/java/datadog/trace/common/metrics/Aggregator.java # dd-trace-core/src/main/java/datadog/trace/common/metrics/ConflatingMetricsAggregator.java # dd-trace-core/src/main/java/datadog/trace/common/metrics/PeerTagSchema.java # dd-trace-core/src/main/java/datadog/trace/common/metrics/SpanSnapshot.java # dd-trace-core/src/test/groovy/datadog/trace/common/metrics/AggregateMetricTest.groovy # dd-trace-core/src/test/groovy/datadog/trace/common/metrics/ConflatingMetricAggregatorTest.groovy # dd-trace-core/src/test/java/datadog/trace/common/metrics/ConflatingMetricsAggregatorBootstrapTest.java # dd-trace-core/src/test/java/datadog/trace/common/metrics/PeerTagSchemaTest.java Merge remote-tracking branch 'origin/dougqh/optimize-metric-key' into dougqh/control-tag-cardinality Merge remote-tracking branch 'origin/master' into dougqh/optimize-metric-key Merge remote-tracking branch 'origin/dougqh/optimize-metric-key' into dougqh/control-tag-cardinality Make AggregateEntry.ERROR_TAG / TOP_LEVEL_TAG package-private The class itself is package-private, so the public modifier on these constants is meaningless and misleads about the actual access surface. All six call sites (ConflatingMetricsAggregator + tests) are in the same package and continue to compile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge remote-tracking branch 'origin/dougqh/optimize-metric-key' into dougqh/control-tag-cardinality Move AggregateEntry equality contract to test-only helper Eliminates the dual-equality-contract maintenance hazard on AggregateEntry. Production code never invoked equals/hashCode -- AggregateTable bucketing goes through keyHash + matches(SpanSnapshot) directly. The contract existed only to support Spock mock argument matchers in tests. - Delete equals/hashCode from production AggregateEntry; class stays final. - Make peerTagNames/peerTagValues fields package-private so a sibling helper in the same package can read them. - Add src/test AggregateEntryTestUtils.equals/hashCode that implements the same field-wise contract (raw-array based, consistent with hashOf) for tests. - Update Spock argument matchers from `writer.add(fixture)` to `writer.add({ AggregateEntryTestUtils.equals(it, fixture) })`. For loop-driven expectations, hoist the fixture into a per-iteration `def expected = ...` local so it's captured by value rather than by reference to the loop variable. - Update the JUnit contract tests to drive the helper directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge remote-tracking branch 'origin/dougqh/optimize-metric-key' into dougqh/control-tag-cardinality # Conflicts: # dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java # dd-trace-core/src/test/groovy/datadog/trace/common/metrics/ClientStatsAggregatorTest.groovy # dd-trace-core/src/test/java/datadog/trace/common/metrics/AggregateEntryTest.java Move AggregateEntry equality contract to test-only helper Mirrors the #11382 cleanup. Production AggregateEntry never invokes equals/hashCode -- AggregateTable bucketing goes through keyHash + Canonical.matches directly. The contract existed only to support Spock mock argument matchers. - Delete equals/hashCode from production AggregateEntry; class stays final. - Add src/test AggregateEntryTestUtils.equals/hashCode that implements the same field-wise contract (peerTags compared as an encoded list, consistent with hashOf on this branch). - Update Spock argument matchers from `writer.add(AggregateEntry.of(...))` to `writer.add({ AggregateEntryTestUtils.equals(it, AggregateEntry.of(...)) })`. - For loop-driven expectations, hoist the fixture into a per-iteration `def expected = ...` local so it's captured by value rather than by reference to the loop variable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Consolidate AggregateEntryFixtures into AggregateEntryTestUtils Both classes existed only to support tests against AggregateEntry -- one for positional-args fixture construction, the other for value- based equality matching. The split was artificial; folding them into a single AggregateEntryTestUtils removes a file and gives test sites one place to look for AggregateEntry test helpers. - Move `of(...)` into AggregateEntryTestUtils alongside the existing `equals(a, b)` / `hashCode(e)` helpers. - Delete AggregateEntryFixtures.java. - Rename 51 caller sites across ConflatingMetricAggregatorTest and SerializingMetricWriterTest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge remote-tracking branch 'origin/dougqh/optimize-metric-key' into dougqh/control-tag-cardinality # Conflicts: # dd-trace-core/src/test/groovy/datadog/trace/common/metrics/ClientStatsAggregatorTest.groovy # dd-trace-core/src/test/java/datadog/trace/common/metrics/AggregateEntryFixtures.java # dd-trace-core/src/test/java/datadog/trace/common/metrics/AggregateEntryTestUtils.java Document deliberate cohesion + single-writer invariant on AggregateEntry Two doc-only additions surfacing design context that reviewers would otherwise have to reconstruct: - AggregateEntry: name the "5 responsibilities concentrated on one object" tradeoff explicitly (UTF8 caches + label fields + raw peerTag arrays + encoded peerTag list + counter/histogram state). Prior MetricKey + AggregateMetric design allocated two objects per unique key on miss; folding them yields one. The class is wider as a result; that's the trade we chose. - AggregateEntry + AggregateTable: note that the single-writer invariant is convention-enforced -- the @SuppressFBWarnings documents the assumption but nothing checks the calling thread at runtime. Point to ClearSignal as the explicit mechanism for funneling cross-thread mutators back onto the aggregator thread. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge remote-tracking branch 'origin/dougqh/optimize-metric-key' into dougqh/control-tag-cardinality # Conflicts: # dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java Avoid recomputing keyHash on AggregateTable miss On the miss path, AggregateTable.findOrInsert computed the snapshot hash for the lookup, then AggregateEntry.forSnapshot computed it again via the same hashOf(s) call to set keyHash on the new entry. Three reads per snapshot field on a miss (findOrInsert hashOf + forSnapshot hashOf + constructor canonicalize), with two of those also paying for the per-call Arrays.hashCode(peerTagValues). Pass the hash that findOrInsert already computed into forSnapshot instead. Two reads per field on miss, one Arrays.hashCode(peerTagValues) per miss. Kept a no-arg forSnapshot overload for test callers that don't have a precomputed hash on hand. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Tighten client-stats cardinality plumbing - Drop unused PeerTagSchema.hashCode/equals + cachedHashCode field; the schema is never compared via Object.equals or used as a Set/Map key. hasSameTagsAs(Set<String>) is the only schema-equivalence primitive in use, and it's a name-content check that doesn't go through hashCode. - Canonical.populatePeerTags now skips null values directly instead of round-tripping them through handler.register only to filter EMPTY back out. Cheap win on sparse-null peer-tag arrays from capturePeerTagValues. - Canonical.matches reordered to compare highest-cardinality fields (resource, service, operationName) first for faster short-circuit on bucket-chain collisions; UTF8 fields use direct .equals (the EMPTY-sentinel invariant guarantees non-null on both sides) instead of Objects.equals + dead null branches. - PropertyCardinalityHandler/TagCardinalityHandler.register compute the mixed hash once and reuse the start index for both the current-cycle and prior-cycle probe paths. The probe helper is inlined since it's no longer shared. - ClientStatsAggregator.reconcilePeerTagSchema now flushes the outgoing schema's accumulated blockedCounts via resetCardinalityHandlers() before discarding it on a tag-set change, otherwise partial-cycle block telemetry would silently disappear. - Document the "one ClientStatsAggregator per JVM" invariant implied by AggregateEntry's static cardinality handlers and PeerTagSchema.INTERNAL, plus the load-bearing size guard in TagCardinalityHandler.isBlockedResult that prevents accidental sentinel materialization. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Document AggregateEntry.clear key-field persistence + SignalItem singleton contract AggregateEntry.clear(): note that only per-cycle counters/histograms reset; the label fields (resource, service, ..., peerTagNames, peerTagValues) are the entry's bucket identity and persist across cycles so subsequent same-key snapshots reuse the entry. Stale entries get reaped by AggregateTable.expungeStaleAggregates. SignalItem: document the singleton fire-and-forget contract -- the inherited CompletableFuture is completed on first handling and never reset, so callers that want one-shot completion semantics (e.g. forceReport) must allocate a fresh instance instead of reusing the STOP/REPORT/CLEAR singletons. Pre-existing pattern on master (this PR added the CLEAR singleton following the same convention); doc just makes the contract explicit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'dougqh/optimize-metric-key' into dougqh/control-tag-cardinality # Conflicts: # dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java # dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java Encapsulate EMPTY-as-absent sentinel + align hashOf field order with matches Optional fields on AggregateEntry (serviceSource, httpMethod, httpEndpoint, grpcStatusCode) carry UTF8BytesString.EMPTY when the snapshot had no value. SerializingMetricWriter (and its test) previously read this contract via reference-eq against EMPTY, leaking the storage choice into callers. Add hasServiceSource / hasHttpMethod / hasHttpEndpoint / hasGrpcStatusCode predicates on AggregateEntry; switch the four serializer sites and the mirroring four test sites to call them. The EMPTY-sentinel is now an internal implementation detail of AggregateEntry. Reorder AggregateEntry.hashOf so its field order mirrors Canonical.matches (UTF8 fields first, then peer-tag list, then primitives). The hash value itself is order-stable across all callers; this is purely so future readers can reason about lookup and equality in lockstep. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Preserve warm TagCardinalityHandlers across peer-tag schema rebuilds When peer-tag discovery returns a different tag set than the cached schema carries, ClientStatsAggregator.reconcilePeerTagSchema replaces the schema. Previously every per-tag TagCardinalityHandler was rebuilt from scratch, losing the prior-cycle UTF8 cache for tags that survived the rebuild -- so a persistent tag like peer.hostname re-allocated UTF8BytesStrings for every value on the cycle following the rebuild. Split PeerTagSchema.resetCardinalityHandlers into two operations: - resetCardinalityHandlers: full rotate (called at end-of-cycle on the cached schema). - flushBlockedCounts: telemetry-only flush (used by reconcile on the outgoing schema before discard, so partial-cycle counts still reach HealthMetrics without disturbing the handlers). Add a donor overload PeerTagSchema.of(names, state, healthMetrics, previous) that transfers TagCardinalityHandler instances by name for any tag present in both the previous and replacement schemas. Names absent from the previous schema get fresh handlers; names absent from the replacement schema are dropped along with the outgoing schema. reconcilePeerTagSchema now calls flushBlockedCounts then constructs the replacement via the donor overload. The end-of-cycle reset that runs immediately after reconcile rotates the (now-transferred) handlers in the normal way, so cycle N+1 sees cycle N's UTF8 values as priorValues for persisting tags. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Revert "Preserve warm TagCardinalityHandlers across peer-tag schema rebuilds" This reverts commit 703c9e11639d0a7fd5a2d274d4812faf73c66767. Add trace.stats.cardinality.limits.enabled flag (default off) Cardinality limiting alters the wire format under high cardinality -- overflow values get the "blocked_by_tracer" sentinel and collapse into one aggregate bucket. Customers with dashboards or alerts keyed on specific tag values would see the sentinel value appear unexpectedly if their workload exceeds the per-field budgets. Put the substitution behavior behind a config flag so the rollout is opt-in. With the flag off (the new default), the per-field handlers still act as bounded UTF8 reuse caches sized by their cardinality budget, but over-cap values get a freshly-allocated UTF8BytesString instead of the sentinel and flow to distinct aggregate buckets. The wire format is unchanged from master for any workload. With the flag on, current behavior (sentinel substitution + bucket collapse). Handler refactor: - PropertyCardinalityHandler / TagCardinalityHandler take a useBlockedSentinel constructor arg. On cap-exhaust the cache no longer claims a slot (since it's full), but prior-cycle reuse still runs so repeat over-cap values pay only the probe, not the allocation. - TagCardinalityHandler.isBlockedResult now reads cacheBlocked directly rather than calling blockedByTracer(), so query-time never forces the sentinel to materialize when limits are disabled. - Test-convenience single-arg constructors default useBlockedSentinel to true so existing tests of the limits-enabled mode don't churn; new tests cover the disabled mode. Wiring: - AggregateEntry exposes static final LIMITS_ENABLED read from Config.get() at class init, threaded through all PropertyCardinality Handlers and the PeerTagSchema-built TagCardinalityHandlers. Config: - GeneralConfig.TRACE_STATS_CARDINALITY_LIMITS_ENABLED (default false) - Config.isTraceStatsCardinalityLimitsEnabled() - Registered in metadata/supported-configurations.json Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Doc the regime shift after cardinality-limits flag landed Three small doc additions calling out behavior that's correct in code but easy to miss on a cold read: - AggregateTable.evictOneStale: explain that this is no longer just a pathological-case backstop. With LIMITS_ENABLED=false (the new default), over-cap values flow to distinct buckets and maxAggregates becomes the load-bearing cardinality enforcement -- the cursor- resumed scan was added for this regime. - AggregateEntry.LIMITS_ENABLED: document the over-cap repeat tradeoff in disabled mode (over-cap values can't promote into the current cache so repeats re-allocate) and the class-init caveat (static final read of Config, frozen for the JVM at first class load -- tests needing to exercise the limits-on path through the static handlers must construct handlers directly with explicit useBlockedSentinel args). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Drop useless @SuppressFBWarnings on AggregateEntry spotbugs now flags three suppression annotations as unnecessary: - Class-level AT_NONATOMIC_OPERATIONS_ON_SHARED_VARIABLE + AT_STALE_THREAD_WRITE_OF_PRIMITIVE — the int counter fields are no longer mutated cross-thread now that producer threads only enqueue SpanSnapshots and the aggregator thread is the sole writer. - clear() AT_NONATOMIC_64BIT_PRIMITIVE on the duration field — same reason; the long write is single-threaded. The class Javadoc already documents the single-writer invariant, so removing the annotations doesn't lose any documentation; the prose paragraph that referenced "the SuppressFBWarnings below" is updated in place. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Update stale Javadoc on AggregateEntry's no-equals contract The "use the TestAggregateEntry subclass in src/test" reference pointed to a subclass that was replaced earlier in the stack by the AggregateEntryTestUtils helper class. Test-side value-equality is now a helper, not a subclass; AggregateEntry stayed final. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Tighten AggregateEntry / PeerTagSchema surface area 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) <noreply@anthropic.com> Tighten AggregateEntry surface — drop one-line factory, doc the conventions Five small cleanups sur…

dougqh added type: feature Enhancements and improvements tag: performance Performance related changes tag: no release notes Changes to exclude from release notes comp: metrics Metrics tag: ai generated Largely based on code generated by an AI or LLM labels May 15, 2026

dougqh commented May 15, 2026

View reviewed changes

Comment thread dd-trace-core/src/main/java/datadog/trace/common/metrics/PeerTagSchema.java Outdated

dougqh mentioned this pull request May 16, 2026

Memory-efficiency pass on ClientStatsAggregator + adversarial benchmark #11389

Closed

5 tasks

dougqh force-pushed the dougqh/optimize-metric-key branch from 050a998 to 3738c85 Compare May 18, 2026 19:20

dougqh and others added 2 commits May 18, 2026 15:24

dougqh force-pushed the dougqh/control-tag-cardinality branch from 8020ec4 to 1221b2b Compare May 18, 2026 19:25

dougqh and others added 19 commits May 18, 2026 15:40

Apply spotless formatting to Hashtable and LongHashingUtils

031dc89

Bring the new util/ files in line with google-java-format (tabs → spaces, line wrapping, javadoc list markup) so spotlessCheck passes in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Add benchmark results to HashtableBenchmark header

a534e4f

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Merge branch 'dougqh/util-hashtable' into dougqh/optimize-metric-key

7ab47f7

Merge branch 'dougqh/util-hashtable' into dougqh/optimize-metric-key

46f147a

Drop d1_/d2_ prefix from per-table benchmark methods

df7f98f

Holdover from when both lived in a shared HashtableBenchmark; redundant now that each lives in its own class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

bric3 approved these changes Jun 29, 2026

View reviewed changes

dougqh and others added 4 commits June 29, 2026 16:00

Merge branch 'master' into dougqh/control-tag-cardinality

3c6ca3e

Restore dual-role and prior-cycle design paragraphs to PropertyCardin…

05a5ef7

…alityHandler Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dougqh and others added 3 commits June 30, 2026 13:12

Add missing Tags import to CoreSpan after merge

9f5e1f7

The getSpanKindString() default method references Tags.SPAN_KIND but the import was not present after the merge conflict resolution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Apply spotless formatting

825d5e2

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dougqh enabled auto-merge June 30, 2026 17:30

dougqh and others added 9 commits June 30, 2026 14:40

Merge branch 'master' into dougqh/control-tag-cardinality

10a8c32

Merge branch 'master' into dougqh/control-tag-cardinality

b997f77

Merge branch 'master' into dougqh/control-tag-cardinality

bfe993d

Update dd-trace-core/src/main/java/datadog/trace/common/metrics/Prope…

4dbcb95

…rtyCardinalityHandler.java Co-authored-by: Brice Dutheil <brice.dutheil@gmail.com>

Merge branch 'master' into dougqh/control-tag-cardinality

7c82574

Merge branch 'master' into dougqh/control-tag-cardinality

bccb6fa

dougqh added this pull request to the merge queue Jul 1, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 1, 2026

gh-worker-dd-mergequeue-cf854d Bot merged commit b1b6910 into master Jul 1, 2026
583 checks passed

gh-worker-dd-mergequeue-cf854d Bot deleted the dougqh/control-tag-cardinality branch July 1, 2026 05:12

github-actions Bot added this to the 1.64.0 milestone Jul 1, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Per-component / tag cardinality limits in client-side stats - #11387

Per-component / tag cardinality limits in client-side stats#11387
gh-worker-dd-mergequeue-cf854d[bot] merged 269 commits into
masterfrom
dougqh/control-tag-cardinality

dougqh commented May 15, 2026 •

edited

Loading

Uh oh!

Uh oh!

dougqh commented Jun 29, 2026

Uh oh!

bric3 left a comment •

edited

Loading

Uh oh!

bric3 Jun 29, 2026

Uh oh!

dougqh Jun 29, 2026

Uh oh!

dougqh Jun 30, 2026

Uh oh!

Uh oh!

Uh oh!

dd-octo-sts Bot commented Jul 1, 2026

Uh oh!

gh-worker-devflow-routing-ef8351 Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

dougqh commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Does This Do

Motivation

Additional Notes

Claude's Summary

Benchmark

ClientStatsAggregatorDDSpanBenchmark — producer publish() latency

Aggregator bench suite vs v1.62.0 + master + #11382

Test plan

Uh oh!

Uh oh!

dougqh commented Jun 29, 2026

Uh oh!

bric3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bric3 Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

dougqh Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

dougqh Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dd-octo-sts Bot commented Jul 1, 2026

Uh oh!

gh-worker-devflow-routing-ef8351 Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dougqh commented May 15, 2026 •

edited

Loading

`ClientStatsAggregatorDDSpanBenchmark` — producer publish() latency

bric3 left a comment •

edited

Loading

gh-worker-devflow-routing-ef8351 Bot commented Jul 1, 2026 •

edited

Loading