Add configurable logging system #407

enigbe · 2024-11-20T16:01:23Z

Overview

This PR introduces a flexible logging system for LDK Node by implementing a LogWriter interface that supports writing logs to different destinations.

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,
- Forward to a log implementer,
- Relay logs to a custom logger.
Exposes LogWriter to bindings.
Test logging to different destinations with:
- In-memory log logger,
- In-memory LogWriter logger.

Related Issue(s)

Expose logger interface #309
Cleaner implementation of closed draft feat: add customizable logging system #401

tnull

Cool, thank you for looking into this! This already looks pretty good, but I have some comments after the first round of review.

bindings/ldk_node.udl

tnull · 2024-11-27T12:38:22Z

...in/ldk-node-android/lib/src/androidTest/kotlin/org/lightningdevkit/ldknode/AndroidLibTest.kt

@@ -28,13 +28,11 @@ class AndroidLibTest {
        config1.storageDirPath = tmpDir1
        config1.listeningAddresses = listOf(listenAddress1)
        config1.network = Network.REGTEST
-        config1.logLevel = LogLevel.TRACE


Should we still set TRACE level, just by the new means?

Yes we should. I created a new CustomLogWriter class via which we can set the log level and also test LogWriter. Ran into some issues with NoPointer/Pointer.NULL when testing this (due to unfamiliarity with Kotlin) and would appreciate any pointers to resolve.

Cargo.toml

src/builder.rs

tnull · 2024-11-27T13:37:12Z

src/logger.rs

 pub(crate) enum Writer {
 	/// Writes logs to the file system.
 	FileWriter(FilesystemLogger),
 	/// Forwards logs to the `log` facade.
 	LogFacadeWriter(LogFacadeLogger),
+	/// Forwards logs to custom writer.


nit: s/custom writer/a custom writer/

tnull · 2024-11-27T13:37:30Z

tests/common/mod.rs

 }

 /// Simple in-memory mock `log` logger for tests.
+#[derive(Debug)]


Why is this Debug?

NodeBuilder is Debug, and it fields an optional LogWriterConfig. The config object has to be Debug and custom log writer variant needs to implement it for this reason.

tnull · 2024-11-27T13:38:00Z

tests/common/mod.rs

@@ -271,9 +274,18 @@ impl MockLogger {
 	}
 }

+/// [`MockLogger`] as `log` logger - destination for [`Writer::LogFacadeWriter`]


This seems to belong in a different commit? Also, let's not add documents on trait implementations (internal ones in particular).

Noted. I thought the documentation added clarity given that the same MockLogger object was being used as a custom logger and a log logger.

I think the MockLogger belongs to the right commit, as I have been careful about what goes into a commit. However, I may have missed this and will be doubly-careful going forward.

tnull · 2024-11-27T13:39:56Z

tests/integration_tests_rust.rs

@@ -828,13 +831,16 @@ fn generate_bip21_uri() {
 		},
 		Err(e) => panic!("Failed to generate URI: {:?}", e),
 	}
+
+	assert!(mock_logger.retrieve_logs().last().unwrap().contains("Invoice created: lnbcrt"));


As mentioned above: please avoid such potentially-flaky asserts. Also, we should really be testing the log facade separately instead of sprinkling in these asserts to unrelated testcases.

tnull · 2024-11-27T13:41:07Z

tests/integration_tests_rust.rs

 }

 #[test]
 fn unified_qr_send_receive() {
 	let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();
 	let chain_source = TestChainSource::Esplora(&electrsd);

+	// Setup `log` facade logger.


Same goes for arbitrary comments: please only add comments where they add to the context. Comments like this don't provide additional information over just reading the next line, they just increase the (visual) noise when reading code.

tnull · 2024-12-09T08:14:56Z

@enigbe Is there any update on this? Please let me know if you're hitting any blockers.

This seems to need a minor rebase by now.

enigbe · 2024-12-10T03:50:34Z

No blockers on this. I'll be pushing updates later today.

* Add flexible log writer interface for multiple destinations * Implement filesystem writing capability via FilesystemLogger * Prefix LDK-based objects with 'Ldk' for consistency * Add configuration options for log file path and log level

* Add support for user-provided custom logger to write logs to, allowing users to provide any logger that implements LogWriter * Add test to cover this use case, implementing Log- Writer for the mock, in-memory MockLogger.

This commit includes several improvements aimed at simplifying code, improving readability, and optimizing performance: - Inline enum struct fields to reduce indirection and clutter. - Rename structs to improve clarity and better reflect their purpose. - Enable specific feature flags in dependencies for a more streamlined build. - Eliminate unnecessary data allocations to optimize memory usage. - Correct minor grammatical error in documentation.

- allocates Strings only when necessary with `uniffi` feature, otherwise, keeps LogRecord fileds as lifetime references.

This commit adds a CustomLogWriter class to the kotlin library test, configuring the writer via the exposed node builder and tests the ability to log to the custom writer destination.

…ehavior - Removed trait name in description. - Expanded the description of the `LogWriter` trait to clarify its behavior when the `uniffi` feature is enabled or disabled. - Improved explanation of the trait’s responsibilities, including handling log messages and forwarding to outputs.

- Add a custom Debug implementation for LogWriterConfig to work around NodeBuilder's Debug constraints. - Remove the Debug trait from LogWriter, as it is no longer needed due to the custom Debug impl on LogWriterConfig. - Remove the Debug implementation from Writer for consistency.

- Revert MockLogger configuration and assertions - Remove clutter from setting up LogWriter across all tests

tnull · 2024-12-18T09:32:43Z

@enigbe Please let me know if/when this is ready for the next round of review!

- Remove remnants of TestLogWriter and MockLogger that will be reintroduced in a later PR to add a more structured TestConfig object.

enigbe

@tnull I believe this is ready for another review. I have addressed the majority of the concerns you raised in the first pass.

Regarding your concerns about testing the logging to custom loggers, I agree that the necessary refactor would be extensive and could detract from the purpose of this PR. As such, I plan to address these test-related changes in a follow-up PR.

Additionally, I encountered some challenges testing a custom logger in Kotlin and would greatly appreciate your guidance or suggestions.

tnull

Did another round of review, already looks pretty good.

I took a quick look at the Kotlin failures, but also couldn't immediately spot what's up, let me know if you want me to have a closer look though.

Btw, you could consider rebasing on #426 (or on main after it lands) which generally fixes pre-existing CI failures.

tnull · 2025-01-06T12:48:13Z

bindings/ldk_node.udl

@@ -563,6 +569,18 @@ dictionary NodeAnnouncementInfo {
 	sequence<SocketAddress> addresses;
 };

+dictionary LogRecord {


nit: Since we're (AFAIU?) moving LogLevel up to group with FilesystemLoggerConfig, we should probably do the same with these logging related items.

tnull · 2025-01-06T12:49:47Z

src/config.rs

+	}
+}
+
+pub(crate) fn default_log_file_path() -> String {


Let's just use consts for these static default values, no need to add methods on top of it.

tnull · 2025-01-06T12:58:00Z

src/builder.rs

+		self
+	}
+
+	/// Configures the [`Node`] instance to write logs to the `log` facade.


Could you point the user to the log crate for more information here?

tnull · 2025-01-06T13:08:22Z

src/builder.rs

@@ -391,7 +428,8 @@ impl NodeBuilder {
 	) -> Result<Node, BuildError> {
 		use bitcoin::key::Secp256k1;

-		let logger = setup_logger(&self.config)?;
+		let log_writer_config = self.log_writer_config.clone().unwrap_or_default();


Mhh, can we avoid these clones here? Possibly by having setup_logger taking the whole &Option<LogWriterConfig> and implementing the defaulting behavior once there, which probably would be preferable over doing it in several places anyhow?

tnull · 2025-01-06T13:09:39Z

src/builder.rs

+			let log_file_path = if let Some(fp) = &fs_logger_config.log_file_path {
+				fp
+			} else {
+				&default_log_file_path()


As mentioned above, let's just use the consts directly here.

tnull · 2025-01-06T13:31:17Z

src/logger.rs

+	/// Forwards logs to the `log` facade.
+	LogFacadeWriter { level: LogLevel },
+	/// Forwards logs to a custom writer.
+	CustomWriter(Arc<dyn LogWriter + Send + Sync>),


I think we might be able to drop the explicit + Send + Sync here if we already have LogWriter inherit them above?

tnull · 2025-01-06T13:39:38Z

src/logger.rs

+
+impl LogWriter for Writer {
+	fn log(&self, record: LogRecord) {
+		let log = format!(


Hmm, so format! allocates a string on the heap. As we try to avoid allocations (to reduce heap fragmentation where possible), can we a) only create this string when we're sure we need it to log (i.e, after passing the log level filtering), and b) can we avoid it altogether for the CustomWriter case, and possibly even the LogFacadeWriter case? In the latter case we should be able to give the arguments to the respective macros directly, no?

tnull · 2025-01-06T13:42:28Z

src/logger.rs

+		Ok(Self { writer: Writer::FileWriter { file_path: file_path.to_string(), level } })
+	}
+
+	pub fn new_log_facade(level: LogLevel) -> Result<Self, ()> {


Seems new_log_facade and new_custom_writer are infallible, so no reason to have them return a Result.

tnull · 2025-01-06T13:45:09Z

src/logger.rs

-		if let Some(parent_dir) = Path::new(&log_file_path).parent() {
+pub(crate) struct Logger {
+	/// Specifies the logger's writer.
+	writer: Writer,


If Logger only has this one field, why not have it be the enum directly? I.e., rename Writer to Logger and implement what's needed directly on there?

I have a concern about this approach. I have tried it and realized that if we use the Writer (renamed to Logger) directly, we will run into a problem where the compiler will not be able to decide between traits LdkLogger::log(...) and LogWriter::log(...) for any log_*!(...). An easy work-around would be to rename the LogWriter's log to write but we decided against that a while back. How would you suggest we address this?

tnull · 2025-01-06T13:48:01Z

...in/ldk-node-android/lib/src/androidTest/kotlin/org/lightningdevkit/ldknode/AndroidLibTest.kt

I think we don't need to bother implementing the custom logger on both Android and JVM. Let's pick one, likely JVM as we already do most tests there?

enigbe mentioned this pull request Nov 20, 2024

feat: add customizable logging system #401

Closed

7 tasks

G8XSU requested a review from tnull November 21, 2024 18:37

tnull reviewed Nov 27, 2024

View reviewed changes

enigbe added 9 commits December 11, 2024 13:09

feat(logger): add & impl LogWriter for Writer

abb3b44

fix(bindings): remove config log level from kotlin tests

49e914e

feat: forward logs to log facade

3dbff37

test(logwriter): forward logs to mock in-memory log logger

c1f7858

fix: correct "Forwards" spelling error

a40ccb1

feat(custom): forward logs to a custom logger

7f4b122

* Add support for user-provided custom logger to write logs to, allowing users to provide any logger that implements LogWriter * Add test to cover this use case, implementing Log- Writer for the mock, in-memory MockLogger.

fix(test): fix setting log's global logger twice

bdeb432

chore: revert the renaming of LogLevel to LdkLevel

d8eb0e1

enigbe force-pushed the 2024-11-configurable-logging-system branch from 1d57cab to d8eb0e1 Compare December 11, 2024 12:18

G8XSU mentioned this pull request Dec 12, 2024

Ldk-Server Milestone 3 lightningdevkit/ldk-server#11

Open

9 tasks

enigbe added 2 commits December 13, 2024 05:28

chore: remove unused CustomTlvRecord

e968562

feat: address costly log record re-allocation

0347e04

- allocates Strings only when necessary with `uniffi` feature, otherwise, keeps LogRecord fileds as lifetime references.

enigbe force-pushed the 2024-11-configurable-logging-system branch from 26aff81 to 7fa2928 Compare December 15, 2024 23:43

enigbe added 2 commits December 16, 2024 00:54

test(kotlin-android): set custom log writer via builder method

f32d7e8

This commit adds a CustomLogWriter class to the kotlin library test, configuring the writer via the exposed node builder and tests the ability to log to the custom writer destination.

enigbe force-pushed the 2024-11-configurable-logging-system branch from 7fa2928 to 283fe85 Compare December 16, 2024 00:32

enigbe added 2 commits December 17, 2024 05:19

test: clean up MockLogger and LogWriter setup

e07716d

- Revert MockLogger configuration and assertions - Remove clutter from setting up LogWriter across all tests

enigbe force-pushed the 2024-11-configurable-logging-system branch 3 times, most recently from dcf757d to 0660dfa Compare December 19, 2024 21:38

test(kotlin-jvm): set custom logging via builder method

6766c60

enigbe force-pushed the 2024-11-configurable-logging-system branch from 0660dfa to 6766c60 Compare December 19, 2024 21:54

enigbe added 2 commits December 20, 2024 11:57

test: setup node with filesystem logger at gossip log level

b927b31

- Remove remnants of TestLogWriter and MockLogger that will be reintroduced in a later PR to add a more structured TestConfig object.

chore: drop file path clone when writing to file writer

2a80510

enigbe commented Dec 20, 2024

View reviewed changes

tnull reviewed Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configurable logging system #407

Add configurable logging system #407

enigbe commented Nov 20, 2024

tnull left a comment

tnull Nov 27, 2024

enigbe Dec 20, 2024 •

edited

Loading

tnull Nov 27, 2024

tnull Nov 27, 2024

enigbe Dec 17, 2024 •

edited

Loading

tnull Nov 27, 2024

enigbe Dec 17, 2024

tnull Nov 27, 2024

tnull Nov 27, 2024

enigbe Dec 17, 2024

tnull commented Dec 9, 2024

enigbe commented Dec 10, 2024

tnull commented Dec 18, 2024

enigbe left a comment

tnull left a comment

tnull Jan 6, 2025

tnull Jan 6, 2025

tnull Jan 6, 2025

tnull Jan 6, 2025

tnull Jan 6, 2025

tnull Jan 6, 2025

tnull Jan 6, 2025

tnull Jan 6, 2025

tnull Jan 6, 2025

enigbe Jan 10, 2025

tnull Jan 6, 2025

Add configurable logging system #407

Are you sure you want to change the base?

Add configurable logging system #407

Conversation

enigbe commented Nov 20, 2024

Overview

What this PR does

Related Issue(s)

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enigbe Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enigbe Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Dec 9, 2024

enigbe commented Dec 10, 2024

tnull commented Dec 18, 2024

enigbe left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enigbe Dec 20, 2024 •

edited

Loading

enigbe Dec 17, 2024 •

edited

Loading