Repository navigation
Add feature usage tracking to the User-Agent header - #882
Conversation
| from smithy_core.aio.retries import ( | ||
| RETRY_STRATEGY, | ||
| AsyncAdaptiveRetryStrategy, | ||
| AsyncStandardRetryStrategy, | ||
| ) |
There was a problem hiding this comment.
we should bump smithy-aws-core's smithy-core dependency to the release that introduces RETRY_STRATEGY. It still allows 0.9.x, so a valid installation can fail while importing this module
There was a problem hiding this comment.
We generally should not manually bump our dependency pins since this is handled automatically by our release infra. For something like this that is required by a dependent, a minor version should be triggered by either a breaking or feature changelog entry. Long term we can make this more fine-grain (this is probably more like an enhancement change, but that triggers a patch bump right now. We can probably add a new field in the future to override the bump type since not every feature is a minor bump and not every enhancement is a patch bump).
If this is merged before out next release, nothing should be needed since there already pending breaking changes and features for smithy-core. But adding a feature entry might be safer incase this is released after
| "smithy_aws_core.aio.endpoints.standard_regional": { | ||
| "AsyncEndpointResolver": "rex-protocol[async]", | ||
| "AsyncStandardRegionalEndpointsResolver": "class[async]", | ||
| "AwsFeatureId": "rex-class", |
There was a problem hiding this comment.
can we keep these implementation imports private? The snapshot is picking up a bunch of new exports from user_agent, standard_regional, and aio.client that don’t seem intentional
There was a problem hiding this comment.
rex (re-exports) moving around should be ok. They are tracked in the API snapshot but I think customers understand not to import things that aren't part of a module's __ALL__ official exports.
jonathan343
left a comment
jonathan343
left a comment
There was a problem hiding this comment.
Thanks @ubaskota! Overall this PR looks good. I have a few questions on things I'm confused about.
| encoded = "" | ||
| for feature_id in feature_ids: | ||
| candidate = f"{encoded},{feature_id.value}" if encoded else feature_id.value | ||
| if len(candidate.encode("utf-8")) > MAX_ENCODED_FEATURE_IDS_BYTES: |
There was a problem hiding this comment.
Question: Why is .encode("utf-8") needed here?
There was a problem hiding this comment.
This measures the length of the encoded size. We do sth similar in botocore as well. Metric values are ASCII, so in practice it's the same as len(candidate). Happy to simplify if you prefer.
| # fallback, for example with a signer that doesn't copy them, if a retry | ||
| # still has the header written here, start from its value before m/ was | ||
| # added, so m/ is never added twice and nothing else is removed. | ||
| previous = context.properties.get(_WRITTEN_USER_AGENT) |
There was a problem hiding this comment.
I'm a bit confused by the usage of _WRITTEN_USER_AGENT. Can you explain what case it's protecting against?
There was a problem hiding this comment.
I initially thought the business metrics could get duplicated when a request is retried, so added this check to prevent that. But after further investigation, it looks like the duplication doesn't happen. The SigV4 signer works on a copy of the request's headers, and the generic interceptor rewrites User-Agent on every attempt, so each retry starts without m/. I've removed the check and updated the tests.
Issue #, if available:
Description of changes:
Add a way to record which SDK features a call uses and report them in the User-Agent header's
m/section. Features are tracked per call withregister_feature_idand theAwsFeatureIdenum, and the AWS user agent interceptor appends them to the header after signing, without adding them twice on retries. This PR tracks features for retry mode and theendpoint override.
Testing:
Added unit tests for registration, encoding and the header update, including retries, and integration tests with a generated client. Verified that all existing and new tests pass.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.