Fix AppSec/IAST context dropped when trace.propagation.behavior.extract=ignore/restart#11666
Draft
dougqh wants to merge 237 commits into
Draft
Fix AppSec/IAST context dropped when trace.propagation.behavior.extract=ignore/restart#11666dougqh wants to merge 237 commits into
dougqh wants to merge 237 commits into
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>
…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>
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>
Replaces the per-field DDCache layer inside AggregateEntry with the two new
cardinality handlers. Each per-field handler holds a small HashMap working
set; when its budget is exhausted, subsequent values collapse to a stable
"blocked_by_tracer" sentinel UTF8BytesString rather than growing without
bound. The handlers are reset on the aggregator thread at the end of each
report() cycle (10s default), so the cardinality budget refreshes per
reporting interval.
Caches replaced (limits preserved from the prior DDCache sizes):
RESOURCE_HANDLER 32
SERVICE_HANDLER 32
OPERATION_HANDLER 64
SERVICE_SOURCE_HANDLER 16
TYPE_HANDLER 8
SPAN_KIND_HANDLER 16
HTTP_METHOD_HANDLER 8
HTTP_ENDPOINT_HANDLER 32
GRPC_STATUS_CODE_HANDLER 32
PEER_TAG_HANDLERS per-tag-name TagCardinalityHandler, each 512
Two production-only changes to the handlers as the user wrote them:
- Fixed import: datadog.collections.tagmap6lazy.TagMap doesn't exist;
TagCardinalityHandler now imports datadog.trace.api.TagMap which has the
Entry API the handler uses.
- Added TagCardinalityHandler.register(String) overload so AggregateEntry's
peer-tag canonicalization doesn't have to allocate a TagMap.Entry per
call -- the snapshot already carries peer-tag values as a flattened
String[] {name, value, ...}.
AggregateEntry split into two construction paths:
- forSnapshot(snapshot, agg): the hot path; runs each field through the
appropriate handler.
- of(...): test-only factory; bypasses the handlers and creates UTF8
instances directly, so tests don't pollute static handler state. Content-
equality on the resulting entry still matches the production-built one.
Thread-safety: handlers are HashMap-backed and not safe for concurrent
access. Both forSnapshot and resetCardinalityHandlers must be called from
the aggregator thread. After the prior commits that moved MetricKey
construction to the aggregator thread, this is the only thread that
canonicalizes; the test factory path runs on test threads but doesn't
touch the handlers.
Reset semantics: clearing the handler's working set drops the {value ->
UTF8BytesString} mapping but doesn't invalidate existing AggregateEntry
fields -- those keep their UTF8BytesString references alive on their own.
Subsequent snapshots with the same content still resolve to the existing
entries via content-equality matches(). New values after reset get freshly
allocated UTF8BytesStrings via the handler.
Known limitation (not fixed here): hashOf(SpanSnapshot) hashes from the
raw snapshot fields, not from the post-handler canonical form. So when
cardinality is exceeded, multiple distinct raw values that collapse to
the "blocked_by_tracer" sentinel still produce distinct hashes and land
in different AggregateEntry buckets -- the wire payload will carry
multiple rows that all label as blocked. This is the same behavior the
prior DDCache-based design would have had at capacity. Collapsing those
into a single sentinel entry would require canonicalizing before hashing
and is a follow-up.
Tests: new CardinalityHandlerTest covers PropertyCardinalityHandler and
TagCardinalityHandler in isolation (hit/miss, over-limit blocking, reset
behavior, sentinel stability). Existing ConflatingMetricAggregatorTest /
SerializingMetricWriterTest / AggregateTableTest all pass unchanged
because the test factory bypasses handlers.
Benchmarks (2 forks x 5 iter x 15s) -- producer side unchanged because
the handlers live on the consumer thread:
SimpleSpan bench: 3.114 +- 0.045 us/op (prior: 3.123 +- 0.018)
DDSpan bench: 2.364 +- 0.113 us/op (prior: 2.412 +- 0.022)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The prior commit ran every snapshot through the cardinality handlers but still
hashed the raw snapshot fields. When a field exceeded its cardinality budget
the handlers collapsed many distinct values to a single "blocked_by_tracer"
sentinel, but the raw hashes were still all different -- so the blocked entries
fragmented across the AggregateTable. This commit makes hash + match work off
the canonical (post-handler) UTF8BytesString fields, so blocked values land in
the same bucket and merge into one entry.
How the lookup path changes
---------------------------
A new package-private AggregateEntry.Canonical scratch buffer:
- holds the 10 canonical UTF8BytesString refs, primitives, peerTags list,
and the precomputed keyHash;
- exposes populate(SpanSnapshot) which runs each field through the
appropriate handler and computes the long hash from the canonical refs;
- exposes matches(AggregateEntry) for content-equality lookup;
- exposes toEntry(AggregateMetric) which copies its refs into a fresh
AggregateEntry on miss.
AggregateTable holds one Canonical instance and reuses it per findOrInsert.
On a hit nothing is allocated -- the buffer's refs feed the bucket walk and
matches() directly. On a miss the refs are copied into the new entry and the
buffer is overwritten on the next call.
Hash function
-------------
hashOf now takes UTF8BytesString fields (plus primitives + peerTags list)
instead of raw CharSequence/String from the snapshot. UTF8BytesString.hashCode
returns the underlying String's hash, so:
- content-equal entries built via AggregateEntry.of(...) (test factory,
bypasses handlers) produce the same hash as entries built via
Canonical.toEntry(...) (production, via handlers);
- all values that collapsed to "blocked_by_tracer" share that sentinel
instance and therefore that hashCode -- they land in the same bucket and
merge into one entry.
Matches
-------
The SpanSnapshot-keyed matches() on AggregateEntry is gone. Lookup goes
through Canonical.matches(entry) which compares the buffer's UTF8 fields
against the entry's UTF8 fields via Objects.equals (content equality on
UTF8BytesString). This is needed because across handler resets the
UTF8BytesString instance referenced by an existing entry differs from the
freshly-issued instance for the same content -- content-equality lets the
existing entry survive resets.
The peerTagPairsRaw field on AggregateEntry was previously kept for matching
against snapshot.peerTagPairs (the flat String[]). Canonical.matches uses
List.equals on the encoded UTF8 peerTags directly, so peerTagPairsRaw is
dropped.
New test in AggregateTableTest -- cardinalityBlockedValuesCollapseIntoOneEntry
inserts 50 distinct services into a table whose SERVICE_HANDLER has a
cardinality limit of 32, and asserts the final size is 33 (the 32 in-budget
services plus a single collapsed "blocked_by_tracer" entry, not 50 separate
entries).
Benchmarks (2 forks x 5 iter x 15s) -- producer side unchanged:
SimpleSpan bench: 3.117 +- 0.026 us/op (prior: 3.114 +- 0.045)
DDSpan bench: 2.344 +- 0.114 us/op (prior: 2.364 +- 0.113)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…chema-indexed handlers
Replaces the producer's early {@code (name, value)}-pair encoding with a
schema-based design: peer-tag values are captured into a parallel String
array, and the consumer applies the matching {@link TagCardinalityHandler}
by index using a {@link PeerTagSchema}'s parallel name/handler arrays.
This removes the {@code Map<String, TagCardinalityHandler>} the prior commit
left in {@code AggregateEntry} -- handler lookup is now a single array
dereference instead of a hashmap probe.
PeerTagSchema
-------------
New package-private class that holds:
- {@code String[] names} -- peer-tag names in stable order
- {@code TagCardinalityHandler[] handlers} -- parallel to names
Two schemas exist: a static singleton {@code INTERNAL} for the internal-kind
{@code base.service} case, and a {@code CURRENT} schema for the peer-
aggregation kinds (client/producer/consumer) that lazily refreshes when
{@code features.peerTags()} returns a different set of names.
Each {@link SpanSnapshot} captures the schema reference it was built against
so producer and consumer agree on the indexing even if {@code CURRENT}
changes between capture and consumption.
A fast-path identity check (cached last input Set instance) keeps the
{@code currentSyncedTo} call cheap: when the producer hands in the same
Set instance as last time -- the steady-state case --
{@code currentSyncedTo} returns immediately without iterating names. The
{@code matches()} loop only runs when the Set instance changes, which in
production is rare (only on remote-config reconfiguration).
Snapshot shape
--------------
{@code SpanSnapshot.peerTagPairs} (a flat {@code [name0, value0, name1,
value1, ...]} array) is replaced by:
- {@code PeerTagSchema peerTagSchema} -- nullable; schema for the values
- {@code String[] peerTagValues} -- parallel to schema.names
The producer captures only values; the consumer constructs the encoded
{@code "name:value"} UTF8 forms via {@code schema.handler(i).register(value)}
on its own thread.
Consumer-side cleanups bundled in
---------------------------------
While here, also addresses the perf review items raised against the prior
commit:
- {@code hashOf}'s peer-tag loop is now indexed iteration; no more
iterator allocation per snapshot.
- {@code Canonical} now owns a reusable {@code peerTagsBuffer} ArrayList
that's cleared+refilled per {@code populate} call -- zero allocation
on the hit path. The buffer is copied into an immutable list only on
miss when the entry needs to own it long-term.
- {@code Canonical.matches} uses indexed list comparison; no iterator
alloc in {@code List.equals}.
- The {@code HashMap<String, TagCardinalityHandler> PEER_TAG_HANDLERS}
on {@code AggregateEntry} is gone, replaced by the {@link
PeerTagSchema}'s parallel array layout.
Benchmark (2 forks x 5 iter x 15s)
----------------------------------
SimpleSpan bench: 3.165 +- 0.032 us/op (prior: 3.117 +- 0.026)
DDSpan bench: 2.727 +- 0.018 us/op (prior: 2.344 +- 0.114)
Some producer-side regression from the per-snapshot schema sync (volatile
read + identity check). The fast-path identity comparison keeps it small;
hoisting the sync out of the per-snapshot loop is possible but would change
behavior in the edge case where {@code features.peerTags()} returns
different Sets within a single trace (covered by an existing test). Choosing
correctness over the marginal speedup.
Tests
-----
AggregateTableTest's snapshot builder is updated to construct a schema +
values via {@code PeerTagSchema.currentSyncedTo}, exercising the same code
path as production. Existing peer-tag test in {@code
ConflatingMetricAggregatorTest} still passes unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The "Conflating" in the name dates from the prior design that used a Batch pool + pending map to conflate up to 64 hits per inbox slot. That mechanism is gone -- the producer now publishes one SpanSnapshot per span and the consumer's AggregateTable is the conflation point. The new name matches the existing protocol/metric terminology (HealthMetrics.onClientStat*, stats.flush_payloads, etc.). File renames: ConflatingMetricsAggregator.java -> ClientStatsAggregator.java ConflatingMetricAggregatorTest.groovy -> ClientStatsAggregatorTest.groovy ConflatingMetricsAggregatorBenchmark -> ClientStatsAggregatorBenchmark ConflatingMetricsAggregatorDDSpan* -> ClientStatsAggregatorDDSpan* Plus all symbol references in MetricsAggregatorFactory and the test fixtures that referenced the old class name. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three small follow-ups carried over from a /techdebt pass: - TracerHealthMetrics: previousCounts array was sized 51, but the prior commits added a 52nd reporter (statsInboxFull). Without this fix the new counter's report() call would throw ArrayIndexOutOfBoundsException; the Flush task swallows that exception, so the failure would be silent (statsInboxFull would just never make it to statsd). - Aggregator: removes the now-dead public clearAggregates() method. The ClearSignal route from ClientStatsAggregator.disable() supplanted it several commits ago; the method had no remaining callers. - TagCardinalityHandler: removes the unused register(TagMap.Entry) overload and its isValidType helper. The String-keyed overload covers all current callers (AggregateEntry's peer-tag canonicalization). - PeerTagSchema: spotless-driven javadoc reflow only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ClientStatsAggregator.publish was calling features.peerTags() + PeerTagSchema.currentSyncedTo for every span. Peer-tag configuration is stable for the duration of a single trace publish in production -- DDAgentFeaturesDiscovery returns the same Set instance until remote-config reconfiguration -- so the per-snapshot sync is wasted work. Move the sync to once per publish(trace) and pass the resolved schema to the inner publish(span, isTopLevel, peerAggSchema). INTERNAL-kind spans still use the static PeerTagSchema.INTERNAL regardless. Behavior boundary ----------------- Schema changes from features.peerTags() now take effect at the next publish(trace) call rather than mid-trace. Production-equivalent (a trace takes microseconds to milliseconds; remote-config refreshes are seconds apart), but a Spock test that used `>>> [...]` to mock different peerTags() returns on successive calls within one trace no longer makes sense in the new model. That test is rewritten to assert the production-relevant case: peer-tag NAMES are stable, peer-tag VALUES vary per span, distinct value combinations produce distinct aggregate buckets. Benchmark (2 forks x 5 iter x 15s) ---------------------------------- SimpleSpan bench: 3.133 +- 0.057 us/op (prior: 3.165 +- 0.032) DDSpan bench: 2.454 +- 0.082 us/op (prior: 2.727 +- 0.018) Recovers ~270 ns/op on the DDSpan bench -- most of the regression introduced by the per-snapshot lookup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JFR profiling showed ~21% of producer CPU time spent in tag-map lookups during ClientStatsAggregator.publish. One of those lookups -- span.kind -- is redundant because DDSpanContext already caches the kind as a byte ordinal that resolves to a String via a small array. - Add CoreSpan.getSpanKindString() with a default that falls back to the tag map for non-DDSpan impls; DDSpan overrides to delegate to the context's cached resolution. - Hoist schema.names array out of the capturePeerTagValues loop. - Avoid an unnecessary toString() in isSynthetic by declaring SYNTHETICS_ORIGIN as String and using contentEquals. Benchmark (ClientStatsAggregatorDDSpanBenchmark): before: 2.410 us/op after: 1.995 us/op (~17% improvement) vs. master baseline (6.428 us/op): now ~3.2x faster. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the producer/consumer split, the canonical-key trick that makes cardinality-blocking actually save space, the once-per-trace peer-tag schema sync, the role of each file in datadog.trace.common.metrics, and the rationale behind the redesign from ConflatingMetricsAggregator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LongHashingUtilsTest (14 cases):
- hashCodeX null sentinel + non-null pass-through
- all primitive hash() overloads match the boxed Java hashCodes
- hash(Object...) 2/3/4/5-arg overloads match the chained addToHash
formula they are documented to constant-fold to
- addToHash(long, primitive) overloads match the Object-version
- linear-accumulation invariant (31 * h + v) holds across a sequence
- iterable / deprecated int[] / deprecated Object[] variants match
chained addToHash
- intHash treats null as 0 (observable via hash(null, "x"))
HashtableTest (24 cases across 5 nested classes):
- D1: insert/get/remove/insertOrReplace/clear/forEach, in-place value
mutation, null-key handling, hash-collision chaining with disambig-
uating equals, remove-from-collided-chain leaves siblings intact
- D2: pair-key identity, remove(pair), insertOrReplace matches on
both parts, forEach
- Support: capacity rounds up to a power of two, bucketIndex stays
in range across a wide hash sample, clear nulls every slot
- BucketIterator: walks only matching-hash entries in a chain, throws
NoSuchElementException when exhausted
- MutatingBucketIterator: remove from head-of-chain unlinks, replace
swaps the entry while preserving chain, remove() without prior
next() throws IllegalStateException
Tests live in internal-api/src/test/java/datadog/trace/util and use the
already-present JUnit 5 setup.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bring the new util/ files in line with google-java-format (tabs → spaces, line wrapping, javadoc list markup) so spotlessCheck passes in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Compares Hashtable.D1 and Hashtable.D2 against equivalent HashMap usage for add, update, and iterate operations. Each benchmark thread owns its own map (Scope.Thread), but @threads(8) is used so the allocation/GC pressure that Hashtable is designed to avoid surfaces in the throughput numbers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Guard Support.sizeFor against overflow and use Integer.highestOneBit; reject capacities above 1 << 30 instead of looping forever. - Add braces around single-statement while bodies in BucketIterator. - Split HashtableBenchmark into HashtableD1Benchmark / HashtableD2Benchmark. - Add regression tests for Support.sizeFor bounds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 5-arg Object overload was forwarding only obj0..obj3 to the int overload, silently dropping obj4. Also align LongHashingUtils.hash 3-arg signature with its 2/4/5-arg siblings (int parameters) and strengthen the 5-arg HashingUtilsTest to detect the missing-arg regression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Split D1Tests and D2Tests into HashtableD1Test and HashtableD2Test; extract shared test entry classes into HashtableTestEntries. - Reduce visibility of LongHashingUtils.hash(int...) chaining overloads to package-private; they are internal building blocks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The iterator tests need a populated Hashtable.Entry[] to drive Support.bucketIterator / mutatingBucketIterator. Relaxing D1.buckets from private to package-private lets the same-package tests read it directly, removing the reflection helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The new reason:inbox_full reportIfChanged call advances countIndex to 51, but previousCounts was still sized for 51 counters (max index 50), so the metric never emitted and the resize warning fired every flush. Bump the array to 52 and add a regression test that exercises the flush path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cardinality # Conflicts: # dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java # dd-trace-core/src/main/java/datadog/trace/common/metrics/PeerTagSchema.java
Addresses dougqh's review comment on #11387. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s, HealthMetrics interface
- PropertyCardinalityHandler and TagCardinalityHandler gain a name field and a
lazy statsDTag() accessor (String[]{"tag:<name>"}), built only on first block
- HealthMetrics.onTagCardinalityBlocked takes String[] so callers pass the
pre-built array; TracerHealthMetrics forwards it directly to StatsD
- AggregateEntry.has*() switch from identity (!=EMPTY) to length()>0 so
empty-string values are treated as absent on the wire
- EMPTY_PEER_TAGS renamed to EMPTY_TAGS
- ignoredResources.isEmpty() guard on the publish hot path
- CardinalityHandlerTest updated for the new PropertyCardinalityHandler(name,...) constructor
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Writer ctor, add @nullable - Aggregator.onReportCycle Javadoc: ConflatingMetricsAggregator -> ClientStatsAggregator - PropertyCardinalityHandler gains shouldWarnThisCycle() (cleared on reset()); name is package-private; AggregateEntry.reportIfBlocked() logs warn on first block per field per cycle, matching PeerTagSchema and AdditionalTagsSchema behaviour - SerializingMetricWriter 4-arg constructor was constructing new GitInfoProvider() instead of assigning the injected parameter - SpanSnapshot.httpMethod/httpEndpoint/grpcStatusCode annotated @nullable Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…) at call time PeerTagSchema.INTERNAL was constructed with HealthMetrics.NO_OP so base.service cardinality blocks were silently dropped from StatsD. Removing the healthMetrics field from PeerTagSchema entirely and passing it at call time means INTERNAL and all other schemas report through the same real HealthMetrics instance. PeerTagSchema.of() drops the HealthMetrics parameter accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add identity check (existing == value) before contentEquals in both the cur-cycle and prior-cycle probe loops. In steady state the same UTF8BytesString instance recurs every span, so contentEquals is never reached. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add identity check (curKey == value / priorKey == value) before equals() in both probe loops. Incoming values are often string literals or interned strings so the reference check hits reasonably often. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Accidentally dropped when publish(CoreSpan, boolean) was refactored to publish(CoreSpan, boolean, PeerTagSchema). Short-circuits before any tag extraction or SpanSnapshot allocation when the inbox is at capacity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d NO_STATE, rename resetHandlers, Config.toString Back-ports the self-contained #11387 review-comment fixes that were mistakenly committed to the stacked child branch (dougqh/metrics-arbitrary-tags) instead of here: - Remove the per-registration warnedCardinality warn-once set from PeerTagSchema; emit the warn once per cycle in resetHandlers() when blocked > 0. Drops the now-dead TagCardinalityHandler.isBlockedResult() and updates CardinalityHandlerTest accordingly. - Add an explanatory NO_STATE constant for the null state passed by never-reconciled schemas (INTERNAL singleton, test schemas). - Rename PeerTagSchema.resetCardinalityHandlers -> resetHandlers (callers in AggregateEntry and ClientStatsAggregator updated; the aggregator's own no-arg orchestration method keeps its name). - Add traceStatsCardinalityLimitsEnabled to Config.toString() for diagnostics. - Fix SerializingMetricWriterTest: validate the header map size against the injected git provider (shaCommit) instead of the global GitInfoProvider.INSTANCE, which returns null in environments without resolvable git info and made the test fail there. The LIMITS_ENABLED relocation and CardinalityBlocks extraction were intentionally NOT ported: they depend on PropertyHandlers/AdditionalTagsSchema, which only exist on the child branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The file lives in src/traceAgentTest/groovy/datadog/trace/common/metrics/ but had no package declaration, so it compiled into the default package and hit java.lang.IllegalAccessError accessing package-private PeerTagSchema (and the other package-private metrics classes it uses) at runtime, failing the traceAgentTest job. Declaring `package datadog.trace.common.metrics` restores legal access. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After moving the test into package datadog.trace.common.metrics, its imports of the package's own classes became same-package imports rejected by CodeNarc's ImportFromSamePackage rule (codenarcTraceAgentTest). Removed them; classes resolve in-package. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r.register spotbugsMain flagged ES_COMPARING_PARAMETER_STRING_WITH_EQ on the intentional reference-equality fast-path that short-circuits the .equals() call when the stored key and probe value are the same instance. Suppressed with @SuppressFBWarnings + justification (repo convention). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Same stale-API problem as on #11402: the Groovy test built entries via AggregateEntry.hashOf(SpanSnapshot) and a (SpanSnapshot,long) constructor that don't exist here (hashOf takes the canonicalized UTF8 field list), so it only failed at runtime in traceAgentTest. Rewritten as a self-contained JUnit 5 Java test that builds entries through AggregateTable.findOrInsert (15-arg SpanSnapshot on this branch -- no additionalTagValues). Agent wiring replicated inline; metrics meter initialized in @BeforeAll. One test, unchanged intent. NOTE: local compile/lint not yet re-verified (Gradle cache was locked by a concurrent build); the file is the verified #11402 migration minus the additionalTagValues arg. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…overy Drops the redundant `final State previous = discoveryState;` local (introduced earlier in the stack); discoverIfOutdated now reads discoveryState.lastTimeDiscovered directly, matching master. Reviewers (sarahchen6, amarziali) flagged this for reversion; the revert had landed on #11402 but not here on #11387 where the change actually originates. File is now identical to master. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…aise RESOURCE limit - Revert DDAgentFeaturesDiscovery: remove orgPropagationMarker field/parser/getter added accidentally; unrelated to this PR's goal - CoreSpan: refactor isKind to delegate to getSpanKindString (removes duplication); use already-imported Tags.SPAN_KIND instead of FQN in getSpanKindString - MetricCardinalityLimits: raise RESOURCE from 128 to 256 — typical service range is 30-200 unique values so 128 sat in the middle of normal workloads - ClientStatsAggregatorTest: add setup() to call resetCardinalityHandlers() before each test, preventing cross-test handler pollution from static state - docs: remove AI-generated client_metrics_design.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Drop the spurious `forceKeep = false` before `break` in ClientStatsAggregator.publish(): the assignment silently discarded a force-keep signal accumulated from earlier spans in the same trace whenever a later span matched an ignored resource. Update three test helpers that were stubbing the removed 2-arg unsafeGetTag(SPAN_KIND, default) overload — the aggregator now calls getSpanKindString() directly, so the old stubs never matched and spanKind was silently null in those tests. Complete the revert of the stray OrgGuard integration in CoreTracer (the DDAgentFeaturesDiscovery side was reverted in the previous commit; the CoreTracer side was missed). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ct=ignore/restart With IGNORE or RESTART, CoreSpanBuilder nulls the remote TagContext parent before buildSpanContext reaches the instanceof TagContext branch that copies requestContextDataAppSec/requestContextDataIast. The AppSec context is lost and GatewayBridge callbacks no-op, silently bypassing WAF/RASP for all inbound requests in AppSec-enabled services configured to ignore distributed tracing extraction. Fix: promote the AppSec/IAST data from the TagContext into the builder fields before nulling the parent. The builder-field path in buildSpanContext applies these after the parent is gone, preserving the request context while still correctly dropping trace identifiers and sampling priority. Fixes: APMSP-3198 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
dougqh
added
comp: asm iast
Contributor
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
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
1 participant
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.
Summary
trace.propagation.behavior.extract=ignoreorrestart,CoreSpanBuilder.startnulls the remoteTagContextparent beforebuildSpanContextreaches theinstanceof TagContextbranch that copiesrequestContextDataAppSec/requestContextDataIastinto theDDSpanContext.DDSpanContextends up with null AppSec context, causing everyGatewayBridgecallback to returnNoopFlow.INSTANCE— silently bypassing WAF/RASP for all inbound requests in AppSec-enabled services configured to ignore distributed tracing extraction. IAST is similarly affected.TagContextinto the builder fields (builderRequestContextDataAppSec/builderRequestContextDataIast), whichbuildSpanContextapplies after the parent is gone. Trace identifiers and sampling priority are still correctly dropped.Test plan
CoreSpanBuilderTest.appSecContextPreservedFromTagContextWithIgnoreBehavior— asserts AppSec/IAST context survivesextract=ignoreCoreSpanBuilderTest.appSecContextPreservedFromTagContextWithRestartBehavior— same forextract=restart./gradlew :dd-trace-core:test --tests "datadog.trace.core.CoreSpanBuilderTest.appSecContext*"Fixes: APMSP-3198
tag: no release note
tag: ai generated
🤖 Generated with Claude Code