-
Notifications
You must be signed in to change notification settings - Fork 227
RFC: Server Request Metrics #4458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
rcoh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall seems good.
The RFC could definitely be fleshed out a bit more — I don't totally get how it will work with operation specific metrics etc.
| #[smithy_metrics(extension)] | ||
| #[metrics(flatten)] | ||
| operation_metrics: OperationMetrics, | ||
| custom_metric: Option<String> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a custom request level metric right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
|
|
||
| The focal struct that users can add to their service as a tower `Layer` for metrics, containing `new` and `builder` methods to get a `MetricsLayer` with the default configuration or a `MetricsLayerBuilder`, respectively. Because the `MetricsLayer` is specific to the struct provided in the type parameter, the `MetricsLayerBuilder` will be a product of the `#[smithy_metrics]` proc macro expansion, responsible for providing methods to customize things like the metrics initialization and setting custom request/response metrics. Contrarily, the `MetricsLayerConfig` along with its builder will be explicitly defined for general configuration not bound to any specific type parameter, such as enabling/disabling default metrics. | ||
|
|
||
| Contains a generic type parameter bound by a marker trait with a default of `DefaultMetrics`, which will be a type defined in the library that uses the `#[smithy_metrics]` expansion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fleshed out this section in the latest commits
|
|
||
| - `MetricsLayer::new()` for the default metrics and extensions | ||
|
|
||
| - `MetricsLayer::<CustomMetrics>::new()` for the default metrics with potential renaming or additional extensions from attribute proc macros |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is custom metrics here?
|
|
||
| A proc macro that can be placed on a metrique metrics struct for purposes such as the addition of default request/response metrics fields, the implementation of a marker trait, and the expansion of a `MetricsLayerBuilder` with concrete type being the annotated struct. | ||
|
|
||
| This will also come with `#[smithy_metrics(rename(x = "y"))]` to rename default fields and `#[smithy_metrics(extension)]` to mark struct fields for insertion to the request extensions to be used in custom middleware or operation handlers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would you use this rename vs metrique rename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rename is for renaming of the out-of-the-box metrics that will get added to a metrics struct as part of the proc macro expansion (e.g. request_id, service_name, operation_name, etc). Since those fields won't be visible, they'd need to be applied at the outer level and then the expansion will apply the actual metrics rename.
| .build(); | ||
| } | ||
|
|
||
| #[smithy_metrics] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably nice to have something like #[smithy_metrics::root], #[smithy_metrics:: operation]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I currently have it like #[smithy_metrics] and #[smithy_metrics(extension)], I can create a bikeshedding thread at the end to flesh these out.
|
|
||
| #[metrics] | ||
| struct OperationMetrics { | ||
| get_pokemon_species_metrics: Option<String> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need to manually set this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is an example of a user-defined metrics struct for control of metrics in their operation handlers
|
This seems specific to metrique, was there not consideration given to our own runtime observability API: https://github.com/smithy-lang/smithy-rs/tree/main/rust-runtime/aws-smithy-observability |
5ebaf8b to
860daae
Compare
|
Added some more content to the RFC explaining some of the open questions, and added a preliminary implementation of the metrics layer interface. Yes the design only has metrique in mind. Maybe future scope can extend to aws-smithy-observability as well. |
|
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. |
0dde16e to
0b97c30
Compare
0b97c30 to
5654ba7
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
3c5802d to
c506136
Compare
|
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. |
ba8bcac to
a6c26fb
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
b6a1b9b to
1fdb9fe
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Motivation and Context
Description
See rendered preview
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.