misc: credentials business metrics #1442

0marperez · 2024-10-16T14:34:33Z

Issue #

Description of changes

-Emitting credentials related business metrics
-Tests for credentials related business metrics

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

github-actions · 2024-10-16T14:43:14Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-10-16T16:44:23Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-10-16T17:18:08Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

lauzadis · 2024-10-17T13:07:23Z

...time/aws-config/common/test-resources/default-provider-chain/imds_assume_role/test-case.json

@@ -7,7 +7,8 @@
      "secret_access_key": "secretkeycorrect",
      "session_token": "tokencorrect",
      "expiry": 1632249686,
-      "accountId": "130633740322"
+      "accountId": "130633740322",
+      "business_metrics": ["p","0", "i"]


If this is just meant to resolve credentials from IMDS, why are there three credentials provider metrics emitted?

The test actually uses this config file. So p is for the named provider, 0 is for IMDS, and i is for the assume role call with the credentials resolved from IMDS. I think it might be a good idea to change the docs for this test because I made the same assumption when I first added the business metrics.

lauzadis · 2024-10-17T13:09:10Z

...untime/aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/profile/ProfileChain.kt

+    /**
+     * The source used to create the [RoleArn]
+     */
+    val roleArnSource: RoleArnSource? = null,


naming: source makes this more readable, roleArn.source vs. roleArn.roleArnSource

lauzadis · 2024-10-17T13:09:41Z

...config/common/test/aws/sdk/kotlin/runtime/auth/credentials/ProfileCredentialsProviderTest.kt

@@ -318,4 +326,286 @@ class ProfileCredentialsProviderTest {
        val expected = credentials("AKID-Default", "Default-Secret", accountId = "12345")
        assertEquals(expected, actual)
    }
+
+    @Test
+    fun staticCredentialsBusinessMetrics() = runTest {


naming: static -> profile

lauzadis · 2024-10-17T13:11:50Z

codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/BusinessMetricsIntegration.kt

+        override val name: String = "credentialsOverrideBusinessMetricsMiddleware"
+        override fun render(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) {
+            writer.withBlock(
+                "if (config.credentialsProvider.#T != #S ) {",


Compare classes directly rather than their simple names, if (config.credentialsProvider !is ManagedCredentialsProvider)

lauzadis · 2024-10-17T13:14:45Z

codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/BusinessMetricsIntegration.kt

+                "ManagedCredentialsProvider",
+            ) {
+                write(
+                    "op.context.#T(#T.Credentials.CREDENTIALS_CODE)",


This middleware will emit CREDENTIALS_CODE any time a custom credentials provider is configured and used. I thought that this metric referred to using the StaticCredentialsProvider.

If someone configures a custom ProfileCredentialsProvider, should we really not be emitting the CREDENTIALS_PROFILE metric?

CREDENTIALS_CODE is meant for when there's any code to setup credentials, so configuring a custom / non-default credentials provider, the StaticCredentialsProvider, session objects for boto, etc. We don't have a business metric for StaticCredentialsProvider but it falls under CREDENTIALS_CODE. We could get more specific in the future though, we would just have to discuss adding a metric for something like the StaticCredentialsProvider that all SDKs share.

CREDENTIALS_PROFILE is confusing, it's meant for static credentials in a profile. It was called something else but I think the name changed during review. Same as StaticCredentialsProvider, we don't have a metric for ProfileCredentialsProvider. The focus for the initial metrics was more on the default credentials provider chain related metrics not all credentials providers but we can go back and add more metrics soon because I think they would be very useful.

We still capture most of what's going on, e.g. If a user sets the ProfileCredentialsProvider we would see that they set a non-default credentials provider, and then we could see where in the profile the credentials are resolved from because every profile option is captured in the metrics.

CREDENTIALS_CODE is now mapped to StaticCredentialsProvider. It's not in the provider itself because it's an implementation detail for other providers.

CREDENTIALS_CODE is now mapped to StaticCredentialsProvider. It's not in the provider itself because it's an implementation detail for other providers.

I don't think I understand the point being made here. Why can't we move this specialized logic into StaticCredentialsProvider? This custom middleware and the symbol references seem way more complex than just tracking the metric in static code the same way we do for all other providers.

The issue is that it's used in the ProfileCredentialsProvider. When testing I saw the StaticCredentialsProvider business metrics being emitted when using ProfileCredentialsProvider.

lauzadis · 2024-10-17T13:17:57Z

...nfig/common/src/aws/sdk/kotlin/runtime/auth/credentials/StsWebIdentityCredentialsProvider.kt

+    /**
+     * Indicates if the class was created using [fromEnvironment]
+     */
+    private var createdFromEnvironment: Boolean = false


I understand why you needed to add this but it seems a little hacky.

We only call StsWebIdentityCredentialsProvider.fromEnvironment in one spot, the default chain, can the metrics emission for that be lifted to that point and then we can remove this createdFromEnvironment here?

I originally had it in the default credentials provider chain but it wasn't working correctly so I moved it here. The default credentials provider chain doesn't have attributes so I was creating some and then merging them with the ones from the resolve call, but something about that wasn't working correctly. I can try again for a bit and see if I can make it work.

…der is configured by user

…f StsWebIdentityCredentialsProvider

github-actions · 2024-10-17T19:16:38Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

…alsProvider

github-actions · 2024-10-28T19:55:00Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

…bility changes

github-actions · 2024-10-28T20:22:32Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-10-31T21:23:41Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

…lly for some reason 0_o

github-actions · 2024-10-31T21:29:10Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

ianbotsf · 2024-10-31T21:51:10Z

...mmon/src/aws/sdk/kotlin/runtime/http/interceptors/businessmetrics/AwsBusinessMetricsUtils.kt

+/**
+ * Emits a business metric into [Credentials.attributes]
+ * @param identifier The identifier of the [BusinessMetric] to be emitted.
+ */
+@InternalApi
+public fun Credentials.emitBusinessMetric(identifier: String): Credentials =


Nit: "Emit" feels like the wrong word here. If the credentials are never used then we won't have "emitted" a metric in the sense that it was included in the UA. Perhaps Credentials.withBusinessMetric?

Same comment would go for every other use/variation of emitBusinessMetric . It's what we've been using so far.

I agree with Ian, it's a "nit" comment but would improve readability

ianbotsf · 2024-10-31T21:56:10Z

aws-runtime/aws-config/api/aws-config.api

 public final class aws/sdk/kotlin/runtime/auth/credentials/LazilyInitializedCredentialsProvider : aws/smithy/kotlin/runtime/auth/awscredentials/CredentialsProvider {
-	public fun <init> (Ljava/lang/String;Lkotlin/jvm/functions/Function0;)V
-	public synthetic fun <init> (Ljava/lang/String;Lkotlin/jvm/functions/Function0;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public fun <init> (Ljava/lang/String;Laws/smithy/kotlin/runtime/businessmetrics/BusinessMetric;Lkotlin/jvm/functions/Function0;)V
+	public synthetic fun <init> (Ljava/lang/String;Laws/smithy/kotlin/runtime/businessmetrics/BusinessMetric;Lkotlin/jvm/functions/Function0;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
 	public fun resolve (Laws/smithy/kotlin/runtime/collections/Attributes;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
 	public fun toString ()Ljava/lang/String;
 }


Correctness: Backwards-incompatible changes

ianbotsf · 2024-10-31T21:57:38Z

...config/common/src/aws/sdk/kotlin/runtime/auth/credentials/DefaultChainCredentialsProvider.kt

-        LazilyInitializedCredentialsProvider("EnvironmentStsWebIdentityCredentialsProvider") {
+        LazilyInitializedCredentialsProvider(
+            "EnvironmentStsWebIdentityCredentialsProvider",
+            AwsBusinessMetric.Credentials.CREDENTIALS_ENV_VARS_STS_WEB_ID_TOKEN,
+        ) {


Question: Why should the LazilyInitializedCredentialsProvider care about its underlying delegate's metric? Can't StsWebIdentityCredentialsProvider record the metric itself?

That was originally what I had, you can see it here. There's some conversation you can follow

This doesn't really address my initial concern of the business metrics seeming to be hacked into the credentials provider implementation. I agree with Ian, LazilyInitializedCredentialsProvider should not have to know about business metrics

ianbotsf · 2024-10-31T22:02:33Z

aws-runtime/aws-http/api/aws-http.api

Correctness: More backward-incompatible changes.

ianbotsf · 2024-10-31T22:10:01Z

codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/BusinessMetricsIntegration.kt

+                "ManagedCredentialsProvider",
+            ) {
+                write(
+                    "op.context.#T(#T.Credentials.CREDENTIALS_CODE)",


CREDENTIALS_CODE is now mapped to StaticCredentialsProvider. It's not in the provider itself because it's an implementation detail for other providers.

I don't think I understand the point being made here. Why can't we move this specialized logic into StaticCredentialsProvider? This custom middleware and the symbol references seem way more complex than just tracking the metric in static code the same way we do for all other providers.

github-actions · 2024-10-31T23:53:26Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-11-01T01:03:02Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-11-01T01:15:13Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

github-actions · 2024-11-01T01:43:54Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

lauzadis

Overall it looks good and getting close to merge. My last concern is around LazilyInitializedCredentialsProvider being too closely coupled with business metrics

lauzadis · 2024-11-04T17:26:21Z

.../aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/ProfileCredentialsProvider.kt

@@ -86,6 +88,7 @@ public class ProfileCredentialsProvider @InternalSdkApi constructor(
    public val httpClient: HttpClientEngine? = null,
    public val configurationSource: AwsConfigurationSource? = null,
 ) : CloseableCredentialsProvider {
+    private val credentialsBusinessMetrics: MutableSet<BusinessMetric> = mutableSetOf()


correctness: MutableSet does not guarantee order of elements

A generic unordered collection of elements that does not support duplicate elements, and supports adding and removing elements.
https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/-mutable-set/

It actually does

From MutableSet:

LinkedHashSet
The implementation of the MutableSet interface, backed by a InternalMap implementation.

From LinkedHashSet:

The implementation of the MutableSet interface, backed by a InternalMap implementation.

The insertion order is preserved by the corresponding InternalMap implementation.

You can double check we're using LinkedHashSet by getting the ::class.simpleName of the business metrics

That's the current implementation, but it's not guaranteed and could change in a future version of Kotlin. It's unlikely, and I'm open to keeping this MutableSet but just wanted to call it out 🤷

lauzadis · 2024-11-04T17:27:36Z

...mmon/src/aws/sdk/kotlin/runtime/http/interceptors/businessmetrics/AwsBusinessMetricsUtils.kt

+@InternalApi
+public fun Credentials.emitBusinessMetrics(metrics: Set<BusinessMetric>): Credentials {
+    var credentials = this
+    metrics.forEach { identifier ->


nit: this is no longer an identifier but the metric itself

lauzadis · 2024-11-04T17:31:30Z

.../aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/ProfileCredentialsProvider.kt

            creds = assumeProvider.resolve(attributes)
        }

        logger.debug { "Obtained credentials from profile; expiration=${creds.expiration?.format(TimestampFormat.ISO_8601)}" }
-        return creds
+        return creds.emitBusinessMetrics(credentialsBusinessMetrics)


note: We're calling emitBusinessMetrics on a creds which might already have business metrics applied, which would result in duplication if you weren't using a MutableSet (which you might not want to be, based on my other comment above)

sonarqubecloud · 2024-11-04T19:00:01Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.6% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-11-04T19:03:39Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)

0marperez added 5 commits October 14, 2024 18:53

misc: emit credentials business metrics

faee8bb

Profile credentials business metrics tests

a50e692

Bug fixes around business metrics

8d61e07

Added business metrics to default chain credentials provider tests

18c80e7

Self review

8244764

0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Oct 16, 2024

Allow running protocol tests with snapshot smithy kotlin versions

9c3967c

Revert previous commit

92e091e

0marperez marked this pull request as ready for review October 16, 2024 17:09

0marperez requested a review from a team as a code owner October 16, 2024 17:09

lauzadis reviewed Oct 17, 2024

View reviewed changes

0marperez added 3 commits October 17, 2024 11:02

Naming changes

a4e5eca

Fix CODE_CREDENTIALS metric to only emit when static credntials provi…

1a51409

…der is configured by user

Refactor CREDENTIALS_ENV_VARS_STS_WEB_ID_TOKEN business metrics out o…

d7ba2b9

…f StsWebIdentityCredentialsProvider

0marperez added 2 commits October 24, 2024 10:57

Move credential business metric emission to credentials return time

668ed18

Remove duplicate test coverage & newline in LazilyInitializedCredenti…

1bdaa80

…alsProvider

Refactor ProfileCredentialsProvider, unused function param, and reada…

03ca91f

…bility changes

Move credentials business metrics to credentials attributes

6a7b812

0marperez force-pushed the credentials-business-metrics branch from 3f082bb to 6a7b812 Compare October 31, 2024 21:13

Add JvmName import that didn't cause build failures when missing loca…

02db28d

…lly for some reason 0_o

ianbotsf requested changes Oct 31, 2024

View reviewed changes

Remove breaking changes from smithy kotlin

b74a50b

Set<String> -> Set<BusinessMetric> business metrics

864dc65

Self review

82c7a3f

Self review & refactoring

573facd

lauzadis requested changes Nov 4, 2024

View reviewed changes

PR feedback

85b78d7

lauzadis approved these changes Nov 4, 2024

View reviewed changes

0marperez merged commit de5dc7f into credentials-chain-order Nov 4, 2024
11 checks passed

0marperez deleted the credentials-business-metrics branch November 4, 2024 20:05

misc: credentials business metrics #1442

misc: credentials business metrics #1442

Conversation

0marperez commented Oct 16, 2024

Issue #

Description of changes

github-actions bot commented Oct 16, 2024

github-actions bot commented Oct 16, 2024

github-actions bot commented Oct 16, 2024

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

0marperez Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

0marperez Oct 17, 2024 • edited Loading

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 17, 2024

github-actions bot commented Oct 28, 2024

github-actions bot commented Oct 28, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

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

Choose a reason for hiding this comment

github-actions bot commented Oct 31, 2024

github-actions bot commented Nov 1, 2024

github-actions bot commented Nov 1, 2024

github-actions bot commented Nov 1, 2024

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

sonarqubecloud bot commented Nov 4, 2024

Quality Gate passed

github-actions bot commented Nov 4, 2024

0marperez Oct 17, 2024 •

edited

Loading

0marperez Oct 17, 2024 •

edited

Loading