SetMeterProvider might miss the delegation for instruments and registries
#5780
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5780 +/- ##
=====================================
Coverage 84.5% 84.5%
=====================================
Files 272 272
Lines 22784 22798 +14
=====================================
+ Hits 19268 19282 +14
Misses 3173 3173
Partials 343 343 |
XSAM
changed the title
SetMeterProvider might miss the delegation for instruments and registry.SetMeterProvider might miss the delegation for instruments and registries
XSAM
requested review from
MrAlias,
dashpole,
MadVikingGod,
pellared,
hanyuancheung and
dmathieu
as code owners
There was a problem hiding this comment.
This change means that every measurement through the global API now requires at a minimum ~100ns to acquire the lock. This is on the hot-path and a performance evaluation should have been provided prior to this change being accepted.
The original design was aware of the case where a recording was performed concurrently with SetMeterProvider and it was dropped. That was intentional. There is a trade-off of performance and it was originally decided that speed on the hot-path is more important than synchronicity.
Are there panics or race conditions this addresses? If not these changes should be reverted until it can be shown that the performance impact is not significant.
The drop would happen when initiating the instrument, not recording. The recording to instruments that didn't Imagine users initiating instruments while setting global meter provider. Some instruments were not able to record anything during the entire runtime of the application. This looks like race conditions to me. |
|
@MrAlias it isn't every measurement, right? It is every instrument instantiation? Or am I misreading |
|
It looks like I misread this change. Looks good. Please disregard. |
### Added - Support `OTEL_EXPORTER_OTLP_LOGS_INSECURE` and `OTEL_EXPORTER_OTLP_INSECURE` environments in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc`. (#5739) - The `WithResource` option for `NewMeterProvider` now merges the provided resources with the ones from environment variables. (#5773) - The `WithResource` option for `NewLoggerProvider` now merges the provided resources with the ones from environment variables. (#5773) - Add UTF-8 support to `go.opentelemetry.io/otel/exporters/prometheus`. (#5755) ### Fixed - Fix memory leak in the global `MeterProvider` when identical instruments are repeatedly created. (#5754) - Fix panic instruments creation when setting meter provider. (#5758) - Fix panic on instruments creation when setting meter provider. (#5758) - Fix an issue where `SetMeterProvider` in `go.opentelemetry.io/otel` might miss the delegation for instruments and registries. (#5780) ### Removed - Drop support for [Go 1.21](https://go.dev/doc/go1.21). (#5736, #5740, #5800)
Closes #5757
This PR fixes an issue where
SetMeterProvidermight miss the delegation for instruments and registries. This bug brings a concurrent issue that could possibly make instruments and registries unable to operate correctly, such as recording, after using theSetMeterProvidermethod. The data put on these instruments and registries might be lost.