Skip to content
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

feat: telemetry coroutine extensions #864

Merged
merged 3 commits into from
Jun 16, 2023
Merged

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Jun 14, 2023

Issue #

N/A

Description of changes

This PR expands the telemetry API with coroutine extensions that the actual generated SDK clients and runtime will mostly use.

  • Adds a new ContextManager interface. This is used to bridge the gap between the external calling code/observability backend and the SDK notion of context which relies on coroutine context for API boundary propagation.
    • Consider this experimental for now as we figure out the best way to deal with some of the rubber meets the road issues around actually plugging in an observability backend like OTeL
  • Adds coroutine extensions for logging, trace span creation, and propagation of Context, TelemetryProvider, etc.

Example Instrumentation

The following is a strawman example of what instrumenting an operation might look like. This will likely change and/or get abstracted to be less verbose/more reusable.

private suspend fun testOperation(provider: TelemetryProvider) {
    val tracer = provider.tracerProvider.getOrCreateTracer("S3")
    val currentContext = provider.contextManager.current()

    val telemetryCtx = TelemetryProviderContext(provider) + TelemetryContextElement(currentContext)
    val span = tracer.createSpan(
        "S3.GetObject",
        attributesOf {
            "rpc.method" to "GetObject"
            "rpc.service" to "S3"
        },
        SpanKind.CLIENT,
        currentContext,
    )

    withSpan(span, telemetryCtx) {
        span.setAttribute("rpc.system", "aws-api")
        testChild()
    }
}

private suspend fun testChild() {
    // FIXME - would require getting a tracer for child spans...
    // thats _ok_ but probably want to use an existing service client tracer instance if possible?
}

This leaves some open questions on how we want

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

@aajtodd aajtodd requested a review from a team as a code owner June 14, 2023 19:24
@sonarcloud
Copy link

sonarcloud bot commented Jun 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Comment on lines +68 to +74
} catch (ex: Exception) {
if (ex !is CancellationException) {
span.setStatus(SpanStatus.ERROR)
}
span.recordException(ex, true)
throw ex
} finally {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: When getting a CancellationException, we'll skip setting the span status to ERROR but we'll still call recordException which will set the error attribute to true. Is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 What should we do with a CancellationException? Should it result in any of the normal exception related attributes being set on a span? Should it record different attributes? Nothing at all?

I went looking for guidance but didn't find anything. I could see doing something like:

when (ex) {
    is CancellationException -> span.setAttribute("cancelled", true)
    else -> {
        span.setStatus(SpanStatus.ERROR)
        span.recordException(ex, true)
    }
}

Or just ignoring cancellation exceptions w.r.t span attributes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it depends on if we intend to complete canceled spans or not. Assuming we do intend to complete them, then I think marking something is important (your cancelled = true attribute is a good idea) but definitely not the exception...that's an implementation detail of the language/coroutines, not an indication of unexpected behavior in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this in next PR

@aajtodd aajtodd merged commit 85bcc33 into observability Jun 16, 2023
@aajtodd aajtodd deleted the telemetry-coroutine-ext branch June 16, 2023 14:44
aajtodd added a commit that referenced this pull request Jul 7, 2023
aajtodd added a commit that referenced this pull request Jul 17, 2023
aajtodd added a commit that referenced this pull request Mar 11, 2024
* feat: identity API upstream changes (#864)
* track upstream codegen changes (#879)
* track upstream IdentityProvider and Attributes changes (#881)
* add base class for credentials config (#883)

BREAKING CHANGE: `CredentialsProvider` method name and signature changed. `signer` property removed from service client config in favor of `authSchemes` override.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants