Skip to content

Conversation

@benthecarman
Copy link
Collaborator

@benthecarman benthecarman commented Dec 3, 2025

Before we were dropping all our logs on the floor besides a few printlns
in the code. This implements a logger for the log facade that writes
logs to the console as well as to the log file. We also add to the config
options for setting the log file and log level.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Dec 3, 2025

I've assigned @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

.set_time_format_rfc3339() // set time format to rfc3339 (e.g. 2025-12-03T21:44:52.169154084Z)
.build();
if let Err(e) = CombinedLogger::init(vec![
SimpleLogger::new(config_file.log_level, log_config.clone()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't really clear from the docs as to why we need this. Does it got to stdout?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, added a comment

rand = { version = "0.8.5", default-features = false }
async-trait = { version = "0.1.85", default-features = false }
toml = { version = "0.8.9", default-features = false, features = ["parse"] }
simplelog = { version = "0.12.0", default-features = false }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than pulling in that depedency, can we just add a simple logger that implements the log facade? It's really trivial, for reference see the MockLogFacadeLogger in LDK Node tests: https://github.com/lightningdevkit/ldk-node/blob/198ff306bb70d1b6eb7bf9a4213716e22ae1db90/tests/common/logging.rs#L22

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

/// The minimum log level to display
level: LevelFilter,
/// The file to write logs to, buffered and protected by a mutex for thread-safe access
file: Mutex<BufWriter<File>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keeping the open file handle in a Mutex works, but then we should handle SIGHUP and make sure we drop and reopen the file handle to ensure compatibility with system tools such as logrotate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added handling for this, made things a little more complicated but nothing too bad

@benthecarman benthecarman force-pushed the log branch 2 times, most recently from 320676e to b345b27 Compare December 8, 2025 05:10
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, just two minor comments.

Before we were dropping all our logs on the floor besides a few printlns
in the code. This implements a logger for the log facade that writes
logs to the console as well as to the log file. We also add to the config
options for setting the log file and log level.
@tnull tnull merged commit e389521 into lightningdevkit:main Dec 9, 2025
7 checks passed
@benthecarman benthecarman deleted the log branch December 9, 2025 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants