Emit additional metric tags on the OTLP stats export path (client-side stats) - #12069
Emit additional metric tags on the OTLP stats export path (client-side stats)#12069dougqh wants to merge 4 commits into
Conversation
dougqh
added
comp: metrics
|
🎯 Code Coverage (details) 🔗 Commit SHA: 94e310e | Docs | Datadog PR Page | Give us feedback! |
🟢 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. |
PerfectSlayer
added
type: feature
|
@codex review |
💡 Codex Review Bound the configurable additional-tag table size
When Snapshot the new configuration in Config
When this setting is enabled, the getter re-queries AGENTS.md reference: AGENTS.md:L37-L44  Preserve empty additional-tag values in OTLP
When a configured span tag is explicitly set to Prevent additional tags from duplicating built-in attributes
When a configured key matches an attribute already emitted above, such as Cap the combined additional-tag cardinality
perf: When multiple configured tags vary independently, giving each key its own budget does not bound the aggregate-key cardinality: two keys with the default limit of 100 permit 10,000 tuples, while the normal aggregate-table default is 2,048 rows and the tight-heap default is only 256. Once those rows are live in a reporting cycle, AGENTS.md reference: AGENTS.md:L77-L77 Move the new unit test to JUnit 5
This newly added case is a core metrics unit test, not an instrumentation or smoke test, but it is implemented as a Spock feature in a Groovy file. Move this coverage to a JUnit 5 Java test so the added test follows the repository's explicit framework requirement. AGENTS.md reference: AGENTS.md:L63-L63 Include additional tags in the test equality contract
The new overload folds  Remove the unused production-only test helper
A repo-wide search at this commit finds no caller of AGENTS.md reference: AGENTS.md:L76-L76 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
| // values may) and emits it as an OTLP string attribute. Skips malformed slots (no ':', empty key | ||
| // or empty value) defensively. | ||
| private static void emitAdditionalTag(OtlpMetricVisitor metric, UTF8BytesString additionalTag) { | ||
| String packed = additionalTag.toString(); |
There was a problem hiding this comment.
Not thrilled with the need to split here, but I'm leaving client-side stats optimized for interaction with Datadog agent and our protocol for the moment.
The problems also run deeper than just the split.
There is also an issue with encoding to UTF8 as well.
That's probably solvable with the our UTF8 cache, but I'm leaving that to another PR.
dougqh
changed the title
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Please try again by commenting @autotest review.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a2e8c49f4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| metric.visitAttribute( | ||
| STRING_ATTRIBUTE, packed.substring(0, separator), packed.substring(separator + 1)); |
There was a problem hiding this comment.
When DD_TRACE_STATS_ADDITIONAL_TAGS contains a name already emitted by this writer, such as span.name, status.code, or http.response.status_code, this raw key creates two KeyValue entries for the same OTLP attribute; the latter example can even have both integer and string values. Receivers may retain different occurrences, so built-in dimensions and their types become ambiguous. Reserve these names, skip duplicates, or settle the namespace before emitting user tags.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same as the above Datadog comment.
3a2e8c4 to
763cf32
Compare
|
Thanks @codex. Reconciling against the current branch — this review was posted against Resolved on the base PR #11402 (this branch was rebased onto it):
Fixed on this branch: Accepted by design / deferred: |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
A user can configure an additional metric tag named http.route, service.name, or another standard OTLP attribute because the schema only rejects empty and colon-containing keys. The writer then appends that tag after the standard attribute, producing duplicate keys with conflicting values and potentially corrupting semantic dimensions for downstream OTLP consumers.
🤖 Datadog Autotest · Commit d61f2a0 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| // "key:value" UTF8 strings in schema order. Emitted in both modes as plain OTLP string | ||
| // attributes keyed by the tag name. NOTE: the attribute-key representation (raw tag name vs a | ||
| // datadog.* namespace) is an open cross-team question with the OTLP/agent side -- see the PR. | ||
| for (UTF8BytesString additionalTag : entry.getAdditionalTags()) { |
There was a problem hiding this comment.
OTLP client-side metric points can have corrupted HTTP semantic attributes or be rejected when a valid user configuration overlaps a standard OTLP key.
Assertion details
- Input: Set DD_TRACE_STATS_ADDITIONAL_TAGS to
http.routeand process a span that has an HTTP route tag of/users/{id}but an additional-tag value such ascustom-route. - Expected: The emitted data point has one unambiguous
http.routesemantic attribute representing the standard HTTP route, while the user-configured dimension is represented without colliding with reserved OTLP keys. - Actual: emitDataPointAttributes writes the standard
http.routeattribute first, then emitAdditionalTag writes anotherhttp.routeattribute from the packed user tag. The protobuf and JSON collectors append both entries; there is no collision check or namespace, so consumers that collapse attributes by key can observecustom-routeas the HTTP route, and consumers that reject duplicate keys can reject the point.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
@dougqh Is this a valid concern? Should we handle this the same way that native CSS handles it?
There was a problem hiding this comment.
I had Claude do an analysis of this for both the Datadog & OTLP serialization. Here are the findings...
Yes, it's a valid concern — but it's specific to the OTLP path, and native doesn't have it by design.
On the native msgpack path (SerializingMetricWriter), additional tags are written into a dedicated AdditionalMetricTags array, structurally separate from the built-in dimensions (Resource, HTTPStatusCode, PeerTags, …). So configuring http.route as an additional tag lands in its own section and never touches the built-in route — no collision possible.
OTLP data-point attributes are a single flat KeyValue bag. We emit the built-ins first (span.name, span.kind, service.name, http.request.method, http.response.status_code, http.route, rpc.response.status_code, status.code, plus datadog.* in default mode), then loop the user's additional tags with raw keys. So a user tag whose name equals a built-in produces two entries for the same key, and a receiver may keep either or reject the point. That's exactly what the P1 describes.
The two ways to reproduce native's separation on a flat bag are (a) a key namespace like datadog.*, or (b) don't let a user tag overwrite a built-in. Since we've settled on raw keys (a) is out, so the fix has to be collision-avoidance at emit time: reserve the built-in attribute keys and skip any additional tag that collides — built-in wins. Corrupting or duplicating a standard semantic attribute is worse than dropping a user tag that's almost certainly a misconfiguration (the name is already emitted as a first-class dimension).
One implementation note: this reservation must live in OtlpStatsMetricWriter, not in the shared AdditionalTagsSchema. On the native path those same names are legitimate and non-colliding, so rejecting them at schema construction would wrongly break native. I'll compute the collision set once at writer construction (intersect the configured tag names with the built-in key set, warn once), and skip those slots at emit — no per-point cost.
I'll push that change; happy to revisit if we ever reintroduce a namespace.
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>
Aggregation treats an explicitly-empty configured tag ("key:") as a distinct
dimension from an absent one, so it forms a separate aggregate row. The OTLP
writer dropped the empty-value slot, which meant an absent-tag row and an
empty-value row exported identical attribute sets -- two separately-aggregated
points indistinguishable to the backend. Emit the empty value as key="" so the
OTLP attributes stay faithful to the aggregate key; only truly-malformed slots
(no ':' or empty key) are still skipped.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The helper's equals() omitted getAdditionalTags() while keyHash folds it in, so entries differing only in additional tags compared equal yet produced different hash codes -- letting matcher-based tests silently accept missing or wrong additional dimensions. Compare the arrays positionally (schema order) to match the key identity. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d61f2a0 to
94e310e
Compare
mhlidd
left a comment
mhlidd
left a comment
There was a problem hiding this comment.
LGTM following fixes to Codex comments.
| // "key:value" UTF8 strings in schema order. Emitted in both modes as plain OTLP string | ||
| // attributes keyed by the tag name. NOTE: the attribute-key representation (raw tag name vs a | ||
| // datadog.* namespace) is an open cross-team question with the OTLP/agent side -- see the PR. | ||
| for (UTF8BytesString additionalTag : entry.getAdditionalTags()) { |
There was a problem hiding this comment.
@dougqh Is this a valid concern? Should we handle this the same way that native CSS handles it?
| // Additional metric tags: user-configured span-derived dimensions, carried as packed | ||
| // "key:value" UTF8 strings in schema order. Emitted in both modes as plain OTLP string | ||
| // attributes keyed by the tag name. NOTE: the attribute-key representation (raw tag name vs a | ||
| // datadog.* namespace) is an open cross-team question with the OTLP/agent side -- see the PR. |
There was a problem hiding this comment.
Discussed w/ Munir - let's keep the raw key instead of datadog.*
| metric.visitAttribute( | ||
| STRING_ATTRIBUTE, packed.substring(0, separator), packed.substring(separator + 1)); |
There was a problem hiding this comment.
Same as the above Datadog comment.
3 participants
What Does This Do
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.Motivation
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.Additional Notes
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