Add the date when the node was started to the name of the logfile. #116

ulrichard · 2023-06-02T15:18:05Z

This makes it easier to delete old logs

tnull

Good point, we def. should look into log rotation. While splitting by startup date ist straightforward, it doesn't protect us from hug log files in case of long-running instances.
That said, we can probably start out with this approach and look into 'proper' log rotation in the future.

src/lib.rs

tnull · 2023-06-05T11:54:52Z

src/lib.rs

@@ -383,8 +383,15 @@ impl Builder {
 		let bdk_data_dir = format!("{}/bdk", config.storage_dir_path);
 		fs::create_dir_all(bdk_data_dir.clone()).expect("Failed to create BDK data directory");

+		let logs_dir = format!("{}/logs", config.storage_dir_path);
+		fs::create_dir_all(logs_dir.clone()).expect("Failed to create logs directory");


Ah, I think this is superfluous actually, as we're creating the directory in the logger directly:

ldk-node/src/logger.rs

Lines 17 to 24 in 427e74c

impl FilesystemLogger {

pub(crate) fn new(file_path: String, level: Level) -> Self {

if let Some(parent_dir) = Path::new(&file_path).parent() {

fs::create_dir_all(parent_dir).expect("Failed to create log parent directory");

}

Self { file_path, level }

}

}

tnull

LGTM, mod nit and one comment.

Happy to land without the symlink part though, leave it up to you whether you want to include it.

tnull · 2023-06-05T12:03:29Z

src/lib.rs

 		// Initialize the Logger
-		let log_file_path = format!("{}/ldk_node.log", config.storage_dir_path);
+		let log_file_path = format!(
+			"{}/ldk_node_{}.log",


It might also be nice to create an symlink from ldk_node_latest.log or similar to the most-recent logfile, which would easily allow to track the latest one.

I like the idea of the symlink, but it looks like there is no universal cross platform API for this. https://doc.rust-lang.org/std/fs/fn.soft_link.html I don't know what platforms are supported by ldk-node. And handling multiple variants doesn't seem wroth the effort at the moment.

Mh, yes, I think we can just go with with the unix variant as Windows builds might already be broken. Happy to look into it further if we get a user request for Windows, but until then we should just assume we're on some type of UNIX.

src/lib.rs

tnull · 2023-06-05T15:42:58Z

src/lib.rs

@@ -1743,7 +1756,7 @@ impl<K: KVStore + Sync + Send + 'static> Node<K> {
 	/// # use ldk_node::bitcoin::Network;
 	/// # let mut config = Config::default();
 	/// # config.network = Network::Regtest;
-	/// # config.storage_dir_path = "/tmp/ldk_node_test/".to_string();
+	/// # config.storage_dir_path = "/tmp/ldk_node_test_list_payments".to_string();


This change seems unrelated and should be removed.

tnull · 2023-06-05T15:44:10Z

src/lib.rs

+		);
+		let logger = Arc::new(FilesystemLogger::new(log_file_path.clone(), config.log_level));
+		let log_file_symlink = format!("{}/ldk_node.log", config.storage_dir_path);
+		if Path::new(&log_file_symlink).exists() {


Can we make the is_symlink check here, too, so we won't remove anything but the symlink?

tnull

Generally looks good, but I'd prefer to have the symlink logic in the logger to have it one place.

Also, this needs a rebase now I'm afraid. Sorry!

tnull · 2023-06-06T07:05:54Z

src/lib.rs

@@ -1743,7 +1764,7 @@ impl<K: KVStore + Sync + Send + 'static> Node<K> {
 	/// # use ldk_node::bitcoin::Network;
 	/// # let mut config = Config::default();
 	/// # config.network = Network::Regtest;
-	/// # config.storage_dir_path = "/tmp/ldk_node_test/".to_string();
+	/// # config.storage_dir_path = "/tmp/ldk_node_test".to_string();


nit: I think this change is still not necessary.

tnull · 2023-06-06T07:12:08Z

src/logger.rs

@@ -19,6 +19,12 @@ impl FilesystemLogger {
 		if let Some(parent_dir) = Path::new(&file_path).parent() {
 			fs::create_dir_all(parent_dir).expect("Failed to create log parent directory");
 		}
+		// make sure the file exists, so that the symlink has something to point to.
+		fs::OpenOptions::new()


Mh, can we then move the symlink logic also here? Also, lets keep all logs (including the symlink) in the logs folder, so just have ldk_node_latest.log in there point to the most recent one.

This makes it easier to delete old logs Create a symlink to the latest log file.

tnull

LGTM!

) This commit reverts the changes introduce by PR lightningdevkit#116 which added the date to log files when the node is started & a symlink to track the latest log file. This reversal is necessary to simplify the work required to integrate with OS-level log tools.

ulrichard force-pushed the feature/log_per_date branch 2 times, most recently from 54a39cb to 86f32b0 Compare June 5, 2023 08:13

tnull reviewed Jun 5, 2023

View reviewed changes

src/lib.rs Show resolved Hide resolved

ulrichard force-pushed the feature/log_per_date branch from 86f32b0 to e72792e Compare June 5, 2023 11:51

tnull reviewed Jun 5, 2023

View reviewed changes

ulrichard force-pushed the feature/log_per_date branch from e72792e to e13fb20 Compare June 5, 2023 12:02

tnull reviewed Jun 5, 2023

View reviewed changes

ulrichard force-pushed the feature/log_per_date branch 2 times, most recently from 71ff89c to cef2e4b Compare June 5, 2023 15:06

tnull reviewed Jun 5, 2023

View reviewed changes

ulrichard force-pushed the feature/log_per_date branch from cef2e4b to ff8c039 Compare June 5, 2023 18:57

tnull reviewed Jun 6, 2023

View reviewed changes

ulrichard force-pushed the feature/log_per_date branch from ff8c039 to ba3fc5c Compare June 6, 2023 14:30

Add the date when the node was started to the name of the logfile.

2f087c8

This makes it easier to delete old logs Create a symlink to the latest log file.

ulrichard force-pushed the feature/log_per_date branch from ba3fc5c to 2f087c8 Compare June 6, 2023 14:38

tnull approved these changes Jun 9, 2023

View reviewed changes

tnull merged commit 97d238c into lightningdevkit:main Jun 9, 2023

tnull mentioned this pull request Jun 15, 2023

Make log symlink relative #126

Closed

tnull mentioned this pull request Oct 28, 2024

Expose logger interface #309

Closed

enigbe mentioned this pull request Oct 31, 2024

revert: removes date from & symlink to log files #394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add the date when the node was started to the name of the logfile. #116

Add the date when the node was started to the name of the logfile. #116

Uh oh!

ulrichard commented Jun 2, 2023

Uh oh!

tnull left a comment

Uh oh!

Uh oh!

tnull Jun 5, 2023

Uh oh!

tnull left a comment •

edited

Loading

Uh oh!

tnull Jun 5, 2023

Uh oh!

ulrichard Jun 5, 2023

Uh oh!

tnull Jun 5, 2023

Uh oh!

Uh oh!

tnull Jun 5, 2023

Uh oh!

tnull Jun 5, 2023

Uh oh!

tnull left a comment

Uh oh!

tnull Jun 6, 2023

Uh oh!

tnull Jun 6, 2023

Uh oh!

tnull left a comment

Uh oh!

Uh oh!

	impl FilesystemLogger {
	pub(crate) fn new(file_path: String, level: Level) -> Self {
	if let Some(parent_dir) = Path::new(&file_path).parent() {
	fs::create_dir_all(parent_dir).expect("Failed to create log parent directory");
	}
	Self { file_path, level }
	}
	}

Add the date when the node was started to the name of the logfile. #116

Add the date when the node was started to the name of the logfile. #116

Uh oh!

Conversation

ulrichard commented Jun 2, 2023

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull left a comment •

edited

Loading