-
Notifications
You must be signed in to change notification settings - Fork 40
feat(ratelimitprocessor): add class-based limiting #791
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
Conversation
Introduces class-based dynamic rate limiting to the `ratelimitprocessor` allowing different rate limiting based on the received client metadata. The new architecture allows users to define named rate limit classes in the processor configuration, each with its own static rate, burst, and whether or not they support dynamic rate limiting. A new class resolver extension can then be used to map unique keys to class names, enabling dynamic assignment of rate limits based on custom resolution providers. The processor now supports a clear precedence order: per-key overrides take highest priority, followed by resolved class, default class, and finally the global fallback configuration. More telemetry has been added to provide visibility into rate limit resolution, dynamic escalations, degraded operation, and resolver failures. See the updated README for detailed usage instructions. Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
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.
Pull Request Overview
Introduces class-based dynamic rate limiting to the ratelimitprocessor allowing different rate limiting based on received client metadata. The new architecture defines named rate limit classes with their own static rates, bursts, and dynamic escalation policies, with a configurable precedence order for rate limit resolution.
- Adds class-based rate limiting architecture with named classes and resolver extension support
- Implements rate limit resolution precedence: overrides → resolved class → default class → fallback
- Enhances telemetry with new metrics for resolver failures, degraded operations, and dynamic escalations
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
processor/ratelimitprocessor/config.go | Adds Class type, class validation, and resolveRateLimit function with precedence logic |
processor/ratelimitprocessor/gubernator.go | Implements ClassResolver interface and integrates class resolution into rate limiting |
processor/ratelimitprocessor/factory.go | Updates factory functions to pass telemetry builder to rate limiters |
processor/ratelimitprocessor/processor.go | Refactors processor constructors to accept pre-built telemetry builder |
processor/ratelimitprocessor/local.go | Updates local rate limiter to use new resolveRateLimit function |
processor/ratelimitprocessor/metadata.yaml | Defines new telemetry metrics for class resolution and dynamic escalation |
processor/ratelimitprocessor/internal/metadata/generated_telemetry.go | Generated telemetry code for new metrics |
processor/ratelimitprocessor/testdata/config.yaml | Adds test configurations for invalid class scenarios |
processor/ratelimitprocessor/gubernator_test.go | Comprehensive tests for class resolution, telemetry, and resolver behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
||
metrics := pmetric.NewMetrics() | ||
for i := 0; i < numWorkers; i++ { | ||
for range numWorkers { |
Copilot
AI
Sep 29, 2025
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.
[nitpick] Using for range
without index variable when iterating a specific number of times is less clear than the traditional for i := 0; i < numWorkers; i++
pattern. The index i
was likely used in the loop body for identification purposes.
for range numWorkers { | |
for i := 0; i < numWorkers; i++ { |
Copilot uses AI. Check for mistakes.
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.
meh.
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.
Copilot swallowed a lot of JS code 😆
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.
Small nits (feel free to ignore), otherwise LGTM!
|
||
// StaticOnly disables dynamic rate escalation for this class. | ||
// When true, effective rate will always be the static Rate. | ||
StaticOnly bool `mapstructure:"static_only"` |
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: Is there a reason to not use dynamic
with default value to false
? static_only
feels a bit awkward to me.
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.
yeah probably a much better key name 😂 . My idea was to always have dynamic rate limiting unless explicitly disabled. If we use dynamic
as a key, then that's reversed. I'm open to other suggestions for better naming.
ignore_dynamic
skip_dynamic
But static is opposite of dynamic so that's why I chose that name.
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.
My idea was to always have dynamic rate limiting unless explicitly disabled.
Ah that makes sense. ignore_dynamic
feels a bit more aligned to this but I am okay to keep static_only
too.
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, mostly nonblocking comments.
static_only: false # allow gradual increase. | ||
|
||
# Default class when resolver returns unknown class | ||
default_class: "trial" |
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: wouldnt it be better to move this inside classes key to act as a catch all?
classes:
default: ?
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.
That's a bit awkard since classes
is a map[string]Class
. That means that you always require a named class "default"
to be declared, is that what you meant?
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.
yeah I meant default to be always present, not a big deal, feels a bit awkward to have a separate flag to choose what is the default class.
Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
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
Introduces class-based dynamic rate limiting to the
ratelimitprocessor
allowing different rate limiting based on the received client metadata.The new architecture allows users to define named rate limit classes in the processor configuration, each with its own static rate, burst, and whether or not they support dynamic rate limiting. A new class resolver extension can then be used to map unique keys to class names, enabling dynamic assignment of rate limits based on custom resolution providers.
The processor now supports a clear precedence order: per-key overrides take highest priority, followed by resolved class, default class, and finally the global fallback configuration.
More telemetry has been added to provide visibility into rate limit resolution, dynamic escalations, degraded operation, and resolver failures.
See the updated README for detailed usage instructions.