[APMSVLS-485] feat(traces): span-derived primary tags - #1336
Open
lucaspimentel wants to merge 7 commits into
Open
[APMSVLS-485] feat(traces): span-derived primary tags#1336lucaspimentel wants to merge 7 commits into
lucaspimentel wants to merge 7 commits into
Conversation
|
lucaspimentel
force-pushed
the
lpimentel/span-derived-primary-tags
branch
from
August 21, 2026 20:17
c87989a to
f99baf9
Compare
lucaspimentel
force-pushed
the
lpimentel/bump-libdatadog-72fa8685
branch
from
August 21, 2026 21:46
5110d8d to
7996ec2
Compare
This was referenced Aug 21, 2026
lucaspimentel
force-pushed
the
lpimentel/span-derived-primary-tags
branch
from
August 21, 2026 23:31
f99baf9 to
7ff07f2
Compare
lucaspimentel
changed the base branch from
lpimentel/bump-libdatadog-72fa8685
to
lpimentel/bound-stats-cardinality
August 21, 2026 23:31
Member
Author
|
@codex 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: 7ff07f23b8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Adds experimental span-derived dimensions to Lambda trace-stat aggregation.
Changes:
- Parses and gates additional tag configuration.
- Wires tag keys and cardinality limits into the stats concentrator.
- Adds validation, warnings, and tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
bottlecap/src/config/mod.rs |
Adds gated configuration and tests. |
bottlecap/src/traces/stats_concentrator_service.rs |
Applies tags and cardinality limits. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
lucaspimentel
force-pushed
the
lpimentel/bound-stats-cardinality
branch
from
August 25, 2026 13:39
4a88378 to
1ca9352
Compare
lucaspimentel
force-pushed
the
lpimentel/span-derived-primary-tags
branch
from
August 25, 2026 13:43
a201e5c to
80b1f16
Compare
lucaspimentel
marked this pull request as ready for review
August 25, 2026 15:50
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a446908753
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // datadog.yaml whenever the gate itself only arrives with the later env-var pass. | ||
| // `apply_experimental_features_gate` applies the gate once, after every source has | ||
| // merged. | ||
| if !source.trace_stats_additional_tags.is_empty() { |
There was a problem hiding this comment.
When datadog.yaml supplies trace_stats_additional_tags and DD_TRACE_STATS_ADDITIONAL_TAGS is explicitly set to an empty value, the environment source deserializes to an empty vector and this condition skips the assignment, leaving the lower-precedence YAML keys enabled. This prevents operators from clearing YAML-provided dimensions through the environment even though environment settings are expected to override YAML; preserve whether the field was present (for example with an optional source value) and merge an explicitly empty list.
Useful? React with 👍 / 👎.
lucaspimentel
changed the title
Wire DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED, DD_TRACE_STATS_ADDITIONAL_TAGS, and DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT into StatsConcentratorService, matching the Serverless Compatibility Layer (datadog-trace-agent). Lets users configure span meta keys as additional stats aggregation dimensions (ClientGroupedStats.additional_metric_tags), gated behind the experimental features flag. libdd-trace-stats (pinned via #1332) already implements additional_metric_tag_keys end-to-end; this only adds the bottlecap-side config plumbing. The deprecated span_derived_primary_tags proto field (superseded by additional_metric_tags) intentionally stays empty. Depends on #1332 (libdatadog/serverless-components rev bump); base this branch on lpimentel/bump-libdatadog-72fa8685 until that merges.
libdatadog warns about out-of-range cardinality limits but still applies them. Validate DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT before passing it through: - 0 collapsed every additional metric tag into tracer_blocked_value; now falls back to libdatadog's default of 100. - Values at or above the whole-key limit (7000) are clamped to 6999, mainly to silence libdatadog's misconfiguration warning. Per-field limits are applied before the whole-key limit, so such a value is effectively unbounded rather than inert; either way, reaching it needs ~7k distinct tag combinations in a single 10s bucket, which will not happen in a Lambda invocation. Both log a warning naming the effective value.
libdatadog aggregates on at most 4 additional metric tag keys, sorting alphabetically and dropping the rest, so excess keys are chosen by alphabetical accident rather than by anything the user expressed. Its warning names the dropped keys but not the kept ones, the selection rule, or the env var; restate all three in bottlecap's voice. Truncation itself stays in libdatadog. Also clarify the cardinality limit warnings. The Go trace agent reads 0 as "no cap", so spell out that 0 does not mean unlimited here and point at unsetting DD_TRACE_STATS_ADDITIONAL_TAGS to actually disable the dimension. Per-field limits apply before the whole-key limit, so a limit at or above the whole-key limit is effectively unbounded rather than inert; the clamp mainly silences libdatadog's misconfiguration warning.
The excess-key warning predicted libdatadog's normalization instead of reading it: a hand-copied `MAX_ADDITIONAL_METRIC_TAG_KEYS = 4` mirroring a private upstream constant, plus a local re-implementation of its sort/dedup/truncate. Both could drift silently, and the copy would then name the wrong keys as dropped. libdatadog already exposes the survivors via `SpanConcentrator::additional_metric_tag_keys()`, so ask for them instead: diff the requested list against the kept list, move the warning to after `SpanConcentrator::new`, and delete the constant and the mirror. The effective cap is now `kept.len()` rather than a number we assert on faith. The reworked test builds a real concentrator and checks which keys survive, so it exercises upstream's actual rule -- and confirms the cap is in fact 4, which nothing previously verified. Also drop the #1332 reference from the `resolve_cardinality_limits` doc comment; it is stale once that PR merges, and the rationale reads better stated directly. 🤖
….yaml Additional trace stats tags configured in datadog.yaml were silently dropped whenever the experimental-features gate was enabled through the environment instead. Config sources merge one at a time (datadog.yaml first, then env vars), and the gate was checked during each merge, so the yaml values were cleared before the env-var pass could turn the gate on. The gate now runs once, after every source has merged, so the gate and the values it gates can come from different sources in either order. 🤖
…tats field When too many distinct additional metric tag sets collapsed, the warning advised removing request ids and path parameters from resource names, which has no effect on additional-tag cardinality. That case now points at the knobs that do help: listing fewer keys in DD_TRACE_STATS_ADDITIONAL_TAGS, choosing keys with fewer distinct values, or raising DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT. The other fields keep the reduce-cardinality advice. 🤖
lucaspimentel
force-pushed
the
lpimentel/span-derived-primary-tags
branch
from
August 25, 2026 17:15
a446908 to
2544bdb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked PRs:
datadog-agent-trace-samplercrate serverless-components#141libdatadogto72fa8685andserverless-componentsto9daae40#1332Overview
Adds bottlecap-side config for span-derived primary tags (APMSVLS-485), matching the Serverless Compatibility Layer (
datadog-trace-agent):DD_TRACE_EXPERIMENTAL_FEATURES_ENABLEDfalseDD_TRACE_STATS_ADDITIONAL_TAGSmetakeys to use as additional stats aggregation dimensions.DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT100These feed
StatsConcentratorService→SpanConcentrator::new'sadditional_metric_tag_keys, whichlibdd-trace-stats(via #1332's bump to72fa8685) already implements end-to-end: extraction from spanmeta, inclusion in the aggregation key hash, cardinality limiting, and export asClientGroupedStats.additional_metric_tags. So this PR is pure wiring, exactly as #1332 predicted.Both gated settings reset to their defaults when the experimental gate is off, so a stale env var can't leak through.
No libdatadog change is needed. The ticket points at
span_derived_primary_tags(field 22) being stubbed asvec![], but that field is deprecated in favor ofadditional_metric_tags(field 23) — see the comment instats.proto. Field 22 intentionally stays empty; this PR uses the modern replacement.Misconfiguration handling
libdatadog warns about bad input but still applies it, so both settings are validated here and the warnings restated in bottlecap's terms.
DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT:0would collapse every additional metric tag into thetracer_blocked_valuesentinel — silent, total data loss for the dimension → falls back to the default of100. Worth noting the Go trace agent reads0as no cap, so a user carrying that setting over would otherwise lose exactly what they meant to keep unbounded. "Unbounded" is deliberately not offered: chore(deps): updatelibdatadogto72fa8685andserverless-componentsto9daae40#1332 bounded this precisely to cap concentrator memory in a memory-capped Lambda.7000) → clamped to6999, mainly to silence libdatadog's misconfiguration warning. Per-field limits are applied before the whole-key limit, so such a value is effectively unbounded rather than strictly inert; either way, reaching it needs ~7k distinct tag combinations inside a single 10s bucket, which will not happen in a Lambda invocation.DD_TRACE_STATS_ADDITIONAL_TAGS: libdatadog aggregates on at most 4 keys, sorting alphabetically and dropping the rest. Sozone,tenant_id,region,shard,customersilently dropszone— by alphabetical accident, not by anything the user expressed. This is arguably the nastier failure of the two, since the config appears to work and only the dimensions are wrong. libdatadog's warning names the dropped keys but not the kept ones, the selection rule, or the env var, so bottlecap now logs all three. Truncation itself stays in libdatadog.SCL parity divergence
serverless-components@9daae40applies this env var raw (.parse::<usize>().ok(), no range check), so after this PRDD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT=0means "default100" in Lambda and "collapse everything" in Azure Functions. Given the SCL-parity framing,resolve_cardinality_limitsshould be ported upstream todatadog-trace-agentas a follow-up.On per-bucket cardinality resets
The cardinality limit is tracked per 10s
StatsBucketand resets per bucket. Unlike the rate limiter in #1320, this is not broken by Lambda freeze/thaw, so it is deliberately left alone:span.start + span.duration), not processing time. A freeze just routes the next invocation's spans into a different bucket, which is correct.Bucketing stays load-bearing for the concentrator memory and
/v0.6/statspayload bounds.Testing
Config (
src/config/mod.rs) — the experimental gate on/off for both settings, comma-separated parsing with surrounding whitespace, and an unparseable cardinality limit falling back toNone.Concentrator (
src/traces/stats_concentrator_service.rs):test_additional_metric_tags_populated_when_configured— a configured key present in spanmetasurfaces asdatacenter:us-east-1on the exportedClientGroupedStats.test_additional_metric_tags_empty_by_default— unset config exports no additional tags even when the span carries a matchingmetakey.test_resolve_cardinality_limits— unset/0/in-range/at-or-above-whole-key validation, and that overridingadditional_tags_limitleaves the other limits at their defaults.test_kept_and_dropped_additional_metric_tag_keys— the alphabetical keep/drop split reported for an over-cap key list, derived from the concentrator's ownadditional_metric_tag_keys()getter (so the kept set can't drift from libdatadog's real cap), and that duplicates collapse before the cap applies.cargo test --lib,cargo clippy --lib --all-targets -- -D warnings, andcargo fmt --checkall pass.