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.

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

enhancement(throttle transform): Refactor throttle/rate limiter logic into reusable wrapper #22719

Merged
merged 2 commits into from
Mar 26, 2025

Conversation

ArunPiduguDD
Copy link
Contributor

@ArunPiduguDD ArunPiduguDD commented Mar 24, 2025

Summary

We want to re-use pieces of the throttle processor implementation, in particular the rate-limiter logic. Refactors out constructing / running the rate limiter (mainly by running I mean periodically flushing old keys) into a separate struct that can be re-used elsewhere. Also refactors out some metrics related code, and moves the throttle implementation inside it's own folder in transforms (further split throttle.rs into 2 files, config.rs and transform.rs to mimic the file structure seen in other transform implementations such as sample, dedupe, etc.)

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?

Tested with vector config/pipeline containing throttle processor

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.
    • make check-all is a good command to run locally. This check is
      defined here. Some of these
      checks might not be relevant to your PR. For Rust changes, at the very least you should run:
      • cargo fmt --all
      • cargo clippy --workspace --all-targets -- -D warnings
      • cargo nextest run --workspace (alternatively, you can run cargo test --all)
  • 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

@github-actions github-actions bot added the domain: transforms Anything related to Vector's transform components label Mar 24, 2025
@ArunPiduguDD ArunPiduguDD force-pushed the refactor-throttle-transform branch from 85022b1 to 909e54c Compare March 24, 2025 15:55
@ArunPiduguDD ArunPiduguDD changed the title refactor(throttle): Refactor rate limiter logic into reusable wrapper enhancement(throttle): Refactor rate limiter logic into reusable wrapper Mar 24, 2025
@ArunPiduguDD ArunPiduguDD force-pushed the refactor-throttle-transform branch from 909e54c to 9e17937 Compare March 25, 2025 19:31
@ArunPiduguDD ArunPiduguDD changed the title enhancement(throttle): Refactor rate limiter logic into reusable wrapper enhancement(throttle transform): Refactor rate limiter logic into reusable wrapper Mar 25, 2025
@ArunPiduguDD ArunPiduguDD force-pushed the refactor-throttle-transform branch from 9e17937 to 6495dd4 Compare March 26, 2025 17:50
@ArunPiduguDD ArunPiduguDD added the no-changelog Changes in this PR do not need user-facing explanations in the release changelog label Mar 26, 2025
@datadog-vectordotdev
Copy link

datadog-vectordotdev bot commented Mar 26, 2025

Datadog Report

Branch report: refactor-throttle-transform
Commit report: 718d93f
Test service: vector

✅ 0 Failed, 7 Passed, 0 Skipped, 25.49s Total Time

@ArunPiduguDD ArunPiduguDD requested review from bruceg and pront March 26, 2025 18:42
@ArunPiduguDD ArunPiduguDD marked this pull request as ready for review March 26, 2025 18:42
@ArunPiduguDD ArunPiduguDD requested a review from a team as a code owner March 26, 2025 18:42
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.

Thanks @ArunPiduguDD, the transform is much more readable now.

}

impl<C, I> TaskTransform<Event> for Throttle<C, I>
where
C: clock::Clock<Instant = I> + Send + 'static + Clone,
C: clock::Clock<Instant = I> + Send + Sync + 'static + Clone,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Do you need the Sync?

Copy link
Contributor Author

@ArunPiduguDD ArunPiduguDD Mar 26, 2025

Choose a reason for hiding this comment

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

Yea it's required when spawning the key flush thread for the rate-limiter

@ArunPiduguDD ArunPiduguDD changed the title enhancement(throttle transform): Refactor rate limiter logic into reusable wrapper enhancement(throttle transform): Refactor throttle/rate limiter logic into reusable wrapper Mar 26, 2025
@ArunPiduguDD ArunPiduguDD added this pull request to the merge queue Mar 26, 2025
Merged via the queue into master with commit 44562f6 Mar 26, 2025
57 checks passed
@ArunPiduguDD ArunPiduguDD deleted the refactor-throttle-transform branch March 26, 2025 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: transforms Anything related to Vector's transform components no-changelog Changes in this PR do not need user-facing explanations in the release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants