[APMSVLS-485] fix(traces): bound trace stats cardinality and report collapsed keys - #1338
Conversation
Use libdatadog's default cardinality limits for trace stats aggregation instead of leaving cardinality unbounded. Functions whose span resources embed request ids or path parameters no longer grow concentrator memory and the stats payload without bound; keys beyond a limit collapse into the overflow bucket, matching the trace agent's behavior. 🤖
Bounding stats cardinality is silent: keys past a limit are rewritten to `tracer_blocked_value` and the affected stats stop being attributable, with nothing in the logs. Warn once per sandbox per signal instead. Two signals are needed, because they fail in opposite directions: - whole-key overflow, read from `FlushResult.collapsed_spans`, which carries a span count; - per-field collapse, found by scanning the flushed payload for the sentinel, which does not. `collapsed_spans` alone would be silent for the canonical Lambda failure. Per-field limits are applied before the whole-key limit, so a single dimension exploding -- request ids or path parameters in resource names -- collapses resources at 1024 first, which shrinks the distinct whole-key space and can stop `collapsed_spans` ever leaving 0. The payload scan covers that case: per-field collapse rewrites only the field that overflowed, while the whole-key overflow entry has every field set to the sentinel, so `service` (never rewritten per-field) identifies and skips it. The per-field counters libdatadog already keeps are not usable here: `StatsBucket::collapsed_fields_metrics()` is reachable and Copy, but its per-combination counts have no public accessor without the dogstatsd or telemetry features, which bottlecap does not enable. Once upstream exposes them this can report how many keys collapsed rather than only which fields. No environment variable is named. The limits are not customer-tunable here, and both candidates mislead: libdatadog's own message blames DD_TRACE_STATS_CARDINALITY_LIMIT, which bottlecap does not read at all, and DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT governs only additional tags. Reducing application-side cardinality is the only real remediation, so that is what the warnings recommend. No per-flush debug! either -- libdatadog already emits one for whole-key overflow. 🤖
|
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Bounds trace-stats cardinality using libdatadog defaults and reports collapsed aggregation keys.
Changes:
- Enables default per-field and whole-key cardinality limits.
- Adds once-per-sandbox collapse warnings.
- Adds collapse and cardinality regression tests.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if stats.service == TRACER_BLOCKED_VALUE { | ||
| continue; | ||
| } | ||
| if stats.resource == TRACER_BLOCKED_VALUE { |
![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: 018dfdc364
ℹ️ 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".
| if stats.service == TRACER_BLOCKED_VALUE { | ||
| continue; | ||
| } | ||
| if stats.resource == TRACER_BLOCKED_VALUE { |
There was a problem hiding this comment.
When an application legitimately uses tracer_blocked_value as a service, resource, or HTTP endpoint, this payload scan becomes ambiguous: a matching service suppresses real per-field collapse warnings, while a matching resource or endpoint emits a false warning even when no limit was exceeded. Use collapse metadata from the concentrator (or expose an upstream per-field signal) rather than inferring collapse solely from field equality.
Useful? React with 👍 / 👎.
| /// | ||
| /// Both are needed. Per-field limits are applied *before* the whole-key limit | ||
| /// (`aggregation.rs:589` runs ahead of the check at `:603`), so a single-dimension explosion — | ||
| /// request ids in resource names, the canonical Lambda failure — collapses resources first, | ||
| /// which shrinks the distinct whole-key space and can stop `collapsed_spans` ever leaving 0. |
There was a problem hiding this comment.
Condense this 20-line rationale and the similarly extended new doc blocks to the invariant needed to maintain the function; the upstream line-number narrative, rejected-alternative history, and future-work discussion make the implementation harder to scan and directly conflict with the repository requirement to keep comments concise.
AGENTS.md reference: AGENTS.md:L13-L16
Useful? React with 👍 / 👎.
Stacked PRs:
datadog-agent-trace-samplercrate serverless-components#141libdatadogto72fa8685andserverless-componentsto9daae40#1332Overview
Adopts libdatadog's default trace-stats cardinality limits, and reports it when they collapse
keys. Split out of #1332 so the customer-visible half of that bump gets reviewed on its own
rather than buried in a
chore(deps)PR.1.
fix(traces): bound trace stats cardinalityPass
Some(CardinalityLimitConfig::default())and take libdatadog's defaults (Noneis equivalent — libdatadog resolves it withunwrap_or_default()— but passing the defaults explicitly makes the limits the collapse warnings quote provably the ones in force, instead of a second copy that could drift) — 7000 whole-key, 1024resource, 512 http endpoint, 512 peer tags, 100 additional tags. #1332 pins all five to
effectively-unbounded values to preserve pre-bump behavior, which also opts out of the four new
per-field protections: a function whose span resources embed request ids or path parameters
yields one aggregation group per distinct resource per 10s bucket, each carrying two ddsketches,
so concentrator memory and the
/v0.6/statspayload grow without bound inside a memory-cappedLambda. Defaults also restore parity with SCL's
datadog-trace-agent(same libdatadog rev) andthe Go agent, both of which keep them.
User-visible: for functions with high-cardinality span resources, resources past the 1,024th
in a 10-second bucket are now reported under
tracer_blocked_valueinstead of individually. Thatis the intended trade for bounded memory.
This also removes the four unbounded per-field tracking
HashSet<u64>s that #1332's pinningnecessarily leaves behind (bounding them is collapsing, so no pinning avoids both), which is
why these two PRs should not be split across a release.
2.
feat(traces): report collapsed stats keysCollapse is otherwise silent — keys are rewritten to
tracer_blocked_valueand the affectedstats stop being attributable, with nothing in the logs. Warn once per sandbox per signal.
Two signals are needed, because they fail in opposite directions:
FlushResult.collapsed_spanscollapsed_spansalone would be silent for the canonical Lambda failure. Per-field limits areapplied before the whole-key limit (
aggregation.rs:589runs ahead of the check at:603), soone dimension exploding — request ids in resource names — collapses resources at 1,024 first,
which shrinks the distinct whole-key space and can stop
collapsed_spansever leaving 0. Thecounter bottlecap can read is the one least likely to fire on the case that matters.
The payload scan covers it: per-field collapse rewrites only the field that overflowed, while
the whole-key overflow entry has every field set to the sentinel, so
service— never rewrittenper-field — identifies and skips that entry. Otherwise one whole-key overflow would masquerade as
all four fields collapsing at once.
Why not libdatadog's per-field counters.
StatsBucket::collapsed_fields_metrics()isreachable and
Copy, but its per-combination counts have no public accessor without thedogstatsd/telemetryfeatures, which bottlecap does not enable (libdd-trace-statsis declareddefault-features = false, onlyhttps/fips). Once upstream exposes them, this can reporthow many keys collapsed rather than only which fields. Tracked as a follow-up libdatadog PR we will author (not a Jira ticket); not a blocker for this stack.
Why no environment variable is named. The limits are not customer-tunable here, and both
candidates mislead: libdatadog's own message blames
DD_TRACE_STATS_CARDINALITY_LIMIT, whichbottlecap does not read at all, and
DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMITgovernsonly
additional_tags. Reducing application-side cardinality is the only real remediation, sothat is what the warnings recommend. No per-flush
debug!either — libdatadog already emits onefor whole-key overflow at
span_concentrator/mod.rs:442, and it would be a near-duplicate.TRACER_BLOCKED_VALUEis hand-copied: upstream declares itpub, butmod aggregationisprivate and it is not re-exported, so it cannot be imported. A one-line upstream
pub usewouldremove the copy; also tracked as a follow-up libdatadog PR.
Testing
cargo test— 544 + 20 tests passcargo clippy --all-targets -- -D warnings— cleancargo fmt --check— cleanNew tests, deliberately kept distinct because the obvious ones assert the wrong thing:
test_observe_collapsed_fields— each field detected independently; the whole-key overflowentry is skipped rather than counted as all four collapsing; both tag lists covered, which
encode a valueless sentinel differently (
peer_tagsas the bare key,additional_metric_tagswith a trailing colon); collapse in any bucket of the flush counts.test_resource_collapse_observed_without_whole_key_overflow— the regression test for thetrap above. Exceeds
resource_limit(1,024) while staying underwhole_key_limit(7,000), andasserts the collapse is observed while asserting no whole-key overflow entry exists.
test_collapse_warns_once_per_signal— each signal warns at most once, and the two areindependent, so whole-key overflow does not suppress a later per-field collapse. Also the only
coverage of the whole-key branch:
new()always builds the concentrator with libdatadog'sdefaults, so driving keys past 7,000 without first tripping 1,024 is not reachable through the
public API. Asserts on service state, not log output.
test_cardinality_limit_applied(from commit 1) — note this passes via per-field collapseand says nothing about
collapsed_spans; it is not a substitute for the two above.