Dynamic rate limiter #4390
Dynamic rate limiter #4390
Conversation
service/frontend/service.go
Outdated
| EnableDynamicRateLimiting dynamicconfig.BoolPropertyFn | ||
| DynamicRateLimitingRefreshInterval dynamicconfig.DurationPropertyFn | ||
| DynamicRateLimitingLatencyThreshold dynamicconfig.FloatPropertyFn | ||
| DynamicRateLimitingErrorThreshold dynamicconfig.FloatPropertyFn | ||
| DynamicRateLimitingRateBackoffStepSize dynamicconfig.FloatPropertyFn | ||
| DynamicRateLimitingRateIncreaseStepSize dynamicconfig.FloatPropertyFn |
There was a problem hiding this comment.
If you have a common set of dynamic config properties that are all used together, consider: define a struct for them, use a dynamic config "map" (which gives you a map[string]any) and then write a function to unpack the map into the struct (slightly annoying but you can cheat and do json.Marshal+json.Unmarshal to get it for free)
There was a problem hiding this comment.
@dnr I updated the dynamic config properties, let me know what you think.
I'm a little concerned about two things:
- Will
json.Marshalandjson.Unmarshaladd significant overhead when updating the dynamic values? - The field names to use in that map are somewhat harder to find. I included them on the comment on the constant for
dynamicconfig.HistoryPersistenceDynamicRateLimitingParams, do you think that will be clear enough or will users have trouble understanding what config options are available?
|
|
||
| func (rl *HealthRequestRateLimiterImpl) refreshDynamicParams() { | ||
| var options dynamicRateLimitingOptions | ||
| b, err := json.Marshal(rl.params()) |
There was a problem hiding this comment.
Hmm.. I thought the point of defining the keys explicitly was to avoid the json stuff? So, those constants are only used in the default value and nowhere else?
I would think if you're going to define string constants for the map keys, you should use them to read the map
| dynamicRateLimitEnabledKey: dynamicRateLimitEnabledDefault, | ||
| dynamicRateLimitRefreshIntervalKey: dynamicRateLimitRefreshIntervalDefault, | ||
| dynamicRateLimitLatencyThresholdKey: dynamicRateLimitLatencyThresholdDefault, | ||
| dynamicRateLimitErrorThresholdKey: dynamicRateLimitErrorThresholdDefault, | ||
| dynamicRateLimitBackoffStepSizeKey: dynamicRateLimitBackoffStepSizeDefault, | ||
| dynamicRateLimitIncreaseStepSizeKey: dynamicRateLimitIncreaseStepSizeDefault, |
There was a problem hiding this comment.
Personally I find this kind of hard to read and I'd just put the defaults inline in this map. It is effectively a constant itself
| rl.rateLimiter.SetRate(rl.curRateMultiplier * rl.rateFn()) | ||
| rl.rateLimiter.SetBurst(int(rl.rateToBurstRatio * rl.rateFn())) |
There was a problem hiding this comment.
should you maybe update rl.rateLimiter always here, in case rateFn returns something different?
What changed?
Adding a new rate limiter that dynamically adjusts its rate based on signals from the persistence layer.
Why?
For system protection
How did you test it?
A new integration test confirms that the rate limiter adjusts itself
More manual testing will be required
Potential risks
Over aggressive rate limiting
The rate limit seesaws back and forth
Is hotfix candidate?