Unverified Commit f50c8eb9 authored by Aaron Todd's avatar Aaron Todd Committed by GitHub
Browse files

remove sdkId transform by default (#4064)

## Motivation and Context
fixes https://github.com/awslabs/aws-sdk-rust/issues/1252

## Description
The service ID we [were
setting](https://github.com/smithy-lang/smithy-rs/blob/5f7113f506f301296311ef637e40d226e0a6e548/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/ServiceEnvConfigDecorator.kt#L27)
for service specific environment configuration was wrong. It was wrong
in two ways:

1. The
[sdkId](https://github.com/smithy-lang/smithy-rs/blob/7db3e9f08e422e386c56d5916c69ab495d739ee8/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/util/Smithy.kt#L157)
extension we defined strips whitespace and lowercases the ID from the
model.
2. It also used `toSnakeCase()` which will not have the same effect as
internally that uses `splitOnWordBoundaries` which uses heuristics to
determine where a word boundary is and isn't restricted to spaces.

It looks like we've always applied this transform to sdkId from what I
can tell with the original use being to set the operation metadata for
the service name and it was carried through when making an extension.
This PR makes `sdkId()` return the ID from the model untouched. I think
this is the safest default as remembering when you can use the extension
vs not is error prone. Any required/desired transformations should be
done localized to where it's used. In this case the
`ServiceEnvConfigDecorator` doesn't need to transform sdkId as the
[runtime already does
it](https://github.com/smithy-lang/smithy-rs/blob/d1bbd018618da9ab8f8bdb1f27d9ec75c42d2505/aws/rust-runtime/aws-runtime/src/env_config.rs#L351)

## Testing

Tested on the bedrock-agent and bedrock-agent-runtime models described
in the issue

## Questions

There were only four uses of the `sdkId()` function I could find:
1. Operation metadata
2. Span names
3. Service env config
4. Default retry partition naming

I think this is a safe change though it will result in different log
output. Unclear if anyone would be predicating off service name if we
expose it anywhere (e.g. in an interceptor).

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] For changes to the AWS SDK, generated SDK code, or SDK runtime
crates, I have created a changelog entry Markdown file in the
`.changelog` directory, specifying "aws-sdk-rust" in the `applies_to`
key.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
parent f10556df
Loading
Loading
Loading
Loading
+12 −0
Original line number Diff line number Diff line
---
applies_to:
- aws-sdk-rust
authors:
- aajtodd
references:
- aws-sdk-rust#1252
breaking: false
new_feature: false
bug_fix: true
---
Fix service specific endpoint url keys
+4 −4
Original line number Diff line number Diff line
@@ -585,18 +585,18 @@ dependencies = [

[[package]]
name = "clap"
version = "4.5.32"
version = "4.5.33"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "6088f3ae8c3608d19260cd7445411865a485688711b78b5be70d78cd96136f83"
checksum = "e2c80cae4c3350dd8f1272c73e83baff9a6ba550b8bfbe651b3c45b78cd1751e"
dependencies = [
 "clap_builder",
]

[[package]]
name = "clap_builder"
version = "4.5.32"
version = "4.5.33"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "22a7ef7f676155edfb82daa97f99441f3ebf4a58d5e32f295a56259f1b6facc8"
checksum = "0123e386f691c90aa228219b5b1ee72d465e8e231c79e9c82324f016a62a741c"
dependencies = [
 "anstyle",
 "clap_lex",
+1 −2
Original line number Diff line number Diff line
@@ -13,7 +13,6 @@ import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
import software.amazon.smithy.rust.codegen.core.smithy.RustCrate
import software.amazon.smithy.rust.codegen.core.util.dq
import software.amazon.smithy.rust.codegen.core.util.sdkId
import software.amazon.smithy.rust.codegen.core.util.toSnakeCase

class ServiceEnvConfigDecorator : ClientCodegenDecorator {
    override val name: String = "ServiceEnvConfigDecorator"
@@ -24,7 +23,7 @@ class ServiceEnvConfigDecorator : ClientCodegenDecorator {
        rustCrate: RustCrate,
    ) {
        val rc = codegenContext.runtimeConfig
        val serviceId = codegenContext.serviceShape.sdkId().toSnakeCase().dq()
        val serviceId = codegenContext.serviceShape.sdkId().dq()
        rustCrate.withModule(ClientRustModule.config) {
            Attribute.AllowDeadCode.render(this)
            rustTemplate(
+6 −6
Original line number Diff line number Diff line
@@ -1768,18 +1768,18 @@ dependencies = [

[[package]]
name = "clap"
version = "4.5.32"
version = "4.5.33"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "6088f3ae8c3608d19260cd7445411865a485688711b78b5be70d78cd96136f83"
checksum = "e2c80cae4c3350dd8f1272c73e83baff9a6ba550b8bfbe651b3c45b78cd1751e"
dependencies = [
 "clap_builder",
]

[[package]]
name = "clap_builder"
version = "4.5.32"
version = "4.5.33"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "22a7ef7f676155edfb82daa97f99441f3ebf4a58d5e32f295a56259f1b6facc8"
checksum = "0123e386f691c90aa228219b5b1ee72d465e8e231c79e9c82324f016a62a741c"
dependencies = [
 "anstyle",
 "clap_lex",
@@ -4631,9 +4631,9 @@ checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65"

[[package]]
name = "value-bag"
version = "1.10.0"
version = "1.11.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "3ef4c4aa54d5d05a279399bfa921ec387b7aba77caf7a682ae8d86785b8fdad2"
checksum = "943ce29a8a743eb10d6082545d861b24f9d1b160b7d741e0f2cdf726bec909c5"

[[package]]
name = "version_check"
+2 −2
Original line number Diff line number Diff line
@@ -86,12 +86,12 @@ async fn metrics_have_expected_attributes() {
    let call_duration_attributes =
        extract_metric_attributes(&finished_metrics, "smithy.client.call.duration");
    assert!(call_duration_attributes[0].contains(&KeyValue::new("rpc.method", "GetObject")));
    assert!(call_duration_attributes[0].contains(&KeyValue::new("rpc.service", "s3")));
    assert!(call_duration_attributes[0].contains(&KeyValue::new("rpc.service", "S3")));

    let attempt_duration_attributes =
        extract_metric_attributes(&finished_metrics, "smithy.client.call.attempt.duration");
    assert!(attempt_duration_attributes[0].contains(&KeyValue::new("rpc.method", "GetObject")));
    assert!(attempt_duration_attributes[0].contains(&KeyValue::new("rpc.service", "s3")));
    assert!(attempt_duration_attributes[0].contains(&KeyValue::new("rpc.service", "S3")));

    // The attempt metric contains an attempt counter attribute that correctly increments
    assert!(attempt_duration_attributes
Loading