Limiter extension interface for memorylimiterextension draft#12601
Limiter extension interface for memorylimiterextension draft#12601jmacd wants to merge 5 commits into
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12601 +/- ##
==========================================
- Coverage 92.18% 92.16% -0.02%
==========================================
Files 469 472 +3
Lines 25394 25572 +178
==========================================
+ Hits 23409 23569 +160
- Misses 1574 1591 +17
- Partials 411 412 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package limit // import "go.opentelemetry.io/collector/extension/xextension/limit" |
There was a problem hiding this comment.
Based on auth case, I think this should be consistent and be named extensionlimit
| type Extension interface { | ||
| extension.Extension | ||
|
|
||
| // GetClient will create a client for use by the specified | ||
| // component. The component can use the client to limit | ||
| // admission to the pipeline. | ||
| GetClient(ctx context.Context, kind component.Kind, id component.ID) (Client, error) | ||
| } |
There was a problem hiding this comment.
Some questions/asks here:
- Need to be consistent with decision in [extensionauth] Split extensionauth.Client by protocol type #12574, and not include the
extension.Extensioninto this. - Do we need the "extra" client interface? Asking because we don't follow this pattern in
extensionauthbut we do this instorageextension. If we need that (there may be good reasons to have it), I would like to make the auth follow the same pattern. cc @mx-psi
There was a problem hiding this comment.
It makes more sense to me, without further context, to use the approach on my PR: if multiple instances of the client are required then the user can define multiple extensions (limiter/1, limiter/2...)
There was a problem hiding this comment.
I didn't mean to be inconsistent with #12574, sorry. That approach looks fine to me.
As noted, the "extra" interface appears valuable to me in the sense that static context (e.g., component metadata) can be used to specialize the limiting conditions at start time similar to x/storage. See my comment in #12603 (comment) on this topic. Today's configgrpc and confighttp helpers do not include the component metadata, so to be realistic I removed the extra parameters. If we remove the extra parameters and do not arrange to pass component metadata (e.g., signal kind, component ID) via context, then there is no reason for the extra interface.
See I opened several drafts--the copy you are commenting on, here, is an older iteration of mine, where I tried to follow the x/storage model before I realized it would not work. In #12600 the interface became simply GetLimiter(ctx), see https://github.com/open-telemetry/opentelemetry-collector/pull/12600/files#diff-8be8b8c994bb03af15a8c1a7cc8953d44053c7b40726d0ec99f365ca29eee608R19.
| // (2) the provided ctx becomes canceled, or | ||
| // (3) the limiter determines that the request should fail. |
There was a problem hiding this comment.
To clarify, in this case returned err is not nil.
| var nopClientInstance Client = &nopClient{} | ||
|
|
||
| // NewNopClient returns a nop client | ||
| func NewNopClient() Client { |
There was a problem hiding this comment.
We usually have this in a limittest package/module. Please follow that model.
|
See #12603 (comment). |
3 participants
Description
This demonstrates how the existing memorylimiter extension will be upgraded to support the proposed limiter extension interface.
Link to tracking issue
Part of #9591.
Testing
TODO
Documentation
TODO