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.

Sign up for GitHub

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

Fix log-facade logging and clarify max level #454

Merged
merged 4 commits into from
Feb 4, 2025
Merged

Fix log-facade logging and clarify max level #454

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb74862
Rename `level` argument to `max_log_level`
tnull Feb 1, 2025
b0370f8
f Builder docs
tnull Feb 1, 2025
5464709
Fix log-facade logging
tnull Feb 1, 2025
9f2714c
Have `set_log_facade_logger` take an *optional* `max_log_level`
tnull Feb 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bindings/ldk_node.udl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,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(string? log_file_path, LogLevel? max_log_level);
void set_log_facade_logger(LogLevel max_log_level);
void set_log_facade_logger(LogLevel? max_log_level);
void set_custom_logger(LogWriter log_writer);
void set_network(Network network);
[Throws=BuildError]
Expand Down
15 changes: 9 additions & 6 deletions src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ impl Default for LiquiditySourceConfig {
#[derive(Clone)]
enum LogWriterConfig {
File { log_file_path: Option<String>, max_log_level: Option<LogLevel> },
Log(LogLevel),
Log { max_log_level: Option<LogLevel> },
Custom(Arc<dyn LogWriter>),
}

Expand All @@ -124,7 +124,7 @@ impl std::fmt::Debug for LogWriterConfig {
.field("max_log_level", max_log_level)
.field("log_file_path", log_file_path)
.finish(),
LogWriterConfig::Log(max_log_level) => {
LogWriterConfig::Log { max_log_level } => {
f.debug_tuple("Log").field(max_log_level).finish()
},
LogWriterConfig::Custom(_) => {
Expand Down Expand Up @@ -349,8 +349,8 @@ impl NodeBuilder {
///
/// If set, the `max_log_level` sets the maximum log level. Otherwise, the latter defaults to
/// [`DEFAULT_LOG_LEVEL`].
pub fn set_log_facade_logger(&mut self, max_log_level: LogLevel) -> &mut Self {
self.log_writer_config = Some(LogWriterConfig::Log(max_log_level));
pub fn set_log_facade_logger(&mut self, max_log_level: Option<LogLevel>) -> &mut Self {
self.log_writer_config = Some(LogWriterConfig::Log { max_log_level });
self
}

Expand Down Expand Up @@ -680,7 +680,7 @@ impl ArcedNodeBuilder {
///
/// If set, the `max_log_level` sets the maximum log level. Otherwise, the latter defaults to
/// [`DEFAULT_LOG_LEVEL`].
pub fn set_log_facade_logger(&self, log_level: LogLevel) {
pub fn set_log_facade_logger(&self, log_level: Option<LogLevel>) {
self.inner.write().unwrap().set_log_facade_logger(log_level);
}

Expand Down Expand Up @@ -1326,7 +1326,10 @@ fn setup_logger(
Logger::new_fs_writer(log_file_path, max_log_level)
.map_err(|_| BuildError::LoggerSetupFailed)?
},
Some(LogWriterConfig::Log(log_level)) => Logger::new_log_facade(*log_level),
Some(LogWriterConfig::Log { max_log_level }) => {
let max_log_level = max_log_level.unwrap_or_else(|| DEFAULT_LOG_LEVEL);
Logger::new_log_facade(max_log_level)
Copy link
Contributor

@joostjager joostjager Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

One thing that I noticed about the log facade is that you can set log level GOSSIP, but that isn't visible anymore after this PR. The standard log only knows TRACE of course. Not a problem I think?

And perhaps some explanation on how the RUST_LOG env var interacts with this log level would be helpful too. It wasn't very clear to me how that works. Maybe there is a way to set RUST_LOG programmatically?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One thing that I noticed about the log facade is that you can set log level GOSSIP, but that isn't visible anymore after this PR. The standard log only knows TRACE of course. Not a problem I think?

Yeah, I think it's not super important. We could consider adding something special in the target or message itself, but I'd wait for a user to complain about it. So far, I assume it's fine to squash TRACE and GOSSIP together in this case.

And perhaps some explanation on how the RUST_LOG env var interacts with this log level would be helpful too. It wasn't very clear to me how that works. Maybe there is a way to set RUST_LOG programmatically?

Ah, RUST_LOG is entirely out-of-scope here, as it's a special behavior of env_logger, which is just one of the backends for the log facade. env_logger users simply need to read their docs and discover that it's mandatory to set RUST_LOG (see https://docs.rs/env_logger/latest/env_logger/#enabling-logging).

},

Some(LogWriterConfig::Custom(custom_log_writer)) => {
Logger::new_custom_writer(Arc::clone(&custom_log_writer))
Expand Down
Loading