Fix flaky micrometer tests #5106
Conversation
|
Hmm, apparently that's still not it, still getting these |
| @@ -21,10 +21,15 @@ | |||
| import java.util.function.ToLongFunction; | |||
| import javax.annotation.Nullable; | |||
|
|
|||
| // lalala test | |||
mateuszrzeszutek
force-pushed
the
fix-micrometer-flaky-tests
branch
from
43d11a1 to
60487a9
Compare
|
Okay, I think these cryptic NPEs I observed earlier may be related to #5051, not micrometer tests (probably), so I'm reopening it. |
| private final Meter meter; | ||
| // using a weak ref so that the AsyncInstrumentRegistry (which is stored in a static maps) does | ||
| // not hold strong references to Meter (and thus make it impossible to collect Meter garbage). | ||
| // in practice this should never return null |
There was a problem hiding this comment.
it's not obvious to me why Meter can't be GC'd before AsyncInstrumentRegistry, can you explain briefly, or what do you think about using weak value in the cache instead Cache<Meter, WeakReference<AsyncInstrumentRegistry>>?
There was a problem hiding this comment.
It can't be GC'd because the OpenTelemetryMeterRegistry maintains a strong reference to both Meter and AsyncInstrumentRegistry -- so if the registry instance is collected then the registry cannot possibly be used anymore. I'll add a comment.
or what do you think about using weak value in the cache instead
Cache<Meter, WeakReference<AsyncInstrumentRegistry>>?
Hmm, in case the meter registry is GC'd the WeakReference<AsyncInstrumentRegistry> could get nulled out even if the Meter is not GC'd yet (because the OpenTelemetry instance is still used) - this hypothetical scenario would break the async instruments, cause if you create a new OpenTelemetryMeterRegistry for the same OpenTelemetry/Meter the async instrument registry will get cleared out.
There was a problem hiding this comment.
oh, got it 👍
I'll add a comment
❤️
* Fix flaky micrometer tests * Add comment
Fixes #5103
This PR presents yet another reason why we'd like to be able to use multiple callbacks in async instruments (open-telemetry/opentelemetry-specification#2249) -- turns out the library instrumentation tests were randomly failing because each of them created a new
OpenTelemetryMeterRegistry, and they did not share state - and theOpenTelemetryused in library tests is shared between all test classes.