-
Notifications
You must be signed in to change notification settings - Fork 8
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
Provide a common atomic Rate Limiter implementation #560
Conversation
60c49cd
to
915011f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #560 +/- ##
==========================================
+ Coverage 72.79% 72.88% +0.08%
==========================================
Files 243 245 +2
Lines 34683 34951 +268
==========================================
+ Hits 25249 25473 +224
- Misses 9434 9478 +44
|
BenchmarksComparisonBenchmark execution time: 2024-09-04 10:57:05 Comparing candidate commit 01e7245 in PR branch Found 25 performance improvements and 1 performance regressions! Performance is the same for 24 metrics, 2 unstable metrics. scenario:benching string interning on wordpress profile
scenario:credit_card/is_card_number/
scenario:credit_card/is_card_number/ 3782-8224-6310-005
scenario:credit_card/is_card_number/ 378282246310005
scenario:credit_card/is_card_number/37828224631
scenario:credit_card/is_card_number/378282246310005
scenario:credit_card/is_card_number/37828224631000521389798
scenario:credit_card/is_card_number_no_luhn/
scenario:credit_card/is_card_number_no_luhn/ 3782-8224-6310-005
scenario:credit_card/is_card_number_no_luhn/ 378282246310005
scenario:credit_card/is_card_number_no_luhn/37828224631
scenario:credit_card/is_card_number_no_luhn/378282246310005
scenario:credit_card/is_card_number_no_luhn/37828224631000521389798
scenario:redis/obfuscate_redis_string
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
BaselineOmitted due to size. |
b482de9
to
3b7b39a
Compare
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
3b7b39a
to
f8ea4a1
Compare
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
44c984f
to
3b1d468
Compare
3b1d468
to
e776ee0
Compare
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.
Partial review.
.github/workflows/lint.yml
Outdated
@@ -44,7 +44,7 @@ jobs: | |||
run: rustup install ${{ matrix.rust_version }} && rustup default ${{ matrix.rust_version }} && rustup component add clippy | |||
- name: Run clippy on ${{ matrix.platform }} ${{ matrix.rust_version }} | |||
shell: bash | |||
run: cargo clippy --all-targets --all-features -- -D warnings "$([ ${{ matrix.rust_version }} = 1.71.1 ] && echo -Aunknown-lints)" | |||
run: cargo clippy --all-targets --all-features -- -D warnings $([ ${{ matrix.rust_version }} = 1.71.1 ] && echo -Aunknown-lints -Aclippy::cast_ref_to_mut) |
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.
should we make this change in a separate PR?
since this has broader impact than just ddcommon
, would be good to get review from others in the repo. also helps if we ever have to revert this PR, this seems like a reasonable change to not have tied to this rate limiter implementation?
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.
Note that this only affects the 1.71.1 stable clippy. Sometimes clippy changes between versions, but we run clippy against nightly and stable. Sometimes these can be conflicting.
Had to be done in the past and seems necessary now.
pub trait Limiter { | ||
/// Takes the limit per interval. | ||
/// Returns false if the limit is exceeded, otherwise true. | ||
fn inc(&self, limit: u32) -> bool; |
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.
we need to support passing in the rate limit and changing it dynamically at check time because if it is shared across processes and each process is using a different limit?
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.
It's more about the scenario where a limit is changed - but gradually rolled out (e.g. webserver graceful restarted). It should gradually adapt to the new limit.
It's less about having two actively concurring limits for the same thing for a prolonged time, though that scenario also works.
#[cfg_attr(miri, ignore)] | ||
fn test_rate_limiter() { | ||
let limiter = LocalLimiter::default(); | ||
// Two are allowed, then one more because a small amount of time passed since the first one |
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.
is there a way for us to mock now
? seems like this test will be flaky if we rely on execution 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.
This test isn't flaky as long as it runs within ~100ms (which really is plenty of time).
In fact this test was flaky because ... it was running faster than the precision of the timer allowed. Hence the test adds 100 µs in between calls.
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
ddcommon/src/rate_limiter.rs
Outdated
/// The implementation is a sliding window: every time the limiter is increased, the as much time as | ||
/// has passed is also refilled. |
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.
Some words are missing.
Is this correct: The implementation is a sliding window: every time the limiter is increased, the amount of time that has passed is also refilled.
?
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 :-)
As opposed to #552, this implementation is based on atomics and can be used for communication via shared memory.