Use Kotlin context element in agent instrumentation #1618

anuraaga · 2020-11-11T09:22:49Z

Follow-on to #1617

@johnbley @iNikem mentioned he would chat with you about the approach of using context element, including the notion that makeCurrent in kotlin code is illegal and shouldn't really be supported by us. Figured having this in front of your eyes will help the discussion :)

Fixes #1411

anuraaga · 2020-11-11T09:30:45Z

instrumentation/kotlin-coroutines/javaagent/src/test/kotlin/KotlinCoroutineTests.kt

-    delay(10)
-    a2(iter)
-    scope.close()
+    withContext(span.asContextElement()) {


These changes are the key to the proposal. My theory is span.makeCurrent is illegal code - the only reason it works here is because the agent is present, but if the agent was not present, resumed coroutines could see completely unrelated contexts, which is a disaster. The idea is that our agent is here to enable tracing, not to allow illegal code. How to "codify" that it's illegal is a tricky point though but something we will want to think about. Current idea is that https://github.com/open-telemetry/opentelemetry-java/blob/master/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/context/StrictContextStorage.java#L99 can also check the stacktrace to see if it is being called from a coroutine, and not from the context element. Dunno if it'll work but if it does, I think we end up in a nice place :)

Also kudos to @adriancole for giving the idea of stack-walking as a potential mechanism for checking I was stuck on that.

Does this require application authors who use auto-instrumentation and simply use (e.g.) kotlin suspend functions in their logic (without any manual instrumentation) to explicitly use (kotlin) withContext to pass our (otel) context? That is, if app logic is written in kotlin and taking advantage of auto-instrumentation of inbound and outbound connections, will this still work without the app author having to take extra steps?

Don't think so - the agent with this code is making sure to automatically propagate the parent at the time the coroutine is launched into its context, so that parent is always available. If all they're doing is using libraries which are auto-instrumented by the agent, it should work fine without calling withContext. It's only when doing manual instrumentation within Kotlin that they need to use withContext instead of makeCurrent. How does that sound?

I can see the confusion of this particular test, since it's a javaagent test, but also uses the library instrumentation directly.

do you think we need the nested spans to validate launchConcurrentSuspendFunctions? if not, we could remove a2 and b2 and not create any nested scopes at all, and not need to use the library instrumentation in the test.

if you think the nested scopes is a good test showing interop, maybe we can move it out into a separate test class ...InteropTest.

The nesting spans are crucial to the test - without any form of instrumentation, we were previously reporting nonsensical traces like "a causes b causes a2" and "b2 causes a".

. It's only when doing manual instrumentation within Kotlin that they need to use withContext instead of makeCurrent. How does that sound?

That sounds great; I have no problem making kotlin instrumentation authors work a little to preserve (otel) context. I actually originally tried something like this (using the kotlin coroutine context system more "naturally) but was unable to make it work. I'm glad you were able to figure it out! l'm going to try to find some time to manually validate with a "real" kotlin app and see how this change works out in practice.

@johnbley #1620 links to a real, though simple kotlin app that exhibits problems with span propagation

trask

nice library instrumentation breakout 👍 and great point about the problem of makeCurrent under kotlin coroutines 😬

trask · 2020-11-12T02:13:33Z

.../library/src/main/java/io/opentelemetry/instrumentation/kotlincoroutines/ContextElement.java

+    return context;
+  }
+
+  @NotNull


is @NotNull useful here (and below)?

trask · 2020-11-12T02:24:13Z

...brary/src/main/kotlin/io/opentelemetry/instrumentation/kotlincoroutines/ContextExtensions.kt

+ * and restores the previous [Context] on suspension.
+ */
+fun Span.asContextElement(): CoroutineContext {
+  return ContextElement(Java8BytecodeBridge.currentContext().with(this))


just because this is weird/surprising

Suggested change

return ContextElement(Java8BytecodeBridge.currentContext().with(this))

// Java8BytecodeBridge is needed in order to support Kotlin which generally targets Java 6 bytecode

return ContextElement(Java8BytecodeBridge.currentContext().with(this))

trask · 2020-11-12T03:00:51Z

instrumentation/kotlin-coroutines/javaagent/src/test/kotlin/KotlinCoroutineTests.kt

-    delay(10)
-    a2(iter)
-    scope.close()
+    withContext(span.asContextElement()) {


I can see the confusion of this particular test, since it's a javaagent test, but also uses the library instrumentation directly.

do you think we need the nested spans to validate launchConcurrentSuspendFunctions? if not, we could remove a2 and b2 and not create any nested scopes at all, and not need to use the library instrumentation in the test.

if you think the nested scopes is a good test showing interop, maybe we can move it out into a separate test class ...InteropTest.

anuraaga · 2020-11-12T03:28:22Z

...brary/src/main/kotlin/io/opentelemetry/instrumentation/kotlincoroutines/ContextExtensions.kt

+ * Returns a [CoroutineContext] which will make this [Context] current when resuming a coroutine
+ * and restores the previous [Context] on suspension.
+ */
+fun Context.asContextElement(): CoroutineContext {


By the way, I'm wondering whether these should be in the SDK instead of instrumentation. Mainly since I was imagining adding to the javadoc for makeCurrent to not use it in Kotlin coroutines and use this instead. But that doesn't make as much sense when crossing repos.

language-specific extensions to the API, e.g. opentelemetry-extension-kotlin? I like it

Sent open-telemetry/opentelemetry-java#2066 for the library aspect of this PR

I agree - this is a great approach.

holyjak · 2020-11-12T16:43:48Z

Great work!
I would like to use OT in a Kotlin app now(ish). Is there a workaround I can apply or is it best to wait a few days for this to be merged and then build the agent from master? Thank you!

iNikem · 2020-11-17T18:20:31Z

@anuraaga What are your plans for making this real PR?

anuraaga · 2020-11-18T04:37:20Z

@iNikem Going to do it after updating to snapshots :)

anuraaga · 2020-11-25T08:02:27Z

Updated to use the SDK's kotlin extension

anuraaga requested review from trask, iNikem and johnbley November 11, 2020 09:22

anuraaga mentioned this pull request Nov 11, 2020

Add a library module for kotlin coroutines which provides helper meth… #1617

Closed

anuraaga commented Nov 11, 2020

View reviewed changes

anuraaga mentioned this pull request Nov 12, 2020

Span propagation seems not to be happening in Kotlin #1620

Closed

trask approved these changes Nov 12, 2020

View reviewed changes

anuraaga commented Nov 12, 2020

View reviewed changes

anuraaga mentioned this pull request Nov 12, 2020

Add kotlin extension for interaction with Context in coroutines. open-telemetry/opentelemetry-java#2066

Merged

johnbley approved these changes Nov 12, 2020

View reviewed changes

anuraaga mentioned this pull request Nov 14, 2020

Kotlin coroutine missing trace #1550

Closed

Use SDK helper

9a8011b

anuraaga force-pushed the kotlin-simpler-advice branch from fb6c6bd to 9a8011b Compare November 25, 2020 08:02

anuraaga marked this pull request as ready for review November 25, 2020 08:02

anuraaga requested review from jkwatson, mateuszrzeszutek and tylerbenson as code owners November 25, 2020 08:02

Revert unintended.

b1e40ed

mateuszrzeszutek approved these changes Nov 25, 2020

View reviewed changes

anuraaga merged commit 822be11 into open-telemetry:master Nov 25, 2020

sleeping-barber mentioned this pull request Apr 8, 2021

Broken trace in Kotlin services #2748

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Kotlin context element in agent instrumentation #1618

Use Kotlin context element in agent instrumentation #1618

anuraaga commented Nov 11, 2020 •

edited

Loading

anuraaga Nov 11, 2020

anuraaga Nov 11, 2020 •

edited

Loading

johnbley Nov 11, 2020

anuraaga Nov 11, 2020

trask Nov 12, 2020

johnbley Nov 12, 2020

johnbley Nov 12, 2020

holyjak Nov 12, 2020

trask left a comment

trask Nov 12, 2020

trask Nov 12, 2020

trask Nov 12, 2020

anuraaga Nov 12, 2020

trask Nov 12, 2020

anuraaga Nov 12, 2020

johnbley Nov 12, 2020

holyjak commented Nov 12, 2020

iNikem commented Nov 17, 2020

anuraaga commented Nov 18, 2020

anuraaga commented Nov 25, 2020

	return ContextElement(Java8BytecodeBridge.currentContext().with(this))
	// Java8BytecodeBridge is needed in order to support Kotlin which generally targets Java 6 bytecode
	return ContextElement(Java8BytecodeBridge.currentContext().with(this))

Use Kotlin context element in agent instrumentation #1618

Use Kotlin context element in agent instrumentation #1618

Conversation

anuraaga commented Nov 11, 2020 • edited Loading

Choose a reason for hiding this comment

anuraaga Nov 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holyjak commented Nov 12, 2020

iNikem commented Nov 17, 2020

anuraaga commented Nov 18, 2020

anuraaga commented Nov 25, 2020

anuraaga commented Nov 11, 2020 •

edited

Loading

anuraaga Nov 11, 2020 •

edited

Loading