-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
85022b1
to
909e54c
Compare
909e54c
to
9e17937
Compare
…reusable wrappers
9e17937
to
6495dd4
Compare
Datadog ReportBranch report: ✅ 0 Failed, 7 Passed, 0 Skipped, 25.49s Total Time |
There was a problem hiding this 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.
src/transforms/throttle/transform.rs
Outdated
} | ||
|
||
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, |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
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 thethrottle
implementation inside it's own folder intransforms
(further splitthrottle.rs
into 2 files,config.rs
andtransform.rs
to mimic the file structure seen in other transform implementations such assample
,dedupe
, etc.)Change Type
Is this a breaking change?
How did you test this PR?
Tested with vector config/pipeline containing throttle processor
Does this PR include user facing changes?
Checklist
make check-all
is a good command to run locally. This check isdefined 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 runcargo test --all
)Cargo.lock
), pleaserun
dd-rust-license-tool write
to regenerate the license inventory and commit the changes (if any). More details here.References