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

Kotlin gRPC conflict with OpenTelemetry Kotlin/gRPC instrumentation #7837

Closed
devkanro opened this issue Feb 16, 2023 · 2 comments · Fixed by #7879
Closed

Kotlin gRPC conflict with OpenTelemetry Kotlin/gRPC instrumentation #7837

devkanro opened this issue Feb 16, 2023 · 2 comments · Fixed by #7879
Labels
bug Something isn't working repro provided

Comments

@devkanro
Copy link

Describe the bug
Our server combines Kotlin Coroutine, gRPC, and OpenTelemetry, in this situation can result in unexpected behavior when using withContext to switch the gRPC context.

Check out the image below:
Frame 2 (1)

  • GrpcContextElement is a ThreadContextElement from grpc-kotlin, it store current grpc context instance and will recover it in coroutines.
  • KotlinContextElement is a ThreadContextElement from optentelemetry, it store the otlp context snapshot and will recover it in coroutines.
  • ContextStorageBridge is the override Context.Storage imple from optentelemetry, it will replace the default grpc storage and store grpc context in otlp context.

When I want assign a new key value pair to gRPC context in kotlin coroutine, I will do this:

withContext(GrpcContextElement(Context.withValue(MY_KEY, MY_VALUE))) {
    // do my job
}

It works without tracing, then something strange happens with OpenTelemetry on.

// OT will inject KotlinContextElement to current coroutine context, it stored otpl context snapshot.
// And ContextStorageBridge will override the grpc context storage and read grpc context from otpl context.
// At this moment, we assume that the grpc context state is 'A', and the otplContext state is 'a'.
// 
// A means gRPC context state
// a = (A, <other context>) means the A gRPC context stored in a otpl context with other context value
// KotlinContextElement(a) means the KotlinContextElement stored the a otpl context
// GrpcContextElement(A) means the GrpcContextElement stored the A gRPC context


// First, we append the new key value pair to current grpc context(A), make it changed to B.
// Then we create a GrpcContextElement store the B grpc context.
withContext(GrpcContextElement(Context.current().withValue(MY_KEY, MY_VALUE))) {

    // The GrpcContextElement(B) is the rightmost element in coroutine context chain, it will update the thread state.
    // B grpc context will be attached in current thread, it will be handle by ContextStorageBridge and create a new otpl context named b, b = (B, <other context>).
    // When we get values in grpc context it will get it from B grpc context which in b otpl context.
    // It works fine.

    // KotlinContextElement(a) will be handled next, it will recover the otpl context a to current thread.
    // This is where the problem arises, the a context will be made current.
    // When we get values in grpc context it will get it from A grpc context which in a otpl context.

    // do my buggy job here, the grpc context is A, the otpl context is a.
}

What version are you using?
1.23.0

@devkanro devkanro added the bug Something isn't working label Feb 16, 2023
@trask
Copy link
Member

trask commented Feb 17, 2023

hi @devkanro! it would probably be most helpful if you could provide a small standalone repro that could be used to better understand the issue

@devkanro
Copy link
Author

@trask Please check the devkanro/openTelemetry-issue-7837

laurit added a commit that referenced this issue Feb 24, 2023
Resolves
#7837
`org.jetbrains.kotlinx:kotlinx-coroutines-core:1.6.1` adds a second
`newCoroutineContext` that we shouldn't instrument. When we instrument
it the order of
[`KotlinContextElement`](https://github.com/open-telemetry/opentelemetry-java/blob/main/extensions/kotlin/src/main/java/io/opentelemetry/extension/kotlin/KotlinContextElement.java)
and user added `ThreadContextElement` gets reversed. If user added
`ThreadContextElement` changes opentelemetry context then these changes
will get overwritten by `KotlinContextElement`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working repro provided
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants