fix: Move HeaderInterceptor to use our Middleware API #281
Conversation
cprice404
force-pushed
the
migrate-header-interceptor-to-middleware
branch
from
96f6df7
to
963378f
Compare
cprice404
changed the base branch from
main
to
handle-continuation-exception-types
cprice404
force-pushed
the
migrate-header-interceptor-to-middleware
branch
2 times, most recently
from
c215338
to
3d5ef7c
Compare
| public ILoggerFactory? LoggerFactory { get; } | ||
|
|
||
| public IMiddleware WithLoggerFactory(ILoggerFactory loggerFactory); |
There was a problem hiding this comment.
Why do these live on IMiddleware instead of being concerns of the implementation? Conceptually, a LoggerFactory getter seems orthogonal to the job of an IMiddleware, as advertised by its interface.
There was a problem hiding this comment.
The Configuration object is where a user defines their logging configuration.
The user may also pass in a list of Middlewares and I don't want or need them to have to also pass in the logging config to each and every one of those Middlewares.
Therefore the Configuration object needs to be able to loop over the middlewares and configure their logging, if the user didn't explicitly add a logging config to the middleware.
Therefore the IMiddleware interface must expose a way for the Configuration to get/set their logging configuration.
There was a problem hiding this comment.
The Configuration object is where a user defines their logging configuration.
Sure, so shouldn't middleware implementations which consume Configuration either take in Configuration or their relevant explicit Configuration dependencies in their constructors?
if the user didn't add logging, then why does their middleware need logging configuration? For middleware-framework logging it seems like you'd want a middleware-framework logger instance, no? And for middleware-implementation logging, if you don't log then why should there be an interface dependency?
I think this pattern (migrating from ILoggerFactory to ILoggerFactory?) is mixing setter-injection and constructor-injection. It's unclear to me how setter-injection is made appropriate or necessary by this pr. Maybe we should chat or zoom separately
There was a problem hiding this comment.
Discussed this with Kenny offline. I agree there's some code smell on this and we talked through a few ideas to experiment with. The scope of those changes is bigger than appropriate for this PR so I am going to try to find some time to look at that separately later today.
| return await wrapped.ResponseAsync; | ||
| } | ||
|
|
||
| private async Task<MiddlewareResponseState<TResponse>> WrapWithMiddleware<TRequest, TResponse>( |
There was a problem hiding this comment.
This function looks completely independent from this class other than this._middlewares. Is it copypasted from somewhere else? Would you like to instead move this function to be an extension on IList<IMiddleware>?
edit: yup, appears to be copypasted from DataGrpcManager. Can you DRY instead?
There was a problem hiding this comment.
Addressed in 1899667
Prior to this commit, we had a gRPC Interceptor that was responsible for setting headers (cache name, auth token, etc.) that were sent to the server. We attempted to implement our Middleware layer using gRPC interceptors but because their API is not asynchronous, there was no way to accomplish the asynchronous tasks that we needed to accomplish, so we had to introduce a bespoke layer for our Middleware that uses the same patterns and some of the same constructs as the gRPC Interceptors do, but which supported an asynchronous API. This left us with a mix of both Interceptors (for the Header Interceptor) and Middlewares. This proved to be problematic when attempting to implement request retries, because we could not control the order that the interceptor ran in relative to the middlewares. Thus whenever we were executing our retry logic, the header interceptor would be re-run and it would add a second copy of all of the headers, which made the request invalid. It's important that we be able to exercise full control over the order that all of the middlewares are executed, so in this PR we migrate the Header Interceptor over to be a Middleware instead. This allows us to position it in the correct spot in the ordered list of Middlewares. Because the control plane client was also using the Headers interceptor, this commit also adds Middleware support for the control plane client and migrates it from the old Headers Interceptor to the new Headers Middleware.
cprice404
force-pushed
the
migrate-header-interceptor-to-middleware
branch
from
3d5ef7c
to
3e10ed2
Compare
|
rebased, now will add a commit to address Kenny's DRY comment. |
cprice404
force-pushed
the
migrate-header-interceptor-to-middleware
branch
from
a88d690
to
1899667
Compare
Prior to this commit, we had a gRPC Interceptor that was
responsible for setting headers (cache name, auth token, etc.)
that were sent to the server.
We attempted to implement our Middleware layer using gRPC interceptors
but because their API is not asynchronous, there was no way to
accomplish the asynchronous tasks that we needed to accomplish,
so we had to introduce a bespoke layer for our Middleware that uses
the same patterns and some of the same constructs as the gRPC
Interceptors do, but which supported an asynchronous API.
This left us with a mix of both Interceptors (for the Header Interceptor)
and Middlewares. This proved to be problematic when attempting to
implement request retries, because we could not control the order that
the interceptor ran in relative to the middlewares. Thus whenever
we were executing our retry logic, the header interceptor would be
re-run and it would add a second copy of all of the headers, which
made the request invalid.
It's important that we be able to exercise full control over the
order that all of the middlewares are executed, so in this PR
we migrate the Header Interceptor over to be a Middleware instead.
This allows us to position it in the correct spot in the ordered
list of Middlewares.
Because the control plane client was also using the Headers interceptor,
this commit also adds Middleware support for the control plane client
and migrates it from the old Headers Interceptor to the new Headers
Middleware.