feat: add shutdown in TracerProvider
#1855
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1855 +/- ##
=======================================
+ Coverage 74.5% 74.6% +0.1%
=======================================
Files 122 122
Lines 19859 19952 +93
=======================================
+ Hits 14807 14902 +95
+ Misses 5052 5050 -2 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Lalit Kumar Bhasin <[email protected]>
Co-authored-by: Lalit Kumar Bhasin <[email protected]>
There was a problem hiding this comment.
LGTM. A changelog entry is recommended, as this is removing a significant perf bottleneck. (i tried stress testing - 8-9M/sec to 20M/sec with this PR which is huge improvement)
Do you plan to follow up with removal of global shutdowns similar to what is done for metrics? #1743
| #[derive(Clone, Debug)] | ||
| pub struct TracerProvider { | ||
| inner: Arc<TracerProviderInner>, | ||
| is_shutdown: Arc<AtomicBool>, |
There was a problem hiding this comment.
Is there any benefit to creating another Arc pointer just for is_shutdown? Could we reuse the existing Arc pointer inner to also hold is_shutdown inside it?
| } | ||
| } else { | ||
| Err(TraceError::Other( | ||
| "tracer provider already shut down".into(), |
There was a problem hiding this comment.
We should probably use Mutex or RwLock instead of atomic variable to track shutdown. With the current setup, if there are two threads that are racing to shutdown the tracerprovider, the thread which fails the compare and swap and immediately come to the else condition. It wouldn't return an error message saying "tracer provider already shut down" without even waiting or verifying that the other thread has indeed completed the shutdown.
There was a problem hiding this comment.
Will rephrasing the error to say "tracer provider is already shutting down or has been shut down" be useful here, instead of using the Mutex/RwLock ?
There was a problem hiding this comment.
It depends on what kind of experience we are targeting. I feel that shutting down a tracer provider does not have to be a perf optimized operation so it's okay to use locks. Shutdown would anyway not be a frequent scenario. I also like the clear status that locking offers about whether the provider is shutdown or not.
There was a problem hiding this comment.
The mutex/rwlock/atomic - is checked in hot path, so it needs to be performant!
There was a problem hiding this comment.
https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/src/logs/log_processor.rs#L93-L97 Logs.
Looks like we are inconsistent here. so that need to be taken care as well!
There was a problem hiding this comment.
The mutex/rwlock/atomic - is checked in hot path, so it needs to be performant!
Got it! In that case, we should make use of Arc<Cell<bool>> to track is_shutdown instead of atomics as that should offer better performance.
There was a problem hiding this comment.
why Arc<Cell<bool>> - this can cause race condition and data corruption right, as it's not thread safe.
There was a problem hiding this comment.
My bad! I thought we could use Mutex in conjunction with Arc<Cell<T>> to save the atomic read operation on hot-path. Rust wouldn't allow that.
There was a problem hiding this comment.
Cell<T> is not Send and Sync so compiler won't allow this
4 participants
Towards #1801
Changes
shutdowninTracerProvider.Tracernow takes aTracerProviderinstead ofWeak<TracerProviderInner>, removing the upgrading costTracerProviderhas already shutdownTracerProviderhas already shutdownMerge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes