Emit additional metric tags on the OTLP stats export path#12069
Draft
dougqh wants to merge 335 commits into
Draft
Emit additional metric tags on the OTLP stats export path#12069dougqh wants to merge 335 commits into
dougqh wants to merge 335 commits into
Conversation
- 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>
Mirrors the TagMap pattern: pairs the existing forEach(Consumer) with a forEach(T context, BiConsumer<T, TEntry>) overload so callers can hand side-band state to a non-capturing lambda and avoid the fresh-Consumer-per-call allocation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Factors the unchecked (TEntry) cast out of D1.forEach / D2.forEach (and the BiConsumer variants) into Support.forEach(buckets, ...). The cast now lives in one place, mirroring how Entry.next() handles it, and the D1/D2 methods become one-liners. Downstream higher-arity tables built on Support gain the same helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds Support.bucket(buckets, keyHash) which returns the bucket head already cast to the caller's concrete entry type. D1.get and D2.get now drop the raw-Entry intermediate variable and walk the chain via Entry.next() directly. The unchecked cast lives in one place, consistent with Entry.next() and Support.forEach. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Holdover from when both lived in a shared HashtableBenchmark; redundant now that each lives in its own class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…bleIterator Three consumer-facing helpers that callers building higher-arity tables on top of Hashtable.Support kept open-coding: - MAX_RATIO_NUMERATOR / _DENOMINATOR: the 4/3 multiplier for sizing a bucket array from a target working-set under a 75% load factor. - insertHeadEntry(buckets, bucketIndex, entry): the (setNext + array-store) pair for splicing a new entry at the head of a bucket chain. - MutatingTableIterator + Support.mutatingTableIterator(buckets): walks every entry in the table (not filtered by hash) with remove() support, for sweeps like eviction and expunge that aren't keyed to a specific hash. Sibling of MutatingBucketIterator. Tests cover the table-wide iterator at head-of-bucket and mid-chain removal, empty buckets between live entries, exhaustion, and remove-without-next. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… 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>
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>
- 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>
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>
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>
…sistency 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>
…etrics-background-work
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>
…optimize-metric-key
- 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>
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>
…read
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>
…optimize-metric-key
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>
…optimize-metric-key
…bility 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>
…bility 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>
Consistent with additionalTagValues: annotation on the type, after final. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Faithful port: all 5 methods (7 cases incl. two 2-row where: blocks), the msgpack-decode ValidatingSink harness, and the 10001-element data build preserved. where: -> @MethodSource / @TableTest. Reviewed clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fold SerializingMetricWriterAdditionalTagsTest into the migrated SerializingMetricWriterTest: extend the shared ValidatingSink to validate the additional-tags payload field (hasAdditionalTags folded into the map-size math; validation block after the peer-tags loop, matching the writer's field order), and move the 3 additional-tags cases (set / omitted / null-slots) over. Delete the standalone test, removing ~110 lines of duplicate msgpack-decode harness. One decode path now. Per Codex review, endorsed by sarahchen6. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…length values is captured against the same additionalTagsSchema, so it is either null or exactly schema.size() long. Guard the array we actually index (values == null || values.length == 0) and document the invariant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ignoredResources is fixed for the aggregator's lifetime and typically empty. Computing !isEmpty() once and gating the per-span lookup on it lets the common case skip both the set lookup and the getResourceName() call. The JIT would hoist the invariant itself; this just reads more clearly and makes the getResourceName() skip explicit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a17ce32 added DD_TRACE_STATS_ADDITIONAL_TAGS to the default experimental-features set, so the =all expansion test must expect it too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Only AggregateEntry.java conflicted; the other overlapping files auto-merged
into the reconciled design (Aggregator.clearAggregate(), AggregateTable.populate()
+ instance PropertyHandlers).
AggregateEntry: kept the reconciled design (instance handlers, populate(),
clearAggregate(), additionalTags, recordDurations) and layered back master's two
requirements from the experimental OTLP writer (OtlpStatsMetricWriter, added
independently mid-reconciliation):
- restored the okDuration/errorDuration split (getOkDuration()/getErrorDuration());
getDuration() now returns their sum for the native msgpack path
- made the class + accessors public (the OTLP writer reads them cross-package)
Also fixed two auto-merge leftovers: AggregateEntryTestUtils.clear() -> clearAggregate(),
and SerializingMetricWriterTest's WithConfigExtension import (package moved to
datadog.trace.test.junit.utils.config).
Verified: dd-trace-core metrics + core.otlp.metrics tests green (19 classes,
0 failures), internal-api ConfigTest green (412, 0 failures).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ng reporter The cardinality-block warn fired once per limited field/tag on every reporting cycle (default 10s), flooding logs indefinitely for any field stuck over its limit. Replace the per-field per-cycle log.warn in the reset paths with a single CardinalityLimitReporter that accumulates block counts by tag name and emits one RatelimitedLogger summary (5-minute window). Counts are keyed by tag name in the reporter rather than held on the handlers, so a peer-tag schema rebuild loses nothing: each reset flushes the cycle's delta by name before the outgoing handler is discarded, and a surviving tag's replacement handler keeps adding to the same entry -- no per-tag transfer or handler reuse needed. The per-cycle HealthMetrics flush is unchanged. The reporter backs its String->count accumulator with the in-house Hashtable.D1 (in-place count mutation, no per-update allocation). Add D1.Entry.key() so the summary can read tag names back when iterating -- the convenience Map-replacement tier should expose the key like a Map entry does. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, rename TagBlockEntry - Drop the static APPEND_ENTRY BiConsumer field; inline the summary lambda into summarize(), still non-capturing via forEach's context argument. - Pre-size the summary StringBuilder from the entry count. - Rename TagBlockCount -> TagBlockEntry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…aren't lost On a peer-tag schema change, a producer can read the old cachedPeerTagSchema just before the swap and enqueue its SpanSnapshot behind the cycle's REPORT signal. That straggler drains the next cycle and increments the retired schema's block counters after the pre-swap flush already ran, so those counts were silently dropped from both HealthMetrics and the rate-limited summary (actual aggregation/sentinel behavior was unaffected -- only the diagnostic count). Hold the outgoing schema in previousPeerTagSchema for exactly one more reset cycle and flush it once more at the top of resetCardinalityHandlers before releasing it. Stragglers land within the next interval (well before the next REPORT), so a one-cycle horizon captures them. Aggregator-thread confined; no producer-side cost. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The addToHash(hash, Object[]) / (hash, Object[], len) additions have no caller on this branch and drop HashingUtils below the 0.7 branch-coverage gate. Split them into their own PR. LongHashingUtils keeps its equivalent additions (still 0.833 branch coverage, above the gate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cover the new array-folding helpers: full-array fold vs an explicit chain, non-zero seed carry-through, partial-len folding, and len==0 (loop not entered) so both loop-branch outcomes are exercised. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fill the coverage gaps called out in review of the span-derived additional metric tags: - Per-value length cap (200): exactly-200 passes, 201 collapses to tracer_blocked_value; tested at the handler level and end-to-end through AdditionalTagsSchema, pinning the shipped magnitude. Also covers the sentinel-disabled path skipping the length branch. Matches the RFC MUST and dd-trace-dotnet's AdditionalTagMaxValueLength=200. - Per-key cardinality budgets are independent across configured keys. - Blocked counts on the additional-tags reset path emit the health metric (collapsed:<tag>) and record to the rate-limited reporter. - Rate-limited blocked counts are retained across suppressed cycles and surface, summed, in the eventually-emitted warning, then cleared. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Limits RFC The approved Cardinality Limits RFC (section 5) tags stats collapses on two distinct health-metric axes: cardinality-limit collapses as collapsed:<field> and per-value length (>200 char) collapses as oversized:<field>, both under the lowercased protobuf field name (additional_metric_tags), not the individual tag key. Previously TagCardinalityHandler lumped both collapse kinds into one blockedCount and AdditionalTagsSchema emitted collapsed:<tagname> for both. Now: - TagCardinalityHandler tracks collapsedCount (cardinality) and oversizedCount (length) separately; reset() returns the cardinality count and oversizedCount() exposes the length count (read before reset()). - AdditionalTagsSchema emits collapsed:additional_metric_tags and oversized:additional_metric_tags, aggregated across keys. - The human-facing rate-limited reporter still records under the specific tag name (the RFC log names the triggering tag). Peer-tag telemetry (statsDTag) is unchanged; peer tags never oversize. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Answers Alexey's review question on MetricCardinalityLimits: 100 is the DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT default from the approved Cardinality Limits RFC (with a note that .NET applies the same magnitude over combinations while we apply it per key -- both sanctioned by the RFC), and 200 is the per-value char cap matching the RFC and dd-trace-dotnet's AdditionalTagMaxValueLength. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ground the 100/200 rationale in the approved Cardinality Limits RFC only, per review preference to not couple our docs to dd-trace-dotnet internals. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fixes - Rename PropertyHandlers -> CoreHandlers and resetPropertyHandlers/ resetHandlers -> resetCoreHandlers; the meaningful axis is core (always present) vs peer (remote config) vs additional (local config), and property-vs-tag is an implementation detail of the core set. Reorder the AggregateTable/Canonical constructors so coreHandlers precedes additionalTagsSchema. - Add MetricCardinalityLimits.USE_BLOCKED_SENTINEL: a compile-time escape hatch (shipped true) to revert to the pre-capping behavior during the internal rollout; wire all handler construction sites to it. Replaces the bogus MetricCardinalityLimits.ENABLED javadoc link. - Move the previous-peer-tag-schema flush into reconcilePeerTagSchema(), ahead of the swap; drop "straggler"/"lockstep" jargon. - Rename Canonical.populate -> populateFrom. - Assorted javadoc/comment fixes (thread-confinement note, getDuration, post-increment style). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per the Span-Derived Primary Tags V1 RFC, an invalid (<= 0) DD_TRACE_STATS_*_CARDINALITY_LIMIT must fall back to the default rather than fail. getTraceStatsCardinalityLimit now clamps non-positive values to the caller-supplied default and logs at debug; previously such a value flowed into the handler constructors and threw IllegalArgumentException, breaking ClientStatsAggregator construction. Also correct the MetricCardinalityLimits javadoc, which wrongly claimed the additional-tag limit is not configurable -- it is resolved via DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT, matching the RFC. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The approved Cardinality Limits RFC keys collapse telemetry by the lowercased protobuf field name. Two core-handler tags didn't match: - operation is carried on the wire as the field "name", so its health tag is now collapsed:name (the human-facing CardinalityLimitReporter still says "operation"). PropertyCardinalityHandler gains a decoupled statsDField for this case. - peer tags now aggregate into a single collapsed:peer_tags health metric across all configured peer tags (mirroring AdditionalTagsSchema), instead of emitting a per-tag-name collapsed:<tag> metric. Per-name detail is preserved in the human-facing reporter. TagCardinalityHandler.statsDTag() is now unused and removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Span-Derived Primary Tags RFC specifies the additional_metric_tags field is present whenever the feature is configured -- as an empty array for spans that matched no configured key -- not gated on whether a given entry carried any tags. SerializingMetricWriter previously omitted the field whenever the entry's packed array was empty, which conflated "feature off" with "feature on, no match". It now takes a configured flag (threaded from AdditionalTagsSchema.size() > 0) and emits the field, empty array included, whenever configured; when the feature is off the field is omitted entirely so non-users pay zero payload overhead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The httpMethod/httpEndpoint/grpcStatusCode/additionalTagValues fields are @nullable but their constructor params weren't, disagreeing with the already-annotated peerTagSchema/peerTagValues params. Annotation-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- AggregateEntry.errorLatencies: note it is not thread-safe (bric3 suggestion) - PeerTagSchema.resetHandlers: adopt bric3's clearer javadoc wording Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The native SerializingMetricWriter path already carries user-configured span-derived additional metric tags (DD_TRACE_STATS_ADDITIONAL_TAGS), but the OTLP export path dropped them twice: ClientStatsAggregator built the OTLP writer through the schema-less (EMPTY) constructor, and OtlpStatsMetricWriter never read entry.getAdditionalTags(). Route the OTLP aggregator ctor through the schema-carrying ctor (capture is already unconditional once the schema is real) and split each packed "key:value" additional tag into a plain OTLP string attribute keyed by the tag name, in both default and otel-semantics modes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dougqh
added
comp: metrics
This comment has been minimized.
This comment has been minimized.
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.
What
Emit user-configured additional metric tags (
DD_TRACE_STATS_ADDITIONAL_TAGS) on the OTLP client-side-stats export path, matching the native msgpack (SerializingMetricWriter) path.Why
Additional metric tags landed for the native path in #11402. On the OTLP path they were dropped twice:
ClientStatsAggregatorconstructedOtlpStatsMetricWriterthrough the schema-less (AdditionalTagsSchema.EMPTY) aggregator constructor, so the tags were never captured off the span.OtlpStatsMetricWriternever readentry.getAdditionalTags(), so even a populated entry wouldn't emit them.How
additionalTagsSchemaFrom(config)). Capture + per-cycle reset are already unconditional once the schema is real, so no other aggregator change is needed.OtlpStatsMetricWriter, split each packed"key:value"additional tag at the first:and emit it as a plain OTLP string attribute keyed by the tag name, in both default and otel-semantics modes. Malformed slots (no:, empty key/value) are skipped defensively.Open question (cross-team)
The attribute-key representation — raw tag name vs a
datadog.*namespace — is an open question with the OTLP/agent side. Flagged inline in the code; happy to adjust the key scheme once that's settled.Testing
New JUnit 5 cases in
OtlpStatsMetricWriterTestdecode the emitted protobuf and assert::)datadog.*)Stacked on #11402 (native additional-tags) — this is that seam's OTLP counterpart.
🤖 Generated with Claude Code