-
Notifications
You must be signed in to change notification settings - Fork 231
Rate limit restructure (server-side) #933
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
Open
khetzl
wants to merge
53
commits into
master
Choose a base branch
from
rate_limiting_rework
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
53 commits
Select commit
Hold shift + click to select a range
f80ed18
Rate limit restructure
khetzl 56b7389
Add tests, sliding window under leaky bucket token bursts
khetzl 80412c7
Move tests to separate file, avoid failed tests leaking
khetzl 0af2d5f
Define new metrics for ar_limiter
khetzl 99266c7
fix: apply metrics in ar_limiter, update tests to mock prometheus calls
khetzl 9f999f9
Start ar_limit_sup
khetzl 4cf6935
HTTP API Server now uses ar_limiter, and metrics collector
khetzl 00476e2
fix: ar_limiter_sup:start_link/0
khetzl d8cecd0
fix: ar_limiter_sup:start_link/0
khetzl 0cb8632
fix typo, test, and an interesting bug it hid
khetzl cca8147
Rework tests, cleanup sliding window timestamps, additional metrics
khetzl d9602b4
ar_limiter efault values to reflect similar numbers to what's in the …
khetzl fb9efeb
collector tests, add tests to github wf, minor cleanup
khetzl 01c6dbc
limiter collector tests, hardwired config for metrics endpoint
khetzl bf2d29d
http iface tests, ar_config details trough ar_limiter_sup
khetzl ba6c789
Apply all config parameters to general limiter pool
khetzl ce49360
With limiter settings part of the config, ar_http_iface tests can be …
khetzl 6644dd2
remove commented code
khetzl ee527f3
use the right metrics name for leaky bucket token use
khetzl ef64e2f
arweave_limiter as a self-contained Erlang app
khetzl 744249e
rename tests in github action
khetzl 0ffad03
fix module name
khetzl d593010
remove arweave_config and arweave_limiter from arweave.app.src as it …
khetzl d54afbe
start applications with their API functions, rather than with app.src…
khetzl be39ec7
further fixes to startup and metrics collector registration
khetzl 63f490c
app.src and placeholders to fix tests
khetzl 4d04b4f
rename limiter pools to groups
khetzl fc74273
peers per port, option to disable limiter group, and new group for lo…
khetzl 08dfb82
remove limiting from ar_blacklist_middleware
khetzl 7d8310c
Replicate previous default settings for different paths
khetzl 4b8530a
route requrests to appropriate rate limiting groups
khetzl 14e27e5
Remove commented code from bin/arweave startup script
khetzl 828d716
Test config default values
khetzl 49c7cf9
TEST -> AR_TEST
khetzl 845c1be
RLG init function argument naming
khetzl 2f69441
POSIX list in bin/arweave script
khetzl 487ca7f
Add documentation for rate limiter middleware cowboy handler
khetzl daf5ab7
Remove redundant trap_exit
khetzl 8af7f4b
Fix RLG State typos
khetzl 7060f50
RLG reduce_for_peer tests
khetzl 7e8e1a3
return config with warning log messages
khetzl 954a4d2
fix config issue for limiter
khetzl 6a9b5ca
Add comments for limiter code
khetzl 7b5807b
More explaination
khetzl 1a5d3ab
histogram to observer arweave_limiter_group performance
khetzl b000004
minor metrics related fixes, and tweaks, additional tests
khetzl 9092ed6
limit based on IP
khetzl 0ef5ce2
Handle reset_all for RLGs when monitored processes go down
khetzl 3369c0d
Take RLG tick_reduction from config
khetzl 2bee806
http iface limiter config_peer_to_ip_addr to convert anything into ip…
khetzl cde5bb4
Address config loading issues discovered by AI, allow parser to pick …
khetzl 1629e91
Merge branch 'master' into rate_limiting_rework
khetzl acd08fd
fix tests again
khetzl 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 |
|---|---|---|
|
|
@@ -35,4 +35,5 @@ release/output | |
| node_modules | ||
| screenlog.0 | ||
| _* | ||
| *~ | ||
| *.swp | ||
This file contains hidden or 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 hidden or 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.
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.
To be honest I didn't realize we had a
disable blacklistoption, and I'm not sure if it is currently in use. It looks like it would completely disable rate limiting on the server side. @ldmberman @humaite are you aware of any nodes that use it?It's possible some large miners use it on internal nodes used to server data to mining nodes (for example). But I'm not sure it would even be that effective since by default clients will self-throttle regardless of the server-side behavior.
This is a long way of asking:
enableanddisableoptions are poorly understood and rarely used, so if we want to disable server-side rate limiting a more formal config is a better choice. Only question is whether anyone is using the current flag?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 intended to remove this, we shouldn't allow unrestricted access to resources to the public.
Trusted peers can be listed so their requests are limited via a less restrictive Rate Limiter Group. (Currently, no limiting at all)
Note: Even if the
disable blacklistflag is enabled in the current release, a global concurrency limit is enforced by cowboy/ranch.Uh oh!
There was an error while loading. Please reload this page.
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.
Got it. I agree it's correct to remove. However worth providing some context:
disable blacklistflag it doesn't necessarily mean they or we are providing unrestricted access to the public.local_peersbut since arweave doesn't currently support any sort of wildcards/subnetting, and because the ndoe shutdown/boot time can take a long time, some of the miners had trouble managing rate limiting for their internal nodes. It's in that scenario where I could seedisable blacklistbeing useful (setup an internal data-sharing node, disable blacklist, and then let all your other nodes connect to without worrying about needing to restart periodically to update local_peers)All that said: I do think fine to remove. But I wanted to call out that context as our operating model is often different than that of non-blockchain companies (e.g. we build infrastructure software to be run by 3rd parties as well as by us internally - and those 3rd parties vary from novice to enterprise-level)