-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Rough sketch (draft 3) limiter extension and middleware #12700
base: main
Are you sure you want to change the base?
Conversation
…missed a form of gRPC interceptor. For both client and server gRPC cases in the middleware extension API, introduce a method to obtain a stats handler. ClientStatsHandler() and ServerStatsHandler() methods will be added, returning (grpc.StatsHandler, error). In limitermiddleware, add support for the two new methods. Implement the StatsHandler interface with empty methods for now. The type is named stats.Handler, package documented https://pkg.go.dev/google.golang.org/[email protected]/stats#Handler
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 reviewed extensionmiddleware
and extensionlimiter
:
- I have very few comments that we can discuss about;
- We should merge these asap; matches exactly with what I expected to see.
- Next on my list will be the configs then the convertors.
// ClientStatsHandler returns a gRPC stats handler for client-side operations. | ||
ClientStatsHandler() (stats.Handler, error) |
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 sure if this should be part of the GRPCClient
or different GRPCStatsClient
?
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 rolled this into a GetGRPCClientOptions() ([]grpc.DialOption, error) and I am happy with the result.
// Key identifies the type of weight being limited | ||
Key string |
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.
Should we use an enum here instead?
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 agree
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.
Done. (TODO: I believe I need an Unmarshal function)
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.
Alternatively, maybe we could get rid of the key, and make it part of the caller's configuration to specify different limiters? #12700 (comment)
// | ||
// On success, it returns a ReleaseFunc that should be called | ||
// when the resources are no longer needed. | ||
Acquire(ctx context.Context, weights []Weight) (ReleaseFunc, error) |
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 sure I understand the need of a slice of weights vs just one.
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 it's because the caller might not know what weights are important to 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 understand. May not be bad to avoid an unnecessary slice allocation here, and since the list of keys is limited to maybe embed all the sizes in one Weight?
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 had been considering a case where multiple weights are known at once, and that there might be a cheaper call as a result, but I've removed this complication--it helps for other reasons to resolve the limiter by its WeightKey (the enum) when the component starts. This gives the middleware component (i.e., limitermiddleware) a chance to disable features that are not in use. This means no gRPC interceptors unless a limiter is using the request_count weight key, and no stats handler unless a limiter is using the network_bytes weight key.
// ServerStatsHandler returns a gRPC stats handler for server-side operations. | ||
ServerStatsHandler() (stats.Handler, error) |
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.
Same question as for client, not sure if this should be here or a separate interface. Idea is that in general you either have implementations for the 2 interceptors or for the stats handler.
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 noted above, I've added to extensionlimiter a new Provider interface, which returns RateLimiter or ResourceLimiters on request. This means that the stats handler and/or interceptor are only created as needed.
// GetGRPCClientMiddleware attempts to select the appropriate extensionmiddleware.GRPCClient from the list of extensions, | ||
// based on the component id of the extension. If a middleware is not found, an error is returned. | ||
// This should be only used by gRPC clients. | ||
func (m Middleware) GetGRPCClientMiddleware(_ context.Context, extensions map[component.ID]component.Component) (extensionmiddleware.GRPCClient, error) { |
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 can be "GetGRPCClientOptions" and return grpc.DialOptions
then here we can look for any supported interfaces:
- interceptors
- stats handler
- limiter
Then we can convert limiter to an interceptor here automatically and for the rest we call the right APIs to create them and to convert them to options.
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 understand what you mean -- take a look at the updates in this PR, which go part of the way you're suggesting. Presently, I kept the limiter and middleware separation, but I followed the example in #12702 the way you implied above, so the extensionmiddleware.GRPCClient will return a list of dial options, the GRPCServer will return a list of server options. The HTTP client and server are relatively untouched, however I am using a name that is different than extensionauth.HTTPClient on purpose.
I like the way things have shaped up here. I like the interfaces you've defined and I think it sets us going in the right direction |
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 @bogdandrutu, @mattsains. I've improved this PR (still a draft) based on your feedback.
I'm happy with how the gRPC rate limits are implemented, and I added HTTP network-bytes limits to flush out the skeleton of this approach. The http.RoundTripper and http.Handler will be created when either of the two weight keys it supports are used: network_bytes and request_count.
Note that request_items and resident_bytes will be implemented at a different level in the receiver(s).
However, there are still likely some challenges. There are existing middlewares with a pre-defined order, including auth, headers, compression, and opentelemetry instrumentation. To add a limiter we want to go before compression. To turn compression or opentelemetry instrumentation into extensions, which sounds nice, also implies a transition plan. I could imagine making the middleware configurable with a default of [compression, opentelemetry]; if you want to add a rate limiter you'll have to include compression and opentelemetry in the proper order.
// GetGRPCClientMiddleware attempts to select the appropriate extensionmiddleware.GRPCClient from the list of extensions, | ||
// based on the component id of the extension. If a middleware is not found, an error is returned. | ||
// This should be only used by gRPC clients. | ||
func (m Middleware) GetGRPCClientMiddleware(_ context.Context, extensions map[component.ID]component.Component) (extensionmiddleware.GRPCClient, error) { |
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 understand what you mean -- take a look at the updates in this PR, which go part of the way you're suggesting. Presently, I kept the limiter and middleware separation, but I followed the example in #12702 the way you implied above, so the extensionmiddleware.GRPCClient will return a list of dial options, the GRPCServer will return a list of server options. The HTTP client and server are relatively untouched, however I am using a name that is different than extensionauth.HTTPClient on purpose.
// Key identifies the type of weight being limited | ||
Key string |
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.
Done. (TODO: I believe I need an Unmarshal function)
// | ||
// On success, it returns a ReleaseFunc that should be called | ||
// when the resources are no longer needed. | ||
Acquire(ctx context.Context, weights []Weight) (ReleaseFunc, error) |
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 had been considering a case where multiple weights are known at once, and that there might be a cheaper call as a result, but I've removed this complication--it helps for other reasons to resolve the limiter by its WeightKey (the enum) when the component starts. This gives the middleware component (i.e., limitermiddleware) a chance to disable features that are not in use. This means no gRPC interceptors unless a limiter is using the request_count weight key, and no stats handler unless a limiter is using the network_bytes weight key.
// ClientStatsHandler returns a gRPC stats handler for client-side operations. | ||
ClientStatsHandler() (stats.Handler, error) |
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 rolled this into a GetGRPCClientOptions() ([]grpc.DialOption, error) and I am happy with the result.
// ServerStatsHandler returns a gRPC stats handler for server-side operations. | ||
ServerStatsHandler() (stats.Handler, error) |
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 noted above, I've added to extensionlimiter a new Provider interface, which returns RateLimiter or ResourceLimiters on request. This means that the stats handler and/or interceptor are only created as needed.
"context" | ||
) | ||
|
||
// WeightKey is an enum type for common rate limits |
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.
// WeightKey is an enum type for common rate limits | |
// WeightKey identifies a category of limits, such as requests or network bytes. | |
// | |
// WeightKey* constants are provided for common limit keys for the convenience of consumers | |
// of the Provider interface. Limiter implementations may support only specific weight keys | |
// (e.g. a memory limiter may support only WeightKeyResidentMemory), whereas others may | |
// support any weight key (e.g. rate limiting by an arbitrary `<key>/second`). |
Sort of question/suggestion combo: should we support arbitrary keys? I think so, but I realise that in some cases only certain types of limits may be relevant.
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.
... now I'm back to wondering if this key should be needed at all.
Would it make sense for the middleware to also handle WeightKeyResidentMemory? I think probably not, but if it did then would it be the same provider? I would expect it would be memorylimiterextension. On the flip side, it probably would not make sense for memorylimiterextension to handle the other keys. So the Provider interface feels a bit off to me.
Have you considered the following kind of configuration for the middleware?
extensions:
# Unhelpful names to highlight that the name of the rate limiter is arbitrary.
# Each instance would be independent of the others.
ratelimiter/foo:
metadata_keys: [x-tenant-id] # at this level, limit each tenant independently
rate: 1
burst: 10
ratelimiter/bar:
rate: 2
burst: 20
ratelimiter/baz:
rate: 3
burst: 30
# Limiter middleware would have well-defined configuration for each kind of
# limit it might want to impose. Each one points to a separate limiter extension.
limitermiddleware:
network_bytes:
limiter: ratelimiter/foo
request_items:
limiter: ratelimiter/bar
request_count:
limiter: ratelimiter/baz
memorylimiter:
limit_percentage: 80
receivers:
otlp:
protocols:
http:
endpoint: :4318
include_metadata: true # adds "x-tenant-id" header to client metadata in context
middleware:
- ratelimitermiddleware
- memorylimiter
P.S. the core Provider interface and contract with middleware and other callers is really the only thing bothering me - otherwise this is looking great. Thank you for working on it! ❤️ |
Description
After prior drafts, summarized in #12603, with feedback from @bogdandrutu and @axw, I explored adding limiters via middleware structured as two separate configuration and two separate extensions.
This draft includes only the outline of 6 (six!) new modules, which piece together to support a variety of limiter and interceptor behaviors. While I am concerned about the scope of this (#9591, #7441), this appears to be a good direction:
There are implied changes here as well as TODOs:
Link to tracking issue
Part of #9591 #7441 #12603
Testing
NONE: for discussion
Documentation
NONE: TODO