-
Notifications
You must be signed in to change notification settings - Fork 55
feat: add tracing framework #756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
| region = provider.region ?: GLOBAL_STS_PARTITION_ENDPOINT | ||
| credentialsProvider = provider.credentialsProvider | ||
| httpClientEngine = provider.httpClientEngine | ||
| tracer = traceSpan.asNestedTracer("STS-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question/nit: Perhaps the trailing - separator should be handled internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
| } | ||
|
|
||
| val call = httpClient.call(tokenReq) | ||
| val call = httpClient.call(SdkHttpRequest(tokenReq)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Where is this change stemming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was accidentally left over from a now-removed refactor. I'll revert it.
|
|
||
| override suspend fun getCredentials(): Credentials { | ||
| val chainException = lazy { CredentialsProviderException("No credentials could be loaded from the chain: $this") } | ||
| override suspend fun getCredentials(): Credentials = coroutineContext.withChildTraceSpan("Credentials chain") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: This isn't listed in your span taxonomy FYI (I think it's fine here just mentioning it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep you're right, I forgot I left this here. I'll update the taxonomy definition.
I'll also be posting the taxonomy in a new section of tracing.md in the smithy-kotlin PR. I'll make sure to include this there as well.
|
Kudos, SonarCloud Quality Gate passed!
|
|
A new generated diff is ready to view. |








Issue #
Closes smithy-kotlin#677
Description of changes
These are the aws-sdk-kotlin changes to adopt the new tracing framework introduced in smithy-kotlin#737.
Note: This is the final, mergeable version of #731, including changes from the rejected approaches in #680 (context receivers) and #708 (ExecutionContext).
Companion PR: smithy-kotlin#737
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.