[extension/awsmiddleware] Add AWS middleware extension interface #116
Conversation
jefchien
force-pushed
the
middleware-ext
branch
from
e7bb46e to
a700898
Compare
jefchien
force-pushed
the
middleware-ext
branch
from
a700898 to
7de365d
Compare
| { | ||
| "Name": "metric3", | ||
| "Unit": "Seconds", | ||
| { |
There was a problem hiding this comment.
This wasn't previously formatted, which was breaking the linting validation.
| } | ||
| } | ||
| for _, handler := range mw.ResponseHandlers() { | ||
| if err := addHandlerToList(&handlers.Unmarshal, namedResponseHandler(handler), handler.Position()); err != nil { |
There was a problem hiding this comment.
Do the unmarshal/deserialize steps apply to the response only? That would make sense. The diagram from the AWS middleware documentation is confusing because it shows both the input and output go through this step, but the description says
Deserialize | Deserialize responses from the protocol format into a structured type or error.
which implies that it applies to the response only.
This does seem like it will address the problem of extracting operational data since the extension will be able to save state across the request and response handlers.
There was a problem hiding this comment.
From the unit testing, it only applies to the response.
| output, err := s3Client.ListBuckets(&s3v1.ListBucketsInput{}) | ||
| require.NoError(t, err) | ||
| assert.NotNil(t, output) | ||
| assert.GreaterOrEqual(t, recorder.Latency(), testLatency) |
There was a problem hiding this comment.
Could this potentially be flaky? Are we checking more than just that the latency is strictly positive?
There was a problem hiding this comment.
It's checking that the end time (when the handler got the response) occurred at least after the expected latency. I don't think it can be flaky because the test server always sleeps for that amount of time, which will prevent the response from happening before then.
| // RequestHandler allows for custom processing of requests. | ||
| type RequestHandler interface { | ||
| metadata | ||
| HandleRequest(r *http.Request) |
There was a problem hiding this comment.
At a high level, do you see the usage of *http.Request and *http.Response as appropriate? This was on my mind as something that might become more clear during the implementation.
These objects are general enough that they can be applied to more than just AWS, but they're also not accounting for the other possibilities with middleware like changing default request behavior in this example: https://aws.github.io/aws-sdk-go-v2/docs/middleware/#writing-a-custom-middleware.
Some alternatives are to apply middleware to any or maybe some high level AWS request/response object. Then the extension implementing this interface could have type checks to apply the specific behavior it intends. What are your thoughts after spending time on this?
There was a problem hiding this comment.
I think for our usecase the http.Request/Response are enough. I want to avoid any as much as possible. It makes it hard for others to implement the interfaces and know what to expect.
The example you linked is at the initialize step. At the build and deserialize steps, it no longer has any of the parameters or inputs that would be useful. If we're not providing access to the other steps, then I don't think there's value in making it more generic.
There was a problem hiding this comment.
Makes sense, so we still have ways of extending this in the future, if doing that would even be valuable. So we're not entering a 1-way door decision.
| } | ||
|
|
||
| // addHandlerToList adds the handler to the list based on the position. | ||
| func addHandlerToList(handlerList *request.HandlerList, handler request.NamedHandler, position HandlerPosition) error { |
There was a problem hiding this comment.
super duper mild nit - I personally dislike the [verb] to [thing] naming convention, but that is purely personal preference. It would be nice if we could just use append but that's already reserved for slices :/ oh well
jefchien
dismissed stale reviews from adam-mateen, bryce-carey, and SaxyPandaBear
via
506bd65
jefchien
merged commit 1f23619
into
amazon-contributing:aws-cwa-dev
Description: We want to have the option to customize the way AWS client requests/responses are handled for OTEL components. Both AWS SDK versions enable this, but in different ways:
To make configuring this easier and more streamlined, we're introducing an AWS middleware extension interface with functions available to configure both SDK versions. AWS components will be updated in another PR to use this configuration.
Link to tracking Issue: N/A
Testing: Added unit tests with mocks for both AWS SDK v1/v2.
Documentation: N/A