Limiter extension API interfaces (draft 4) #12953

jmacd · 2025-04-30T22:29:25Z

Description

See #12603. This follows discussion in #12700 and has been updated extensively based on feedback. This PR is too large to merge as-is but can be taken as a model for a series of smaller PRs.

Link to tracking issue

Part of #9591.

Testing

TODO

Documentation

Done.

extension/extensionlimiter/README.md

extension/extensionlimiter/rate.go

extension/extensionlimiter/wrapper.go

extension/extensionlimiter/rate.go

axw · 2025-05-02T03:34:32Z

extension/extensionlimiter/README.md

+risks wasting memory. In general, an overloaded limiter that is
+saturated SHOULD fail requests immediately.
+
+Limiter implementations SHOULD consider the context deadline when


Should SHOULD be MUST? I tend to think considering context deadline should be mandatory. But maybe I'm being pedantic - did you just mean here that limiters MAY choose not to block if they can anticipate that the context deadline will come before the limiter is no longer saturated?

Following @bogdandrutu's suggestion, I've separated the MustDeny method (which takes only context) from the Acquire/Limit methods which are weight-dependent.

extension/extensionlimiter/README.md

axw · 2025-05-02T03:39:56Z

extension/extensionlimiter/README.md

+Limiters implementations MAY block the request or fail immediately,
+subject to internal logic. A limiter aims to avoid waste, which


What is the internal logic? Are you saying this is a property of the limiter, rather than the caller?

If we made it caller-defined, then I think we could get rid of the MustDeny call: instead you would make a non-blocking request for 0 items. If the limiter is saturated, that would return an error; otherwise it would return success without affecting the capacity.

I have raised a question about non-blocking methods in the open-questions section of the README.

…tor into jmacd/limiter_v4

jmacd · 2025-05-06T22:20:10Z

Reviewers, please see the open questions:

https://github.com/open-telemetry/opentelemetry-collector/pull/12953/files#diff-50c94a038eae5ba5747b0b2c502b0753f2af3664671c4aaec46d4916b828dc25

bogdandrutu

The public API looks good, some naming suggestions and some go implementation in the helper.

bogdandrutu · 2025-05-08T23:14:12Z

extension/extensionlimiter/extensionlimiter.go

+
+// Checker is for checking when a limit is saturated.  This can be
+// called prior to the start of work to check for limiter saturation.
+type Checker interface {


Up for discussion (not sure): Should this be the "basic" limiter, then call it "Limiter" instead?

I had actually named this Limiter in an earlier draft. Yes.

I don't care all that much, but "Check" seems more appropriate than "Limit" to me. Although the outcome of checking the saturation/threshold/capacity/whatever is still limiting, we're not presenting a thing to be limited to the API - we're just checking whether the saturation/threshold/capacity/whatever has been reached.

Regardless of that, I have a nit: can we please keep the interface and method name consistent? e.g. rather than type Checker interface {MustDeny(context.Context) error}, prefer type SaturationChecker interface {CheckSaturation(context.Context) error}.

bogdandrutu · 2025-05-08T23:15:55Z

extension/extensionlimiter/limiterhelper/checker.go

+	var err error
+	for _, lim := range ls {
+		if lim == nil {
+			continue
+		}
+		err = errors.Join(err, lim.MustDeny(ctx))
+	}
+	return err


errors.Join -> is a bit inefficient when lim.MustDeny(ctx) returns nil. I prefer the multierr.Append which does a better job.

bogdandrutu · 2025-05-08T23:17:09Z

extension/extensionlimiter/extensionlimiter.go

+// CheckerProvider is an interface to obtain checkers for a group of
+// weight keys.
+type CheckerProvider interface {
+	// GetChecker returns a checker for a group of weight keys.
+	GetChecker(...Option) (Checker, error)
+}


What is the point of having the extra layer of "Provider" for the "Checker"?

I was thinking of the Option as a placeholder for the open questions, maybe the way to pass the Signal and Component identity. If the actual limiter instance has the options, I expect the basic limiter instance to have the options too.

bogdandrutu · 2025-05-08T23:21:59Z

extension/extensionlimiter/weight.go

+// checked at a certain stage.  The receiver and middleware can both
+// be responsible for applying limits, and this type helps ensure
+// limits are applied only across cooperating sub-components.
+type WeightSet []WeightKey


Can you please add this later when the first usage comes? I don't want to argue more about this PR, and I am unable to understand where this is used.

This is very much not a big deal to me, we can use []WeightKey instead.

The first usage is inside this PR, the helper method Contains used to prevent double-limiting request count in middlware.

Examine how the receiver and middleware cooperate to not limit the same thing twice. The receiver knows which weight keys are applied in middleware (because it knows) and it configures a limiterhelper wrapper for the remaining weight keys. I've listed the standard weight keys here, to try and help users see the mechanic.

The wrapper and the middleware are both able to configure request_count limits. For a push-based receiver, we do this in middleware, but for an arbitrary receiver we might put this logic in the wrapper because it is natural there too: in that case, you would pass three keys to the wrapper). The wrapper could be used to add a made-up network bytes limit too, maybe using the uncompressed size of the data as a proxy for receivers that do not use middleware.

bogdandrutu · 2025-05-08T23:22:54Z

extension/extensionlimiter/weight.go

+// StandardNotMiddlewareKeys methods return the list of middleware
+// keys that can be automatically configured through middleware and
+// not.
+type WeightKey string


Not sure how important this is, do you see a big usage of this on the critical path? If yes, should we use an enum as int instead, since that will be a bit faster for type checks?

My expectation is that weight keys are used during Start() where the providers are called to initialize the actual limiter instances. We have discussed some ideas about new things that could be rate limited, and I believe @axw previously suggested this might be an open set, not a closed one; either way the goal should be to bind limiters once via a provider and use them w/o passing around weight keys at runtime.

bogdandrutu · 2025-05-08T23:24:53Z

extension/extensionlimiter/rate.go

+	CheckerProvider
+
+	// GetRateLimiter returns a rate limiter for a weight key.
+	GetRateLimiter(WeightKey, ...Option) (RateLimiter, error)


Do you think we should have different Options for different providers (different types so we can expand them independently)?

I was thinking about passing in Signal kind, Component ID via these options. I wouldn't mind making these two parameters obligatory, but I don't know how to make it so--the confighttp and configgrpc wrappers don't have this info either.

bogdandrutu · 2025-05-08T23:27:26Z

extension/extensionlimiter/limiterhelper/wrapper.go

+// configmiddleware or limiterhelper is responsible for constructing
+// the correct wrapper from these two kinds of limiter; users will use
+// this interface consistently.
+type LimiterWrapper interface {


Do you expect others to implement this interface? Otherwise we should have this as a struct.

I see the point in the style you're after -- instead of mocking an interface, you should pass in an adapter and place a mock call inside your function.

bogdandrutu · 2025-05-08T23:28:07Z

extension/extensionlimiter/limiterhelper/wrapper.go

+// the appropriate interface for callers that can easily wrap a
+// function call, because for wrapped calls there is no distinction
+// between rate limiters and resource limiters.
+type LimiterWrapperProvider interface {


Do you expect others to implement this interface? Otherwise we should have this as a struct.

I see. I do not need this to be an interface, however I wonder about all the mock-based tests I've ever written with gomock and so on, whether we lose access to this style of test. I'll make this a struct.

bogdandrutu · 2025-05-08T23:31:20Z

extension/extensionlimiter/README.md

+- The protocol name
+- The signal kind
+- The caller's component ID


I am not convinced about protocol, but I see the other 2 being useful.

Sure. Note that protocol is already distinguished through middleware configuration, so a user could configure separate limiters if the distinction matters.

bogdandrutu · 2025-05-08T23:36:08Z

extension/extensionlimiter/README.md

+functions. No examples are provided. How will limiters configure, for
+example, tenant-specific limits?
+
+##### Data-dependent limits


If #39199 is accepted, do you still need support here?

bogdandrutu · 2025-05-09T16:30:50Z

extension/extensionlimiter/extensionlimiter.go

+
+// Checker is for checking when a limit is saturated.  This can be
+// called prior to the start of work to check for limiter saturation.
+type Checker interface {


Do we need to support a limiter that is just "Checker" like the current memory limiter? I would suggest yes.

Yes. So, would you say there are three fundamental limiter APIs, the Rate/Resource and Basic? There is a provider for each -- does it make sense, to you, that there Rate/Resource providers embed the basic limiter provider (as I have called CheckerProvider in this PR)?

Callers would be expected to check for more-specific extension APIs before the less-specific one.

…tor into jmacd/limiter_v4

axw

Apologies for the delay. LGTM overall, just a few minor comments - I think naming could be improved, but that won't affect the API significantly.

axw · 2025-05-21T03:00:56Z

extension/extensionlimiter/README.md

+keys.  Because a `Checker` can be consulted more than once by a
+receiver and/or middleware, it is possible for requests to be denied
+over the saturation of limits they were already granted. Users should


I don't understand this. I mean I understand that if you call the limiter first, and then check saturation that the latter may fail. Why would you do that? Isn't that a logic error in the caller, or are there legitimate scenarios where you would do this...?

axw · 2025-05-21T03:05:03Z

extension/extensionlimiter/README.md

+as follows. The HTTP client config object's `middlewares` field
+automatically configures network bytes and request count limits:


Re network bytes: would that be limiting on the response body size, and wrapping the net/http.Response.Body for HTTP?

axw · 2025-05-21T03:23:21Z

extension/extensionlimiter/README.md

+Another option is to add support for non-blocking limit requests. For
+example, to apply limits using information derived from the
+OpenTelemetry resource, we might do something like this pseudo-code:


I think we could do something like this with the partitioning processor:

split data by resource

configure a limiter to silently drop data without error when saturated

axw · 2025-05-21T03:39:31Z

extension/extensionlimiter/extensionlimiter.go

+
+// Checker is for checking when a limit is saturated.  This can be
+// called prior to the start of work to check for limiter saturation.
+type Checker interface {


I don't care all that much, but "Check" seems more appropriate than "Limit" to me. Although the outcome of checking the saturation/threshold/capacity/whatever is still limiting, we're not presenting a thing to be limited to the API - we're just checking whether the saturation/threshold/capacity/whatever has been reached.

Regardless of that, I have a nit: can we please keep the interface and method name consistent? e.g. rather than type Checker interface {MustDeny(context.Context) error}, prefer type SaturationChecker interface {CheckSaturation(context.Context) error}.

axw · 2025-05-21T03:59:59Z

extension/extensionlimiter/limiterhelper/middleware.go

+// MiddlewareIsLimiter returns true if a middleware configuration
+// represents a valid limiter, returns false for not found or invalid
+// cases. If the named extension is found but is not a limiter,
+// returns (false, nil).
+func MiddlewareIsLimiter(host component.Host, middleware configmiddleware.Config) (bool, error) {
+	_, ok, err := middlewareIsLimiter(host, middleware)
+	return ok, err
+}


Do we need this, given that MiddlewareToLimiterWrapperProvider will return ErrNotALimiter? i.e. we could just call MiddlewareToLimiterWrapperProvider and check for that error.

jmacd · 2025-05-27T15:37:43Z

@axw Apologies, I have created a new draft github.com//pull/13051. I mean to incorporate all of the feedback above. In particular, I would note that I've reverted the name of the basic limiter to BaseLimiter in the new PR and kept the original name MustDeny. I appreciate your request to make this more consistent, and I've added consistency in the other interfaces (e.g., ResourceLimiter has a method named ReserveResource, RateLimiter has ReserveRate).

jmacd added 7 commits April 30, 2025 13:20

lint

2eb77f5

close

04380f4

readme

7444153

move multi-limiter

e597fa1

data-dep example

e6675c8

lint

ac0d1ec

lint

b3e4554

jmacd requested a review from a team as a code owner April 30, 2025 22:29

jmacd requested a review from dmitryax April 30, 2025 22:29

github-actions bot added the receiver/otlp label Apr 30, 2025

jmacd requested a review from axw April 30, 2025 22:30

jmacd marked this pull request as draft April 30, 2025 22:33

bogdandrutu reviewed May 1, 2025

View reviewed changes

axw reviewed May 2, 2025

View reviewed changes

jmacd added 8 commits May 5, 2025 15:30

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

ffad28a

…tor into jmacd/limiter_v4

wip split Checker (was Limiter)

b8ed41d

rename

d07da61

move wrapper into limiterhelper

f3a2f2f

style

8b31cd6

call checker once

40aee98

checker all keys

137be27

readme

4a44264

bogdandrutu approved these changes May 8, 2025

View reviewed changes

bogdandrutu reviewed May 8, 2025

View reviewed changes

bogdandrutu reviewed May 9, 2025

View reviewed changes

jmacd added 2 commits May 12, 2025 09:47

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

f873ca8

…tor into jmacd/limiter_v4

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

3194963

…tor into jmacd/limiter_v4

jmacd mentioned this pull request May 20, 2025

Limiter extension API interfaces and implementation helpers (**draft 5**) #13051

Closed

axw approved these changes May 21, 2025

View reviewed changes

jmacd closed this May 27, 2025

This was referenced Jun 20, 2025

Limiter extension API interfaces and implementation helpers (**draft 6**) #13241

Closed

Limiter extension APIs and implementation helpers (**draft 7**) #13265

Draft

		Limiters implementations MAY block the request or fail immediately,
		subject to internal logic. A limiter aims to avoid waste, which

		as follows. The HTTP client config object's `middlewares` field
		automatically configures network bytes and request count limits:

Limiter extension API interfaces (**draft 4**) #12953

Limiter extension API interfaces (**draft 4**) #12953

Uh oh!

Conversation

jmacd commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Link to tracking issue

Testing

Documentation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmacd commented May 6, 2025

Uh oh!

bogdandrutu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axw left a comment

Choose a reason for hiding this comment

Uh oh!

Limiter extension API interfaces (draft 4) #12953

Limiter extension API interfaces (draft 4) #12953

jmacd commented Apr 30, 2025 •

edited

Loading