misc: credentials business metrics #1164

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

Issue #

Description of changes

-Emits an Identity's attributes BusinessMetrics into an ExecutionContext during SDK call
-Business metrics utils

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

…s business metrics

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

...col/http-client/common/src/aws/smithy/kotlin/runtime/http/operation/SdkOperationExecution.kt

+    if (identityAttributes.contains(BusinessMetrics)) {
+        identityAttributes[BusinessMetrics]
+            .toList()
+            .reversed()
+            .forEach { metric ->
+                context.emitBusinessMetric(metric)
+            }
+    }


Question: Why is reversing the order significant? Could this just be:

identity.attributes.getOrNull(BusinessMetrics)?.forEach(context::emitBusinessMetric)

This new approach of tracking credentials BM in credentials attributes reverses the order in which credentials BM are recorded. The profile credentials provider starts from the latest node, you can see it's reversed before credentials start being resolved.

ianbotsf · 2024-10-31T21:43:24Z

runtime/runtime-core/api/runtime-core.api

 public final class aws/smithy/kotlin/runtime/businessmetrics/BusinessMetricsUtilsKt {
-	public static final fun containsBusinessMetric (Laws/smithy/kotlin/runtime/operation/ExecutionContext;Laws/smithy/kotlin/runtime/businessmetrics/BusinessMetric;)Z
-	public static final fun emitBusinessMetric (Laws/smithy/kotlin/runtime/operation/ExecutionContext;Laws/smithy/kotlin/runtime/businessmetrics/BusinessMetric;)V
+	public static final fun containsBusinessMetric (Laws/smithy/kotlin/runtime/collections/Attributes;Laws/smithy/kotlin/runtime/businessmetrics/BusinessMetric;)Z
+	public static final fun emitBusinessMetric (Laws/smithy/kotlin/runtime/collections/MutableAttributes;Laws/smithy/kotlin/runtime/businessmetrics/BusinessMetric;)V
+	public static final fun emitBusinessMetric (Laws/smithy/kotlin/runtime/collections/MutableAttributes;Ljava/lang/String;)V
+	public static final fun emitBusinessMetrics (Laws/smithy/kotlin/runtime/collections/MutableAttributes;Ljava/util/Set;)V
 	public static final fun getAccountIdBasedEndpointAccountId ()Laws/smithy/kotlin/runtime/collections/AttributeKey;
 	public static final fun getBusinessMetrics ()Laws/smithy/kotlin/runtime/collections/AttributeKey;
 	public static final fun getServiceEndpointOverride ()Laws/smithy/kotlin/runtime/collections/AttributeKey;
-	public static final fun removeBusinessMetric (Laws/smithy/kotlin/runtime/operation/ExecutionContext;Laws/smithy/kotlin/runtime/businessmetrics/BusinessMetric;)V
+	public static final fun removeBusinessMetric (Laws/smithy/kotlin/runtime/collections/MutableAttributes;Laws/smithy/kotlin/runtime/businessmetrics/BusinessMetric;)V
 }


Correctness: These are backwards-incompatible changes. We should not change the signatures of existing methods. If we need new capabilities (I'm not yet convinced we do in this case) we should add new methods/overloads.

You are correct here, I'll fix the breaking changes.

ianbotsf · 2024-10-31T21:46:54Z

...me/runtime-core/common/src/aws/smithy/kotlin/runtime/businessmetrics/BusinessMetricsUtils.kt

 @InternalApi
-public fun ExecutionContext.emitBusinessMetric(metric: BusinessMetric) {
-    if (this.attributes.contains(BusinessMetrics)) {
-        this.attributes[BusinessMetrics].add(metric.identifier)
+public fun MutableAttributes.emitBusinessMetric(identifier: String) {
+    if (this.contains(BusinessMetrics)) {
+        this[BusinessMetrics].add(identifier)
    } else {
-        this.attributes[BusinessMetrics] = mutableSetOf(metric.identifier)
+        this[BusinessMetrics] = mutableSetOf(identifier)
    }
 }


Question: Why would we want to track metrics as pure strings? We already have well-named constants for BusinessMetric instances. It seems like this could only increase the likelihood of emitting an invalid metric identifier.

We need to save the state of credentials business metrics here. Once the metrics are in the business metrics attribute the constants information is gone and all that is left is the identifier.

In hindsight it would've been useful to have the business metrics attribute be a set of business metrics not of set of string and convert them to strings (using their identifiers) before adding them to the UA header.

0marperez · 2024-11-01T01:35:06Z

Reminder for myself: DO NOT RELEASE this until you get changes tracking the business metrics attribute from Set<String> -> Set<BusinessMetric> into aws-sdk-kotlin main❗️

lauzadis · 2024-11-04T16:36:02Z

...col/http-client/common/src/aws/smithy/kotlin/runtime/http/operation/SdkOperationExecution.kt

+ * Emits an [Identity]'s attributes' [BusinessMetrics] into the [ExecutionContext]
+ */
+private fun emitIdentityBusinessMetrics(identity: Identity, context: ExecutionContext) =
+    identity.attributes.getOrNull(BusinessMetrics)?.toList()?.reversed()?.forEach(context::emitBusinessMetric)


Why .reversed(), does order matter?

Yes, according to the design doc.

The feature IDs MUST be included in the business metrics in the same order they are used.

lauzadis · 2024-11-04T16:36:38Z

...me/runtime-core/common/src/aws/smithy/kotlin/runtime/businessmetrics/BusinessMetricsUtils.kt

 import aws.smithy.kotlin.runtime.collections.get
 import aws.smithy.kotlin.runtime.operation.ExecutionContext

 /**
 * Keeps track of all business metrics along an operations execution
 */
 @InternalApi
-public val BusinessMetrics: AttributeKey<MutableSet<String>> = AttributeKey("aws.sdk.kotlin#BusinessMetrics")
+public val BusinessMetrics: AttributeKey<MutableSet<BusinessMetric>> = AttributeKey("aws.sdk.kotlin#BusinessMetrics")


correctness: This is not a part of your PR but an earlier miss, we should not be referencing aws.sdk.kotlin in smithy-kotlin if possible

Good catch, I'll change this to aws.smithy.kotlin#BusinessMetrics

0marperez added 3 commits October 14, 2024 18:03

misc: reset attributes between provider chain attempts

f32de91

misc: add function to emit business metrics with attributes as receiver

980abe6

Bug fixes & refactoring of new business metrics utils

169c96a

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

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 approved these changes Oct 16, 2024

View reviewed changes

0marperez added 8 commits October 17, 2024 13:48

Remove simpleName runtime type

eb4999d

Downstream attributes utils

287e050

Got rid of unnecessary code in identity provider chain for credential…

cb76cd5

…s business metrics

Fix type bug throwing unnecessary exception

69a9e1b

Remove * import

eb32e8f

Move credentials business metrics to credentials attributes

6c022f0

Remove TODO

2c09d37

Undo commit from main

69466cf

0marperez force-pushed the credentials-business-metrics branch from fab7c1c to 69466cf Compare October 31, 2024 21:13

Clean and API dump

598b715

ianbotsf requested changes Oct 31, 2024

View reviewed changes

0marperez added 5 commits October 31, 2024 19:48

Remove breaking changes

a7e2c7e

One liner for emitIdentityBusinessMetrics

b5a4a38

Ktlint

1cc05b1

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

d92f45b

Self review

266cf9f

lauzadis approved these changes Nov 4, 2024

View reviewed changes

Fix attribute key

b7e1165

lauzadis approved these changes Nov 4, 2024

View reviewed changes

0marperez merged commit 1614926 into credentials-chain-order Nov 4, 2024
10 checks passed

0marperez deleted the credentials-business-metrics branch November 4, 2024 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: credentials business metrics #1164

misc: credentials business metrics #1164

0marperez commented Oct 16, 2024 •

edited

Loading

ianbotsf Oct 31, 2024

0marperez Oct 31, 2024 •

edited

Loading

ianbotsf Oct 31, 2024

0marperez Oct 31, 2024

ianbotsf Oct 31, 2024

0marperez Oct 31, 2024

0marperez commented Nov 1, 2024 •

edited

Loading

lauzadis Nov 4, 2024

0marperez Nov 4, 2024

lauzadis Nov 4, 2024

0marperez Nov 4, 2024 •

edited

Loading

misc: credentials business metrics #1164

misc: credentials business metrics #1164

Conversation

0marperez commented Oct 16, 2024 • edited Loading

Issue #

Description of changes

Choose a reason for hiding this comment

0marperez Oct 31, 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

0marperez commented Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0marperez Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

0marperez commented Oct 16, 2024 •

edited

Loading

0marperez Oct 31, 2024 •

edited

Loading

0marperez commented Nov 1, 2024 •

edited

Loading

0marperez Nov 4, 2024 •

edited

Loading