-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Limiter extension API interfaces and implementation helpers (**draft 5**) #13051
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_v5
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.
In general LGTM extensionlimiter
package, for the "limiterhelper" only looked very high level and I will do a more deeper review after we go over the first round of comments related to the main extensionlimiter
package.
// providers with Options and add a Config type, but none are | ||
// supported yet and this PR contains only interfaces, not need for | ||
// options in core repository components. | ||
type Option interface { |
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: I would name the file option not config.
var _ RateLimiterProvider = struct { | ||
GetRateLimiterFunc | ||
GetBaseLimiterFunc | ||
}{} |
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: Not sure these kind of checks are relevant to be enforced here.
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.
👍
|
||
// BaseLimiter is for checking when a limit is saturated. This can be | ||
// called prior to the start of work to check for limiter saturation. | ||
type BaseLimiter interface { |
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: Rename to BasicLimiter? or Limiter?
Reasoning: For me "base" means that it will be embedded into something else in order to use it, and cannot be used as a standalone. Please correct me if I am wrong.
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.
Thanks. I'm open to either of these, and I agree with your reasoning that "base" isn't great. @axw has suggested that the type and the method name should be related, so if we have Limiter then the API method should be Limit(), if we have Checker it should be Check(), however I find it difficult to apply this pattern to MustDeny. Should this be something else completely, like SaturationChecker with a method named CheckSaturation as @axw proposed?
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 explain my thought process:
I would typically name interfaces after a capability, hence naming the interface after a method. This is also suggested in https://go.dev/doc/effective_go#interfaces_and_types:
Interfaces with only one or two methods are common in Go code, and are usually given a name derived from the method, such as io.Writer for something that implements Write.
So to me, RateLimiter and ResourceLimiter feel natural, but BaseLimiter does not -- you're not limiting a base. Also, something like a "base" sounds like a mixin type, which would be relevant for implementation (e.g. a struct) rather than the capability/behaviour (interface).
It seems to me that the ability to check saturation is the capability here, hence SaturationChecker/CheckSaturated.
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.
It's also possible that we don't really need to expose this interface at all, but just have a saturation check method on each of RateLimiter and ResourceLimiter. See also my other questions about BaseLimiter.
1. Network bytes | ||
2. Request count | ||
3. Request items | ||
4. Memory size |
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: "Request bytes" instead of memory size?
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.
OK, so it'll be
- Network bytes (compressed)
- Request bytes (uncompressed)
Still, I want to document this as the amount of memory used by the request, not the uncompressed size of the payload (even though it will often equal this value). For a stream protocol (e.g., OTel-Arrow) there will be memory in the stream state that is referred to by the request (e.g., dictionaries) in such a way that:
a. The uncompressed size of the payload will underestimate the memory held by the request
b. The uncompressed size of the OTLP object will overestimate the memory helped by the request
Therefore, components in special circumstances should use judgement: limit by how much memory is tied up by the request in the pipeline, and prefer to overestimate than to underestimate. As an example, repeated requests in an OTel-Arrow stream will refer to the same dictionary entries, meaning a precise estimate of memory referenced by individual requests will overestimate how much is used by a pipeline, due to aliasing within the pipeline.
interface, and callers are expected to check for saturation by | ||
invoking `MustDeny` before making individual requests with the limiter. |
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 would like to include the idea of "as early as possible" into this explanation.
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.
Will do.
be called at the proper time, then any kind of limiter can be applied | ||
in the form of a `ResourceLimiter`. If the extension is a basic or | ||
rate limiter in this scenario, use the `BaseToResourceLimiterProvider` | ||
or `RateToResourceLimiterProvider` adapters. |
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 commented in the BaseToResourceLimiterProvider for some issues that I can imagine, but in general I am not sure the path is to make all look the same vs have helpers for the 3 types independent.
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.
The approach I've taken is to provide every possible conversion function, which is a fairly small set. BaseToRate, BaseToResource, or RateToResource providers are for situations where LimiterWrapper can't be used (these are "independent", in the sense you mean it, I think), in which case circumstances vary. Two examples follow for Rate and Resource limits.
As pointed at here, middleware is often in a place where only a Rate limit can be applied -- typically for compressed bytes -- and in this case BaseToRate applies (e.g., to adapt memorylimiterextension for middleware).
In scenarios where LimiterWrapper does not apply -- having to do with flow control (e.g. for stream backpressure) -- but where the Resource limiter can be applied because the application is able to release after the request finishes. Here, a Resource limiter interface should be preferred because its interface can be adapted from all the others. In these cases you will use RateToResource and BaseToResource.
flow and resource usage through extensions which are configured | ||
through middleware and/or directly by pipeline components. | ||
|
||
## Overview |
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 of the top section should be for "end users" how to configure limiters in some of the most common cases.
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 will work on this -- although I haven't actually produced anything for end users that is new here, only the memorylimiterextension could be documented (and it is already linked below).
I would agree to update this with links to the implementations. I would like to request input from @axw on this topic, because while I have created helper implementations of the rate-limiter (from golang.org/x/time/rate) and the resource-limiter. Both of these have two numeric parameters, and we still have to discuss how a real limiter will be configured, which will lead back to our open questions (e.g., how to configure the limiter based on client metadata, how to configure the limiter based on the component name or the signal type?).
|
||
### Built-in limiters | ||
|
||
#### Base |
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: Called this basic at the beginning of the document, please be consistent.
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.
Yes, I've been inconsistent. See https://github.com/open-telemetry/opentelemetry-collector/pull/13051/files/ba136deb9590d6fc5999c1af548f59965cbd0599#r2106643694.
#### OTLP receiver | ||
|
||
Limiters applied through middleware are an implementation detail, | ||
simply configure them using `configgrpc` or `confighttp`. For the | ||
OTLP receiver (e.g., with two `ratelimiter` extensions): | ||
|
||
```yaml | ||
extensions: | ||
ratelimiter/limit_for_grpc: | ||
# rate limiter settings for gRPC | ||
ratelimiter/limit_for_grpc: | ||
# rate limiter settings for HTTP | ||
|
||
receivers: | ||
otlp: | ||
protocols: | ||
grpc: | ||
middlewares: | ||
- ratelimiter/limit_for_grpc | ||
http: | ||
middlewares: | ||
- ratelimiter/limit_for_http | ||
``` | ||
|
||
Note that the OTLP receiver specifically supports multiple protocols | ||
with separate middleware configurations, thus it configures limiters | ||
for request items and memory size on a protocol-by-protocol basis. |
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.
Please bear with me on this:
I am trying to evaluate if for end users (not how we implement it, which we can do with same structs) they would like to see:
extensions:
ratelimiter/limit_for_grpc:
# rate limiter settings for gRPC
ratelimiter/limit_for_grpc:
# rate limiter settings for HTTP
receivers:
otlp:
protocols:
grpc:
middlewares:
- ratelimiter/limit_for_grpc
http:
middlewares:
- ratelimiter/limit_for_http
OR
extensions:
ratelimiter/limit_for_grpc:
# rate limiter settings for gRPC
ratelimiter/limit_for_grpc:
# rate limiter settings for HTTP
receivers:
otlp:
protocols:
grpc:
limiters:
- ratelimiter/limit_for_grpc
http:
limiters:
- ratelimiter/limit_for_http
Suggestion being in confighttp/configgrpc to add a Limiters []configmiddleware.Config mapstructure:"limiters,omitempty"
similar with https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configgrpc/configgrpc.go#L111 but just call it Limiters.
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 is a side-note, but a relevant one.
Keep in mind that our present definition for configmiddleware.Config
is
// Middleware defines the extension ID for a middleware component.
type Config struct {
// ID specifies the name of the extension to use.
ID component.ID `mapstructure:"id,omitempty"`
// prevent unkeyed literal initialization
_ struct{}
}
which means the config reads with an id
like:
grpc:
middlewares:
- id: ratelimiter/limit_for_grpc
Does squash
support what you've written? Otherwise the way it reads, I think you want something like
type Config = component.ID
instead of a struct
for middleware config.
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.
As for your question about separating limiters and middleware, I think we should focus on the HTTP case, where it is pretty typical to interleave limiters and other kinds of middleware. There is a potential to refactor confighttp
so that some of the existing setup would move into middleware components, for example:
- compression middleware
- opentelemetry instrumentation
at this point, more questions are raised, because we have discussed limiting by both compressed and uncompressed weights. gRPC gives you compressed and uncompressed in the same stats handler call, but HTTP does compression in middleware, so the network-bytes limit has to be applied before middleware.
In my presentation here, I've implemented "memory_size" -- you suggested request_bytes, sure -- in the receiver. If we want to implement request-size earlier, in the middleware, then we need to configure HTTP limiters before and after the compression middleware.
For this reason, I think limiters should remain in the list of middleware.
For a receiver that otherwise does not use middleware, I think it would be appropriate to follow your example above, that is to have a Limiters []configmiddleware.Config
field where we expect to find only limiters.
If you agree, I would file separate tracking issues for the process of moving compression and/or opentelemetry instrumentation into middleware components. This would allow building the collector with alternative compression libraries and/or OTel SDKs, 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.
Jmacd's comment makes me feel sympathetic to just putting everything in a middlewares
list
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.
In my ideal world, users should be able to configure component-specific rate limiters independently of transport. Maybe they use middleware that uses the same limiter extension, maybe not -- the user should be able to choose.
I would also want to get rid of the "StandardNotMiddlewareKeys" function and leave it to users to choose which limits they want to apply. This can be done by having multiple, specific limiter configurations. e.g.:
extensions:
ratelimiter/limit_for_grpc:
# rate limiter settings for gRPC
ratelimiter/limit_for_http:
# rate limiter settings for HTTP
ratelimiter/limit_for_otlp:
# rate limiter settings for OTLP, independent of transport
receivers:
otlp:
# limiters holds OTLP-specific limiters. For transport-level limiters, configure middleware.
limiters:
# requests defines a limiter for processing OTLP requests (batches), independent of signal.
requests: ratelimiter/limit_for_otlp
# items defines a limiter for processing OTLP items: log records, spans, data points, profile samples
items: ratelimiter/limit_for_otlp
# bytes defines a limiter for processing OTLP-encoded bytes.
bytes: ratelimiter/limit_for_otlp
protocols:
grpc:
middlewares:
- ratelimiter/limit_for_grpc
http:
middlewares:
- ratelimiter/limit_for_http
If you agree, I would file separate tracking issues for the process of moving compression and/or opentelemetry instrumentation into middleware components.
Yes please. FWIW, we just internally found a need to do the same for auth, IMO that would be good to have too.
scraper: | ||
http: | ||
middlewares: | ||
- ratelimiter/scraper |
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 know you will tell me "I told you", but how can we configure limiters for non grpc/http scrapers like hostmetrics/filebased?
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 mean yes, the point of this section was to answer your question. We could go with either of the options you presented, also I made the mistake about id:
here too: we should think about whether we want to eliminate the id:
prefix, see https://github.com/open-telemetry/opentelemetry-collector/pull/13051/files/ba136deb9590d6fc5999c1af548f59965cbd0599#r2106696658.
Note to incorporate feedback in #12953 (review) |
return struct { | ||
extensionlimiter.WaitTimeFunc | ||
extensionlimiter.CancelFunc | ||
}{}, nil |
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 would strongly advise against anon struct definitions here, please move the definition to be a Nop type.
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.
Sorry for the delay. Overall I think it's looking good.
I left a few related questions about BaseLimiter - it feels like it could be removed, and left to extensions to present either a RateLimiter or ResourceLimiter that does something specific to that extension where there's no natural rate or resource.
I left a suggestion for how we could structure the component-specific limits vs. transport-specific limits, which I think also provides an answer to @bogdandrutu's question about scraper receivers.
|
||
// BaseLimiter is for checking when a limit is saturated. This can be | ||
// called prior to the start of work to check for limiter saturation. | ||
type BaseLimiter interface { |
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 explain my thought process:
I would typically name interfaces after a capability, hence naming the interface after a method. This is also suggested in https://go.dev/doc/effective_go#interfaces_and_types:
Interfaces with only one or two methods are common in Go code, and are usually given a name derived from the method, such as io.Writer for something that implements Write.
So to me, RateLimiter and ResourceLimiter feel natural, but BaseLimiter does not -- you're not limiting a base. Also, something like a "base" sounds like a mixin type, which would be relevant for implementation (e.g. a struct) rather than the capability/behaviour (interface).
It seems to me that the ability to check saturation is the capability here, hence SaturationChecker/CheckSaturated.
// | ||
// See the README for more recommendations. | ||
type RateLimiter interface { | ||
// ReserveRate is modeled on pkg.go.dev/golang.org/x/time/rate#Limiter.ReserveN |
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 would caution against overindexing on that package. I quite like its API, but reservation is easy for local rate limiters, not so easy for distributed ones.
We wouldn't be able to adapt our rate limiter to this API, since Gubernator just does not support reservation. Similarly, it wouldn't be possible to use https://pkg.go.dev/github.com/juju/ratelimit (though I don't think we would be able to use it here anyway -- it's LGPL)
IIUC, your goal is to enable non-blocking behaviour, and not specifically to enable reservation/cancellation -- is that correct? If so, it is possible to make the rate limiters non-blocking without supporting reservations. For example, you could return a duration that the caller is expected to wait before proceeding, like:
type RateLimiter interface {
// RateLimit reserves n tokens, either immediately or in the future,
// returning the duration that the caller must wait before those tokens
// are available.
RateLimit(ctx context.Context, n int) (time.Duration, error)
}
Internally, a golang.org/x/time/rate
-based implementation would create a reservation to implement this interface.
The key difference here is that once you've requested the limit, there's no going back.
if lim == nil { | ||
return nil, nil | ||
} | ||
blocking := NewBlockingResourceLimiter(lim) |
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 feels a bit too prescriptive. The wrapper API seems like it should be orthogonal to whether the limiter is blocking or non-blocking. Then for example, users could configure their receivers to either return immediately with 429 or block. Either way I would expect this limiter provider to be used, it would just be a matter of whether it internally blocks or not.
// MiddlewareToBaseLimiterProvider returns a base limiter provider | ||
// from middleware. Returns a package-level error if the middleware | ||
// does not implement exactly one of the limiter interfaces (i.e., | ||
// rate or resource). | ||
func MiddlewareToBaseLimiterProvider(ext extensionlimiter.BaseLimiterProvider) (extensionlimiter.BaseLimiterProvider, error) { | ||
return getMiddleware( | ||
ext, | ||
identity[extensionlimiter.BaseLimiterProvider], | ||
baseProvider[extensionlimiter.RateLimiterProvider], | ||
baseProvider[extensionlimiter.ResourceLimiterProvider], | ||
) | ||
} |
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.
Would anything ever need just a BaseLimiter? I'm wondering if the BaseLimiterProvider interface is actually needed, or if clients can request either a RateLimiterProvider or a ResourceLimiterProvider, given an extension.
// BaseToRateLimiterProvider allows a base limiter to act as a rate | ||
// limiter. | ||
func BaseToRateLimiterProvider(blimp extensionlimiter.BaseLimiterProvider) extensionlimiter.RateLimiterProvider { |
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 think this name needs to be a bit more descriptive about the behaviour. A key aspect is that if the limit is not currently saturated, any number of concurrent operations may will be allowed through: the operation will not directly affect the saturation. (Maybe indirectly, e.g. by increasing memory usage)
I also wonder if there really can be a one-size-fits-all for this? Perhaps it should be left to each extension to choose how to adapt? Do you foresee anything other than memorylimiter implementing only BaseLimiter and neither RateLimiter nor ResourceLimiter?
|
||
// BaseLimiter is for checking when a limit is saturated. This can be | ||
// called prior to the start of work to check for limiter saturation. | ||
type BaseLimiter interface { |
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.
It's also possible that we don't really need to expose this interface at all, but just have a saturation check method on each of RateLimiter and ResourceLimiter. See also my other questions about BaseLimiter.
#### OTLP receiver | ||
|
||
Limiters applied through middleware are an implementation detail, | ||
simply configure them using `configgrpc` or `confighttp`. For the | ||
OTLP receiver (e.g., with two `ratelimiter` extensions): | ||
|
||
```yaml | ||
extensions: | ||
ratelimiter/limit_for_grpc: | ||
# rate limiter settings for gRPC | ||
ratelimiter/limit_for_grpc: | ||
# rate limiter settings for HTTP | ||
|
||
receivers: | ||
otlp: | ||
protocols: | ||
grpc: | ||
middlewares: | ||
- ratelimiter/limit_for_grpc | ||
http: | ||
middlewares: | ||
- ratelimiter/limit_for_http | ||
``` | ||
|
||
Note that the OTLP receiver specifically supports multiple protocols | ||
with separate middleware configurations, thus it configures limiters | ||
for request items and memory size on a protocol-by-protocol basis. |
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.
In my ideal world, users should be able to configure component-specific rate limiters independently of transport. Maybe they use middleware that uses the same limiter extension, maybe not -- the user should be able to choose.
I would also want to get rid of the "StandardNotMiddlewareKeys" function and leave it to users to choose which limits they want to apply. This can be done by having multiple, specific limiter configurations. e.g.:
extensions:
ratelimiter/limit_for_grpc:
# rate limiter settings for gRPC
ratelimiter/limit_for_http:
# rate limiter settings for HTTP
ratelimiter/limit_for_otlp:
# rate limiter settings for OTLP, independent of transport
receivers:
otlp:
# limiters holds OTLP-specific limiters. For transport-level limiters, configure middleware.
limiters:
# requests defines a limiter for processing OTLP requests (batches), independent of signal.
requests: ratelimiter/limit_for_otlp
# items defines a limiter for processing OTLP items: log records, spans, data points, profile samples
items: ratelimiter/limit_for_otlp
# bytes defines a limiter for processing OTLP-encoded bytes.
bytes: ratelimiter/limit_for_otlp
protocols:
grpc:
middlewares:
- ratelimiter/limit_for_grpc
http:
middlewares:
- ratelimiter/limit_for_http
If you agree, I would file separate tracking issues for the process of moving compression and/or opentelemetry instrumentation into middleware components.
Yes please. FWIW, we just internally found a need to do the same for auth, IMO that would be good to have too.
Description
Draft demonstrating the following advances on the previous draft:
configmiddleware
integrationmemorylimiterextension
Follows drafts
1: #12558
2: #12633
3: #12700
4: #12953
Link to tracking issue
Part of #9591.
Part of #12603.
Testing
NONE: This is a demonstration, smaller PRs will be broken apart and tested individually if reviewers like the look of this, the full assembly.
Documentation
Updated.