-
Notifications
You must be signed in to change notification settings - Fork 57
Rate limit webhooks via distributed process #3184
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
base: main
Are you sure you want to change the base?
Conversation
ba2649a
to
e950099
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3184 +/- ##
========================================
+ Coverage 1.36% 2.84% +1.48%
========================================
Files 360 361 +1
Lines 13815 13856 +41
========================================
+ Hits 188 394 +206
+ Misses 13627 13462 -165 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cc0d4e7
to
684275b
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.
@jyeshe I always enjoy PRs that force me to do some learning :).
Nice code - just one question to make sure that have not missed something: Would I be correct in saying that the distributed rate limiter is disabled in Lightning, but will be enabled in the billing app?
Also, I did not run the tests as I have an error with the Sentry library dependency (I guess this is what happens when I don't refresh the repo for two weeks :D) and I do not want to block the PR while I am figuring it out. If you feel it is important that I do run these tests please let me know and I will do so.
Hey @rorymckinley, that's correct. So far there is no requirement to enable it on Lightning. I think we are fine with the local tests, I am willing to see it more on a K8S cluster. I am optimistic with the results on a low latency private network. |
28cfd61
to
a88a109
Compare
a88a109
to
1271253
Compare
Description
This PR provides a rate limiter based on distributed process that works as token bucket. The token bucket has a capacity (credits for requests) and a refill rate. For example if the capacity is 10 and refill per second is 2, the user can make 10 requests and after that 2 requests per second. When stops to make requests, it refills up to 10 credits and can make 10 requests in 1 second again.
It uses CRDTs via Horde library only to replicate the process and by doing so it doesn't need to replicate the state of the rate limiting cache.
Closes #3185
Validation steps
iex --sname node1@localhost --cookie hordecookie -S mix phx.server
RTM=false PORT=4001 iex --sname node2@localhost --cookie hordecookie -S mix phx.server
Lightning.DistributedRateLimiter.inspect_table()
on both iex shells and they shall show the same process and node.Additional notes for the reviewer
The rate limiting cache state is transient and considered negligible in case of a network split in a way that after the process is recreated in another node it doesn't care about the previous state. If nothing happened, no network split, with the proposed configuration, after 5 seconds the credits would have been completed restored anyways for all projects.
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
:owner
,:admin
,:editor
,:viewer
)