Add interceptor for collecting client metrics #4021
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Remove use of SystemTime.elapsed(), fix private type in pub docs, and style updates
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
There was a problem hiding this comment.
Good start but I have some correctness/approach concerns.
Also unrelated to metrics, (and this can be a follow on PR) we probably want to try to align some of our spans with the SRA as well before we ship all this.
e.g. the root operation span for all smithy services should include attributes rpc.service and rpc.method (AWS SDK services would also set rpc.system to aws-api which would need to happen in codegen)
| default_sleep_impl_plugin(), | ||
| default_time_source_plugin(), | ||
| default_timeout_config_plugin(), | ||
| enforce_content_length_runtime_plugin(), | ||
| default_stalled_stream_protection_config_plugin_v2(behavior_version), | ||
| metrics_runtime_plugin( |
There was a problem hiding this comment.
This isn't going to quite work the way we want.
The OperationBuilder isn't used in generated code and we apply default plugins when we create the client (see also here).
At that point we won't have the operation name.
We also want to be very careful to create the instruments and have them live on the client (i.e. we don't want them created per/operation, the attributes (dimensions) recorded with the metric will differentiate operations/services/etc). Default runtime plugins sort of gets us there (though you have to understand it's used differently by codegen than it is say by the runtime (e.g. IMDS cred provider in aws-config).
There was a problem hiding this comment.
Added this bit to codegen in d7a3c3a, but leaving this chunk here for the manually created credentials clients.
Fails for credentials clients with manually constructed operations
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Add dynamic metrics scope via codegen
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
There was a problem hiding this comment.
Thanks for the change!
Semver failure on UnwindSafe is indeed puzzling 🤔
The check seems to have been introduced almost 2.5 years ago, even before the previous version of the cargo semver version we used.
| @@ -201,6 +203,12 @@ impl<I, O, E> OperationBuilder<I, O, E> { | |||
| self | |||
| } | |||
|
|
|||
| /// Configures the metrics scope for the builder. | |||
| pub fn scope(mut self, scope: &'static str) -> Self { | |||
There was a problem hiding this comment.
Just to double check, since this is a pub API, are we sure that scope always takes &'static str but not something like Cow<'static, str>?
There was a problem hiding this comment.
I do not love the &'static str type here, but I don't think we have another option. The type is motivated by the type on ProvideMeter.get_meter() method which takes an &'static str. That was in turn motivated by the OpenTelemetry MeterProvider.meter method which also takes &'static str. So we are kind of stuck with &'static str all the way since it can't be constructed at runtime.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Motivation and Context
Description
Testing
Checklist
.changelogdirectory, specifying "client," "server," or both in theapplies_tokey..changelogdirectory, specifying "aws-sdk-rust" in theapplies_tokey.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.