Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add
shutdown
inTracerProvider
#1855feat: add
shutdown
inTracerProvider
#1855Changes from 2 commits
fdc4b58
6db7b40
22d171a
038f8ae
924e33c
31a0a57
39fab0e
639cb2d
9f18239
d3057ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 41 in opentelemetry-sdk/src/trace/provider.rs
Codecov / codecov/patch
opentelemetry-sdk/src/trace/provider.rs#L29-L41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any benefit to creating another
Arc
pointer just foris_shutdown
? Could we reuse the existingArc
pointer inner to also holdis_shutdown
inside it?Check warning on line 165 in opentelemetry-sdk/src/trace/provider.rs
Codecov / codecov/patch
opentelemetry-sdk/src/trace/provider.rs#L165
Check warning on line 172 in opentelemetry-sdk/src/trace/provider.rs
Codecov / codecov/patch
opentelemetry-sdk/src/trace/provider.rs#L172
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use
Mutex
orRwLock
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mutex/rwlock/atomic - is checked in hot path, so it needs to be performant!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! In that case, we should make use of
Arc<Cell<bool>>
to trackis_shutdown
instead of atomics as that should offer better performance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! I thought we could use
Mutex
in conjunction withArc<Cell<T>>
to save the atomic read operation on hot-path. Rust wouldn't allow that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cell<T>
is notSend
andSync
so compiler won't allow thisCheck warning on line 177 in opentelemetry-sdk/src/trace/provider.rs
Codecov / codecov/patch
opentelemetry-sdk/src/trace/provider.rs#L175-L177
Check warning on line 219 in opentelemetry-sdk/src/trace/provider.rs
Codecov / codecov/patch
opentelemetry-sdk/src/trace/provider.rs#L219