Commit f5472d9
Add span-derived primary tags (CSS v1.3.0) (#11402)
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…1 parent 9b7597b commit f5472d9
44 files changed
Lines changed: 2775 additions & 689 deletions
File tree
- dd-trace-api/src/main/java/datadog/trace/api
- config
- dd-trace-core/src
- jmh/java/datadog/trace/common/metrics
- main/java/datadog/trace/common/metrics
- test
- groovy/datadog/trace/common/metrics
- java/datadog/trace/common/metrics
- traceAgentTest/java/datadog/trace/common/metrics
- internal-api/src
- main/java/datadog/trace
- api
- util
- test
- groovy/datadog/trace/api
- java/datadog/trace
- api
- util
- metadata
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 5 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
297 | 297 | | |
298 | 298 | | |
299 | 299 | | |
300 | | - | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
301 | 305 | | |
302 | 306 | | |
303 | 307 | | |
| |||
Lines changed: 1 addition & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
87 | 87 | | |
88 | 88 | | |
89 | 89 | | |
| 90 | + | |
90 | 91 | | |
91 | 92 | | |
92 | 93 | | |
| |||
Lines changed: 105 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
Lines changed: 2 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
57 | 57 | | |
58 | 58 | | |
59 | 59 | | |
60 | | - | |
| 60 | + | |
61 | 61 | | |
62 | 62 | | |
63 | 63 | | |
| |||
75 | 75 | | |
76 | 76 | | |
77 | 77 | | |
| 78 | + | |
78 | 79 | | |
79 | 80 | | |
80 | 81 | | |
| |||
Lines changed: 2 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
33 | 33 | | |
34 | 34 | | |
35 | 35 | | |
36 | | - | |
| 36 | + | |
37 | 37 | | |
38 | 38 | | |
39 | 39 | | |
| |||
42 | 42 | | |
43 | 43 | | |
44 | 44 | | |
| 45 | + | |
45 | 46 | | |
46 | 47 | | |
47 | 48 | | |
| |||
Lines changed: 2 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
52 | | - | |
| 52 | + | |
53 | 53 | | |
54 | 54 | | |
55 | 55 | | |
| |||
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
| 65 | + | |
65 | 66 | | |
66 | 67 | | |
67 | 68 | | |
| |||
Lines changed: 2 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
47 | 47 | | |
48 | 48 | | |
49 | 49 | | |
50 | | - | |
| 50 | + | |
51 | 51 | | |
52 | 52 | | |
53 | 53 | | |
| |||
65 | 65 | | |
66 | 66 | | |
67 | 67 | | |
| 68 | + | |
68 | 69 | | |
69 | 70 | | |
70 | 71 | | |
| |||
Lines changed: 2 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
43 | 43 | | |
44 | 44 | | |
45 | 45 | | |
46 | | - | |
| 46 | + | |
47 | 47 | | |
48 | 48 | | |
49 | 49 | | |
| |||
61 | 61 | | |
62 | 62 | | |
63 | 63 | | |
| 64 | + | |
64 | 65 | | |
65 | 66 | | |
66 | 67 | | |
| |||
Lines changed: 132 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
0 commit comments