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.

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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

enigbe
Copy link
Contributor

@enigbe enigbe commented Oct 31, 2024

What this PR does:

Reverts the changes introduce by #116 which added the dates to log files whenever 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.

Related Issue

@enigbe enigbe marked this pull request as draft October 31, 2024 17:07
@enigbe enigbe force-pushed the revert-filesystem-logging-to-simple-file branch from 57a04f1 to ee51f47 Compare October 31, 2024 17:08
@enigbe enigbe marked this pull request as ready for review October 31, 2024 17:30
)

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.
@enigbe enigbe force-pushed the revert-filesystem-logging-to-simple-file branch from ee51f47 to 24cf565 Compare October 31, 2024 17:31
@@ -1231,6 +1231,8 @@ fn build_with_store_internal(
})
}

/// Sets up the node logger, creating a new log file if it does not exist, or utilizing
/// the existing log file.
fn setup_logger(config: &Config) -> Result<Arc<FilesystemLogger>, BuildError> {
let log_dir = match &config.log_dir_path {
Some(log_dir) => String::from(log_dir),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Github won't comment on the line below this, but let's drop the {storage_path_dir}/logs folder now that we'll only have one log file. Rather, let's just default to {storage_path_dir}/ldk_node.log.

@enigbe enigbe force-pushed the revert-filesystem-logging-to-simple-file branch from 109ab9c to c030775 Compare November 3, 2024 23:04
@enigbe enigbe force-pushed the revert-filesystem-logging-to-simple-file branch from c030775 to dc9f840 Compare November 3, 2024 23:17
@enigbe enigbe requested a review from tnull November 4, 2024 10:15
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Basically LGTM I think.

One question, up to to you.

@@ -232,8 +232,8 @@ fn start_stop_reinit() {
node.sync_wallets().unwrap();
assert_eq!(node.list_balances().spendable_onchain_balance_sats, expected_amount.to_sat());

let log_file_symlink = format!("{}/logs/ldk_node_latest.log", config.clone().storage_dir_path);
assert!(std::path::Path::new(&log_file_symlink).is_symlink());
let log_file = format!("{}/ldk_node.log", config.clone().storage_dir_path);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I wonder if now that we're only gonna have one file, we maybe should replace Config::log_dir_path with log_file_path, i.e., let users specify the full file name? Up to you, could also leave as is.

Note that if we change this in Config it might be a temporary solution only, as we should move log-related configuration options out of Config going forward when we allow to configure custom loggers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think changing to log_file_path is clearer. I'll make the change now and can look at moving the log-related config when we are ready.

Copy link
Collaborator

Choose a reason for hiding this comment

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

SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants