refactor: update FilesystemLogWriter to FileWriter

1. Remove left-over log_dir & log_file_path related fields from Config
2. Rename log_dir to log_file_path
3. Update the creation of new FileWriter to handle recursive parent directory
4. Fix integration tests

Author: enigbe

src/builder.rs

@@ -18,9 +18,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::{
-	default_format, log_error, log_info, FilesystemLogWriter, LdkNodeLogger, Logger,
-};
+use crate::logger::{default_format, log_error, log_info, FileWriter, LdkNodeLogger, Logger};
 use crate::message_handler::NodeCustomMessageHandler;
 use crate::payment::store::PaymentStore;
 use crate::peer_store::PeerStore;
@@ -302,12 +300,6 @@ impl NodeBuilder {
 		self
 	}
 
-	/// Sets the log file path if the log file needs to live separate from the storage directory path.
-	pub fn set_log_file_path(&mut self, log_dir_path: String) -> &mut Self {
-		self.config.log_file_path = Some(log_dir_path);
-		self
-	}
-
 	/// Sets the Bitcoin network used.
 	pub fn set_network(&mut self, network: Network) -> &mut Self {
 		self.config.network = network;
@@ -608,11 +600,6 @@ impl ArcedNodeBuilder {
 		self.inner.write().unwrap().set_storage_dir_path(storage_dir_path);
 	}
 
-	/// Sets the log file path if logs need to live separate from the storage directory path.
-	pub fn set_log_file_path(&self, log_file_path: String) {
-		self.inner.write().unwrap().set_log_file_path(log_file_path);
-	}
-
 	/// Sets the Bitcoin network used.
 	pub fn set_network(&self, network: Network) {
 		self.inner.write().unwrap().set_network(network);
@@ -633,11 +620,6 @@ impl ArcedNodeBuilder {
 		self.inner.write().unwrap().set_node_alias(node_alias).map(|_| ())
 	}
 
-	/// Sets the level at which [`Node`] will log messages.
-	pub fn set_log_level(&self, level: LogLevel) {
-		self.inner.write().unwrap().set_log_level(level);
-	}
-
 	/// Builds a [`Node`] instance with a [`SqliteStore`] backend and according to the options
 	/// previously configured.
 	pub fn build(&self) -> Result<Arc<Node>, BuildError> {
@@ -1234,8 +1216,8 @@ fn build_with_store_internal(
 fn setup_logger(config: &Config) -> Result<Arc<LdkNodeLogger>, BuildError> {
 	match config.logging_config {
 		LoggingConfig::Custom(ref logger) => Ok(logger.clone()),
-		LoggingConfig::Filesystem { ref log_dir, log_level } => {
-			let filesystem_log_writer = FilesystemLogWriter::new(log_dir.clone())
+		LoggingConfig::Filesystem { ref log_file_path, log_level } => {
+			let filesystem_log_writer = FileWriter::new(log_file_path.clone())
 				.map_err(|_| BuildError::LoggerSetupFailed)?;
 			Ok(Arc::new(
 				LdkNodeLogger::new(

src/config.rs

@@ -23,7 +23,7 @@ use bitcoin::Network;
 use std::time::Duration;
 
 // Config defaults
-const DEFAULT_STORAGE_DIR_PATH: &str = "/tmp/ldk_node/";
+const DEFAULT_STORAGE_DIR_PATH: &str = "/tmp/ldk_node";
 const DEFAULT_NETWORK: Network = Network::Bitcoin;
 const DEFAULT_BDK_WALLET_SYNC_INTERVAL_SECS: u64 = 80;
 const DEFAULT_LDK_WALLET_SYNC_INTERVAL_SECS: u64 = 30;
@@ -104,12 +104,7 @@ pub(crate) const WALLET_KEYS_SEED_LEN: usize = 64;
 pub struct Config {
 	/// The path where the underlying LDK and BDK persist their data.
 	pub storage_dir_path: String,
-	/// The path where logs are stored.
-	///
-	/// If set to `None`, logs can be found in `ldk_node.log` in the [`Config::storage_dir_path`]
-	/// directory.
-	pub log_file_path: Option<String>,
-	/// In the default configuration logs can be found in the `logs` subdirectory in
+	/// In the default configuration logs can be found in the [`DEFAULT_STORAGE_DIR_PATH`] subdirectory in
 	/// [`Config::storage_dir_path`], and the log level is set to [`DEFAULT_LOG_LEVEL`].
 	pub logging_config: LoggingConfig,
 	/// The used Bitcoin network.
@@ -168,7 +163,6 @@ impl Default for Config {
 	fn default() -> Self {
 		Self {
 			storage_dir_path: DEFAULT_STORAGE_DIR_PATH.to_string(),
-			log_file_path: None,
 			logging_config: LoggingConfig::default(),
 			network: DEFAULT_NETWORK,
 			listening_addresses: None,
@@ -190,7 +184,7 @@ pub enum LoggingConfig {
 	/// most recent log file, which is created and timestamped at initialization.
 	Filesystem {
 		/// The absolute path where logs are stored.
-		log_dir: String,
+		log_file_path: String,
 		/// The level at which we log messages.
 		///
 		/// Any messages below this level will be excluded from the logs.
@@ -203,7 +197,7 @@ pub enum LoggingConfig {
 impl Default for LoggingConfig {
 	fn default() -> Self {
 		Self::Filesystem {
-			log_dir: format!("{}/{}", DEFAULT_STORAGE_DIR_PATH, "logs"),
+			log_file_path: format!("{}/{}", DEFAULT_STORAGE_DIR_PATH, "ldk_node.log"),
 			log_level: DEFAULT_LOG_LEVEL,
 		}
 	}

src/logger.rs

@@ -15,6 +15,7 @@ use chrono::Utc;
 use std::fmt::Debug;
 use std::fs;
 use std::io::Write;
+use std::path::Path;
 use std::sync::Mutex;
 
 pub struct LdkNodeLogger {
@@ -47,20 +48,27 @@ impl Logger for LdkNodeLogger {
 	}
 }
 
-pub(crate) struct FilesystemLogWriter {
+pub(crate) struct FileWriter {
 	log_file: Mutex<fs::File>,
 }
 
-impl FilesystemLogWriter {
-	/// Creates a new filesystem logger.
+impl FileWriter {
+	/// Creates a new filesystem writer.
 	pub(crate) fn new(log_file_path: String) -> Result<Self, ()> {
+		if let Some(parent_dir) = Path::new(&log_file_path).parent() {
+			fs::create_dir_all(parent_dir).map_err(|e| {
+				eprintln!("ERROR: Failed to create log file directory: {}", e);
+				()
+			})?;
+		}
+
 		let log_file = Mutex::new(
-			fs::OpenOptions::new()
-				.create(true)
-				.append(true)
-				.open(log_file_path.clone())
-				.map_err(|e| eprintln!("ERROR: Failed to open log file: {}", e))?,
+			fs::OpenOptions::new().create(true).append(true).open(&log_file_path).map_err(|e| {
+				eprintln!("ERROR: Failed to open log file: {}", e);
+				()
+			})?,
 		);
+
 		Ok(Self { log_file })
 	}
 

tests/common/mod.rs

@@ -11,7 +11,7 @@
 use ldk_node::config::{Config, EsploraSyncConfig};
 use ldk_node::io::sqlite_store::SqliteStore;
 use ldk_node::payment::{PaymentDirection, PaymentKind, PaymentStatus};
-use ldk_node::{Builder, Event, LightningBalance, LogLevel, Node, NodeError, PendingSweepBalance};
+use ldk_node::{Builder, Event, LightningBalance, Node, NodeError, PendingSweepBalance};
 
 use lightning::ln::msgs::SocketAddress;
 use lightning::ln::{PaymentHash, PaymentPreimage};
@@ -231,8 +231,6 @@ pub(crate) fn random_config(anchor_channels: bool) -> Config {
 	println!("Setting random LDK node alias: {:?}", alias);
 	config.node_alias = alias;
 
-	config.log_level = LogLevel::Gossip;
-
 	config
 }
 

tests/integration_tests_rust.rs

@@ -234,8 +234,13 @@ fn start_stop_reinit() {
 	node.sync_wallets().unwrap();
 	assert_eq!(node.list_balances().spendable_onchain_balance_sats, expected_amount.to_sat());
 
-	let log_file = format!("{}/ldk_node.log", config.clone().storage_dir_path);
-	assert!(std::path::Path::new(&log_file).exists());
+	let log_conf = &config.logging_config;
+	match log_conf {
+		ldk_node::config::LoggingConfig::Filesystem { log_file_path, log_level: _ } => {
+			assert!(std::path::Path::new(&log_file_path).exists());
+		},
+		ldk_node::config::LoggingConfig::Custom(_) => (),
+	}
 
 	node.stop().unwrap();
 	assert_eq!(node.stop(), Err(NodeError::NotRunning));