-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Middleware: extension interface (part 1/4) #12843
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
Middleware: extension interface (part 1/4) #12843
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12843 +/- ##
=======================================
Coverage 91.46% 91.46%
=======================================
Files 493 493
Lines 26984 26984
=======================================
Hits 24682 24682
Misses 1818 1818
Partials 484 484 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
For the spell checker we need to allow the |
|
Also please fix the checks: |
jade-guiton-dd
left a comment
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 don't have much context on the middleware work, but lints aside, this looks good to me.
| // GRPCClient is an interface for gRPC client middleware extensions. | ||
| type GRPCClient interface { | ||
| // GetGRPCClientOptions returns the gRPC dial options to use for client connections. | ||
| GetGRPCClientOptions() ([]grpc.DialOption, 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.
How certain are we that DialOptions/ServerOptions fulfill all the envisioned use cases for middlewares? (I don't know of any counter-examples but just want to confirm)
I'm also wondering if some extensions might require context on what receiver/exporter they're being applied to, or what configgrpc.Config is being applied
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 fair question. I've answered it partly in the README, see 4056d60.
This is one of two chances to identify which component is connecting a middleware extension--if the confighttp or configgrpc struct that contains the middleware reference would pass in this information, we could use it, but we don't have that information presently. To be clear, confighttp and configgrpc and configauth also do not know which component they are acting for. We could use the Context passed to component Start() to identify the component that is starting, as a workaround. Should we add Context arguments to these functions? Should we consider modifying confighttp and configgrpc now to include the calling component explicitly?
Note that we don't know which signal is responsible for the activation, which is at least equally relevant. In the context of middleware, you can typically inspect a path or method name to learn which signal is involved. For the limiter extension that I prototyped as part of #12700, the same would not apply, and we will have to decide whether to be explicit (e.g., via a Settings argument) or implicit if we want to pass this information to the extension at Start() or at runtime.
Can we talk about how the top-level Makefile runs a different spelling program than the Github action?
😭 My personal hell is having people point out robot feedback, especially in a repo where the top-level Makefile provides no useful help. ;) |
Description
Adds the extension API from #12842.
Link to tracking issue
Part of #12603.
Testing
Not tested. See configmiddleware.
Documentation
Added.