-
Notifications
You must be signed in to change notification settings - Fork 554
Modify loggerprovider shutdown, flush to return single Result and handle repeat shutdown calls #1750
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
Modify loggerprovider shutdown, flush to return single Result and handle repeat shutdown calls #1750
Changes from 7 commits
f63fd0e
a99f47d
a08cb03
272969c
2190017
648b82c
1ebba36
f96e66a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,5 +24,5 @@ fn main() { | |
tracing_subscriber::registry().with(layer).init(); | ||
|
||
error!(name: "my-event-name", target: "my-system", event_id = 20, user_name = "otel", user_email = "[email protected]"); | ||
drop(provider); | ||
let _ = provider.shutdown(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,16 +4,19 @@ use crate::{ | |
runtime::RuntimeChannel, | ||
}; | ||
use opentelemetry::{ | ||
global::{self}, | ||
logs::LogResult, | ||
global, | ||
logs::{LogError, LogResult}, | ||
trace::TraceContextExt, | ||
Context, InstrumentationLibrary, | ||
}; | ||
|
||
#[cfg(feature = "logs_level_enabled")] | ||
use opentelemetry::logs::Severity; | ||
|
||
use std::{borrow::Cow, sync::Arc}; | ||
use std::{ | ||
borrow::Cow, | ||
sync::{atomic::Ordering, Arc}, | ||
}; | ||
use std::{sync::atomic::AtomicBool, time::SystemTime}; | ||
|
||
use once_cell::sync::Lazy; | ||
|
@@ -105,17 +108,29 @@ impl LoggerProvider { | |
} | ||
|
||
/// Shuts down this `LoggerProvider` | ||
pub fn shutdown(&self) -> Vec<LogResult<()>> { | ||
// mark itself as already shutdown | ||
self.is_shutdown | ||
.store(true, std::sync::atomic::Ordering::Relaxed); | ||
// propagate the shutdown signal to processors | ||
// it's up to the processor to properly block new logs after shutdown | ||
self.inner | ||
.processors | ||
.iter() | ||
.map(|processor| processor.shutdown()) | ||
.collect() | ||
pub fn shutdown(&self) -> LogResult<()> { | ||
if self | ||
.is_shutdown | ||
.compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) | ||
.is_ok() | ||
{ | ||
// propagate the shutdown signal to processors | ||
// it's up to the processor to properly block new logs after shutdown | ||
let mut errs = vec![]; | ||
for processor in &self.inner.processors { | ||
if let Err(err) = processor.shutdown() { | ||
errs.push(err); | ||
} | ||
} | ||
|
||
if errs.is_empty() { | ||
Ok(()) | ||
} else { | ||
Err(LogError::Other(format!("{errs:?}").into())) | ||
} | ||
} else { | ||
Err(LogError::Other("logger provider already shut down".into())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should make a dedicate LogError variant for repeat shutdown. I image in most use cases users don't care as long as the shutdown has invoked at least once There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree to fix it. Not in this PR, as the current structure of placing these errors in the API is not correct, and needs a refactoring. Will cover it as part of #1042 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd mark this as releasing blocking and add a todo in code too. It will be very anti-intutive if the users has to parse the error string to understand if the errors is caused by repeated shutdown There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes for stable or even beta release. But not for #1738 which is still alpha for logs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry if this is already answered. What is wrong with returning vec of LogResult for shutdown? Is it only for consistency with metrics? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. And also, if any individual sub-component faced issue while shutdown, they should be emitting their own logging. Shutdown() just need to capture the overall state. But I am not very sure we are doing it correctly. The proper solution should be coming once we address #1146 (that also is very much related to #761 as it is hard to solve internal logging without fixing the infinite telemetry/circular dep problem) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok thanks, that makes sense. |
||
} | ||
} | ||
} | ||
|
||
|
@@ -485,6 +500,25 @@ mod tests { | |
assert_eq!(counter.load(std::sync::atomic::Ordering::SeqCst), 3); | ||
} | ||
|
||
#[test] | ||
fn shutdown_idempotent_test() { | ||
let counter = Arc::new(AtomicU64::new(0)); | ||
let logger_provider = LoggerProvider::builder() | ||
.with_log_processor(ShutdownTestLogProcessor::new(counter.clone())) | ||
.build(); | ||
|
||
let shutdown_res = logger_provider.shutdown(); | ||
assert!(shutdown_res.is_ok()); | ||
|
||
// Subsequent shutdowns should return an error. | ||
let shutdown_res = logger_provider.shutdown(); | ||
assert!(shutdown_res.is_err()); | ||
|
||
// Subsequent shutdowns should return an error. | ||
let shutdown_res = logger_provider.shutdown(); | ||
assert!(shutdown_res.is_err()); | ||
} | ||
|
||
#[test] | ||
fn global_shutdown_test() { | ||
// cargo test shutdown_test --features=logs | ||
|
@@ -508,7 +542,7 @@ mod tests { | |
|
||
// explicitly calling shutdown on logger_provider. This will | ||
// indeed do the shutdown, even if there are loggers still alive. | ||
logger_provider.shutdown(); | ||
let _ = logger_provider.shutdown(); | ||
|
||
// Assert | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.