-
Notifications
You must be signed in to change notification settings - Fork 128
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
Distribute ratelimiter requests across the cluster by route #621
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
defmodule Nostrum.Api.RatelimiterGroup do | ||
@moduledoc """ | ||
Tracks ratelimiters and determines correct ratelimiters to use per request. | ||
|
||
## Purpose | ||
|
||
In a multi-node setup, users want to be able to make API requests from any | ||
node in the cluster without having to worry about hitting ratelimits. This | ||
module serves as the mediator between API clients on any nodes and their | ||
target ratelimiter. | ||
|
||
> ### Internal module {: .info} | ||
> | ||
> This module is intended for exclusive usage inside of nostrum, and is | ||
> documented for completeness and people curious to look behind the covers. | ||
|
||
## Approach | ||
|
||
A naive implementation might simply forward requests to the locally (on the | ||
same node) running ratelimiter. However, this falls short when modules on | ||
other nodes want to make API requests, as they then effectively begin | ||
tracking their own ratelimit state, rendering it inconsistent. | ||
|
||
Instead, the approach is that we have a locally running ratelimiter on each | ||
node, all of which are registered via the `:pg` process group managed by this | ||
module. When an API request comes in, we determine its ratelimit bucket (see | ||
`Nostrum.Api.Ratelimiter.get_endpoint/2`) and based on that, determine the | ||
target ratelimiter by selecting it from the list of known ratelimiters via | ||
`:erlang.phash2/2`. | ||
""" | ||
|
||
@scope_name __MODULE__ | ||
@group_name :ratelimiters | ||
|
||
@doc """ | ||
Return a ratelimiter PID to use for requests to the given ratelimiter `bucket`. | ||
""" | ||
@spec limiter_for_bucket(String.t()) :: pid() | ||
def limiter_for_bucket(bucket) do | ||
limiters = :pg.get_members(@scope_name, @group_name) | ||
# "Processes are returned in no specific order." | ||
sorted = Enum.sort(limiters) | ||
total = length(sorted) | ||
selected = :erlang.phash2(bucket, total) | ||
Enum.at(sorted, selected) | ||
end | ||
|
||
@doc "Join the given ratelimiter to the group." | ||
@spec join(pid()) :: :ok | ||
def join(pid) do | ||
:pg.join(@scope_name, @group_name, pid) | ||
end | ||
|
||
# Supervisor API | ||
def start_link(_opts) do | ||
:pg.start_link(@scope_name) | ||
end | ||
|
||
def child_spec(opts) do | ||
%{ | ||
id: __MODULE__, | ||
start: {__MODULE__, :start_link, [opts]}, | ||
type: :worker, | ||
restart: :permanent, | ||
shutdown: 500 | ||
} | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 value configurable? Issues could theoretically arise if you were running multiple bots and had the nodes connected, you'd get 403s that were difficult to debug if you weren't aware of this module.
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 should make not the scope name but the group name configurable to set it to the bot ID that the ratelimiters run for.
Would running multiple bots with distinct configurations even work currently? I think you would run into other confusing bugs.
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.
I think for later we should make the group name configurable in the start_link function. For now I'm not sure we need to, because plenty of other things break. What do you think?