-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Limiter extension APIs and implementation helpers (**draft 7**) #13265
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
…tor into jmacd/limiter_v4
…tor into jmacd/limiter_v4
…tor into jmacd/limiter_v4
…tor into jmacd/limiter_v6
The question is how we avoid double-count certain limits whether they | ||
are implemented in middleware, through a factory, through custom | ||
receiver code, or other. |
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.
What is a concrete example of the problem? The middleware and receiver code operate at different levels, so they should not be counting the same things? i.e. middleware would be counting HTTP/gRPC requests & network bytes, while the receiver is counting application protocol bytes, log records, etc.
extensions: | ||
... | ||
limitermux/grpc: | ||
request_count: | ||
- ratelimiter/1 | ||
network_bytes: | ||
- ratelimiter/2 | ||
request_bytes: | ||
- ratelimiter/3 | ||
- admissionlimiter/1 | ||
request_items: | ||
- ratelimiter/4 | ||
|
||
limitermux/otlp: | ||
... |
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 sort of goes in the direction of what I've had in mind: create a distinct "limitermiddleware" that identifies a rate limiter extension, and defines limits for things the middleware deals with: request_count and network_bytes.
Essentially, I want to separate the configuration of the limiter from the limits. This would require that all rate limiters deal with the same configuration for the specific limits, i.e. refill rate and burst.
Concretely, this would look like:
extensions:
# gubernator provides a rate limiter implementation.
gubernator:
endpoint: 0.0.0.0:1050
peer-discovery:
type: k8s
namespace: foo
selector: bar=baz
# limitermiddleware/gubernator is an instance of limitermiddleware that uses
# the gubernator limiter extension to limit HTTP and gRPC requests by
# request_count & network_bytes.
limitermiddleware/gubernator:
limiter: gubernator
network_bytes:
rate: 123.456
burst: 789
# request_count is unspecified, and so will not be limited.
receivers:
otlp:
protocols:
grpc:
middleware:
- limitermiddleware/gubernator
If that is acceptable, then I think we could extract some common config structs for use by both that middleware and receivers:
package limiterhelper
import "go.opentelemetry.io/collector/extension/extensionlimiter"
type RateLimitsConfig struct {
// RateLimiter identifies a rate limiter extension to use for these limits.
RateLimiter component.ID
// Limits holds the configuration for specific rate limits.
Limits map[extensionlimiter.WeightKey]RateLimitConfig
}
type RateLimitConfig struct {
// Rate is the rate at which the limit will be refilled (up to Burst), measured in items per second.
Rate float64
// Burst is the maximum number of items that will be allowed through at once.
//
// As a special case, if Burst is zero then no rate limiting is applied.
Burst int
}
The RateLimiterProvider.GetRateLimiter
method would be updated so the rate & burst must be passed in.
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 raises another couple of open questions for me:
- where do metadata keys go?
- where do we specify the behaviour when the rate limit has been exceeded
I think both of these belong to the thing calling the rate limiter, independent of the weight key - so probably in the RateLimitsConfig struct above.
// AnyProvider is an any limiter implementation, possibly one of the | ||
// limiter interfaces. This serves as a marker for implementations | ||
// which provider rate limiters of any (one)kind. | ||
type AnyProvider interface { | ||
// unexportedProviderFunc may be embedded using an AnyProviderImpl. | ||
unexportedProviderFunc() | ||
} | ||
|
||
// AnyProviderImpl can be embedded as a marker that a component | ||
// implements one of the rate limiters. | ||
type AnyProviderImpl struct {} | ||
|
||
func (AnyProviderImpl) unexportedProviderFunc() {} |
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'm not convinced we need these. Extensions implement either RateLimiterProvider or ResourceLimiterProvider (or both? I don't understand why they need to be mutually exclusive). Since they are extensions, they must also implement extension.Extension
- thus they already share a common base, and limiterhelper
can deal with that interface.
Then, for example, AnyToRateLimiterProvider
would be changed in one of the following ways (or some variation thereof):
- Accept an
extension.Extension
instead ofAnyProvider
- Accept a
component.Host
andcomponent.ID
- Accept a
component.Host
andRateLimitsConfig
, and return aRateLimiterProvider
using theRateLimiterProvider.RateLimiter
extension component ID, bound to the per-weight configurations inRateLimiterProvider.Limits
The provided helper implementations are based in | ||
`golang.org/x/time/rate` and | ||
`collector-contrib/internal/otelarrow/admission2`. We could instead | ||
create two extension implementations for these. The code is a hundred | ||
lines or so each. |
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.
IMO it's good to keep a clear line of separation between the extension interface and implementations.
|
||
// RateReservation is modeled on pkg.go.dev/golang.org/x/time/rate#Reservation | ||
type RateReservation interface { | ||
// WaitTime returns the duration until this reservation may |
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.
One more thing: I'm not sure this will be good enough for (all) distributed rate limiters. See elastic/opentelemetry-collector-components#619
TL;DR: a Gubernator-based rate limiter won't know up front how long it needs to wait. Rather, we would need to loop until all items have been handled, waiting in between iterations for the limit to be replenished.
Perhaps we could make it a blocking method, and (as you said in a previous draft) make it so that it will return an error if configured to be non-blocking. The API could then look like:
type RateReservation interface {
// Wait waits for the requested number of items to be permitted, or until the context is canceled.
// For non-blocking rate limiters, Wait returns Err??? when it the limit is saturated.
Wait(context.Context) error
// Cancel cancels ...
Cancel()
}
Thank you @axw this great and helpful feedback. I will take a day or two to digest this and come back with an updated proposal. @bogdandrutu I think it would be helpful for you to read @axw's feedback, restricting attention to the README and this concept of "limitermiddleware" which is a point of indirection. |
Description
Improves on the previous drafts:
Middleware config syntax proposal. Further simplified (over-simplified?). In this form, receivers can only configure one limiter. See the updated open questions.
A limiter Tracker struct has been added, provisionally, to discuss (a) how to avoid overcounting, (b) how HTTP middleware knows compression state, (c) how we might instrument limiter behavior, etc.
Follows drafts
1: #12558
2: #12633
3: #12700
4: #12953
5: #13051
6: #13241
Link to tracking issue
Part of #7441
Part of #9591
Part of #12603
Part of #13228
Testing
N/A This will be broken into smaller PRs with tests.
Documentation
Documentation has been updated with new open questions.