Skip to content

Commit

Permalink
Fix log-facade logging and clarify max level
Browse files Browse the repository at this point in the history 
Previously, the log facade logger would always use a `logger` target but
then *also* add the correct module paht and line as part of the normal
log messages. Here, we override the log target correctly.

Moreover, we previously would log all messages at the configured
*maximum* level, not the level in the log record. We therefore also fix
this and rename `level` to `max_log_level` everywhere to clear up any
confusions regarding what the variable does.
  • Loading branch information
@tnull
tnull committed Jan 31, 2025
1 parent 3b36020 commit 869bfc7
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 42 deletions.
4 changes: 2 additions & 2 deletions bindings/ldk_node.udl
View file
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
31 changes: 19 additions & 12 deletions src/builder.rs
View file
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> },
File { log_file_path: Option<String>, max_log_level: Option<LogLevel> },
Log(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
/// 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
/// defaults to [`DEFAULT_LOG_LEVEL`].
pub fn set_log_facade_logger(&mut self, max_log_level: LogLevel) -> &mut Self {
self.log_writer_config = Some(LogWriterConfig::Log(max_log_level));
self
}

Expand Down Expand Up @@ -1305,13 +1312,13 @@ 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),
Expand Down
52 changes: 24 additions & 28 deletions src/logger.rs
View file
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

0 comments on commit 869bfc7

Please sign in to comment.