refactor: simplify log-related code and improve clarity

This commit includes several improvements aimed at simplifying code, improving readability, and optimizing performance:

- Inline enum struct fields to reduce indirection and clutter.
- Rename structs to improve clarity and better reflect their purpose.
- Enable specific feature flags in dependencies for a more streamlined build.
- Eliminate unnecessary data allocations to optimize memory usage.
- Correct minor grammatical error in documentation.

Author: enigbe

Cargo.toml

@@ -75,7 +75,7 @@ libc = "0.2"
 uniffi = { version = "0.27.3", features = ["build"], optional = true }
 serde = { version = "1.0.210", default-features = false, features = ["std", "derive"] }
 serde_json = { version = "1.0.128", default-features = false, features = ["std"] }
-log = { version = "0.4.22", features = ["std"]}
+log = { version = "0.4.22", default-features = false, features = ["std"]}
 
 vss-client = "0.3"
 prost = { version = "0.11.6", default-features = false}

bindings/ldk_node.udl

@@ -35,12 +35,8 @@ enum LogLevel {
 };
 
 dictionary FilesystemLoggerConfig {
-	string log_file_path;
-	LogLevel level;
-};
-
-dictionary LogFacadeLoggerConfig {
-	LogLevel level;
+	string? log_file_path;
+	LogLevel? log_level;
 };
 
 interface Builder {
@@ -58,7 +54,7 @@ interface Builder {
 	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(FilesystemLoggerConfig fs_config);
-	void set_log_facade_logger(LogFacadeLoggerConfig lf_config);
+	void set_log_facade_logger(LogLevel log_level);
 	void set_network(Network network);
 	[Throws=BuildError]
 	void set_listening_addresses(sequence<SocketAddress> listening_addresses);

src/builder.rs

@@ -7,8 +7,8 @@
 
 use crate::chain::{ChainSource, DEFAULT_ESPLORA_SERVER_URL};
 use crate::config::{
-	default_user_config, Config, EsploraSyncConfig, FilesystemLoggerConfig, LogFacadeLoggerConfig,
-	WALLET_KEYS_SEED_LEN,
+	default_log_file_path, default_log_level, default_user_config, Config, EsploraSyncConfig,
+	FilesystemLoggerConfig, WALLET_KEYS_SEED_LEN,
 };
 
 use crate::connection::ConnectionManager;
@@ -19,7 +19,7 @@ use crate::io::sqlite_store::SqliteStore;
 use crate::io::utils::{read_node_metrics, write_node_metrics};
 use crate::io::vss_store::VssStore;
 use crate::liquidity::LiquiditySource;
-use crate::logger::{log_error, log_info, LdkLogger, LogWriter, Logger};
+use crate::logger::{log_error, log_info, LdkLogger, LogLevel, LogWriter, Logger};
 use crate::message_handler::NodeCustomMessageHandler;
 use crate::payment::store::PaymentStore;
 use crate::peer_store::PeerStore;
@@ -109,10 +109,10 @@ impl Default for LiquiditySourceConfig {
 	}
 }
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 enum LogWriterConfig {
 	File(FilesystemLoggerConfig),
-	Log(LogFacadeLoggerConfig),
+	Log(LogLevel),
 	Custom(Arc<dyn LogWriter + Send + Sync>),
 }
 
@@ -324,8 +324,8 @@ impl NodeBuilder {
 	}
 
 	/// Configures the [`Node`] instance to write logs to the `log` facade.
-	pub fn set_log_facade_logger(&mut self, lf_config: LogFacadeLoggerConfig) -> &mut Self {
-		self.log_writer_config = Some(LogWriterConfig::Log(lf_config));
+	pub fn set_log_facade_logger(&mut self, log_level: LogLevel) -> &mut Self {
+		self.log_writer_config = Some(LogWriterConfig::Log(log_level));
 		self
 	}
 
@@ -416,9 +416,7 @@ impl NodeBuilder {
 	) -> Result<Node, BuildError> {
 		use bitcoin::key::Secp256k1;
 
-		let writer = LogWriterConfig::default();
-		let log_writer_config =
-			if let Some(config) = &self.log_writer_config { config } else { &writer };
+		let log_writer_config = self.log_writer_config.clone().unwrap_or_default();
 		let logger = setup_logger(&log_writer_config)?;
 
 		let seed_bytes = seed_bytes_from_config(
@@ -484,9 +482,7 @@ impl NodeBuilder {
 	pub fn build_with_vss_store_and_header_provider(
 		&self, vss_url: String, store_id: String, header_provider: Arc<dyn VssHeaderProvider>,
 	) -> Result<Node, BuildError> {
-		let writer = LogWriterConfig::default();
-		let log_writer_config =
-			if let Some(config) = &self.log_writer_config { config } else { &writer };
+		let log_writer_config = self.log_writer_config.clone().unwrap_or_default();
 		let logger = setup_logger(&log_writer_config)?;
 
 		let seed_bytes = seed_bytes_from_config(
@@ -519,9 +515,7 @@ impl NodeBuilder {
 
 	/// Builds a [`Node`] instance according to the options previously configured.
 	pub fn build_with_store(&self, kv_store: Arc<DynStore>) -> Result<Node, BuildError> {
-		let writer = LogWriterConfig::default();
-		let log_writer_config =
-			if let Some(config) = &self.log_writer_config { config } else { &writer };
+		let log_writer_config = self.log_writer_config.clone().unwrap_or_default();
 		let logger = setup_logger(&log_writer_config)?;
 
 		let seed_bytes = seed_bytes_from_config(
@@ -651,8 +645,8 @@ impl ArcedNodeBuilder {
 	}
 
 	/// Configures the [`Node`] instance to write logs to the `log` facade.
-	pub fn set_log_facade_logger(&self, lf_config: LogFacadeLoggerConfig) {
-		self.inner.write().unwrap().set_log_facade_logger(lf_config);
+	pub fn set_log_facade_logger(&self, log_level: LogLevel) {
+		self.inner.write().unwrap().set_log_facade_logger(log_level);
 	}
 
 	/// Configures the [`Node`] instance to write logs to the provided custom log writer.
@@ -1275,16 +1269,20 @@ fn build_with_store_internal(
 fn setup_logger(config: &LogWriterConfig) -> Result<Arc<Logger>, BuildError> {
 	match config {
 		LogWriterConfig::File(fs_logger_config) => {
-			let log_file_path = &fs_logger_config.log_file_path;
+			let log_file_path = if let Some(fp) = &fs_logger_config.log_file_path {
+				fp
+			} else {
+				&default_log_file_path()
+			};
+			let log_level = fs_logger_config.log_level.unwrap_or(default_log_level());
 
 			Ok(Arc::new(
-				Logger::new_fs_writer(log_file_path.to_string(), fs_logger_config.level)
+				Logger::new_fs_writer(log_file_path, log_level)
 					.map_err(|_| BuildError::LoggerSetupFailed)?,
 			))
 		},
-		LogWriterConfig::Log(log_facade_logger_config) => Ok(Arc::new(
-			Logger::new_log_facade(log_facade_logger_config.level)
-				.map_err(|_| BuildError::LoggerSetupFailed)?,
+		LogWriterConfig::Log(log_level) => Ok(Arc::new(
+			Logger::new_log_facade(*log_level).map_err(|_| BuildError::LoggerSetupFailed)?,
 		)),
 		LogWriterConfig::Custom(custom_log_writer) => Ok(Arc::new(
 			Logger::new_custom_writer(custom_log_writer.clone())

src/config.rs

@@ -435,26 +435,31 @@ pub struct FilesystemLoggerConfig {
 	/// The log file path.
 	///
 	/// This specifies the log file path if a destination other than the storage
-	/// directory, i.e. [`Config::storage_dir_path`], is preferred.
-	pub log_file_path: String,
+	/// directory, i.e. [`Config::storage_dir_path`], is preferred. If unconfigured,
+	/// defaults to [`DEFAULT_STORAGE_DIR_PATH`]/[`DEFAULT_LOG_FILE_PATH`], i.e.
+	/// `/tmp/ldk_node/ldk_node.log`
+	pub log_file_path: Option<String>,
 	/// This specifies the log level.
-	pub level: LogLevel,
-}
-
-/// Configuration options for logging to the `log` facade.
-#[derive(Debug, Clone)]
-pub struct LogFacadeLoggerConfig {
-	/// This specifies the log level.
-	pub level: LogLevel,
+	///
+	/// If unconfigured, defaults to [`DEFAULT_LOG_LEVEL`], i.e. `LogLevel::Debug`
+	pub log_level: Option<LogLevel>,
 }
 
 impl Default for FilesystemLoggerConfig {
 	fn default() -> Self {
-		let log_file_path = format!("{}/{}", DEFAULT_STORAGE_DIR_PATH, DEFAULT_LOG_FILE_PATH);
-		Self { log_file_path, level: DEFAULT_LOG_LEVEL }
+		let log_file_path = default_log_file_path();
+		Self { log_file_path: Some(log_file_path), log_level: Some(default_log_level()) }
 	}
 }
 
+pub(crate) fn default_log_file_path() -> String {
+	format!("{}/{}", DEFAULT_STORAGE_DIR_PATH, DEFAULT_LOG_FILE_PATH)
+}
+
+pub(crate) fn default_log_level() -> LogLevel {
+	DEFAULT_LOG_LEVEL
+}
+
 #[cfg(test)]
 mod tests {
 	use std::str::FromStr;

src/lib.rs

@@ -85,7 +85,7 @@ pub mod graph;
 mod hex_utils;
 pub mod io;
 mod liquidity;
-mod logger;
+pub mod logger;
 mod message_handler;
 pub mod payment;
 mod peer_store;
@@ -110,8 +110,7 @@ pub use event::Event;
 
 pub use io::utils::generate_entropy_mnemonic;
 
-pub use config::{FilesystemLoggerConfig, LogFacadeLoggerConfig};
-pub use logger::{LogLevel, LogRecord, LogWriter};
+pub use config::FilesystemLoggerConfig;
 
 #[cfg(feature = "uniffi")]
 use uniffi_types::*;
@@ -146,6 +145,7 @@ use types::{
 pub use types::{ChannelDetails, CustomTlvRecord, PeerDetails, UserChannelId};
 
 use logger::{log_error, log_info, log_trace, LdkLogger, Logger};
+pub use logger::{LogLevel, LogRecord, LogWriter};
 
 use lightning::chain::BestBlock;
 use lightning::events::bump_transaction::Wallet as LdkWallet;

src/logger.rs

@@ -5,6 +5,8 @@
 // http://opensource.org/licenses/MIT>, at your option. You may not use this file except in
 // accordance with one or both of these licenses.
 
+//! Logging-related objects.
+
 pub(crate) use lightning::util::logger::{Logger as LdkLogger, Record};
 pub(crate) use lightning::{log_bytes, log_debug, log_error, log_info, log_trace};
 
@@ -50,55 +52,43 @@ pub trait LogWriter: Send + Sync + Debug {
 	fn log(&self, record: LogRecord);
 }
 
-#[derive(Debug)]
-pub(crate) struct FilesystemLogger {
-	file_path: String,
-	level: LogLevel,
-}
-
-#[derive(Debug)]
-pub(crate) struct LogFacadeLogger {
-	level: LogLevel,
-}
-
 /// Defines a writer for [`Logger`].
 #[derive(Debug)]
 pub(crate) enum Writer {
 	/// Writes logs to the file system.
-	FileWriter(FilesystemLogger),
+	FileWriter { file_path: String, level: LogLevel },
 	/// Forwards logs to the `log` facade.
-	LogFacadeWriter(LogFacadeLogger),
-	/// Forwards logs to custom writer.
+	LogFacadeWriter { level: LogLevel },
+	/// Forwards logs to a custom writer.
 	CustomWriter(Arc<dyn LogWriter + Send + Sync>),
 }
 
 impl LogWriter for Writer {
 	fn log(&self, record: LogRecord) {
-		let raw_log = record.args.to_string();
 		let log = format!(
 			"{} {:<5} [{}:{}] {}\n",
 			Utc::now().format("%Y-%m-%d %H:%M:%S"),
 			record.level.to_string(),
 			record.module_path,
 			record.line,
-			raw_log
+			record.args
 		);
 
 		match self {
-			Writer::FileWriter(fs_logger) => {
-				if record.level < fs_logger.level {
+			Writer::FileWriter { file_path, level } => {
+				if record.level < *level {
 					return;
 				}
 
 				fs::OpenOptions::new()
 					.create(true)
 					.append(true)
-					.open(fs_logger.file_path.clone())
+					.open(file_path.clone())
 					.expect("Failed to open log file")
 					.write_all(log.as_bytes())
 					.expect("Failed to write to log file")
 			},
-			Writer::LogFacadeWriter(log_facade_logger) => match log_facade_logger.level {
+			Writer::LogFacadeWriter { level } => match level {
 				LogLevel::Gossip => trace!("{}", log),
 				LogLevel::Trace => trace!("{}", log),
 				LogLevel::Debug => debug!("{}", log),
@@ -119,28 +109,24 @@ 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(log_file_path: String, level: LogLevel) -> Result<Self, ()> {
-		if let Some(parent_dir) = Path::new(&log_file_path).parent() {
+	pub fn new_fs_writer(file_path: &str, 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))?;
 
 			// make sure the file exists.
 			fs::OpenOptions::new()
 				.create(true)
 				.append(true)
-				.open(&log_file_path)
+				.open(&file_path)
 				.map_err(|e| eprintln!("ERROR: Failed to open log file: {}", e))?;
 		}
 
-		let fs_writer = FilesystemLogger { file_path: log_file_path, level };
-
-		Ok(Self { writer: Writer::FileWriter(fs_writer) })
+		Ok(Self { writer: Writer::FileWriter { file_path: file_path.to_string(), level } })
 	}
 
 	pub fn new_log_facade(level: LogLevel) -> Result<Self, ()> {
-		let log_facade_writer = LogFacadeLogger { level };
-
-		Ok(Self { writer: Writer::LogFacadeWriter(log_facade_writer) })
+		Ok(Self { writer: Writer::LogFacadeWriter { level } })
 	}
 
 	pub fn new_custom_writer(log_writer: Arc<dyn LogWriter + Send + Sync>) -> Result<Self, ()> {
@@ -150,6 +136,22 @@ impl Logger {
 
 impl LdkLogger for Logger {
 	fn log(&self, record: Record) {
-		self.writer.log(record.into());
+		match &self.writer {
+			Writer::FileWriter { file_path: _, level } => {
+				if record.level < *level {
+					return;
+				}
+				self.writer.log(record.into());
+			},
+			Writer::LogFacadeWriter { level } => {
+				if record.level < *level {
+					return;
+				}
+				self.writer.log(record.into());
+			},
+			Writer::CustomWriter(_arc) => {
+				self.writer.log(record.into());
+			},
+		}
 	}
 }

tests/common/mod.rs

@@ -11,10 +11,11 @@
 use chrono::Utc;
 use ldk_node::config::{Config, EsploraSyncConfig};
 use ldk_node::io::sqlite_store::SqliteStore;
+use ldk_node::logger::{LogLevel, LogRecord, LogWriter};
 use ldk_node::payment::{PaymentDirection, PaymentKind, PaymentStatus};
 use ldk_node::{
-	Builder, CustomTlvRecord, Event, FilesystemLoggerConfig, LightningBalance,
-	LogFacadeLoggerConfig, LogRecord, LogWriter, Node, NodeError, PendingSweepBalance,
+	Builder, CustomTlvRecord, Event, FilesystemLoggerConfig, LightningBalance, Node, NodeError,
+	PendingSweepBalance,
 };
 
 use lightning::ln::msgs::SocketAddress;
@@ -254,7 +255,7 @@ pub(crate) enum TestChainSource<'a> {
 #[derive(Clone)]
 pub(crate) enum TestLogWriter {
 	File(FilesystemLoggerConfig),
-	LogFacade(LogFacadeLoggerConfig),
+	LogFacade(LogLevel),
 	Custom(Arc<dyn LogWriter + Send + Sync>),
 }
 
@@ -387,8 +388,8 @@ pub(crate) fn setup_node(
 		TestLogWriter::File(fs_config) => {
 			builder.set_filesystem_logger(fs_config);
 		},
-		TestLogWriter::LogFacade(lf_config) => {
-			builder.set_log_facade_logger(lf_config);
+		TestLogWriter::LogFacade(log_level) => {
+			builder.set_log_facade_logger(log_level);
 		},
 		TestLogWriter::Custom(log_writer) => {
 			builder.set_custom_logger(log_writer);