Coroutines context propagation instrumentation - #1935

Codecov Report

❌ Patch coverage is 52.72727% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.50%. Comparing base (d04c80d) to head (9492043).
⚠️ Report is 18 commits behind head on main.

@@            Coverage Diff             @@
##             main    #1935      +/-   ##
==========================================
+ Coverage   65.34%   65.50%   +0.16%     
==========================================
  Files         169      177       +8     
  Lines        3872     3966      +94     
  Branches      420      446      +26     
==========================================
+ Hits         2530     2598      +68     
- Misses       1211     1238      +27     
+ Partials      131      130       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

❌ Patch coverage is 52.72727% with 26 lines in your changes missing coverage. Please review.

This is because codecov doesn't currently cover Android instrumentation tests, but in reality the coverage is higher.

Thanks (and good luck) for taking this on! I wasn't sure whether this fits inside AndroidInstrumentation as it doesn't actually capture telemetry. I also had questions around whether it makes sense to allow folks to opt-out (presumably not)? Additionally I'd be curious how it interacts with opentelemetry-java's thread-local approach if the two are used together.

Thanks for the feedback, I added some responses.

Regarding

I'd be curious how it interacts with opentelemetry-java's thread-local approach if the two are used together

I'm not sure I follow the question. This plugin actually relies on opentelemetry-java's thread-local storage to capture and then propagate an existing OTel Context in a coroutine, so they work together by design. Is there a use case that you have in mind where they could clash?

Also, we're probably going to keep this plugin simple forever, so I hope it won't become a headache in the future in case people start asking to support other means to create coroutines. The reason is that it only works because of the way OTel Java propagates its Context within threads, which I know is an OTel Java-specific feature. So when we switch to OTel Kotlin, this plugin will become obsolete. The reason we still decided to create it is that someone brought it up in a recent SIG meeting, where we noticed that the issue for it is currently the most upvoted of all OTel Android, so we decided to address it, but I don't think it should become a maintenance burden because of our future migration plans.

I'm not sure I follow the question. This plugin actually relies on opentelemetry-java's thread-local storage to capture and then propagate an existing OTel Context in a coroutine, so they work together by design. Is there a use case that you have in mind where they could clash?

I guess I'm just asking whether there could be any edge cases where library users expect the default thread-local context storage approach, but end up receiving the coroutine context storage approach? And if so, whether we could set expectations through documenting that one approach wins over the other.

As an example, I'm unclear what the current context would be for ctx and ctx2 in the following scenario:

val executor = Executors.newSingleThreadExecutor()
val scope = CoroutineScope(Dispatchers.Default)

scope.launch {
    tracer.createSpan("a").makeCurrent()
    val ctx = Context.current()
    
    executor.submit { 
        tracer.createSpan("b").makeCurrent()
        val ctx2 = Context.current()
    }
}

I guess I'm just asking whether there could be any edge cases where library users expect the default thread-local context storage approach, but end up receiving the coroutine context storage approach? And if so, whether we could set expectations through documenting that one approach wins over the other.

Based on your example, it seems like the use case you're talking about is the following:

1. User launches coroutine
2. Creates span and makes it current in the coroutine backing thread
3. Launches a non-coroutine thread executor (using Java's executors)
4. Creates span within the Java-managed thread

I think we can see this as a "Coroutine to Regular thread" scenario, in which case, it's not covered by this plugin, so the fully Java-managed thread won't get the context that existed in the Kotlin coroutine-managed one, which is an inherited behavior from the Kotlin extension mechanism that the plugin uses.

I think this can be described as a feature where "Kotlin is aware of Java, but not vice versa", which I think is the ideal approach, as this is a Kotlin-first project and the test scenario can be considered quite niche. However, I agree that some people can probably expect a two-way propagation mechanism, whereas it's only one (from a regular Java thread to a Kotlin coroutine backing one), so I'll add some clarifications to the readme file on this.

I added a clarification here: 9492043 on what to expect in the scenario you mentioned, @fractalwrench. Let me know if it helps or if we could improve it a bit further.

Thanks, that's helpful!