Fix race condition of GlobalOpenTelemetry initialization with AutoConfiguredOpenTelemetrySdkBuilder
#7365
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7365 +/- ##
============================================
+ Coverage 89.75% 89.79% +0.04%
- Complexity 6980 6985 +5
============================================
Files 797 797
Lines 21165 21174 +9
Branches 2057 2057
============================================
+ Hits 18996 19013 +17
+ Misses 1505 1501 -4
+ Partials 664 660 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I don't love the idea of accessing the GlobalOpenTelemetry's private mutex reflectively...
Could we achieve the same effect by adding GlobalOpenTelemetry#set(Supplier<OpenTelemetry> supplier), with an implementation that obtains the the lock on mutex before calling Supplier.get()?
| * OpenTelemetry} object, and finally calls {@link #set(OpenTelemetry)}, all while holding the | ||
| * {@link GlobalOpenTelemetry} mutex. | ||
| */ | ||
| public static <T> T set(Supplier<T> supplier, Function<T, OpenTelemetry> converter) { |
There was a problem hiding this comment.
What about something like: 5f06de2
This API signature will look strange outside of the autoconfigure use case.
I like this approach in general though and would love to get it merged!
There was a problem hiding this comment.
Yeah I didn't like that signature either, your commit made it look much better. I chery-picked it into my branch
|
Won't this lead to some difficult situations where someone has a long-lived handle to an instance that they got from GlobalOpenTelemetry.get() which is no longer the "active" instance? Have we thoroughly thought through the implications of that? Will this behavior be surprising when we support dynamically changing the behavior of GlobalOpenTelemetry via (eg.) opamp? |
never mind. I missed that |
|
Can we write a unit test that verifies that this fixes the race condition? |
A fully deterministic reproducer would be relatively hard to write for this issue since it's a race condition. I could do that, but I'd probably have to resort to artificial sleeps to reproduce the conditions for the race. Would such a test be acceptable @jkwatson ? |
Yes, better than no test at all, so we don't accidentally re-introduce the issue in the future. |
|
Hi @jkwatson, please check the new test in |
...ure/src/test/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: jack-berg <[email protected]>
🚢 |
Fixes #7354.
Not sure if reflection is the best way to achieve this, I've seen it's quite used in the SDK to hide things that should not be directly exposed to the users. Let me know if a proper API exposed byGlobalOpenTelemetrywould be more appropriate.