Skip to content

Conversation

@alextwoods
Copy link
Contributor

@alextwoods alextwoods commented Feb 25, 2025

Description of changes:
Add UserAgent header to requests in generated clients using runtime plugins with separate generic and AWS specific logic.

This change adds a generic (non-aws) plugin that adds a base UserAgent object to the interceptor context that allows other interceptors to add data to the user agent. This is required because the user agent string MUST follow a defined order so we cannot simply append values to the header. The generic interceptor also adds a modify_before_sign interceptor which sets the user agent header from the UserAgent object on the context.

The AWS specific plugin must be generated per service client to give us access to the service library's version and the service ID. The interceptor just adds data to the UserAgent context object.

The UserAgent class logic is largely borrowed from botocore's UserAgent.

The advantages of this approach:

  • UserAgent header MUST follow a defined order. Adding a 'user-agent' object on context allows different interceptors to easily add data to the user agent without breaking the order or having to parse/rebuild the string.
  • This provides easy implementation of business metrics down the road.
  • Uses existing extension points (interceptor/plugin) to modify requests rather than adding additional logic to the core execution pipeline.

Cons/Issues:

  • We need access to certain config values during the request. The interceptor context does not give access to config. We can access a version of config when the plugin is called, but other plugins (including operation level plugins) could modify it after.

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
Contributor

@JordonPhillips JordonPhillips left a comment

Choose a reason for hiding this comment

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

You should run pyupgrade --py312-plus on all this, it'll resolve most of the typing and syntax issues I've called out.

As far as this being in AWS, I don't agree. I think almost all of this can and should be generic. The only real exception would be the app id and execution env, but those could be appended in a separate interceptor

@alextwoods alextwoods marked this pull request as ready for review February 27, 2025 20:09
@alextwoods alextwoods requested a review from a team as a code owner February 27, 2025 20:09
@alextwoods alextwoods changed the title [DRAFT] User agent User agent Feb 27, 2025
Comment on lines 78 to 93
writer.write("""
def aws_user_agent_plugin(config: $1T):
config.interceptors.append(
$2T(
ua_suffix=config.user_agent_extra,
ua_app_id=config.sdk_ua_app_id,
sdk_version=$3T,
service_id='$4L'
)
)
""",
CodegenUtils.getConfigSymbol(c.settings()),
userAgentInterceptor,
versionSymbol,
c.settings().service().getName()
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you move this into Java?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To get the client library version and service id - since both of those are specific to the generated client.

The alternative would be to add that information to the interceptor context (or potentially to use some python metaprogramming hacks?).

Do you have thoughts or preferences between those? (or maybe another obvious way I'm missing to get client library version and service id)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah the service id isn't available right now because we're not writing out service schemas, and thus those traits aren't present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think even with service schemas we'd still need some way for a generic (ie, non codegen) interceptor or plugin to access it. Though there are a few ways we could do that - such as adding it to the interceptor context or the config? Theres probably other pythonic metaprogramming tricks as well (get the caller's namespace and search it?)

Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

I think this is probably my last round of comments, otherwise things look good!

@nateprewitt
Copy link
Contributor

Also looks like the test_environment tests may not have everything properly mocked:

AssertionError: assert 'md/arch#arm64' in 'python/1.2.3 ua/2.1 os/linux#5.4.228-131.415.AMZN2.X86_64 md/arch#x86_64 lang/python#4.3.2 md/pyimpl#Cpython'

It looks like you tested arm64 locally, but GHA is running on x86_64.

alextwoods and others added 3 commits March 3, 2025 12:18
Co-authored-by: Nate Prewitt <nate.prewitt@gmail.com>
Co-authored-by: Nate Prewitt <nate.prewitt@gmail.com>
nateprewitt
nateprewitt previously approved these changes Mar 4, 2025
JordonPhillips
JordonPhillips previously approved these changes Mar 5, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatter has truly failed here... oh well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I had a bit of a fight with it trying to make this a little less garbage. Made it slightly better, but still don't love it.

writer.write("""
@dataclass(init=False)
class $L:
class $T:
Copy link
Contributor

Choose a reason for hiding this comment

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

T is intended for references, not declarations. Why was this change made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to $L for the decleration - Had done it without thinking since its using the symbol as an argument.

/**
* Creates top level __init__.py file.
*/
private void generateServiceModuleInit(CustomizeDirective<GenerationContext, PythonSettings> directive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be done directly in generateInits. I'm refactoring this in a separate PR though so it's fine to have it here for now.

@alextwoods alextwoods dismissed stale reviews from JordonPhillips and nateprewitt via 20b97ed March 5, 2025 17:18
@alextwoods alextwoods merged commit f4b87b9 into smithy-lang:develop Mar 5, 2025
1 check passed
@alextwoods alextwoods deleted the user_agent branch March 5, 2025 19:18
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.

4 participants