Skip to content
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: rename zenoh::try_init_log_from_env to zenoh::init_logging #1165

Open
wants to merge 7 commits into
base: dev/1.0.0
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 83 additions & 41 deletions commons/zenoh-util/src/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,6 @@ pub enum LogLevel {
Off,
}

#[derive(Debug, Clone)]
pub struct InvalidLogLevel(String);

impl fmt::Display for InvalidLogLevel {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "invalid log level {:?}", self.0)
}
}

impl std::error::Error for InvalidLogLevel {}

impl FromStr for LogLevel {
type Err = InvalidLogLevel;
fn from_str(s: &str) -> Result<Self, Self::Err> {
Expand All @@ -62,6 +51,30 @@ impl FromStr for LogLevel {
}
}

impl From<LogLevel> for LevelFilter {
fn from(value: LogLevel) -> Self {
match value {
LogLevel::Trace => LevelFilter::TRACE,
LogLevel::Debug => LevelFilter::DEBUG,
LogLevel::Info => LevelFilter::INFO,
LogLevel::Warn => LevelFilter::WARN,
LogLevel::Error => LevelFilter::ERROR,
LogLevel::Off => LevelFilter::OFF,
}
}
}

#[derive(Debug, Clone)]
pub struct InvalidLogLevel(String);

impl fmt::Display for InvalidLogLevel {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "invalid log level {:?}", self.0)
}
}

impl std::error::Error for InvalidLogLevel {}

/// Initialize zenoh logging using the value of `ZENOH_LOG` environment variable.
///
/// `ZENOH_LOG` is parsed use [`LogLevel::from_str`], possible values are `"debug"`/`"INFO"`/etc.
Expand Down Expand Up @@ -209,7 +222,6 @@ pub fn init_logging_with_level(level: LogLevel) {
/// dynamic libraries. In fact, dynamic library has its own `tracing` global subscriber which need
/// to be initialized, but it would lead to a double initialization for a static library, hence
/// this fallible version.
/// Returns true if the logging was initialized.
pub fn try_init_logging() -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static>> {
let level = env::var("ZENOH_LOG")
.map(|var| var.parse().expect("invalid ZENOH_LOG"))
Expand All @@ -231,15 +243,7 @@ fn try_init_logging_with_level(
.expect("invalid RUST_LOG");
builder.with_env_filter(env_filter).try_init()
} else {
let level_filter = match level {
LogLevel::Trace => LevelFilter::TRACE,
LogLevel::Debug => LevelFilter::DEBUG,
LogLevel::Info => LevelFilter::INFO,
LogLevel::Warn => LevelFilter::WARN,
LogLevel::Error => LevelFilter::ERROR,
LogLevel::Off => LevelFilter::OFF,
};
builder.with_max_level(level_filter).try_init()
builder.with_max_level(level).try_init()
}
}

Expand Down Expand Up @@ -309,22 +313,55 @@ where
}
}

struct CallbackFilter<E, L> {
enabled: E,
max_level_hint: L,
/// An simpler version of [`tracing_subscriber::layer::Filter`].
pub trait LogFilter {
/// See [`Filter::enabled`].
fn enabled<S>(&self, meta: &Metadata, ctx: &Context<S>) -> bool;
/// See [`Filter::max_level_hint`].
fn max_level_hint(&self) -> Option<LevelFilter>;
}

impl LogFilter for LogLevel {
fn enabled<S>(&self, meta: &Metadata, _: &Context<S>) -> bool {
meta.level() < &LevelFilter::from(*self)
}

fn max_level_hint(&self) -> Option<LevelFilter> {
Some((*self).into())
}
}

impl LogFilter for LevelFilter {
fn enabled<S>(&self, meta: &Metadata, _: &Context<S>) -> bool {
meta.level() < self
}

fn max_level_hint(&self) -> Option<LevelFilter> {
Some(*self)
}
}

impl<S, E, L> Filter<S> for CallbackFilter<E, L>
impl LogFilter for EnvFilter {
fn enabled<S>(&self, meta: &Metadata, cx: &Context<S>) -> bool {
self.enabled(meta, cx.clone())
}

fn max_level_hint(&self) -> Option<LevelFilter> {
self.max_level_hint()
}
}

struct LogFilterWrapper<F>(F);

impl<F, S> Filter<S> for LogFilterWrapper<F>
where
S: Subscriber + for<'a> LookupSpan<'a>,
E: Fn(&Metadata) -> bool + 'static,
L: Fn() -> Option<LevelFilter> + 'static,
F: LogFilter,
{
fn enabled(&self, meta: &Metadata<'_>, _: &Context<'_, S>) -> bool {
(self.enabled)(meta)
fn enabled(&self, meta: &Metadata<'_>, cx: &Context<'_, S>) -> bool {
self.0.enabled(meta, cx)
}
fn max_level_hint(&self) -> Option<LevelFilter> {
(self.max_level_hint)()
self.0.max_level_hint()
}
}

Expand All @@ -333,18 +370,23 @@ where
/// This function is mainly meant to be used in zenoh bindings, to provide a bridge between Rust
/// `tracing` implementation and a native logging implementation.
///
/// To be consistent with zenoh API, bindings should parse `ZENOH_LOG` environment variable
/// to set the log level (unless it is set directly in code).
/// However, if `RUST_LOG` environment variable is provided, the callback will be skipped, and
/// [`init_logging`] called instead.
///
/// [`LogEvent`] contains more or less all the data of a `tracing` event.
/// `max_level_hint` will be called only once, and `enabled` once per callsite (span/event).
/// [`tracing::callsite::rebuild_interest_cache`] can be called to reset the cache, and have
/// `max_level_hint`/`enabled` called again.
pub fn init_log_with_callbacks(
enabled: impl Fn(&Metadata) -> bool + Send + Sync + 'static,
max_level_hint: impl Fn() -> Option<LevelFilter> + Send + Sync + 'static,
/// [`LogFilter::max_level_hint`] will be called only once, and [`LogFilter::enabled`] once
/// per callsite (span/event). [`tracing::callsite::rebuild_interest_cache`] can be called
/// to reset the cache, and have these methods called again.
pub fn init_logging_with_callback(
filter: impl LogFilter + Send + Sync + 'static,
callback: impl Fn(LogEvent) + Send + Sync + 'static,
) {
let layer = CallbackLayer(callback).with_filter(CallbackFilter {
enabled,
max_level_hint,
});
if env::var("RUST_LOG").is_ok() {
init_logging();
return;
}
let layer = CallbackLayer(callback).with_filter(LogFilterWrapper(filter));
tracing_subscriber::registry().with(layer).init();
}
4 changes: 2 additions & 2 deletions zenoh/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,9 @@ pub mod core {
}

pub mod logging {
#[cfg(feature = "internal")]
pub use zenoh_util::{init_log_with_callbacks, try_init_logging};
pub use zenoh_util::{init_logging, init_logging_with_level, InvalidLogLevel, LogLevel};
#[cfg(feature = "internal")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • better to use #[zenoh_macros::internal]
  • the function implementations should be under "internal" too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to use #[zenoh_macros::internal]

Indeed

the function implementations should be under "internal" too

Do you mean moving zenoh::logging::try_init_logging to zenoh::internal::try_init_logging?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean the implementation of internal/unstable functions are usually under "internal"/"unstable" cfg too, not only reexports.
It would be nice to check such cases automatically - generate build error if there is some public item inside which is not publicly reexported

Copy link
Contributor Author

@wyfo wyfo Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I will add the feature to zenoh-util.

pub use zenoh_util::{init_logging_with_callback, try_init_logging};
}

/// [Key expression](https://github.com/eclipse-zenoh/roadmap/blob/main/rfcs/ALL/Key%20Expressions.md) are Zenoh's address space.
Expand Down
Loading