feat: add customizable logging system #401

enigbe · 2024-11-12T07:40:58Z

Summary

This PR builds on #393 to add a customizable logging system.

What this PR does

Introduces a LogWriter interface, allowing Writer variants to handle log output destinations. The supported Writer variants can now:
- Write logs to the filesystem,
- Optionally forward to log
- Relay logs to a custom logger.
Makes Writer variants configurable.
Expose LogWriter to bindings (pending).
Test logging to different destinations.

Related Issue

Expose logger interface #309

1. Rename FilesystemLogger to LdkNodeLogger. 2. Make LdkNodeLogger technically customizable, retain default behavior. 3. Add customizable logging to configuration.

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

- Creates a LogWriter interface for different writers to write logs to their preferred destinations. These destinations could be either the filesystem or another logger (e.g. custom logger or any that implements log's facade. - The writer and formatter of LdkNodeLogger can be created by configurable options.

tnull

Thanks for looking into this! Here is a first round of comments.

Generally, I would have preferred a clean start rather than basing it off #393, since, as discussed there, the chosen approach isn't really bindings compatible and breaks a few of our LDK Node-internal preexisting idioms. That said, happy to continue the work in this PR. However, please try to ensure to have commits be discrete changes that always compile, and if possible avoid touching parts of the code again in later commits as it makes reviewing and understanding whats going a lot harder. To this end, you might want to squash and then split out commits for the custom writer and log parts.

tnull · 2024-11-12T08:43:06Z

src/logger.rs


-pub(crate) struct FilesystemLogger {
-	file_path: String,
+pub struct LdkNodeLogger {


Let's drop the LdkNode prefix please, as we also don't prefix other objects. This could just be Logger (although that would mean we'd need to refer to the upstream trait as LdkLogger as we do with others, e.g., LdkFeeEstimator), or find another suitable name that capture its properties.

tnull · 2024-11-12T08:45:28Z

src/logger.rs

-	file_path: String,
+pub struct LdkNodeLogger {
 	level: Level,
+	formatter: Box<dyn Fn(&Record) -> String + Send + Sync>,


Not sure if it's worth introducing a separate heap object for this. Let's just not for now, and re-add it if we really find use for it.

tnull · 2024-11-12T08:49:43Z

src/config.rs

 	pub log_file_path: Option<String>,
+	/// In the default configuration logs can be found in the `logs` subdirectory in
+	/// [`Config::storage_dir_path`], and the log level is set to [`DEFAULT_LOG_LEVEL`].
+	pub logging_config: LoggingConfig,


As mentioned in the original PR, we can't do this as it likely won't be really compatible with bindings and also doesn't match how we handle other things.

Rather, we'd need to add three builder methods to allow configuring the three modes:

a) set_logger_file/set_logger_filesystem taking a FilesystemLogger config object allowing to set loglevel and path (also exposed in the bindings builder)
b) set_logger_facade to use the log facade
c) set_logger_custom taking an Arc<dyn LogWriter + Send + Sync> (also exposed in the bindings builder)

That said, we'd likely want to keep something like LoggingConfig as an internal config object, similar to ChainSourceConfig, EntropySourceConfig, etc.
That is, the API would be controlled by Builder methods, but the 'state' is kept in that object and used to setup in Builder::build.

tnull · 2024-11-12T08:50:08Z

src/config.rs

 	///
 	/// 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>,


This needs to be dropped/moved, too, as it only makes sense for the filesystem writer.

tnull · 2024-11-12T08:51:57Z

src/logger.rs

+pub struct LdkNodeLogger {
 	level: Level,
+	formatter: Box<dyn Fn(&Record) -> String + Send + Sync>,
+	writer: Box<dyn Fn(&String) + Send + Sync>,


We'll need to expose a LogWriter trait that can be used for custom loggers exposed via bindings (see https://mozilla.github.io/uniffi-rs/0.27/udl/interfaces.html#exposing-traits-as-interfaces)

tnull · 2024-11-12T08:59:32Z

src/config.rs

+
+/// Log writer configuration type.
+#[derive(Debug, Clone)]
+pub enum WriterType {


I guess this the internal config object mentioned above, but it should live in builder.rs and should be named accordingly (e.g., LoggingConfig, LogWriterConfig, etc.).

tnull · 2024-11-12T09:00:01Z

src/config.rs

+impl FileWriterConfig {
+	/// Creates a new configuration given the path to the log file
+	/// and the log level.
+	pub fn new(log_file_path: &str, level: Level) -> Self {


This doesn't need a constructor.

tnull · 2024-11-12T09:01:31Z

src/config.rs

+#[derive(Debug, Clone)]
+pub struct CustomWriterConfig {
+	/// Pointer to any custom log writer.
+	pub inner: Arc<dyn LogWriter + Send + Sync>,


This shouldn't live in the public API config object, but just as a field in the (to be renamed) WriterType.

tnull · 2024-11-12T09:02:17Z

src/config.rs

+	/// Pointer to any custom log writer.
+	pub inner: Arc<dyn LogWriter + Send + Sync>,
+	/// Specifies the log level.
+	pub level: Level,


Shouldn't the custom writer decide how to filter for level? I.e., do we really need CustomWriterConfig at all?

tnull · 2024-11-12T09:05:00Z

Cargo.toml


 [features]
 default = []
+log_relay = ["log"]


We don't use any features in LDK Node so far (mostly to keep ~feature-parity of the Rust vs bindings API, for which everything needs to be configurable via the Builder), so please drop this and just take a non-optional dependency on log for now.

enigbe · 2024-11-20T16:03:35Z

Closing this because, given the feedback, I also think it's best to have a clean slate. #407 addresses all the concerns here.

domZippilli and others added 4 commits November 12, 2024 08:11

feat: initial ldk node logger customization

974ee28

1. Rename FilesystemLogger to LdkNodeLogger. 2. Make LdkNodeLogger technically customizable, retain default behavior. 3. Add customizable logging to configuration.

refactor: update FilesystemLogWriter to FileWriter

bfddd4c

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

feat: optionally relay logs to log

51ca997

enigbe changed the title ~~feat: customizable logging system~~ feat: add customizable logging system Nov 12, 2024

tnull reviewed Nov 12, 2024

View reviewed changes

enigbe mentioned this pull request Nov 20, 2024

Add configurable logging system #407

Merged

8 tasks

enigbe closed this Nov 20, 2024

feat: add customizable logging system #401

feat: add customizable logging system #401

Uh oh!

Conversation

enigbe commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What this PR does

Related Issue

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!

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!

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!

enigbe commented Nov 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

enigbe commented Nov 12, 2024 •

edited

Loading

tnull left a comment •

edited

Loading