Update client-side stats to use light weight Hashtable#11382
Open
dougqh wants to merge 143 commits into
Open
Conversation
ConflatingMetricsAggregator.publish does a handful of redundant operations on every span. None individually is large; together they show as ~2.5% on the existing JMH benchmark once the benchmark actually exercises span.kind. - dedup span.isTopLevel(): publish() reads it into a local, then shouldComputeMetric read it again. Pass the cached value in. - resolve spanKind to String once: master called toString() twice per span (once inside spanKindEligible, once at the getPeerTags call site) and used HashSet contains on a CharSequence (which routes through equals on String). Normalize to String up front and reuse. - lazy-allocate the peer-tag list: getPeerTags() always allocated an ArrayList sized to features.peerTags() even when the span had none of those tags set. Defer allocation until the first match; return Collections.emptyList() when none hit. MetricKey already treats null/empty peerTags as emptyList, so no behavior change. Drop the spanKindEligible helper — the HashSet.contains call inlines fine in shouldComputeMetric. Update the JMH benchmark to set span.kind=client on every span. Without it the filter path short-circuits before the peer-tag and toString work, so the wins above aren't measurable. With it: baseline 6.755 us/op (CI [6.560, 6.950], stdev 0.129) optimized 6.585 us/op (CI [6.536, 6.634], stdev 0.033) 2 forks x 5 iterations x 15s. ~2.5% mean improvement and much tighter variance fork-to-fork. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce SpanKindFilter -- a tiny builder-built immutable filter whose state is an int bitmask indexed by the span.kind ordinals already cached on DDSpanContext. Each include* on the builder sets one bit (1 << ordinal); the runtime check is a single AND against (1 << span's ordinal). CoreSpan.isKind(SpanKindFilter) is the new entry point. DDSpan overrides it to do the bit-test directly against the cached ordinal -- no virtual call, no tag-map lookup. The two existing test-only CoreSpan impls (SimpleSpan and TraceGenerator.PojoSpan, the latter in two source sets) implement isKind by reading the span.kind tag and delegating to SpanKindFilter.matches(String), which converts via DDSpanContext.spanKindOrdinalOf and does the same AND. Refactor: DDSpanContext.setSpanKindOrdinal(String) now delegates to a new package-private static spanKindOrdinalOf(String) so the same string-to-ordinal mapping serves both the tag interceptor path and SpanKindFilter.matches. This is groundwork -- nothing in the codebase calls isKind yet. The next commit will replace the HashSet-based eligibility checks in ConflatingMetricsAggregator with SpanKindFilter instances. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the two ELIGIBLE_SPAN_KINDS_FOR_* HashSet<String> constants and the SPAN_KIND_INTERNAL.equals check with three SpanKindFilter instances: METRICS_ELIGIBLE_KINDS, PEER_AGGREGATION_KINDS, INTERNAL_KIND. Eligibility checks now go through span.isKind(filter), which on DDSpan is a volatile byte read against the already-cached span.kind ordinal plus a single bit-test. Also defer the span.kind tag read: previously read at the top of the publish loop and threaded through both shouldComputeMetric and the inner publish. isKind no longer needs the string, so the read can move down into the inner publish where it's still needed for the SPAN_KINDS cache key / MetricKey. Supporting changes: - DDSpanContext.spanKindOrdinalOf(String) is now public so non-DDSpan CoreSpan impls can compute the ordinal at tag-write time. - SpanKindFilter gains a public matches(byte) fast-path overload that callers with a pre-computed ordinal use directly. - SimpleSpan caches the ordinal in setTag(SPAN_KIND, ...), mirroring what TagInterceptor does for DDSpanContext, and its isKind now hits the byte fast path. Without this, the JMH benchmark (which uses SimpleSpan) would re-derive the ordinal on every isKind call and overstate the cost. Benchmark on the bench updated last commit (kind=client on every span, 4 forks x 5 iter x 15s): prior commit 6.585 ± 0.049 us/op this commit 6.903 ± 0.096 us/op The slight regression is a SimpleSpan-via-groovy-dispatch artifact -- the interface call to isKind through CoreSpan, then through SimpleSpan, then through SpanKindFilter.matches, doesn't fold as aggressively as a HashSet contains on a static field. In production DDSpan.isKind inlines to a context field read + ordinal byte read + bit-test, so the production path is faster than the prior HashSet approach. A DDSpan-based benchmark would show this; the existing SimpleSpan-based one doesn't. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The existing ConflatingMetricsAggregatorBenchmark uses SimpleSpan, a groovy mock. That's enough for measuring queue/CHM/MetricKey work, but it conceals the production cost of CoreSpan.isKind: SimpleSpan's isKind goes through groovy interface dispatch into SpanKindFilter.matches, while DDSpan.isKind inlines to a context byte-read + bit-test. This new benchmark uses real DDSpan instances created through a CoreTracer (with a NoopWriter so finishing doesn't reach the agent). Same shape as the SimpleSpan bench (64-span trace, span.kind=client, peer.hostname set). Numbers (2 forks x 5 iter x 15s): master: 6.428 +- 0.189 us/op (HashSet eligibility checks) this branch: 6.343 +- 0.115 us/op (SpanKindFilter bitmask) About 1.3% faster on the production path. The SimpleSpan benchmark in the same conditions shows a ~2.2% slowdown -- the mock's dispatch shape gives a misleading signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Make SpanKindFilter.kindMask and its constructor private now that DDSpan.isKind no longer needs direct field access -- it delegates to SpanKindFilter.matches(byte). The Builder.build() in the same outer class still constructs instances via the private constructor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the producer-side conflation pipeline with a thin per-span SpanSnapshot
posted to the existing aggregator thread. The aggregator now builds the
MetricKey, does the SERVICE_NAMES / SPAN_KINDS / PEER_TAGS_CACHE lookups, and
updates the AggregateMetric directly -- all off the producer's hot path.
What the producer does now, per span:
- filter (shouldComputeMetric, resource-ignored, longRunning)
- collect tag values into a SpanSnapshot (1 allocation per span)
- inbox.offer(snapshot) + return error flag for forceKeep
What moved off the producer:
- MetricKey construction and its hash computation
- SERVICE_NAMES.computeIfAbsent (UTF8 encoding of service name)
- SPAN_KINDS.computeIfAbsent (UTF8 encoding of span.kind)
- PEER_TAGS_CACHE lookups (peer-tag name+value UTF8 encoding)
- pending/keys ConcurrentHashMap operations
- Batch pooling, batch atomic ops, batch contributeTo
Removed entirely:
- Batch.java -- the conflation primitive is no longer needed; the
aggregator's existing LRUCache<MetricKey, AggregateMetric> IS the
conflation point now.
- pending ConcurrentHashMap<MetricKey, Batch>
- keys ConcurrentHashMap<MetricKey, MetricKey> (canonical dedup)
- batchPool MessagePassingQueue<Batch>
- The CommonKeyCleaner role of tracking keys.keySet() on LRU eviction --
AggregateExpiry now just reports drops to healthMetrics.
Added:
- SpanSnapshot: immutable value carrying the raw MetricKey inputs + a
tagAndDuration long (duration | ERROR_TAG | TOP_LEVEL_TAG).
- AggregateMetric.recordOneDuration(long tagAndDuration) -- the single-hit
equivalent of the existing recordDurations(int, AtomicLongArray).
- Peer-tag values flow through the snapshot as a flattened String[] of
[name0, value0, name1, value1, ...]; the aggregator encodes them through
PEER_TAGS_CACHE on its own thread.
Benchmark results (2 forks x 5 iter x 15s):
ConflatingMetricsAggregatorDDSpanBenchmark
prior commit 6.343 +- 0.115 us/op
this commit 2.506 +- 0.044 us/op (~60% faster)
ConflatingMetricsAggregatorBenchmark (SimpleSpan)
prior commit 6.585 +- 0.049 us/op
this commit 3.116 +- 0.032 us/op (~53% faster)
Caveat on the benchmark: without conflation, the producer pushes 1 inbox
item per span instead of ~1 per 64. At the benchmark's synthetic rate the
consumer can't keep up and inbox.offer silently drops. The numbers measure
producer publish() latency only; consumer throughput at realistic span rates
is a follow-up to validate. Tuning maxPending matters more in this design.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
With the per-span SpanSnapshot inbox path, the producer can lose snapshots when the bounded MPSC queue is full -- silently, since inbox.offer() returns a boolean we previously ignored. The conflating-Batch design used to absorb ~64x more producer pressure per inbox slot, so this is a new failure mode worth surfacing. Wire it through the existing HealthMetrics path: - HealthMetrics.onStatsInboxFull() (no-op default). - TracerHealthMetrics gets a statsInboxFull LongAdder and a new reason tag reason:inbox_full reported under the same stats.dropped_aggregates metric used for LRU evictions. Two LongAdders, two tagged time series. - ConflatingMetricsAggregator.publish increments the counter when inbox.offer(snapshot) returns false. This doesn't fix the drop -- tuning maxPending and/or building producer-side batching are the actual fixes. But it makes the failure visible in the same place ops already watches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
…nflating-metrics-background-work
Two general-purpose utilities used by the client-side stats aggregator work (PR #11382 and follow-ups), extracted into their own change so the metrics-specific PRs can build on a smaller, reviewable foundation. - Hashtable: a generic open-addressed-ish bucket table abstraction keyed by a 64-bit hash, with a public abstract Entry type so client code can subclass it for higher-arity keys. The metrics aggregator uses it to back its AggregateTable. - LongHashingUtils: chained 64-bit hash combiners with primitive overloads (boolean, short, int, long, Object). Used in place of varargs combiners to avoid Object[] allocation and boxing on the hot path. No callers within internal-api itself yet -- the metrics aggregator PR will introduce the first usages. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
Standalone classes for swapping the consumer-side LRUCache<MetricKey, AggregateMetric> with a multi-key Hashtable in the next commit. No call sites use them yet. - AggregateEntry extends Hashtable.Entry, holds the canonical MetricKey, the mutable AggregateMetric, and copies of the 13 raw SpanSnapshot fields for matches(). The 64-bit lookup hash is computed via chained LongHashingUtils.addToHash calls (no varargs, no boxing of short/boolean). - AggregateTable wraps a Hashtable.Entry[] from Hashtable.Support.create. findOrInsert(SpanSnapshot) walks the bucket comparing raw fields, falling back to MetricKeys.fromSnapshot on a true miss. On cap overrun, it scans for an entry with hitCount==0 and unlinks it; if none, it returns null and the caller drops the data point. - MetricKeys.fromSnapshot extracts the canonicalization logic (DDCache lookups + UTF8 encoding) from Aggregator.buildMetricKey, so the helper can be called from AggregateTable on miss. This also commits Hashtable and LongHashingUtils (added earlier, previously uncommitted) and lifts Hashtable.Entry / Hashtable.Support visibility so client code outside datadog.trace.util can build higher-arity tables -- the case the javadoc describes but the original visibility didn't actually support. Specifically: Entry is now public abstract with a protected ctor; keyHash, next(), and setNext() are public; Support's create / clear / bucketIndex / bucketIterator / mutatingBucketIterator methods are public. Tests: AggregateTableTest covers hit, miss, distinct-by-spanKind, peer-tag identity (including null vs non-null), cap overrun with stale victim, cap overrun with no victim (returns null), expungeStaleAggregates, forEach, clear, and that the canonical MetricKey is built at insert. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace LRUCache<MetricKey, AggregateMetric> with the AggregateTable added
in the prior commit. The hot path in Drainer.accept becomes:
AggregateMetric aggregate = aggregates.findOrInsert(snapshot);
if (aggregate != null) {
aggregate.recordOneDuration(snapshot.tagAndDuration);
dirty = true;
} else {
healthMetrics.onStatsAggregateDropped();
}
On the steady-state hit path the lookup is a 64-bit hash compute + bucket
walk + matches(snapshot) -- no MetricKey allocation, no SERVICE_NAMES /
SPAN_KINDS / PEER_TAGS_CACHE lookups. The canonical MetricKey is now built
once per unique key at insert time, in MetricKeys.fromSnapshot.
Behavioral change in the cap-overrun path
-----------------------------------------
The old LRUCache evicted least-recently-used: at cap, a new insert would
push out the oldest entry regardless of whether it was live or stale.
AggregateTable instead scans for a hitCount==0 entry to recycle, and drops
the new key if none exists. Practical impact: in the common case where
the table holds a stable set of recurring keys, an unrelated burst of new
keys is dropped (and reported via onStatsAggregateDropped) rather than
evicting the established keys. The existing test that asserted "service0
evicted in favor of service10" is updated to assert the new semantics.
The other cap-related test ("should not report dropped aggregate when
evicted entry was already flushed") still passes unchanged: after report()
clears all entries to hitCount=0, the next wave of inserts recycles them.
Threading fix
-------------
ConflatingMetricsAggregator.disable() used to call aggregator.clearAggregates()
and inbox.clear() directly from the Sink's IO event thread, racing with the
aggregator thread mid-write. The race was tolerable for LinkedHashMap; it
is not for AggregateTable (chain corruption can NPE or loop). disable()
now offers a ClearSignal to the inbox so the aggregator thread itself
performs the table clear and the inbox.clear(). Adds one SignalItem
subclass + one branch in Drainer.accept; preserves the single-writer
invariant for AggregateTable end-to-end.
Removed: LRUCache import, AggregateExpiry inner class, the static
buildMetricKey / materializePeerTags / encodePeerTag helpers (now in
MetricKeys).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
MetricKey existed for two reasons -- the prior LRUCache key role (now handled by AggregateTable's Hashtable.Entry mechanics) and as the labels argument to MetricWriter.add. The first is gone; the second is the only thing keeping MetricKey alive. Fold its UTF8-encoded label fields onto AggregateEntry, change MetricWriter.add to take AggregateEntry directly, and delete MetricKey + MetricKeys. What AggregateEntry now holds ----------------------------- - 10 UTF8BytesString label fields (resource, service, operationName, serviceSource, type, spanKind, httpMethod, httpEndpoint, grpcStatusCode, and a List<UTF8BytesString> peerTags for serialization). - 3 primitives (httpStatusCode, synthetic, traceRoot). - AggregateMetric (the value being accumulated). - The raw String[] peerTagPairs is retained alongside the encoded peerTags -- matches() compares it positionally against the snapshot's pairs; the encoded form is only consumed by the writer. matches(SpanSnapshot) compares the entry's UTF8 forms to the snapshot's raw String / CharSequence fields via content-equality (UTF8BytesString.toString() returns the underlying String in O(1)). This closes a latent bug in the prior raw-vs-raw matches(): if one snapshot delivered a tag value as String and a later snapshot delivered the same content as UTF8BytesString, the old Objects.equals would return false and the table would split into two entries. Content-equality matching collapses them into one. Consolidated caches ------------------- The static UTF8 caches that used to live partly on MetricKey (RESOURCE_CACHE, OPERATION_CACHE, SERVICE_SOURCE_CACHE, TYPE_CACHE, KIND_CACHE, HTTP_METHOD_CACHE, HTTP_ENDPOINT_CACHE, GRPC_STATUS_CODE_CACHE, SERVICE_CACHE) and partly on ConflatingMetricsAggregator (SERVICE_NAMES, SPAN_KINDS, PEER_TAGS_CACHE) are all now on AggregateEntry. The split was duplicating work -- SERVICE_NAMES and SERVICE_CACHE both cached service-name to UTF8BytesString. One cache per field now. API change: MetricWriter.add ---------------------------- Was: add(MetricKey key, AggregateMetric aggregate) Now: add(AggregateEntry entry) The aggregate lives on the entry. Single-arg. SerializingMetricWriter reads the same UTF8 fields off AggregateEntry that it previously read off MetricKey; the wire format is byte-identical. Test impact ----------- AggregateEntry.of(...) takes the same 13 positional args new MetricKey(...) took, so test diffs are mostly mechanical: new MetricKey(args) -> AggregateEntry.of(args) writer.add(key, _) -> writer.add(entry) ValidatingSink in SerializingMetricWriterTest now iterates List<AggregateEntry> directly. ConflatingMetricAggregatorTest's Spock matchers (~36 sites) rely on AggregateEntry.equals comparing the 13 label fields (not the aggregate) so the mock matches by labels regardless of the aggregate state at call time; post-invocation closures verify aggregate state. Benchmarks (2 forks x 5 iter x 15s) ----------------------------------- The change is consumer-thread only; producer publish() is unchanged. SimpleSpan bench: 3.123 +- 0.025 us/op (prior: 3.119 +- 0.018) DDSpan bench: 2.412 +- 0.022 us/op (prior: 2.463 +- 0.041) Both within noise -- the win is structural (one less class, one less allocation per miss, one fewer cache layer) rather than benchmarked. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
050a998 to
3738c85
Compare
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>
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>
6 tasks
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>
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>
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>
Base automatically changed from
dougqh/conflating-metrics-background-work
to
master
May 26, 2026 16:40
…ric-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
Contributor
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master resultsStartup Time
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
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>
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>
dougqh
commented
May 26, 2026
dougqh
added a commit
that referenced
this pull request
May 26, 2026
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>
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>
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>
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>
amarziali
reviewed
May 27, 2026
…leton 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>
Contributor
|
Hi! 👋 Looks like you updated a Git Submodule.
|
amarziali
approved these changes
May 27, 2026
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>
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>
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>
…ntions Five small cleanups surfaced by the design re-review: - Drop AggregateEntry.forSnapshot(SpanSnapshot, long). It wrapped the private constructor for no reason; make the constructor package- private and have AggregateTable.findOrInsert and AggregateEntryTestUtils.forSnapshot call it directly. - Class-level Javadoc now documents the required-vs-optional field absence convention: required fields canonicalize null -> EMPTY, optional fields stay null so the serializer's `!= null` check works. Previously a reader had to infer it from the constructor body. - Field Javadocs on `synthetic` (synthetic-monitoring origin tag) and `traceRoot` (parentId == 0). Both make it onto the wire; neither was obvious to a fresh reader. - Tighten the `peerTagNames` / `peerTagValues` field comment. The previous wording implied package-private was for "test-only" access; in fact production matches() reads them from within the class and the test helper is just one consumer. - Add a `canonicalizeOptional` helper that mirrors `canonicalize` but returns null (not EMPTY) for null input. Folds the four optional- field assignments in the constructor from three-line ternaries into one-liners. Keeps the `instanceof UTF8BytesString` short-circuit consistent across all label fields -- dead code for the String-typed optionals (httpMethod/Endpoint/grpcStatusCode), live for the CharSequence-typed serviceNameSource. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Flagged by codenarcTraceAgentTest (UnusedImport rule). Left over from a prior rewrite of the entry-construction flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
Replaces the MetricKey based HashMap with a new AggregateTable based on the light Hashtable
Motivation
By using the light Hashtable, I'm able to avoid the biggest source of allocation in client-side stats: MetricKey
Hashtable provides utilities for searching the entries without constructing a new composite key object
First, the components are hashed together to find the corresponding bucket
Then the bucket can be traversed to see if the entries match the key components
And the custom Entry can hold multiple fields that comprise the data / metadata needed for metric aggregation and eviction policy
The end result is that both MetricKey and AggregateMetric can be merged into a single class AggregateEntry that is only constructed when there's no existing matching entry
Additional Notes
Stacked on top of master. #11381 (producer/consumer split +
SpanSnapshotinbox) and #11409 (Hashtableutility) have both landed in master, so the diff shown here is only theAggregateTable/AggregateEntry/ClearSignalwork plus follow-up cleanups.Restructures the consumer-side aggregate store. Three logical commits, intended to be reviewed in order:
1. Add
AggregateTable+AggregateEntrybacked byHashtableIntroduces a multi-key hash table that lets the consumer thread look up the {labels → counters} entry directly from a
SpanSnapshot's raw fields — noMetricKeyallocation per snapshot, no per-snapshot UTF8 cache lookups, no CHM operations. Hot-path lookup iskeyHash compute→Hashtable.Support.bucket→ bucket walk →matches(keyHash, snapshot)→ returned entry has the counters to mutate in place.This commit is standalone — no call sites yet, only the new classes + unit tests for hit/miss/cap-overrun/expunge/clear behavior.
2. Swap
Aggregatorto useAggregateTable+ routedisable()clear through aClearSignalReplaces
LRUCache<MetricKey, AggregateMetric>withAggregateTableinAggregator. Drops theAggregateExpirylistener — drop reporting (onStatsAggregateDropped) moves to the cap-overrun path insideDrainer.accept.Threading fix bundled here:
ConflatingMetricsAggregator.disable()used to callaggregator.clearAggregates()andinbox.clear()directly from the Sink's IO callback thread, racing with the aggregator thread. That race was tolerable forLinkedHashMap(worst case = corrupted internal state right before everything got cleared anyway); it's not tolerable forHashtable(chain corruption can NPE or loop).disable()now offers aClearSignalto the inbox so the aggregator thread itself performs the clear — preserves the single-writer invariant forAggregateTableend-to-end. The offer is best-effort; the system self-heals on a subsequent downgrade cycle if the inbox happens to be full (commented at the call site).Cap-overrun semantic change: the old
LRUCacheevicted least-recently-used in O(1).AggregateTableinstead scans for ahitCount==0entry to recycle (O(N) worst-case), and drops the new key if none exists. Practical impact: in steady state, an unrelated burst of new keys gets dropped (and reported viaonStatsAggregateDropped) rather than evicting established keys. The cost trade-off is commented at the eviction site — eviction is expected rare because the cap is sized to the working set; cursor-caching is the future option if a workload runs persistently at cap. The existing test that asserted "service0 evicted in favor of service10" is updated to assert the new semantics; the other cap-related test ("evicted entry was already flushed") still passes unchanged.3. Fold
MetricKey+AggregateMetricintoAggregateEntryMetricKeyexisted for two reasons — being theLRUCachekey (replaced byAggregateTable's Hashtable mechanics) and being the labels arg toMetricWriter.add(the only thing left).AggregateMetricwas the counter/histogram counterpart. Folds both onto a singleAggregateEntry(10 UTF8 label fields + 3 primitives + counters + histograms), changesMetricWriter.add(MetricKey, AggregateMetric)→add(AggregateEntry), and deletesMetricKey.java+MetricKeys.java+AggregateMetric.java.The 12 UTF8 caches that used to be split between
MetricKey(9) andConflatingMetricsAggregator(3, with overlap) are consolidated onAggregateEntry. One cache per field type now.Latent bug fix: the prior
matches(SpanSnapshot)usedObjects.equalson raw fields. If the same logical key was delivered once asStringand once asUTF8BytesString(differentCharSequenceimpls of identical content),Objects.equalsreturned false and the table would split into two entries for the same key. The newmatchesuses content-equality (UTF8BytesString.toString()returns the underlyingStringin O(1)), collapsing them correctly.Test impact:
AggregateEntry.of(...)mirrors the priornew MetricKey(...)positional args, so test diffs are mostly mechanical. About 56 test sites migrated acrossConflatingMetricAggregatorTest,SerializingMetricWriterTest, andMetricsIntegrationTest.Review polish
Follow-up commits address review feedback:
Hashtable.Support.create(maxAggregates, Support.MAX_RATIO)+Support.bucket+Support.insertHeadEntry(buckets, keyHash, entry)+Support.mutatingTableIteratorto delegate to the helpers added on Add Hashtable and LongHashingUtils utilities #11409 — drops ~50 lines of bespoke bucket-array code.BiConsumerconstant (the JIT reuses non-capturing lambdas).AggregateEntry.matches(long keyHash, SpanSnapshot)overload that pre-checks the hash, so chain walks read as one call.@Nullable(javax.annotation) annotations on the four nullable label fields + their getters +of(...)parameters.Objects.equalsimport inAggregateEntry.equals()(no more fully-qualified refs).evictOneStale(O(N) scan rationale) anddisable()(best-effort offer rationale).Additional cleanups
A second round of review surfacing landed these:
.aggregateruntime bug — the legacyentry.aggregate.recordDurations(...)form compiled under Groovy's dynamic dispatch but would have thrownMissingPropertyExceptionat runtime. Fixed to callrecordDurationsdirectly on the entry. (Bot-flagged.)>> { closure }no-op assertions inConflatingMetricAggregatorTest— the>>operator stubs a return value, so the closures verifyinge.getHitCount() == X && ...were being evaluated and discarded. Wrapped 31 sites inassertso Groovy power-assert surfaces mismatches. All 41 tests still pass, so the previously-unverified assertions happened to hold. (Bot-flagged.)recordDurations(int, AtomicLongArray)batch API — vestige of master'sBatchdesign. Production now only callsrecordOneDuration(long). Migrated the three remaining test callers (AggregateEntryTest,SerializingMetricWriterTest,MetricsIntegrationTest) to loops ofrecordOneDurationcalls, then deleted the batched method and itsAtomicLongArrayimports.AggregateEntry.of()colon-split Javadoc warning — the test factory recovers (name, value) pairs from"name:value"strings by splitting at the first:, which is brittle if a peer-tag value contains a colon (URLs, IPv6,service:env). Added an explicit warning so callers know to keep test data colon-free in values.ConflatingMetricsAggregatorDisableTest— new JUnit 5 coverage for thedisable() → ClearSignalthreading routing. The test firesDOWNGRADEDfrom the test thread, waits for the no-flush window, then publishes a marker span with a distinct resource name and asserts the next flush captures only the marker — proving CLEAR actually wiped the original entry from the table. Catches both the missing-clear regression and the bucket-chain-corruption regression that the original threading race could produce.Benchmarks
Producer
publish()latency (single-threaded, 2 forks × 5 iter × 15s)All within noise on the producer side — this PR is a consumer-side refactor, so producer
publish()shouldn't move much. The structural wins (one less class, no per-snapshotMetricKeyallocation on the consumer, no double-cache lookups, smaller per-entry footprint) only become visible when the consumer is hammered hard enough that snapshot processing rate matters. That's exactly what the adversarial bench measures.Adversarial bench suite vs v1.62.0 + master
Re-measured 2026-05-27 with three benches in matrix form: full adversarial (all four label dimensions vary) and two cardinality-isolation companions (only
resourcevaries; onlypeer.hostnamevaries). Same machine state, same JMH config (8 producer threads, 2×15s warmup + 5×15s, 1 fork, throughput mode). TheHighCardinality*andAdversarialbenches were backported onto the v1.62.0 tag using itsConflatingMetricsAggregatorconstructor andHealthMetrics.NO_OP(v1.62.0 predates the inbox split so per-iteration drop counters are not directly comparable).AdversarialMetricsBenchmark(ops/s)HighCardinalityResource(ops/s)HighCardinalityPeer(ops/s)onStatsAggregateDroppedHealthMetrics.NO_OP)Customer headline: a workload migrating from v1.62.0 sees ~5–7× throughput on single-axis cardinality and ~73× on the adversarial all-axes attack — the all-axes case is the one where v1.62.0's LRU cache thrashes catastrophically (per-iteration progression on v1.62.0 Adversarial: 1.08M warmup → 277K → 199K, classic capacity-degradation curve). The master baseline includes #11381's producer/consumer split, so vs-master numbers represent the consumer-side AggregateTable redesign alone; vs-v1.62.0 is the full CSS rework.
The aggregate-drop counter shape confirms the design intent: master and v1.62.0 saturate
tracerMetricsMaxAggregatescontinuously (LRU thrashing in v1.62.0; eviction-stream in master), shedding work at the expensive end of the pipeline. This PR pushes drops out to the producer-side inbox fast path (onStatsInboxFull) where load shedding is cheap, and the consumer-sideonStatsAggregateDroppedcollapses by ~10×.Net code delta: +1280 / −903 = +377 lines across 16 files. The growth is dominated by new test coverage (
AggregateTableTest,AggregateEntryTest) plus the consolidated UTF8 caches landing onAggregateEntry; the production-code core (lessMetricKey+MetricKeys+AggregateMetricminusAggregateEntry's additions) is roughly flat.Known memory items addressed downstream
Two memory concerns visible at this layer of the stack are addressed in subsequent PRs — flagging here so reviewers don't worry about them in isolation:
Two
Histograminstances per entry are eagerly allocated (worst case ~4 MB heap floor at defaultmaxAggregates=2048× 2 × ~1 KB DDSketch). Most entries never see an error and so theerrorLatencieshistogram is unused. Fixed in Memory-efficiency pass on ClientStatsAggregator + adversarial benchmark #11389, which makeserrorLatencieslazy: it staysnulluntil the first error is recorded, andgetErrorLatencies()returns a shared empty histogram in the no-error case.PEER_TAGS_CACHEworst case is 64 outer × 512 inner = 32 K cachedUTF8BytesStringpeer-tag pairs, heap-pinned for the JVM lifetime. Past the inner cap the per-name LRU starts thrashing under unbounded cardinality, and the cache size itself is the only memory backstop. Fixed in Per-component / tag cardinality limits in client-side stats #11387, which adds per-tagTagCardinalityHandlerbudgets that fold overflow values into a"<tag>:blocked_by_tracer"sentinel; cardinality is bounded per reporting interval rather than per JVM lifetime, and the worst-case cache occupancy collapses to the configured budget.Test plan
./gradlew :dd-trace-core:test --tests 'datadog.trace.common.metrics.*'passes (incl. the newAggregateTableTestandAggregateEntryTest)./gradlew :dd-trace-core:compileJava :dd-trace-core:compileTestGroovy :dd-trace-core:compileJmhJava :dd-trace-core:compileTraceAgentTestGroovyall green./gradlew spotlessCheckcleanstats.dropped_aggregatessemantics at high cardinality (especially the new "drop new on cap overrun" path vs. the old "evict LRU" path)🤖 Generated with Claude Code