feat(client): add MetricsRecorder support to client middleware#2149
feat(client): add MetricsRecorder support to client middleware#2149vismayku wants to merge 1 commit into
Conversation
| */ | ||
| export interface InvocationOptions<Native> { | ||
| recorder?: MetricsRecorder<Native>; | ||
| } |
There was a problem hiding this comment.
what else do you expect to add to InvocationOptions?
This type does not need to exist. Other comments explain why.
There was a problem hiding this comment.
I am adding this interface to basically add anything in future and we can avoid any signature changes. I'm fine switching to context for passing metrics recorder.
| /** | ||
| * Invocation-scoped options made visible to every middleware when a command is | ||
| * resolved. Carries per-call concerns supplied at the `client.send(command, options)` | ||
| * call site — currently a {@link MetricsRecorder} — so a middleware can read them |
There was a problem hiding this comment.
| * call site — currently a {@link MetricsRecorder} — so a middleware can read them | |
| * call site, currently a {@link MetricsRecorder}, so a middleware can read them |
| * | ||
| * @public | ||
| */ | ||
| export interface InvocationOptions<Native> { |
There was a problem hiding this comment.
Native is too ambiguous as the type parameter name here at the consumer site.
It's ok in the type definition of MetricsRecorder, but here it should be NativeMetrics.
There was a problem hiding this comment.
Ack. Can switch to NativeMetrics
| next: InitializeHandler<Input, Output>, | ||
| context: HandlerExecutionContext, | ||
| invocationOptions?: InvocationOptions<Native> | ||
| ): InitializeHandler<Input, Output>; |
There was a problem hiding this comment.
no change should be made to the middleware stack interface.
I will explain the alternative in other comments.
| httpLabelCommand( | ||
| args: HttpLabelCommandCommandInput, | ||
| options?: __HttpHandlerOptions | ||
| options?: __HttpHandlerOptions & __InvocationOptions<any> |
There was a problem hiding this comment.
Repeated composed types are hard to read. And this is the most user-facing part of any service client API.
Introduce a new type called RequestOptions or InvocationOptions that extends (is superset of) __HttpHandlerOptions, and generate that here instead.
also,
what was the point of the Native type parameter if clients don't infer it at the public call site?
| (request: FinalizeHandlerArguments<any>) => requestHandler.handle(request.request as HttpRequest, requestOptions), | ||
| handlerExecutionContext | ||
| handlerExecutionContext, | ||
| options |
There was a problem hiding this comment.
add the recorder into the handlerExecutionContext[SMITHY_CONTEXT_KEY] instead of creating a new object for the request lifecycle.
the handler execution context is already matched to the request lifecycle.
There was a problem hiding this comment.
Ack. Can switch to it.
| import type { | ||
| AbsoluteLocation, | ||
| DeserializeHandler, | ||
| Handler, |
There was a problem hiding this comment.
this file must not be changed for this feature
Yeah, Users basically passing the metric instance to the call. Open to suggestions if you want me to switch this as a metric middleware step while creating client. |
|
Also, to add one more thing. |
dbe258a to
95cd771
Compare
Expose a call-site MetricsRecorder to client middleware. client.send(command, { recorder }) now forwards the recorder through Command.resolveMiddleware and MiddlewareStack.resolve as an optional invocationOptions third argument, so any middleware can read it. Adds the generic InvocationOptions<Native> type to @smithy/types (consuming the merged MetricsRecorder<Native> interface). Backward compatible: the parameter is optional and existing two-argument middleware and send() calls are unaffected.
95cd771 to
8345c85
Compare
2 participants
Issue #, if available:
Description of changes:
Expose a call-site MetricsRecorder to client middleware. client.send(command, { recorder }) now forwards the recorder through Command.resolveMiddleware and MiddlewareStack.resolve as an optional invocationOptions third argument, so any middleware can read it. Adds the generic InvocationOptions type to @smithy/types (consuming the merged MetricsRecorder interface).
Backward compatible: the parameter is optional and existing two-argument middleware and send() calls are unaffected.
If one or more of the packages in the
/packagesdirectory has been modified, be sureyarn changeset addhas been run and its output hasbeen committed and included in this pull request. See CONTRIBUTING.md.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.