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

Adds the user agent appId metadata tag #5636

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cenedhryn
Copy link
Contributor

@cenedhryn cenedhryn commented Oct 1, 2024

Motivation and Context

AWS SDKs have added the appId metadata tag which allows users to set an identifier for their application that is added to the user agent string for every request. This PR adds the feature to the AWS SDK for Java v2.

Modifications

  • Adds appId as a client param in SdkDefaultClientBuilder
  • Adds options for configuration appId in environment variables, system settings and config files, and a resolver for determining if any such system value exists.

Testing

  • Unit testing for the resolver
  • e2e functional testing for the overall functionality.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@cenedhryn cenedhryn requested a review from a team as a code owner October 1, 2024 18:40
@cenedhryn cenedhryn changed the title Adds the user agent appId metadata tag to client and system configura… Adds the user agent appId metadata tag Oct 1, 2024
@davidh44 davidh44 enabled auto-merge October 3, 2024 20:59
Copy link

sonarqubecloud bot commented Oct 4, 2024

@davidh44 davidh44 added this pull request to the merge queue Oct 4, 2024
@zoewangg zoewangg removed this pull request from the merge queue due to a manual request Oct 4, 2024
* This configuration option supersedes {@link SdkAdvancedClientOption#USER_AGENT_PREFIX} and
* {@link SdkAdvancedClientOption#USER_AGENT_SUFFIX} and should be used instead of those options.
*/
default B appId(String appId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add this to ClientOverrideConfiguration instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Comment on lines +30 to +31
private Supplier<ProfileFile> profileFile;
private String profileName;
Copy link
Contributor

Choose a reason for hiding this comment

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

final?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they're not final

@@ -124,4 +134,12 @@ private static void appendAdditionalJvmMetadata(StringBuilder builder, SystemUse
appendNonEmptyField(builder, METADATA, lang);
}
}

private static void checkLengthAndWarn(String appId) {
if (appId.length() > 50) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double check, is this only logged once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's only logged once.

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