Wire per-component cardinality limits from Config#11732
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75f72165fd
ℹ️ 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".
| return configProvider.getInteger( | ||
| "trace.stats." + tagName + ".cardinality.limit", defaultLimit); |
There was a problem hiding this comment.
Validate stats cardinality limits before returning them
When any of the new DD_TRACE_STATS_*_CARDINALITY_LIMIT values is set to 0, a negative number, or a value above 2^29, this method returns it unchanged; the callers added in AggregateEntry and PeerTagSchema pass it directly into PropertyCardinalityHandler/TagCardinalityHandler, whose constructors throw for those ranges. In that misconfigured environment the stats classes fail initialization instead of falling back to the default/logging, so the accessor should reject invalid limits before handing them to the handlers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
In my ideal world, config would probably handle this and report a warning immediately at start-up.
Unfortunately, there's no precedence for that in config today.
I'm in full agreement with the sentiment, but I think that's better left to another PR.
| "DD_TRACE_STATS_RESOURCE_CARDINALITY_LIMIT": [ | ||
| { | ||
| "version": "A", | ||
| "type": "integer", |
There was a problem hiding this comment.
Use
int for the new config metadata type
The supported-configurations metadata uses type: "int" for the existing integral settings (including the tracer metrics limits), while these ten new cardinality entries are the only ones using "integer". The local supported-configurations validation/publishing jobs consume this file, so the new configs can be rejected or omitted by downstream metadata consumers even though the agent code reads integers; please use int consistently for these entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yup, int is used instead of integer in the file.
There was a problem hiding this comment.
@mhlidd WDYT to add a test that will check types and failed if there someone made a typo?
OR probably we can support some sort of aliases like int, integer should be treated as the same type...
But I like the test better.
There was a problem hiding this comment.
No need for aliasing, I'll just fix it. I'm curious why Claude decided to use the wrong value though.
There was a problem hiding this comment.
Yep, Just thinking how to prevent issues in future.
Fir example if there will be a test - Claude with high probability will run it, test will fail, Claude will fix it.
I'm curious why Claude decided to use the wrong value though.
I think it is a nature of LLMs, somehow integer has a bigger weight in model that int and LLM produced wrong output.
🟢 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. |
AlexeyKuznetsov-DD
left a comment
There was a problem hiding this comment.
LGTM, but:
- Code like:
Config.get()
.getTraceStatsCardinalityLimit(
"peer_tag", MetricCardinalityLimits.PEER_TAG_VALUE))
looks a bit noisy, probably can be improved if refactored to enum?
2. JSON descriptor should be fixed integer -> int.
| "resource", | ||
| Config.get().getTraceStatsCardinalityLimit("resource", MetricCardinalityLimits.RESOURCE)); |
There was a problem hiding this comment.
QQ: just curious if this MetricCardinalityLimits can be an enum and no need to pass string and int const. Just enum and get the rest from it where needed.
There was a problem hiding this comment.
Yeah, we might be able to do that. The only complication is that we might have to move the enum next to Config.
1741d4f to
b23f870
Compare
This comment has been minimized.
This comment has been minimized.
b23f870 to
2418d84
Compare
…its RFC Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2418d84 to
5b550dd
Compare
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y limits Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
What Does This Do
Adds the ability to configure cardinality limits per component / tag for client-side stats
Motivation
Avoid problems with high cardinality leading to excessive resource consumption while informing the customer (via sentinel value) when a limit was reached
Additional Notes
Unlike #11387 which introduced the limiting mechanism, this PR enables the limits by default.
Config.getTraceStatsCardinalityLimit(tagName, defaultLimit)method following the RFC naming patternDD_TRACE_STATS_{tag_name}_CARDINALITY_LIMITPropertyCardinalityHandlerinstances and theTagCardinalityHandlerinPeerTagSchemato read their limits from Config at class-load time, with defaults fromMetricCardinalityLimitsuseBlockedSentinel = true) now that limits are configurable; previously hardcoded tofalsepending this wiringsupported-configurations.jsonStacked on #11387.
Test plan
CardinalityHandlerTestandAggregateEntryTestcover sentinel substitution behaviorMetricCardinalityLimitsconstantsDD_TRACE_STATS_RESOURCE_CARDINALITY_LIMITetc. are picked up at startup via Configtag: no release notetag: ai generated🤖 Generated with Claude Code