-
Notifications
You must be signed in to change notification settings - Fork 31
feat: add per-project rate limiter settings override #583
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
@simitt, I'd like to double-check with you whether this is the override semantics we're looking for. Note: I also discussed the switch from a request-based to a bytes-based 1 rate limiting with @vigneshshanmugam. So, in the following examples, I will use the Let's start with a few examples. (1) Baseline rate-limiting In this example, the rate limiter applies the same baseline limits to all projects. ratelimit:
metadata_keys:
- x-elastic-project-id
rate: 2046820
burst: 5117050 # In these examples, I'm considering a 2.5x burst ratio
strategy: bytes
throttle_behavior: error (2) Per-project rate override In this example, we increase the rate by 50% over the baseline. ratelimit:
metadata_keys:
- x-elastic-project-id
rate: 2046820
burst: 5117050 # In these examples, I'm considering a 2.5x burst ratio
strategy: bytes
throttle_behavior: error
overrides:
x-elastic-project-id:local:
rate: 3070230 # +50% over baseline (3) Per-project rate and burst override In this example, we increase the rate and burst by 50% over the baseline. ratelimit:
metadata_keys:
- x-elastic-project-id
rate: 2046820
burst: 5117050 # In these examples, I'm considering a 2.5x burst ratio
strategy: bytes
throttle_behavior: error
overrides:
x-elastic-project-id:local:
rate: 3070230 # +50% over baseline
x-elastic-project-id:whatever:
rate: 3070230 # +50% over baseline
burst: 7675575 # +50% over baseline Footnotes |
General approach looks good to me. What would be worth adding is a time interval indicator, e.g. |
115e243
to
6395814
Compare
Adding a |
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.
Thank you @zmoog ! It looks great, I couldn't find issues with your approach.
// If no override is found, the default rate limit settings are returned. | ||
func resolveRateLimitSettings(cfg *Config, uniqueKey string) RateLimitSettings { | ||
result := cfg.RateLimitSettings | ||
if len(cfg.Overrides) > 0 { |
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 condition wouldn't be necessary, right? Just doing if override, ok := cfg.Overrides[uniqueKey]; ok {
would be enough.
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.
Not needed. This optimization isn't worth sacrificing readability for. I'm removing it.
var errs []error | ||
if r.Rate != nil { | ||
if *r.Rate <= 0 { | ||
errs = append(errs, fmt.Errorf("rate must be greater than zero")) |
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.
You know... if we were pushing this upstream, and we ran make lint
, it would complain that we should use errors.New
instead of fmt.Errorf
in this case since we are not using any variables in the message :) You don't have to change it, just something that I noticed.
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.
Oh, thank you Constança! I forgot to run make lint
before committing the changes 🤦
98e1d0b
to
520fe46
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.
LGTM overall
Users can now set a default value for `throttle_interval` and also override it per project when needed. It only has an effect when the rate limiter type is `gubernator`.
It can save us the key hashing to access the map, but I'm not sure it's worth it.
Co-authored-by: Vignesh Shanmugam <[email protected]>
03955c2
to
dc9e4d4
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.
LGTM
ratelimiter: | ||
metadata_keys: | ||
- x-elastic-project-id |
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 keep it generic, lets use x-tenant-id or similar.
| `type` | Type of rate limiter. Options are `local` or `gubernator`. | No | `local` | | ||
| `overrides` | Map of metadata key overrides for the rate limiter. See the possible overrides and examples below. | No | | | ||
|
||
### Possible overrides: |
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.
nit: Overrides based on Metadata keys?
Add a per-project override mechanism for rate limiter settings.
Changes:
ratelimit.overrides
optionratelimit.throttle_interval
optionFor example, in the following config:
In
overrides
, you can override one or more of the following settings:rate
burst
throttle_interval
If not overridden, the base settings apply.
Examples
(1) Simple rate limiter settings (no override)
Notes on
rate
andstrategy
:rate
valuerequests
records
bytes
(2) Per-project rate override
(3) Per-project rate and burst override
(4) Set base
throttle_interval
(5) Set base
throttle_interval
to10s
and override it on a project