-
Notifications
You must be signed in to change notification settings - Fork 453
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
Improve internal opentelemetry logging #2128
Changes from 34 commits
7da284e
573994d
1bfd464
90470c3
5f1b990
47abb6d
46ce8af
bbfc03c
e5260d9
a79b636
197b12a
91e56ea
fa7148e
a78357c
a3fba36
54e0755
4c8cb42
5c4bab9
5e95e00
da3716d
62b9f83
591c45a
b6e2327
2714508
b0c129c
83763b1
e325316
19822b1
6b98c16
d248750
f9e3d2d
d4c1eb7
d5120a2
c103467
b7470b1
30f7a4c
adc3fd2
3c4d20c
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 |
---|---|---|
|
@@ -217,14 +217,26 @@ mod tests { | |
use opentelemetry_sdk::trace; | ||
use opentelemetry_sdk::trace::{Sampler, TracerProvider}; | ||
use tracing::error; | ||
use tracing_subscriber::layer::SubscriberExt; | ||
use tracing_subscriber::prelude::__tracing_subscriber_SubscriberExt; | ||
use tracing_subscriber::Layer; | ||
|
||
pub fn attributes_contains(log_record: &LogRecord, key: &Key, value: &AnyValue) -> bool { | ||
log_record | ||
.attributes_iter() | ||
.any(|(k, v)| k == key && v == value) | ||
} | ||
|
||
fn create_tracing_subscriber( | ||
_exporter: InMemoryLogsExporter, | ||
logger_provider: &LoggerProvider, | ||
) -> impl tracing::Subscriber { | ||
let level_filter = tracing_subscriber::filter::LevelFilter::WARN; // Capture WARN and ERROR levels | ||
let layer = | ||
layer::OpenTelemetryTracingBridge::new(logger_provider).with_filter(level_filter); // No filter based on target, only based on log level | ||
|
||
tracing_subscriber::registry().with(layer) | ||
} | ||
|
||
// cargo test --features=testing | ||
#[test] | ||
fn tracing_appender_standalone() { | ||
|
@@ -234,8 +246,7 @@ mod tests { | |
.with_simple_exporter(exporter.clone()) | ||
.build(); | ||
|
||
let layer = layer::OpenTelemetryTracingBridge::new(&logger_provider); | ||
let subscriber = tracing_subscriber::registry().with(layer); | ||
let subscriber = create_tracing_subscriber(exporter.clone(), &logger_provider); | ||
|
||
// avoiding setting tracing subscriber as global as that does not | ||
// play well with unit tests. | ||
|
@@ -315,8 +326,7 @@ mod tests { | |
.with_simple_exporter(exporter.clone()) | ||
.build(); | ||
|
||
let layer = layer::OpenTelemetryTracingBridge::new(&logger_provider); | ||
let subscriber = tracing_subscriber::registry().with(layer); | ||
let subscriber = create_tracing_subscriber(exporter.clone(), &logger_provider); | ||
|
||
// avoiding setting tracing subscriber as global as that does not | ||
// play well with unit tests. | ||
|
@@ -427,16 +437,15 @@ mod tests { | |
.with_simple_exporter(exporter.clone()) | ||
.build(); | ||
|
||
let layer = layer::OpenTelemetryTracingBridge::new(&logger_provider); | ||
let subscriber = tracing_subscriber::registry().with(layer); | ||
let subscriber = create_tracing_subscriber(exporter.clone(), &logger_provider); | ||
|
||
// avoiding setting tracing subscriber as global as that does not | ||
// play well with unit tests. | ||
let _guard = tracing::subscriber::set_default(subscriber); | ||
drop(tracing_log::LogTracer::init()); | ||
|
||
// Act | ||
log::error!("log from log crate"); | ||
log::error!(target: "my-system", "log from log crate"); | ||
logger_provider.force_flush(); | ||
|
||
// Assert TODO: move to helper methods | ||
|
@@ -493,8 +502,7 @@ mod tests { | |
.with_simple_exporter(exporter.clone()) | ||
.build(); | ||
|
||
let layer = layer::OpenTelemetryTracingBridge::new(&logger_provider); | ||
let subscriber = tracing_subscriber::registry().with(layer); | ||
let subscriber = create_tracing_subscriber(exporter.clone(), &logger_provider); | ||
|
||
// avoiding setting tracing subscriber as global as that does not | ||
// play well with unit tests. | ||
|
@@ -513,7 +521,7 @@ mod tests { | |
let span_id = cx.span().span_context().span_id(); | ||
|
||
// logging is done inside span context. | ||
log::error!("log from log crate"); | ||
log::error!(target: "my-system", "log from log crate"); | ||
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. do we need these changes? 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 we can remove the target, will follow up in subsequent cleanups. |
||
(trace_id, span_id) | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
trace::TraceContextExt, | ||
Context, InstrumentationLibrary, | ||
}; | ||
use opentelemetry::{otel_debug, otel_info, otel_warn}; | ||
|
||
#[cfg(feature = "logs_level_enabled")] | ||
use opentelemetry::logs::Severity; | ||
|
@@ -49,6 +50,12 @@ | |
attributes: Option<Vec<opentelemetry::KeyValue>>, | ||
) -> Logger { | ||
let name = name.into(); | ||
otel_info!( | ||
name: "logger_versioned_creation", | ||
instr_lib_name= &*name, | ||
version = version.as_deref(), | ||
schema_url = schema_url.as_deref(), | ||
); | ||
|
||
let component_name = if name.is_empty() { | ||
Cow::Borrowed(DEFAULT_COMPONENT_NAME) | ||
|
@@ -72,6 +79,9 @@ | |
} | ||
|
||
fn library_logger(&self, library: Arc<InstrumentationLibrary>) -> Self::Logger { | ||
otel_info!( | ||
name: "logger_library_logger_creation" | ||
); | ||
// If the provider is shutdown, new logger will refer a no-op logger provider. | ||
if self.is_shutdown.load(Ordering::Relaxed) { | ||
return Logger::new(library, NOOP_LOGGER_PROVIDER.clone()); | ||
|
@@ -83,19 +93,33 @@ | |
impl LoggerProvider { | ||
/// Create a new `LoggerProvider` builder. | ||
pub fn builder() -> Builder { | ||
otel_info!( | ||
name: "logger_provider_builder" | ||
); | ||
Builder::default() | ||
} | ||
|
||
pub(crate) fn log_processors(&self) -> &[Box<dyn LogProcessor>] { | ||
otel_debug!( | ||
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. It should be good enough to get the processor count information just once during LoggerProvider build. Is there any value in repeatedly logging it? 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. to unblock and merge this PR quickly, I think its best to remove all internal logging that are not error/warning, and we can selective bring them one by one in future PRs. |
||
name: "logger_provider_log_processors", | ||
processor_count= self.inner.processors.len() | ||
); | ||
&self.inner.processors | ||
} | ||
|
||
pub(crate) fn resource(&self) -> &Resource { | ||
otel_debug!( | ||
name: "logger_provider_resource", | ||
resource_count= self.inner.resource.len() | ||
); | ||
&self.inner.resource | ||
} | ||
|
||
/// Force flush all remaining logs in log processors and return results. | ||
pub fn force_flush(&self) -> Vec<LogResult<()>> { | ||
otel_info!( | ||
cijothomas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
name: "logger_provider_force_flush" | ||
); | ||
self.log_processors() | ||
.iter() | ||
.map(|processor| processor.force_flush()) | ||
|
@@ -104,6 +128,9 @@ | |
|
||
/// Shuts down this `LoggerProvider` | ||
pub fn shutdown(&self) -> LogResult<()> { | ||
otel_info!( | ||
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. This should be logged only in case of a successful shutdown? Only in this case: if errs.is_empty() {
Ok(())
} |
||
name: "logger_provider_shutdown" | ||
); | ||
if self | ||
.is_shutdown | ||
.compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) | ||
|
@@ -114,6 +141,10 @@ | |
let mut errs = vec![]; | ||
for processor in &self.inner.processors { | ||
if let Err(err) = processor.shutdown() { | ||
otel_warn!( | ||
name: "logger_provider_shutdown_error", | ||
error = format!("{:?}", err) | ||
); | ||
errs.push(err); | ||
} | ||
} | ||
|
@@ -124,6 +155,9 @@ | |
Err(LogError::Other(format!("{errs:?}").into())) | ||
} | ||
} else { | ||
otel_warn!( | ||
name: "logger_provider_already_shutdown" | ||
); | ||
Err(LogError::Other("logger provider already shut down".into())) | ||
} | ||
} | ||
|
@@ -203,6 +237,11 @@ | |
for processor in logger_provider.log_processors() { | ||
processor.set_resource(logger_provider.resource()); | ||
} | ||
otel_debug!( | ||
name: "logger_provider_build", | ||
resource_count = logger_provider.resource().len(), | ||
processor_count = logger_provider.log_processors().len() | ||
); | ||
logger_provider | ||
} | ||
} | ||
|
@@ -221,6 +260,9 @@ | |
instrumentation_lib: Arc<InstrumentationLibrary>, | ||
provider: LoggerProvider, | ||
) -> Self { | ||
otel_info!( | ||
name: "logger_new" | ||
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 probably add some identification information for the logger such as name, version etc. |
||
); | ||
Logger { | ||
instrumentation_lib, | ||
provider, | ||
|
@@ -247,6 +289,10 @@ | |
|
||
/// Emit a `LogRecord`. | ||
fn emit(&self, mut record: Self::LogRecord) { | ||
otel_debug!( | ||
name: "log_record_emit_start", | ||
record = format!("{:?}", record) | ||
); | ||
let provider = self.provider(); | ||
let processors = provider.log_processors(); | ||
let trace_context = Context::map_current(|cx| { | ||
|
@@ -280,6 +326,12 @@ | |
self.instrumentation_library().name.as_ref(), | ||
); | ||
} | ||
otel_debug!( | ||
name: "log_record_event_enabled_check", | ||
enabled_status = enabled, | ||
level = level.name(), | ||
log_target = target | ||
); | ||
enabled | ||
} | ||
} | ||
|
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 is exporter required to be passed?
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.
Forgot to remove as part of updating the method. Will cleanup in subsequent PR.