misc: credentials business metrics #1164
Conversation
0marperez
added
the
no-changelog
…s business metrics
0marperez
force-pushed
the
credentials-business-metrics
branch
from
fab7c1c
to
69466cf
Compare
| if (identityAttributes.contains(BusinessMetrics)) { | ||
| identityAttributes[BusinessMetrics] | ||
| .toList() | ||
| .reversed() | ||
| .forEach { metric -> | ||
| context.emitBusinessMetric(metric) | ||
| } | ||
| } |
There was a problem hiding this comment.
Question: Why is reversing the order significant? Could this just be:
identity.attributes.getOrNull(BusinessMetrics)?.forEach(context::emitBusinessMetric)There was a problem hiding this comment.
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.
| 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 | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You are correct here, I'll fix the breaking changes.
| @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) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Reminder for myself: DO NOT MERGE this into main until you get changes tracking the business metrics attribute from |
Issue #
Description of changes
-Emits an
Identity's attributesBusinessMetricsinto anExecutionContextduring 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.