feat: business metrics

[0marperez]

Issue #

N/A

Description of changes

This change tracks business metrics in the execution context of an operation. To do so:

• The DefaultEndpointProviderGenerator was modified to include endpoint business metrics in the endpoint attributes
• This change required modifying the DefaultEndpointProviderTestGenerator to be tolerant of business metrics attributes
• A section was declared in EndpointResolverAdapterGenerator to add endpoint business metrics to the execution context if business metrics exist in the endpoint attributes
• Business metrics are now added to the execution context directly in various locations
• Required runtime types to emit business metrics were added

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

[ianbotsf]

Correctness: This is the point at which we know an endpoint was resolved but not the point where we could definitively say the endpoint was used. (A subsequent interceptor may replace the endpoint.) We should find a way to plumb this information closer to where the endpoint is actually used.

[ianbotsf]

Style: Doing this kind of mutable state tracking via class-level fields is difficult to follow and error-prone. The visitor pattern allows using a return type to get information from within the tree but it looks like we currently use ExpressionVisitor<Unit> and skip returning anything.

My suggestion would be to refactor this to keep a context result in the visitor:

data class EndpointInfo(val params: MutableSet<String>) {
    companion object {
        val Empty = EndpointInfo(params = mutableSetOf())
    }

    operator fun plus(other: EndpointInfo) = EndpointInfo(
        params = this.params + other.params,
    )
}

class ExpressionGenerator(…): ExpressionVisitor<EndpointInfo>, … {
    …

    override fun visitRef(reference: Reference): EndpointInfo {
        val referenceName = reference.name.defaultName()
        val isParamReference = isParamRef(reference)

        if (isParamReference) {
            writer.writeInline("params.")
        }
        writer.writeInline(referenceName)

        return if (isParamReference) {
            EndpointInfo(params = mutableSetOf(referenceName))
        } else {
            EndpointInfo.Empty
        }
    }

    …
}

Then you can modify the expression renderer to return/use the information:

fun interface ExpressionRenderer {
    fun renderExpression(expr: Expression): EndpointInfo
}

class DefaultEndpointProviderGenerator(…): ExpressionRenderer {
    …

    override fun renderExpression(expr: Expression): EndpointInfo = expr.accept(expressionGenerator)

    private fun renderEndpointRule(rule: EndpointRule) {
        withConditions(rule.conditions) {
            writer.withBlock("return #T(", ")", RuntimeTypes.SmithyClient.Endpoints.Endpoint) {
                writeInline("#T.parse(", RuntimeTypes.Core.Net.Url.Url)
                val endpointInfo = renderExpression(rule.endpoint.url)
                write("),")

                val hasAccountIdBasedEndpoint = "accountId" in endpointInfo.params
                val hasServiceEndpointOverride = "endpoint" in endpointInfo.params
                val needAdditionalEndpointProperties = hasAccountIdBasedEndpoint || hasServiceEndpointOverride

                if (rule.endpoint.headers.isNotEmpty()) { … }

                if (rule.endpoint.properties.isNotEmpty() || needAdditionalEndpointProperties) {
                    withBlock("attributes = #T {", "},", RuntimeTypes.Core.Collections.attributesOf) {
                        rule.endpoint.properties.entries.forEach { (k, v) -> … }

                        if (hasAccountIdBasedEndpoint) {
                            writer.write("#T to true", RuntimeTypes.Core.BusinessMetrics.accountIdBasedEndPoint)
                        }
                        if (hasServiceEndpointOverride) {
                            writer.write("#T to true", RuntimeTypes.Core.BusinessMetrics.serviceEndpointOverride)
                        }
                    }
                }
            }
        }
    }

    …
}

[lauzadis]

@0marperez Were you able to take a look at this feedback, do you think it's possible to refactor? I also agree that the boolean toggling stands out and seems error-prone for future implementations

[0marperez]

I ran into some difficulties while trying to implement this and decided to leave it as is since the comment is marked as style. I think it's possible so I'll give it another go

[ianbotsf]

Correctness: This appears to be setting the expected attributes based on the actual attributes, which is backwards. If the actual attributes erroneously omitted, for example, account ID then this test wouldn't catch it.

[ianbotsf]

Style: You can avoid this kind of temporary-storage-for-control-flow pattern by using also:

algorithm.compressRequest(context.protocolRequest).also {
    context.executionContext.emitBusinessMetric(BusinessMetrics.GZIP_REQUEST_COMPRESSION)
}

[ianbotsf]

Question: I'm not sure I see the value of dedicated attribute keys for these. At the point we have access to the these attributes, we could be calling emitBusinessMetrics already, right?

[0marperez]

Do you mean in the DefaultEndpointProviderGenerator? If so I don't think we can, we only have access to the endpoint attributes but not to the execution context attributes

[lauzadis]

naming: AccountIdBasedEndpointAccountId

• No camel casing
• EndPoint -> Endpoint
  - Remove second AccountId edit: I see why this was duplicated in the name, seems good to keep it

[lauzadis]

question: Are these enum values correct? I see something different in the internal spec. "J" is S3_EXPRESS_BUCKET, "L" is GZIP_REQUEST_COMPRESSION, etc.

[0marperez]

Yeah, they're outdated now. Thanks

[lauzadis]

naming: AttributeKeys are not usually camel-cased, BusinessMetrics

[lauzadis]

naming: ServiceEndpointOverride

[lauzadis]

naming: BusinessMetric: Enum class names are usually not pluralized,, since an instance of this enum will only ever represent a single metric

[lauzadis]

suggestion: rule.endpoint.properties are mutable, so instead of creating this new boolean / tracking extra state, you can just add the accountIdBasedEndpoint and serviceEndpointOverride parameters to the endpoint params before entering this if statement.

Something like this:

rule.endpoint.properties[Identifier.of(RuntimeTypes.Core.BusinessMetrics.accountIdBasedEndPointAccountId.name)] = Literal.of("params.accountId")
rule.endpoint.properties[Identifier.of(RuntimeTypes.Core.BusinessMetrics.serviceEndpointOverride.name)] = Literal.booleanLiteral(true)

if (rule.endpoint.properties.isNotEmpty() { 
    ...
} 

[lauzadis]

simplification: You can remove the duplicated RuntimeTypes.SmithyClient.Endpoints.Endpoint by replacing #T with #1T

Same for all other lines where you duplicate the #T

[lauzadis]

question: Was this modification required to get endpoint tests passing? It would be nice if we didn't have to customize the endpoint tests based on business metrics

[0marperez]

Yes, the endpoint tests in the model(s) don't include business metrics in the expected. To fix this the model(s) would have to be updated

[lauzadis]

correctness: this is only emitting metrics when a request is retried (first attempt failed), but it seems like the SEP wants us to emit the metrics any time the strategy is used (even on the first attempt).

[lauzadis]

suggestion: Instead of calling this function after almost every interceptor stage, can it just be added as a new interceptor? It can keep an internal var request : HttpRequest that is updated at each stage, and then you can use the readBeforeX interceptor hooks to compare the modifiedRequest

[lauzadis]

Any thoughts on this suggestion? I think it would be cleaner, InterceptorExecutor shouldn't really be responsible for emitting / updating business metrics IMO

[0marperez]

I'm not sure if it would work, the interceptor would only run once but there may be other interceptors that run afterwards or before that change the url and it wouldn't be picked up

[lauzadis]

Currently we're only checking if the URL changed after the modifyX interceptors are executed. We could instead add a new interceptor right after in the readY phase to check if the URL had changed.

It's not a one-way decision so I'm fine to leave it as it is and refactor later if needed.

[lauzadis]

Some of these metrics are AWS-specific and don't belong in smithy-kotlin. We spent a lot of time making sure S3 Express was not mentioned in smithy-kotlin.

Is there any way the AWS-specific enums can be relocated to aws-sdk-kotlin?

[0marperez]

I tried that too but it seems like that would require smithy kotlin have a dependency on the sdk. I think we want to avoid that

[lauzadis]

Yeah, smithy-kotlin can not depend on aws-sdk-kotlin at all, that's a circular dependency.

Fixing this will certainly involve some rearchitecture. You will probably need to make an AwsBusinessMetrics class which implements BusinessMetrics so it can be used interchangeably in all the functions / utilities you wrote. That would also probably mean moving away from enum class, we can discuss offline

[lauzadis]

naming mismatch: emitBusinessMetrics vs. emitBusinessMetric

[lauzadis]

@0marperez Were you able to take a look at this feedback, do you think it's possible to refactor? I also agree that the boolean toggling stands out and seems error-prone for future implementations

[lauzadis]

Any thoughts on this suggestion? I think it would be cleaner, InterceptorExecutor shouldn't really be responsible for emitting / updating business metrics IMO

[lauzadis]

We support some features that aren't listed here. Should we add TODOs to add them in the future?

• WAITER("B")
• PAGINATOR("C")

[0marperez]

Yeah, those features were added to the spec after we started the implementation. I think it's a good idea

[github-actions]

Affected Artifacts

Changed in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
http-client-jvm.jar	332,851	331,000	1,851	0.56%
runtime-core-jvm.jar	883,226	878,947	4,279	0.49%

He's OOTO