enhancement(tag_cardinality_limit transform): Add more fine grained controls tag cardinality#25360
Conversation
|
btw. Do you have any use case why include metrics exclusion to cardinality_tags would be better compare to current possible solution: route + remap which should be more sufficient instead scan cardinality list? If you connect it with enrichment table you can manage metrics from external csv. :FYI another PR for tags exclusion: #25316. |
df32f77 to
f64d3bb
Compare
|
@kaarolch This would be if you wanted to different limits for tags in a metric (or if only one tag on a metric should be excluded from the limit) |
|
Took a look at your PR, if we add support for per tag limits I wonder if |
f4ca43a to
94f0198
Compare
8044081 to
9a136f2
Compare
94f0198 to
8fe11cd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fe11cd4dc
ℹ️ 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".
8fe11cd to
261b894
Compare
261b894 to
93cf811
Compare
9a136f2 to
b402066
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93cf811cf2
ℹ️ 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".
| value_limit: per_tag | ||
| .config | ||
| .value_limit | ||
| .unwrap_or(per_metric.config.value_limit), |
There was a problem hiding this comment.
Honor global value_limit when per-metric limit is unset
When a per_metric_limits entry exists only to set mode/per_tag_limits and omits value_limit, serde still fills per_metric.config.value_limit with the default 500, so a per-tag entry that also omits value_limit inherits 500 here instead of the configured global limit. After the prior Option fix, this still affects configs like global value_limit: 10 plus per_metric_limits.metricA.mode: exact and per_tag_limits.tag1.mode: exact, silently widening that tag to 500 values despite the documented per-tag → per-metric → global resolution.
Useful? React with 👍 / 👎.
b402066 to
445abd6
Compare
93cf811 to
8130348
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 813034873c
ℹ️ 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".
| #[serde(flatten)] | ||
| pub mode: OverrideMode, |
There was a problem hiding this comment.
Allow per-tag entries to inherit mode
When a per_tag_limits entry is intended to override only value_limit (for example per_tag_limits.user_id.value_limit: 10 under an exact per-metric/global config), deserialization still requires a mode here because OverrideMode is non-optional and has no default. That makes value-limit-only per-tag overrides fail to load instead of following the documented per-field resolution order where mode can fall back to the enclosing per-metric/global setting.
Useful? React with 👍 / 👎.
8130348 to
b8f4022
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8f4022c5a
ℹ️ 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".
| mode: { | ||
| description: "Controls the approach taken for tracking tag cardinality." | ||
| required: true |
There was a problem hiding this comment.
Mark per-tag mode as optional in generated docs
For per-tag entries that only override value_limit, the runtime explicitly supports omitting mode (see the new deserialization test and PerTagInner.mode: Option<OverrideMode>), but the generated reference still marks this field as required. Users following or validating against this generated configuration docs will reject supported configs such as per_tag_limits: { env: { value_limit: 3 } }, which the changelog advertises as valid inherited behavior.
Useful? React with 👍 / 👎.
b8f4022 to
5501c78
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5501c78dc4
ℹ️ 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".
| .unwrap_or(&self.config.global), | ||
| None => &self.config.global, | ||
| { | ||
| return pmc.config.limit_exceeded_action; |
There was a problem hiding this comment.
Inherit the global action when no metric action is set
In configs that set limit_exceeded_action: drop_event globally and add a per_metric_limits entry only to define per_tag_limits, omitting limit_exceeded_action on that metric deserializes to the DropTag default and this branch returns it instead of the global action. That makes the metric drop individual tags rather than the whole event, contrary to the documented per-tag inheritance path; the per-metric action needs to be optional or otherwise distinguish omitted from explicitly set.
Useful? React with 👍 / 👎.
| cache_size_per_key: cache_size, | ||
| }) | ||
| } | ||
| None => metric_mode, |
There was a problem hiding this comment.
Honor per-tag cache overrides when inheriting mode
When a per-tag entry sets only cache_size_per_key under an enclosing mode: probabilistic metric, mode is None here and the code returns the per-metric metric_mode unchanged, so the tag-specific cache size is silently ignored. This affects users who omit mode to inherit probabilistic tracking while tuning the tag's Bloom filter size, which the new config comments and schema allow.
Useful? React with 👍 / 👎.
5501c78 to
6dd63ac
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dd63acf61
ℹ️ 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".
| value_limit: per_tag.config.value_limit.unwrap_or(metric_value_limit), | ||
| limit_exceeded_action, | ||
| mode, | ||
| internal_metrics: per_metric.config.internal_metrics, |
There was a problem hiding this comment.
Inherit global internal metric tags for per-metric overrides
When internal_metrics.include_extended_tags is enabled globally and a per_metric_limits entry only overrides limits/mode or adds per_tag_limits, this always uses the per-metric struct's serde default (false) instead of the global setting. That makes rejection counters for those metrics lose the metric_name/tag_key labels despite the new inheritance path documenting per-tag/per-metric/global resolution; make this field optional or merge it with self.config.global.internal_metrics before returning the resolved settings.
Useful? React with 👍 / 👎.
6dd63ac to
8519e7c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8519e7cd21
ℹ️ 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".
| #[configurable_component] | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| pub struct PerTagInner { |
There was a problem hiding this comment.
Reject unknown per-tag override fields
When users configure per_tag_limits, this new struct accepts unknown keys by default, so a typo such as value_limt: 2 or an unsupported per-tag limit_exceeded_action is silently ignored and the tag inherits the enclosing settings instead. That can leave high-cardinality tags effectively less restricted than the operator intended; add deny_unknown_fields or equivalent validation for the per-tag override object.
Useful? React with 👍 / 👎.
8519e7c to
3d11e7d
Compare
3d11e7d to
9c68670
Compare
Summary
Adds additional fine-grained controls for the tag cardinality processor, allowing users to
(1) Add per tag limits within each metric
(2) Add the ability to exclude a specific metric or tag from tracking (if the metric is excluded the tag will be too regardless of any per tag config)
Each tag has it's own config in which the following fields can be configured
(1) Tracking mode - Added a new exclude mode for inner overrides
(2) The value limit
This is based off of the changes in #25372
Vector configuration
How did you test this PR?
Regression test run: https://github.com/vectordotdev/vector/actions/runs/25386735971
Unit tests + used the test config above. Simulated an Otel collector source with the following Python script:
Change Type
Is this a breaking change?
Does this PR include user facing changes?
no-changeloglabel to this PR.References
Notes
@vectordotdev/vectorto reach out to us regarding this PR.pre-pushhook, please see this template.make fmtmake check-clippy(if there are failures it's possible some of them can be fixed withmake clippy-fix)make testgit merge origin masterandgit push.Cargo.lock), pleaserun
make build-licensesto regenerate the license inventory and commit the changes (if any). More details on the dd-rust-license-tool.