feat: add more user agent app id sources #1071

0marperez · 2023-10-10T18:21:35Z

Issue #

closes #945

Description of changes

There will now be more sources to set the user agent app id. Added ones include explicitly set using client configuration and from the AWS shared config active profile. The precedence order is:

client configuration (highest)
aws.userAgentAppId system property
AWS_SDK_UA_APP_ID environment variable
sdk_ua_app_id profile key (lowest)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

aajtodd · 2023-10-11T12:32:33Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/useragent/ResolveUserAgent.kt

+ */
+@InternalSdkApi
+public suspend fun resolveUserAgent(platform: PlatformProvider = PlatformProvider.System, profile: LazyAsyncValue<AwsProfile>): String? =
+    platform.getProperty(AWS_APP_ID_PROP) ?: platform.getenv(AWS_APP_ID_ENV) ?: profile.get().sdkUserAgentAppId


fix: Use EnvironmentSetting<T>.resolve()

AwsSdkSetting.AwsAppId.resolve(platform) ?: profile.get().sdkUserAgentAppId

aajtodd · 2023-10-11T12:42:05Z

aws-runtime/aws-core/common/src/aws/sdk/kotlin/runtime/client/AwsSdkClientConfig.kt

+        /**
+         * The sdk user agent app ID.
+         */
+        public var sdkUserAgentAppId: String?


fix: Lets gives some more context in the documentation on how this is used (same for the immutable val docs).

e.g.

An optional application specific identifier. When set it will be appended to the User-Agent header of every request in the form of app/{sdkUserAgentAppId}. When not explicitly set, this field will attempt to be sourced from the following locations:
* JVM System Property: aws.userAgentAppId
* Environment variable AWS_SDK_UA_APP_ID
* Shared configuration profile attribute: sdk_ua_app_id
See shared configuration settings reference for more information on environment variables and shared config settings.

Also do we want to rename this to just userAgentAppId or simply applicationId?

I think applicationId makes more sense

aajtodd · 2023-10-11T12:44:12Z

...thy-aws-kotlin-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/AwsServiceConfigIntegration.kt

+            baseClass = AwsRuntimeTypes.Core.Client.AwsSdkClientConfig
+            useNestedBuilderBaseClass()
+            documentation = """
+                The SDK user agent app ID used to identify applications.


update docs

…ation id docs

lauzadis

Looks good! I had a few nit-level comments

lauzadis · 2023-10-11T14:17:31Z

.changes/2e8e4fee-c462-45d2-b421-46520d06eb60.json

+{
+    "id": "2e8e4fee-c462-45d2-b421-46520d06eb60",
+    "type": "feature",
+    "description": "Add new sources for user agent app id (explicit, from profile)",


nit: the parenthesis is missing mention of the system property and environment variable options. I think just "Add new sources ..." is a good enough changelog description and listing each one is not necessary

nit: "user agent" -> "User-Agent"

lauzadis · 2023-10-11T14:19:30Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/useragent/ResolveUserAgent.kt

+import aws.smithy.kotlin.runtime.util.PlatformProvider
+
+/**
+ * Attempts to resolve user agent from specified sources. Returns null if none found


nit: you can also use @return for better KDoc integration

lauzadis · 2023-10-11T14:22:04Z

...runtime/aws-config/jvm/test/aws/sdk/kotlin/runtime/config/AbstractAwsSdkClientFactoryTest.kt

-        override val region: String = builder.region ?: error("region is required")
+        override val region: String? = builder.region


question: what changed here, is region no longer required?

The check wasn't required to begin with. I removed it because it was interfering with the applicationId tests. I would've had to add a region to the TestClient from the tests even if it wasn't necessary

lauzadis · 2023-10-11T14:24:44Z

...thy-aws-kotlin-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/AwsServiceConfigIntegration.kt

@@ -33,6 +33,26 @@ class AwsServiceConfigIntegration : KotlinIntegration {
            order = -100
        }

+        val userAgentAppId: ConfigProperty = ConfigProperty {


nit: name should be capitalized UserAgentAppId

lauzadis · 2023-10-11T14:26:15Z

...thy-aws-kotlin-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/AwsServiceConfigIntegration.kt

@@ -33,6 +33,26 @@ class AwsServiceConfigIntegration : KotlinIntegration {
            order = -100
        }

+        val userAgentAppId: ConfigProperty = ConfigProperty {
+            name = "applicationId"
+            symbol = KotlinTypes.String.toBuilder().nullable().build()


nit: this can be simplified to KotlinTypes.String.asNullable()

github-actions · 2023-10-11T15:06:33Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2023-10-11T16:50:17Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

ianbotsf

Minor issues. Fix and ship!

ianbotsf · 2023-10-11T17:00:48Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/AwsSdkSetting.kt

+    /**
+     * Configure the user agent app ID
+     */
+    public val AwsAppId: EnvironmentSetting<String> = strEnvSetting("aws.userAgentAppId", "AWS_SDK_UA_APP_ID")


Nit: Reuse the AWS_APP_ID_ENV and AWS_APP_ID_PROP constants from AwsUserAgentMetadata.kt.

ianbotsf · 2023-10-11T17:03:41Z

aws-runtime/aws-core/common/src/aws/sdk/kotlin/runtime/client/AwsSdkClientConfig.kt

+    /**
+     * An optional application specific identifier.
+     * When set it will be appended to the User-Agent header of every request in the form of: `app/{applicationId}`.
+     * When not explicitly set, this field will attempt to be sourced from the following locations:


Style: "this field will attempt to be sourced from" feels awkward. Perhaps "the value will be loaded from" would be clearer.

ianbotsf · 2023-10-11T17:04:49Z

aws-runtime/aws-core/common/src/aws/sdk/kotlin/runtime/client/AwsSdkClientConfig.kt

+     * - JVM System Properties: aws.userAgentAppId
+     * - Environment variables: AWS_SDK_UA_APP_ID
+     * - Shared configuration profile attribute: sdk_ua_app_id


Nit: Make "properties" and "variables" singular. Also use backticks around the property name, variable name, and config key.

sonarqubecloud · 2023-10-11T18:47:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

github-actions · 2023-10-11T18:50:50Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

0marperez added 4 commits October 10, 2023 14:07

Added new sources for user agent app id

315c35d

Changelog

464abcb

Bug fix

ee1ff25

Increase test duration limit

073cfff

0marperez marked this pull request as ready for review October 10, 2023 20:43

0marperez requested a review from a team as a code owner October 10, 2023 20:43

aajtodd suggested changes Oct 11, 2023

View reviewed changes

0marperez added 2 commits October 11, 2023 10:03

Minor fixes, rename sdkUserAgentAppId to applicationId, update applic…

ef09ee8

…ation id docs

Pull from main

28766dc

lauzadis approved these changes Oct 11, 2023

View reviewed changes

Nit fixes

e367e71

Pull from main

ec4ca48

ianbotsf approved these changes Oct 11, 2023

View reviewed changes

aajtodd approved these changes Oct 11, 2023

View reviewed changes

More nit fixes

d8532e7

0marperez merged commit 24851dc into main Oct 11, 2023
17 checks passed

0marperez deleted the user-agent-app-id-sources branch October 11, 2023 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add more user agent app id sources #1071

feat: add more user agent app id sources #1071

0marperez commented Oct 10, 2023

aajtodd Oct 11, 2023

aajtodd Oct 11, 2023

aajtodd Oct 11, 2023

0marperez Oct 11, 2023

aajtodd Oct 11, 2023

lauzadis left a comment

lauzadis Oct 11, 2023

lauzadis Oct 11, 2023

lauzadis Oct 11, 2023

lauzadis Oct 11, 2023

0marperez Oct 11, 2023

lauzadis Oct 11, 2023

lauzadis Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

ianbotsf left a comment

ianbotsf Oct 11, 2023

ianbotsf Oct 11, 2023

ianbotsf Oct 11, 2023

sonarqubecloud bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

		override val region: String = builder.region ?: error("region is required")
		override val region: String? = builder.region

feat: add more user agent app id sources #1071

feat: add more user agent app id sources #1071

Conversation

0marperez commented Oct 10, 2023

Issue #

Description of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauzadis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

ianbotsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023