Skip to content
Merged
Changes from 7 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
120 changes: 106 additions & 14 deletions cursive-core/src/logger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,107 @@

use lazy_static::lazy_static;
use std::collections::VecDeque;
use std::str::FromStr;
use std::sync::Mutex;

/// Saves all log records in a global deque.
///
/// Uses a `DebugView` to access it.
pub struct CursiveLogger;
///
/// # Examples
///
/// Set log levels from env vars
///
/// ```
/// # use cursive_core::logger::CursiveLogger;
/// CursiveLogger::new()
/// .with_env()
/// .init();
/// ```
///
/// Set log levels explicitly.
///
/// ```
/// # use cursive_core::logger::CursiveLogger;
/// # use log::LevelFilter;
/// CursiveLogger::new()
/// .with_int_filter_level(LevelFilter::Warn)
/// .with_ext_filter_level(LevelFilter::Debug)
/// .init();
/// ```
///
/// Set log queue size.
///
/// ```
/// # use cursive_core::logger::CursiveLogger;
/// CursiveLogger::new()
/// .with_log_size(10_000)
/// .init();
/// ```
pub struct CursiveLogger {
// Log filter level for log messages from within cursive
int_filter_level: log::LevelFilter,
// Log filter level for log messages from sources outside of cursive
ext_filter_level: log::LevelFilter,
// Size of log queue
log_size: usize,
}

impl CursiveLogger {
/// Creates a new CursiveLogger with default log filter levels of `log::LevelFilter::Trace`.
/// Remember to call `init()` to install with `log` backend.
pub fn new() -> Self {
CursiveLogger {
int_filter_level: log::LevelFilter::Trace,
ext_filter_level: log::LevelFilter::Trace,
log_size: 1000,
}
}

/// Sets the internal log filter level.
pub fn with_int_filter_level(mut self, level: log::LevelFilter) -> Self {
self.int_filter_level = level;
self
}

/// Sets the external log filter level.
pub fn with_ext_filter_level(mut self, level: log::LevelFilter) -> Self {
self.ext_filter_level = level;
self
}

static LOGGER: CursiveLogger = CursiveLogger;
/// Sets log filter levels based on environment variables `RUST_LOG` and `CURSIVE_LOG`.
/// If `RUST_LOG` is set, then both internal and external log levels are set to match.
/// If `CURSIVE_LOG` is set, then the internal log level is set to match with precedence over
/// `RUST_LOG`.
pub fn with_env(mut self) -> Self {
if let Ok(rust_log) = std::env::var("RUST_LOG") {
if let Ok(filter_level) = log::LevelFilter::from_str(&rust_log) {
self.int_filter_level = filter_level;
self.ext_filter_level = filter_level;
}
}
if let Ok(cursive_log) = std::env::var("CURSIVE_LOG") {
if let Ok(filter_level) = log::LevelFilter::from_str(&cursive_log) {
self.int_filter_level = filter_level;
}
}
self
}

/// Sets the size of the log queue
pub fn with_log_size(mut self, log_size: usize) -> Self {
self.log_size = log_size;
self
}

/// Installs the logger with log. Calling twice will panic.
pub fn init(self) {
reserve_logs(self.log_size);
log::set_logger(Box::leak(Box::new(self))).unwrap();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We may want to use set_boxed_logger, which hides the leaking away so it feels less dirty (even though it's really the same):
https://docs.rs/log/latest/log/fn.set_boxed_logger.html

I guess a "leak-free" version would keep CursiveLogger zero-sized and rely on a global/singleton state instead, but that may be more complex, with quite limited benefits (prevents leaks if init() is called repeatedly).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I originally had used set_boxed_logger but that's hidden behind the std feature flag for log and I didn't want to change the dependencies unless I had to.

As written, calling init() multiple times panics anyways, so I don't think we have to worry about leaks. Having everything kept in global state might not be a bad idea since we shouldn't expect anyone to create multiple CursiveLogger objects anyways.

I made another branch for this: cursivelogger_global. It is a bit more complicated and I don't know if I like the API ergonomics. One plus though is being able to change the log filter levels after initialization. I don't know if that's worth it.

@gyscos gyscos Mar 8, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why the Mutex<Cell<...>> in that branch? Could a simple mutex work too? Or even RwLock, might make the read-only case faster.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think I like this global branch! No feature change for the log dependency, uses lazy_static that we were already using...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, both work, you're right. I don't know what I was thinking with Mutex<Cell<...>>. Since LOGS is a Mutex anyway I'm not sure there's much of a speed difference.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I created a PR for cursivelogger_global. There is a tiny issue with set_log_size though.

log::set_max_level(log::LevelFilter::Trace);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We could set the maximum of (int, ext) levels to avoid logging trace if it wouldn't be seen anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

}
}

/// A log record.
pub struct Record {
Expand Down Expand Up @@ -44,12 +137,18 @@ pub fn log(record: &log::Record) {
}

impl log::Log for CursiveLogger {
fn enabled(&self, _metadata: &log::Metadata) -> bool {
true
fn enabled(&self, metadata: &log::Metadata) -> bool {
if metadata.target().contains("cursive_core") {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Maybe we should be more restrictive, requiring this to be a prefix of the target?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

metadata.level() <= self.int_filter_level
} else {
metadata.level() <= self.ext_filter_level
}
}

fn log(&self, record: &log::Record) {
log(record);
if self.enabled(record.metadata()) {
log(record);
}
}

fn flush(&self) {}
Expand All @@ -62,14 +161,8 @@ impl log::Log for CursiveLogger {
/// Use a [`DebugView`](crate::views::DebugView) to see the logs, or use
/// [`Cursive::toggle_debug_console()`](crate::Cursive::toggle_debug_console()).
pub fn init() {
// TODO: Configure the deque size?
reserve_logs(1_000);

// This will panic if `set_logger` was already called.
log::set_logger(&LOGGER).unwrap();

// TODO: read the level from env variable? From argument?
log::set_max_level(log::LevelFilter::Trace);
CursiveLogger::new().init();
}

/// Return a logger that stores records in cursive's log queue.
Expand All @@ -78,8 +171,7 @@ pub fn init() {
///
/// An easier alternative might be to use [`init()`].
pub fn get_logger() -> CursiveLogger {
reserve_logs(1_000);
CursiveLogger
CursiveLogger::new()
}

/// Adds `n` more entries to cursive's log queue.
Expand Down