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

Consume the right amount of tokens from the ratelimit bucket #261

Merged
merged 4 commits into from
Mar 20, 2024
Merged

Consume the right amount of tokens from the ratelimit bucket #261

merged 4 commits into from
Mar 20, 2024

Conversation

shtlrs
Copy link
Contributor

@shtlrs shtlrs commented Mar 20, 2024 •
edited
Loading

Closes #242

This fixes the following issues:

  1. The way we were consuming tokens from the bucket was flawed, as we were passing the number of tokens to consume as the bucket key
  2. The number of tokens to consume from each bucket is now dependant on the configuration, instead of the constant value of 1 we used to have.

shtlrs added 2 commits March 20, 2024 10:18
@shtlrs
use the ip address as the key to identify the correct token bucket
cbc6ef3 
The `token_bucket.Limiter.consume` method takes `key` and `num_tokens` params to identify the correct bucket and the number of tokens to consume, which was misused earlier by having the key as the number of tokens to consume

This simplifies the `ratelimit_area` dict even further as the Limiter class will handle identifying the correct bucket for us.
@shtlrs
narrow down the patching scope of the ratelimit config
b79185d
@shtlrs shtlrs force-pushed the 242-fix-ratelimit-consumption branch from 7eb160b to b79185d Compare March 20, 2024 11:18
@shtlrs
pass remote_ip instead of entire request object to should_be_ratelimited
14e4765 
This is done mainly because it'll make unit testing the function alot easier

Also, passing the entire object is beneficial when multiple attributes are needed, which is not needed here.
@shtlrs shtlrs marked this pull request as ready for review March 20, 2024 13:02
@shtlrs shtlrs force-pushed the 242-fix-ratelimit-consumption branch 3 times, most recently from 1f4bcc6 to 0eded05 Compare March 20, 2024 13:28
@shtlrs shtlrs marked this pull request as draft March 20, 2024 13:32
@shtlrs shtlrs force-pushed the 242-fix-ratelimit-consumption branch from 30d5801 to f690aac Compare March 20, 2024 13:51
@shtlrs shtlrs marked this pull request as ready for review March 20, 2024 13:54
@shtlrs
Copy link
Contributor Author

shtlrs commented Mar 20, 2024 •
edited
Loading

@supakeen CI is failing because black's version needs to be bumped.

I've done so in #262

@supakeen supakeen self-requested a review March 20, 2024 21:20
supakeen
supakeen approved these changes Mar 20, 2024
View reviewed changes
Copy link
Owner

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Hah, this has made this all a lot a cleaner; I especially like that Limiter already can do the keys for us :)

@supakeen supakeen merged commit 71fd383 into supakeen:master Mar 20, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supakeen supakeen supakeen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix consume ratelimit config
2 participants
@shtlrs @supakeen