Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(tag_cardinality_limit transform): enable per metric limits for tag_cardinality_limit #22077

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat(tag_cardinality_limit transform): enable per metric limits for tag_cardinality_limit #22077

wants to merge 3 commits into from
+334 −32

Conversation

esensar
Copy link
Contributor

@esensar esensar commented Dec 23, 2024 •
edited
Loading

Summary

This adds the ability to add per metric limits in tag_cardinality_limit transform, besides the global configuration that applies to all metrics. It supports matching metrics by name and optionally by namespace too.

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

Added new tests to transforms::tag_cardinality_limits::tests and also tested manually with the following configuration:

sources:
  static_metrics_test:
    type: static_metrics
    interval_secs: 1
    metrics:
       - name: customA
         value:
           gauge:
             value: 3.0
         kind: absolute
         tags:
           custom_tag: custom_tag_1
           another_tag: test
       - name: customA
         value:
           gauge:
             value: 4.4
         kind: incremental
         tags:
           custom_tag: custom_tag_2
           another_tag: test
       - name: customA
         value:
           gauge:
             value: 4.4
         kind: incremental
         tags:
           custom_tag: custom_tag_3
           another_tag: test2
       - name: customB
         value:
           gauge:
             value: 3.0
         kind: absolute
         tags:
           custom_tag: custom_tag_1
           another_tag: test
       - name: customB
         value:
           gauge:
             value: 4.4
         kind: incremental
         tags:
           custom_tag: custom_tag_2
           another_tag: test
       - name: customB
         value:
           gauge:
             value: 4.4
         kind: incremental
         tags:
           custom_tag: custom_tag_3
           another_tag: test2

transforms:
  tag_cardinality:
    type: "tag_cardinality_limit"
    inputs: ["static_metrics_test"]
    value_limit: 2
    mode: "exact"
    per_metric_limits:
      - name: "customA"
        namespace: "static"
        value_limit: 5
        mode: "exact"

sinks:
  console:
    inputs: ["tag_cardinality"]
    target: "stdout"
    type: "console"
    acknowledgements:
      enabled: false
    encoding:
      codec: "json"

Confirmed that behavior is different for metricA and metricB (metricA didn't get its tags removed due to higher limit).

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

  • Please read our Vector contributor resources.
  • If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
    run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

@esensar
feat(tag_cardinality_limit transform): enable per metric limits for `…
c2fcd7a 
…tag_cardinality_limit`

This adds the ability to add per metric limits in `tag_cardinality_limit` transform, besides the
global configuration that applies to all metrics. It supports matching metrics by name and
optionally by namespace too.

Closes: vectordotdev#15743
@esensar esensar requested a review from a team as a code owner December 23, 2024 16:30
@github-actions github-actions bot added the domain: transforms Anything related to Vector's transform components label Dec 23, 2024
@esensar
Copy link
Contributor Author

esensar commented Dec 23, 2024

I think this could also support #21112 with minor modifications, but I didn't go ahead with it right away, because I didn't want to introduce yet another level of nesting in the accepted_tags map. My idea was to reuse the key currently used for metric name for that custom key too, but that would mean changing the current metric name key to something that would hard to clash with when using custom keys. Not sure if it is a good idea.

@johnhtodd
Copy link

Sorry if I'm being a bit thick here, but I didn't see any user-visible documentation or discussion in the patch set - did I miss it?

@pront
Copy link
Member

pront commented Jan 3, 2025

Sorry if I'm being a bit thick here, but I didn't see any user-visible documentation or discussion in the patch set - did I miss it?

Ah, we need to make generate-component-docs and commit the result.

pront
pront reviewed Jan 3, 2025
View reviewed changes
Copy link
Member

@pront pront left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @esensar, thank you for this contribution. The new tests are much appreciated. Left a few comments but nothing major.

src/transforms/tag_cardinality_limit/config.rs
/// Tag cardinality limits configuration per metric name.
#[configurable(derived)]
#[serde(default)]
pub per_metric_limits: Vec<PerMetricConfig>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It's better to use a map type here from name to PerMetricConfig.

src/transforms/tag_cardinality_limit/mod.rs
value: &TagValueSet,
) -> bool {
let config = self.get_config_for_metric(metric_key).clone();
info!("try_accept_tag using config: {:?}", config);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove info!s here and below. If you would like to keep them we can make them trace!s.

src/transforms/tag_cardinality_limit/mod.rs
fn tag_limit_exceeded(&self, key: &str, value: &TagValueSet) -> bool {
fn tag_limit_exceeded(
&self,
metric_key: &Option<MetricId>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
metric_key: &Option<MetricId>,
metric_key: Option<&MetricId>,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pront pront pront left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
domain: transforms Anything related to Vector's transform components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Limit tag cardinality by metric in tag_cardinality_limit transform
3 participants
@esensar @johnhtodd @pront