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.

Sign up for GitHub

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

Fix log-facade logging and clarify max level #454

Merged
merged 4 commits into from
Feb 4, 2025
Merged

Fix log-facade logging and clarify max level #454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb74862
Rename `level` argument to `max_log_level`
tnull Feb 1, 2025
b0370f8
f Builder docs
tnull Feb 1, 2025
5464709
Fix log-facade logging
tnull Feb 1, 2025
9f2714c
Have `set_log_facade_logger` take an *optional* `max_log_level`
tnull Feb 1, 2025
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
4 changes: 2 additions & 2 deletions bindings/ldk_node.udl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ interface Builder {
void set_gossip_source_rgs(string rgs_server_url);
void set_liquidity_source_lsps2(SocketAddress address, PublicKey node_id, string? token);
void set_storage_dir_path(string storage_dir_path);
void set_filesystem_logger(string? log_file_path, LogLevel? log_level);
void set_log_facade_logger(LogLevel log_level);
void set_filesystem_logger(string? log_file_path, LogLevel? max_log_level);
void set_log_facade_logger(LogLevel? max_log_level);
void set_custom_logger(LogWriter log_writer);
void set_network(Network network);
[Throws=BuildError]
Expand Down
47 changes: 31 additions & 16 deletions src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,20 +111,22 @@ impl Default for LiquiditySourceConfig {

#[derive(Clone)]
enum LogWriterConfig {
File { log_file_path: Option<String>, log_level: Option<LogLevel> },
Log(LogLevel),
File { log_file_path: Option<String>, max_log_level: Option<LogLevel> },
Log { max_log_level: Option<LogLevel> },
Custom(Arc<dyn LogWriter>),
}

impl std::fmt::Debug for LogWriterConfig {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
LogWriterConfig::File { log_level, log_file_path } => f
LogWriterConfig::File { max_log_level, log_file_path } => f
.debug_struct("LogWriterConfig")
.field("log_level", log_level)
.field("max_log_level", max_log_level)
.field("log_file_path", log_file_path)
.finish(),
LogWriterConfig::Log(level) => f.debug_tuple("Log").field(level).finish(),
LogWriterConfig::Log { max_log_level } => {
f.debug_tuple("Log").field(max_log_level).finish()
},
LogWriterConfig::Custom(_) => {
f.debug_tuple("Custom").field(&"<config internal to custom log writer>").finish()
},
Expand Down Expand Up @@ -331,19 +333,24 @@ impl NodeBuilder {
///
/// The `log_file_path` defaults to [`DEFAULT_LOG_FILENAME`] in the configured
/// [`Config::storage_dir_path`] if set to `None`.
/// The `log_level` defaults to [`DEFAULT_LOG_LEVEL`] if set to `None`.
///
/// If set, the `max_log_level` sets the maximum log level. Otherwise, the latter defaults to
/// [`DEFAULT_LOG_LEVEL`].
///
/// [`DEFAULT_LOG_FILENAME`]: crate::config::DEFAULT_LOG_FILENAME
pub fn set_filesystem_logger(
&mut self, log_file_path: Option<String>, log_level: Option<LogLevel>,
&mut self, log_file_path: Option<String>, max_log_level: Option<LogLevel>,
) -> &mut Self {
self.log_writer_config = Some(LogWriterConfig::File { log_file_path, log_level });
self.log_writer_config = Some(LogWriterConfig::File { log_file_path, max_log_level });
self
}

/// Configures the [`Node`] instance to write logs to the [`log`](https://crates.io/crates/log) facade.
pub fn set_log_facade_logger(&mut self, log_level: LogLevel) -> &mut Self {
self.log_writer_config = Some(LogWriterConfig::Log(log_level));
///
/// If set, the `max_log_level` sets the maximum log level. Otherwise, the latter defaults to
/// [`DEFAULT_LOG_LEVEL`].
pub fn set_log_facade_logger(&mut self, max_log_level: Option<LogLevel>) -> &mut Self {
self.log_writer_config = Some(LogWriterConfig::Log { max_log_level });
self
}

Expand Down Expand Up @@ -658,7 +665,9 @@ impl ArcedNodeBuilder {
///
/// The `log_file_path` defaults to [`DEFAULT_LOG_FILENAME`] in the configured
/// [`Config::storage_dir_path`] if set to `None`.
/// The `log_level` defaults to [`DEFAULT_LOG_LEVEL`] if set to `None`.
///
/// If set, the `max_log_level` sets the maximum log level. Otherwise, the latter defaults to
/// [`DEFAULT_LOG_LEVEL`].
///
/// [`DEFAULT_LOG_FILENAME`]: crate::config::DEFAULT_LOG_FILENAME
pub fn set_filesystem_logger(
Expand All @@ -668,7 +677,10 @@ impl ArcedNodeBuilder {
}

/// Configures the [`Node`] instance to write logs to the [`log`](https://crates.io/crates/log) facade.
pub fn set_log_facade_logger(&self, log_level: LogLevel) {
///
/// If set, the `max_log_level` sets the maximum log level. Otherwise, the latter defaults to
/// [`DEFAULT_LOG_LEVEL`].
pub fn set_log_facade_logger(&self, log_level: Option<LogLevel>) {
self.inner.write().unwrap().set_log_facade_logger(log_level);
}

Expand Down Expand Up @@ -1305,16 +1317,19 @@ fn setup_logger(
log_writer_config: &Option<LogWriterConfig>, config: &Config,
) -> Result<Arc<Logger>, BuildError> {
let logger = match log_writer_config {
Some(LogWriterConfig::File { log_file_path, log_level }) => {
Some(LogWriterConfig::File { log_file_path, max_log_level }) => {
let log_file_path = log_file_path
.clone()
.unwrap_or_else(|| format!("{}/{}", config.storage_dir_path, DEFAULT_LOG_FILENAME));
let log_level = log_level.unwrap_or_else(|| DEFAULT_LOG_LEVEL);
let max_log_level = max_log_level.unwrap_or_else(|| DEFAULT_LOG_LEVEL);

Logger::new_fs_writer(log_file_path, log_level)
Logger::new_fs_writer(log_file_path, max_log_level)
.map_err(|_| BuildError::LoggerSetupFailed)?
},
Some(LogWriterConfig::Log(log_level)) => Logger::new_log_facade(*log_level),
Some(LogWriterConfig::Log { max_log_level }) => {
let max_log_level = max_log_level.unwrap_or_else(|| DEFAULT_LOG_LEVEL);
Logger::new_log_facade(max_log_level)
Copy link
Contributor

@joostjager joostjager Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

One thing that I noticed about the log facade is that you can set log level GOSSIP, but that isn't visible anymore after this PR. The standard log only knows TRACE of course. Not a problem I think?

And perhaps some explanation on how the RUST_LOG env var interacts with this log level would be helpful too. It wasn't very clear to me how that works. Maybe there is a way to set RUST_LOG programmatically?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One thing that I noticed about the log facade is that you can set log level GOSSIP, but that isn't visible anymore after this PR. The standard log only knows TRACE of course. Not a problem I think?

Yeah, I think it's not super important. We could consider adding something special in the target or message itself, but I'd wait for a user to complain about it. So far, I assume it's fine to squash TRACE and GOSSIP together in this case.

And perhaps some explanation on how the RUST_LOG env var interacts with this log level would be helpful too. It wasn't very clear to me how that works. Maybe there is a way to set RUST_LOG programmatically?

Ah, RUST_LOG is entirely out-of-scope here, as it's a special behavior of env_logger, which is just one of the backends for the log facade. env_logger users simply need to read their docs and discover that it's mandatory to set RUST_LOG (see https://docs.rs/env_logger/latest/env_logger/#enabling-logging).

},

Some(LogWriterConfig::Custom(custom_log_writer)) => {
Logger::new_custom_writer(Arc::clone(&custom_log_writer))
Expand Down
52 changes: 24 additions & 28 deletions src/logger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,18 +106,18 @@ pub trait LogWriter: Send + Sync {
/// Defines a writer for [`Logger`].
pub(crate) enum Writer {
/// Writes logs to the file system.
FileWriter { file_path: String, level: LogLevel },
FileWriter { file_path: String, max_log_level: LogLevel },
/// Forwards logs to the `log` facade.
LogFacadeWriter { level: LogLevel },
LogFacadeWriter { max_log_level: LogLevel },
/// Forwards logs to a custom writer.
CustomWriter(Arc<dyn LogWriter>),
}

impl LogWriter for Writer {
fn log(&self, record: LogRecord) {
match self {
Writer::FileWriter { file_path, level } => {
if record.level < *level {
Writer::FileWriter { file_path, max_log_level } => {
if record.level < *max_log_level {
return;
}

Expand All @@ -138,28 +138,24 @@ impl LogWriter for Writer {
.write_all(log.as_bytes())
.expect("Failed to write to log file")
},
Writer::LogFacadeWriter { level } => {
Writer::LogFacadeWriter { max_log_level } => {
if record.level < *max_log_level {
return;
}
macro_rules! log_with_level {
($log_level:expr, $($args:tt)*) => {
($log_level:expr, $target: expr, $($args:tt)*) => {
match $log_level {
LogLevel::Gossip | LogLevel::Trace => trace!($($args)*),
LogLevel::Debug => debug!($($args)*),
LogLevel::Info => info!($($args)*),
LogLevel::Warn => warn!($($args)*),
LogLevel::Error => error!($($args)*),
LogLevel::Gossip | LogLevel::Trace => trace!(target: $target, $($args)*),
LogLevel::Debug => debug!(target: $target, $($args)*),
LogLevel::Info => info!(target: $target, $($args)*),
LogLevel::Warn => warn!(target: $target, $($args)*),
LogLevel::Error => error!(target: $target, $($args)*),
}
};
}

log_with_level!(
level,
"{} {:<5} [{}:{}] {}",
Utc::now().format("%Y-%m-%d %H:%M:%S"),
record.level,
record.module_path,
record.line,
record.args
)
let target = format!("[{}:{}]", record.module_path, record.line);
log_with_level!(record.level, &target, " {}", record.args)
},
Writer::CustomWriter(custom_logger) => custom_logger.log(record),
}
Expand All @@ -174,7 +170,7 @@ pub(crate) struct Logger {
impl Logger {
/// Creates a new logger with a filesystem writer. The parameters to this function
/// are the path to the log file, and the log level.
pub fn new_fs_writer(file_path: String, level: LogLevel) -> Result<Self, ()> {
pub fn new_fs_writer(file_path: String, max_log_level: LogLevel) -> Result<Self, ()> {
if let Some(parent_dir) = Path::new(&file_path).parent() {
fs::create_dir_all(parent_dir)
.map_err(|e| eprintln!("ERROR: Failed to create log parent directory: {}", e))?;
Expand All @@ -187,11 +183,11 @@ impl Logger {
.map_err(|e| eprintln!("ERROR: Failed to open log file: {}", e))?;
}

Ok(Self { writer: Writer::FileWriter { file_path, level } })
Ok(Self { writer: Writer::FileWriter { file_path, max_log_level } })
}

pub fn new_log_facade(level: LogLevel) -> Self {
Self { writer: Writer::LogFacadeWriter { level } }
pub fn new_log_facade(max_log_level: LogLevel) -> Self {
Self { writer: Writer::LogFacadeWriter { max_log_level } }
}

pub fn new_custom_writer(log_writer: Arc<dyn LogWriter>) -> Self {
Expand All @@ -202,14 +198,14 @@ impl Logger {
impl LdkLogger for Logger {
fn log(&self, record: LdkRecord) {
match &self.writer {
Writer::FileWriter { file_path: _, level } => {
if record.level < *level {
Writer::FileWriter { file_path: _, max_log_level } => {
if record.level < *max_log_level {
return;
}
self.writer.log(record.into());
},
Writer::LogFacadeWriter { level } => {
if record.level < *level {
Writer::LogFacadeWriter { max_log_level } => {
if record.level < *max_log_level {
return;
}
self.writer.log(record.into());
Expand Down
Loading