Skip to content
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

Allow User Agent to accept business metrics #3781

Merged
merged 7 commits into from
Aug 6, 2024

Conversation

ysaito1001
Copy link
Contributor

@ysaito1001 ysaito1001 commented Aug 2, 2024

Motivation and Context

This PR lays the groundwork for emitting and tracking business metrics related to features like waiters, request compression, and S3 express within the Rust SDKs. It introduces BusinessMetric to the user_agent module and updates AwsUserAgent to support its integration.

Description

While establishing the groundwork for the said motivation, this PR does not send metrics from features yet.
The PR deprecates existing feature and config metadata, which are now superseded by the new business metrics.

Related tasks include

  • implementing versioning for the user agent string format, e.g. ua/2.0 (note that including a user agent version string to x-amz-user-agent could cause many connection recording tests to fail and may also break semver check in CI just like the content-length enforcement test needed to be disabled to work around it)
  • sending metrics from recently implement features, maybe starting with request compression and RPC V2 Cbor (this will also modify x-amz-user-agent in connection recording tests, so the argument above applies)

Testing

  • Added unit tests for BusinessMetric and AwsUserAgent
  • Existing tests in CI

Checklist

  • For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "aws-sdk-rust" in the applies_to key.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

github-actions bot commented Aug 2, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

@ysaito1001 ysaito1001 marked this pull request as ready for review August 2, 2024 18:30
@ysaito1001 ysaito1001 requested review from a team as code owners August 2, 2024 18:30
Copy link

github-actions bot commented Aug 2, 2024

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Contributor

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, couple questions.

could cause many connection recording tests to fail and may also break semver check in CI just like the #3620 needed to be disabled to work around it)

  1. What's our strategy here for predictable UA strings in tests?

  2. Features and metrics are only really known at runtime, how is the UA string manipulated dynamically? e.g. how would an interceptor or other feature set metrics? It might be helpful to think through for all of the metrics we currently could support to see how they would be able to do this.

@ysaito1001
Copy link
Contributor Author

Overall looks good, couple questions.

could cause many connection recording tests to fail and may also break semver check in CI just like the #3620 needed to be disabled to work around it)

1. What's our strategy here for predictable UA strings in tests?

Good question. As we briefly discussed offline, it's likely we add a new tests verification utility to not compare actual vs. expected for user-agent strings for all of the connection recording tests (maybe do so only for Kotlin integ tests that do not have AWS codegen decorators).

2. Features and metrics are only really known at runtime, how is the UA string manipulated dynamically? e.g. how would an interceptor or other feature set metrics? It might be helpful to think through for all of the metrics we currently could support to see how they would be able to do this.

An interceptor that's relevant to the feature in question most likely extracts AwsUserAgent out of the config bag and calls .add_business_metric on it. AwsUserAgent is currently constructed by default in the modify_before_signing method, but if other interceptors need to call a method on it, we may need to move the default-construction of AwsUserAgent to another method that's executed earlier than modify_before_signing during the execution in the orchestrator.

@ysaito1001 ysaito1001 added this pull request to the merge queue Aug 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 6, 2024
@ysaito1001 ysaito1001 added this pull request to the merge queue Aug 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 6, 2024
@ysaito1001 ysaito1001 enabled auto-merge August 6, 2024 02:15
Copy link

github-actions bot commented Aug 6, 2024

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@ysaito1001 ysaito1001 added this pull request to the merge queue Aug 6, 2024
Merged via the queue into main with commit c0f8173 Aug 6, 2024
44 checks passed
@ysaito1001 ysaito1001 deleted the ysaito/ua-to-support-business-metrics branch August 6, 2024 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants