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. |
| @@ -53,3 +53,6 @@ pub mod sdk_feature; | |||
|
|
|||
| /// Smithy support-code for code generated waiters. | |||
| pub mod waiters; | |||
|
|
|||
| /// Interceptor for collecting client metrics. | |||
| pub mod metrics; | |||
There was a problem hiding this comment.
Does the metrics module need to be pub (maybe not in this PR but in the future)?
There was a problem hiding this comment.
What's the rationale on why it's public?
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)
| @@ -53,3 +53,6 @@ pub mod sdk_feature; | |||
|
|
|||
| /// Smithy support-code for code generated waiters. | |||
| pub mod waiters; | |||
|
|
|||
| /// Interceptor for collecting client metrics. | |||
| pub mod metrics; | |||
There was a problem hiding this comment.
What's the rationale on why it's public?
| /// Struct to hold metric data in the ConfigBag | ||
| #[derive(Debug, Clone)] | ||
| struct MeasurementsContainer { | ||
| call_start: SystemTime, |
There was a problem hiding this comment.
should probably use a monotonic clock (i.e. Instant)
There was a problem hiding this comment.
Ahh I see SharedTimeSource returns SystemTime...that is unfortunate. 🤔
| @@ -275,20 +300,24 @@ pub fn default_plugins( | |||
| let behavior_version = params | |||
| .behavior_version | |||
| .unwrap_or_else(BehaviorVersion::latest); | |||
| let service_name = params | |||
There was a problem hiding this comment.
Assuming this is the service name is probably not what we want even though it's what the default is and likely to work (for now anyway).
| 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).
| pub(crate) fn new() -> Result<Self, ObservabilityError> { | ||
| let meter = get_telemetry_provider()? | ||
| .meter_provider() | ||
| .get_meter("MetricsInterceptor", None); |
There was a problem hiding this comment.
This scope is wrong, it should be something like the service name
e.g. in Kotlin when the client is instantiated:
private val telemetryScope = "aws.sdk.kotlin.services.s3"
private val opMetrics = OperationMetrics(telemetryScope, config.telemetryProvider)SRA doesn't talk about this too much but we should probably pick a scope that identifies it's the Rust SDK + the service so aws_sdk_s3::Client or something, we can discuss what makes sense though.
|
|
||
| impl MetricsInterceptor { | ||
| pub(crate) fn new( | ||
| service_name: impl Into<Cow<'static, str>>, |
There was a problem hiding this comment.
Perhaps we should just make service and operation name always available to the interceptor context so that any interceptor can pull them out.
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.