chore: middleware poc #471
Conversation
pgautier404
force-pushed
the
middleware-poc
branch
from
5f9a766 to
6bf8d00
Compare
| mw.requestChan <- fmt.Sprintf("%T", theRequest) | ||
| } | ||
|
|
||
| func (mw *metricsMiddleware) OnResponse(requestId uint64, _ map[string]string) {} |
There was a problem hiding this comment.
out of curiosity, if a user wanted to write middleware that only handled one type of request/response (so, some kind of switch or if statement that would ignore the other request/response types), what would that look like?
Also, what if I wanted to do a timer in a middleware? I would need a way to know which request a response matches up with, right?
There was a problem hiding this comment.
Regarding the timer, that's what the requestId is used for. I've added an example using a custom timing middleware here.
There was a problem hiding this comment.
Regarding the filtering of requests and responses, you can use type assertions in the case of requests. Responses are currently sending type data as a string, but I'm realizing that whole approach needs to be tweaked and we'll be able to use type assertions on those as well. I just updated the middleware example I linked above to include a sample of the filtering.
There was a problem hiding this comment.
re: type filtering - :ack:, and we have the thread below where we can bottom out on that.
re: requestId - that makes sense and is probably a reasonable way to go. Just to throw out another alternative to consider - if you look at how we did this in the JS repo, what we did is create two interfaces, Middleware and MiddlewareRequestHandler. On each request, the Middleware is responsible for instantiating a new instance of the MiddlewareRequestHandler, which is then used to handle just that one single request.
That approach allows callers to create local variables in the MiddlewareRequestHandler, and their scope will be limited to just that one request. So you could just create a simple timer object or start timestamp, and not have to worry about matching it back up with a request id later.
I presume that with your current approach a user would need to maintain a map in order to track multiple requests, and clean up the map when appropriate to prevent it from leaking memory. But I haven't looked at your impl yet, will go check it out.
config/middleware/middleware.go
Outdated
|
|
||
| type Middleware interface { | ||
| OnRequest(requestId uint64, theRequest interface{}, metadata context.Context) | ||
| OnResponse(requestId uint64, theResponse map[string]string) |
There was a problem hiding this comment.
just curious why the response ends up being a map? As opposed to us using the actual response types here (or a struct that contains the actual response plus some additional metadata fields?
There was a problem hiding this comment.
This is a map because I'm coercing the relevant response data into a map in each requester's getResponse() method. There is no supertype for responses (apart from interface{}, which I was trying to avoid) I can use for this signature or in a struct containing the response, so extracting any data from a given response requires a type assertion. Looking at it more closely I think you're totally right about needing to include the response itself. I'm going to rework it using your latter suggestion.
There was a problem hiding this comment.
Ok, I went with just returning the response as an interface{} that can be coerced to the appropriate type as needed and that can report its underlying type for reporting. If we need to add extra metadata we can pretty easily extend it, but I'm starting simple with just the raw response.
There was a problem hiding this comment.
how would you extend it? if one of the goals of changing it to interface{} is to allow users to do a type assertion/cast on the actual response type, and then we decided we wanted to add some metadata fields, how would we add them?
I kind of suspect that the best path here is going to be to create a MiddlewareRequest struct, with a property called .request of type interface{}, and then the same thing with the MiddlewareResponse. (you don't need to change this now, just want to drive towards consensus in case we decide to take this acrossthe finish line.)
There was a problem hiding this comment.
That makes sense. I was thinking of adding a metadata map to the OnResponse args, but until/unless we actually use it it would just present an annoying requirement to pass in an empty map every time. Adding it to a struct makes more sense for sure.
| @@ -77,6 +92,10 @@ func (client scsDataClient) makeRequest(ctx context.Context, r requester) error | |||
| return err | |||
| } | |||
|
|
|||
| for _, mw := range client.middleware { | |||
| mw.OnResponse(requestId, r.getResponse()) | |||
There was a problem hiding this comment.
nice, i like that in this SDK we have one central spot to plumb this through.
There was a problem hiding this comment.
Agreed, but be aware that the data client is the only one set up this way. Adding middleware to the other clients will involve some pretty intensive refactoring to bring them in line.
No description provided.