Add configurable logger

[domZippilli]

Description

Adds logger configuration to ldk-node.

Motivation

The first impulse for this was to be able to write to stdout, as that's more desirable for some containerized infrastructure. But rather than add a specific stdout implementation, we thought it better to make this extensible to write to arbitrary destinations (for example, a network log ingester). This additionally has the benefit of letting you customize how the writing happens; for example the tracing-appender crate uses an mpsc for writers, with a single thread performing all the actual I/O off of the queue.

Details

The PR has three commits:

• Rename FilesystemLogger to LdkNodeLogger. This renames the logger to an implementation-agnostic name. Hopefully this commit is easy to ignore when reviewing implementation.
• Make LdkNodeLogger technically customizable, retain default behavior. This sets up the configurable logger, and then statically configures it to retain the existing behavior.
• Add customizable logging to configuration. This adds configuration points to the Config struct itself. Users of the ::default() configuration should see no change in behavior.

[tnull]

Thanks for looking into this and I def. agree we need to add the capability for custom loggers (see #309) but I'm not sure we want to go with the approach chosen here.

As described over at #309, we probably want to try using the log crate facade if possible (at least for the Rust side), to which end we will try to take an optional dependency upstream. Moreover, the custom logger interface will need to be exposable via UniFFI bindings, which doesn't work with the approach chosen here. We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

Let me know if that makes sense and also what additional requirements you see for a custom logger interface. If you intend to keep working on this, you might want to coordinate with @enigbe which just recently indicated interest in working on it / #309, too.

[domZippilli]

Ah, I see. I was trying to avoid taking a dependency, but if you've already decided you want to use log, then that makes sense. We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

We originally did this in our local fork, as we can only see stdout logs right now. We can keep running with our fork until #309 is resolved.

As far as requirements I'd like to see for #309:

• I'd like to be able to do structured logging. JSON, in our case, but full control over formatting would be best.
• I'd like to be able to specify the device I'm writing to. Originally I did this with a dyn Writer, but for FFI you might need to do something else. At a minimum, I would need stdout.
• I'd like to be able to specify how the writing happens. A non-blocking writer like I mentioned is important for high volume logging.

eta:

We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

That would be very trivially reversed, and that should hold true for any implementation that's extensible enough. Maybe that's a good litmus test for customization, as well.

[tnull]

but if you've already decided you want to use log, then that makes sense.

Yes, we probably will go this way at least here in LDK Node, but possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log, i.e., especially in larger projects with multiple producers LDK stands out as the one logging to its own thing while all other logs are in a single stream. So IMO we should at least try to support it in LDK also.

We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

Right, you can also expose trait objects arguments via Uniffi, but they need to adhere to some rules, i.e., we'll likely need something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder or similar.

As far as requirements I'd like to see for #309:

Cool, thanks. Will keep these in mind! My goal for now is to end up with a slightly improved FilesystemLogger and a pluggable interface so that you can do whatever you want in your logger implementation.

[domZippilli]

something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder

Heh, I tried this as well, too bad I tossed the commit. I think that would work as well, although it left me writing some awkward documentation ... along the lines of, "If you specify this, these other logging fields from config are ignored."

[domZippilli]

possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

[tnull]

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

Yeah, that's a good point, maybe even more so regarding the upstream changes as we might need tracing to accommodate our structured logging macros there. That said, there also is tracing-log which should users that need it at least rudimentary compatibility if we use log.

[domZippilli]

Description

Adds logger configuration to ldk-node.

Motivation

The first impulse for this was to be able to write to stdout, as that's more desirable for some containerized infrastructure. But rather than add a specific stdout implementation, we thought it better to make this extensible to write to arbitrary destinations (for example, a network log ingester). This additionally has the benefit of letting you customize how the writing happens; for example the tracing-appender crate uses an mpsc for writers, with a single thread performing all the actual I/O off of the queue.

Details

The PR has three commits:

• Rename FilesystemLogger to LdkNodeLogger. This renames the logger to an implementation-agnostic name. Hopefully this commit is easy to ignore when reviewing implementation.
• Make LdkNodeLogger technically customizable, retain default behavior. This sets up the configurable logger, and then statically configures it to retain the existing behavior.
• Add customizable logging to configuration. This adds configuration points to the Config struct itself. Users of the ::default() configuration should see no change in behavior.

[tnull]

Thanks for looking into this and I def. agree we need to add the capability for custom loggers (see #309) but I'm not sure we want to go with the approach chosen here.

As described over at #309, we probably want to try using the log crate facade if possible (at least for the Rust side), to which end we will try to take an optional dependency upstream. Moreover, the custom logger interface will need to be exposable via UniFFI bindings, which doesn't work with the approach chosen here. We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

Let me know if that makes sense and also what additional requirements you see for a custom logger interface. If you intend to keep working on this, you might want to coordinate with @enigbe which just recently indicated interest in working on it / #309, too.

[domZippilli]

Ah, I see. I was trying to avoid taking a dependency, but if you've already decided you want to use log, then that makes sense. We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

We originally did this in our local fork, as we can only see stdout logs right now. We can keep running with our fork until #309 is resolved.

As far as requirements I'd like to see for #309:

• I'd like to be able to do structured logging. JSON, in our case, but full control over formatting would be best.
• I'd like to be able to specify the device I'm writing to. Originally I did this with a dyn Writer, but for FFI you might need to do something else. At a minimum, I would need stdout.
• I'd like to be able to specify how the writing happens. A non-blocking writer like I mentioned is important for high volume logging.

eta:

We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

That would be very trivially reversed, and that should hold true for any implementation that's extensible enough. Maybe that's a good litmus test for customization, as well.

[tnull]

but if you've already decided you want to use log, then that makes sense.

Yes, we probably will go this way at least here in LDK Node, but possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log, i.e., especially in larger projects with multiple producers LDK stands out as the one logging to its own thing while all other logs are in a single stream. So IMO we should at least try to support it in LDK also.

We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

Right, you can also expose trait objects arguments via Uniffi, but they need to adhere to some rules, i.e., we'll likely need something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder or similar.

As far as requirements I'd like to see for #309:

Cool, thanks. Will keep these in mind! My goal for now is to end up with a slightly improved FilesystemLogger and a pluggable interface so that you can do whatever you want in your logger implementation.

[domZippilli]

something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder

Heh, I tried this as well, too bad I tossed the commit. I think that would work as well, although it left me writing some awkward documentation ... along the lines of, "If you specify this, these other logging fields from config are ignored."

[domZippilli]

possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

[tnull]

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

Yeah, that's a good point, maybe even more so regarding the upstream changes as we might need tracing to accommodate our structured logging macros there. That said, there also is tracing-log which should users that need it at least rudimentary compatibility if we use log.

[domZippilli]

Description

Adds logger configuration to ldk-node.

Motivation

The first impulse for this was to be able to write to stdout, as that's more desirable for some containerized infrastructure. But rather than add a specific stdout implementation, we thought it better to make this extensible to write to arbitrary destinations (for example, a network log ingester). This additionally has the benefit of letting you customize how the writing happens; for example the tracing-appender crate uses an mpsc for writers, with a single thread performing all the actual I/O off of the queue.

Details

The PR has three commits:

• Rename FilesystemLogger to LdkNodeLogger. This renames the logger to an implementation-agnostic name. Hopefully this commit is easy to ignore when reviewing implementation.
• Make LdkNodeLogger technically customizable, retain default behavior. This sets up the configurable logger, and then statically configures it to retain the existing behavior.
• Add customizable logging to configuration. This adds configuration points to the Config struct itself. Users of the ::default() configuration should see no change in behavior.

[tnull]

Thanks for looking into this and I def. agree we need to add the capability for custom loggers (see #309) but I'm not sure we want to go with the approach chosen here.

As described over at #309, we probably want to try using the log crate facade if possible (at least for the Rust side), to which end we will try to take an optional dependency upstream. Moreover, the custom logger interface will need to be exposable via UniFFI bindings, which doesn't work with the approach chosen here. We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

Let me know if that makes sense and also what additional requirements you see for a custom logger interface. If you intend to keep working on this, you might want to coordinate with @enigbe which just recently indicated interest in working on it / #309, too.

[domZippilli]

Ah, I see. I was trying to avoid taking a dependency, but if you've already decided you want to use log, then that makes sense. We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

We originally did this in our local fork, as we can only see stdout logs right now. We can keep running with our fork until #309 is resolved.

As far as requirements I'd like to see for #309:

• I'd like to be able to do structured logging. JSON, in our case, but full control over formatting would be best.
• I'd like to be able to specify the device I'm writing to. Originally I did this with a dyn Writer, but for FFI you might need to do something else. At a minimum, I would need stdout.
• I'd like to be able to specify how the writing happens. A non-blocking writer like I mentioned is important for high volume logging.

eta:

We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

That would be very trivially reversed, and that should hold true for any implementation that's extensible enough. Maybe that's a good litmus test for customization, as well.

[tnull]

but if you've already decided you want to use log, then that makes sense.

Yes, we probably will go this way at least here in LDK Node, but possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log, i.e., especially in larger projects with multiple producers LDK stands out as the one logging to its own thing while all other logs are in a single stream. So IMO we should at least try to support it in LDK also.

We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

Right, you can also expose trait objects arguments via Uniffi, but they need to adhere to some rules, i.e., we'll likely need something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder or similar.

As far as requirements I'd like to see for #309:

Cool, thanks. Will keep these in mind! My goal for now is to end up with a slightly improved FilesystemLogger and a pluggable interface so that you can do whatever you want in your logger implementation.

[domZippilli]

something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder

Heh, I tried this as well, too bad I tossed the commit. I think that would work as well, although it left me writing some awkward documentation ... along the lines of, "If you specify this, these other logging fields from config are ignored."

[domZippilli]

possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

[tnull]

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

Yeah, that's a good point, maybe even more so regarding the upstream changes as we might need tracing to accommodate our structured logging macros there. That said, there also is tracing-log which should users that need it at least rudimentary compatibility if we use log.

[domZippilli]

Description

Adds logger configuration to ldk-node.

Motivation

The first impulse for this was to be able to write to stdout, as that's more desirable for some containerized infrastructure. But rather than add a specific stdout implementation, we thought it better to make this extensible to write to arbitrary destinations (for example, a network log ingester). This additionally has the benefit of letting you customize how the writing happens; for example the tracing-appender crate uses an mpsc for writers, with a single thread performing all the actual I/O off of the queue.

Details

The PR has three commits:

• Rename FilesystemLogger to LdkNodeLogger. This renames the logger to an implementation-agnostic name. Hopefully this commit is easy to ignore when reviewing implementation.
• Make LdkNodeLogger technically customizable, retain default behavior. This sets up the configurable logger, and then statically configures it to retain the existing behavior.
• Add customizable logging to configuration. This adds configuration points to the Config struct itself. Users of the ::default() configuration should see no change in behavior.

[tnull]

Thanks for looking into this and I def. agree we need to add the capability for custom loggers (see #309) but I'm not sure we want to go with the approach chosen here.

As described over at #309, we probably want to try using the log crate facade if possible (at least for the Rust side), to which end we will try to take an optional dependency upstream. Moreover, the custom logger interface will need to be exposable via UniFFI bindings, which doesn't work with the approach chosen here. We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

Let me know if that makes sense and also what additional requirements you see for a custom logger interface. If you intend to keep working on this, you might want to coordinate with @enigbe which just recently indicated interest in working on it / #309, too.

[domZippilli]

Ah, I see. I was trying to avoid taking a dependency, but if you've already decided you want to use log, then that makes sense. We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

We originally did this in our local fork, as we can only see stdout logs right now. We can keep running with our fork until #309 is resolved.

As far as requirements I'd like to see for #309:

• I'd like to be able to do structured logging. JSON, in our case, but full control over formatting would be best.
• I'd like to be able to specify the device I'm writing to. Originally I did this with a dyn Writer, but for FFI you might need to do something else. At a minimum, I would need stdout.
• I'd like to be able to specify how the writing happens. A non-blocking writer like I mentioned is important for high volume logging.

eta:

We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

That would be very trivially reversed, and that should hold true for any implementation that's extensible enough. Maybe that's a good litmus test for customization, as well.

[tnull]

but if you've already decided you want to use log, then that makes sense.

Yes, we probably will go this way at least here in LDK Node, but possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log, i.e., especially in larger projects with multiple producers LDK stands out as the one logging to its own thing while all other logs are in a single stream. So IMO we should at least try to support it in LDK also.

We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

Right, you can also expose trait objects arguments via Uniffi, but they need to adhere to some rules, i.e., we'll likely need something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder or similar.

As far as requirements I'd like to see for #309:

Cool, thanks. Will keep these in mind! My goal for now is to end up with a slightly improved FilesystemLogger and a pluggable interface so that you can do whatever you want in your logger implementation.

[domZippilli]

something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder

Heh, I tried this as well, too bad I tossed the commit. I think that would work as well, although it left me writing some awkward documentation ... along the lines of, "If you specify this, these other logging fields from config are ignored."

[domZippilli]

possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

[tnull]

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

Yeah, that's a good point, maybe even more so regarding the upstream changes as we might need tracing to accommodate our structured logging macros there. That said, there also is tracing-log which should users that need it at least rudimentary compatibility if we use log.

[domZippilli]

Description

Adds logger configuration to ldk-node.

Motivation

The first impulse for this was to be able to write to stdout, as that's more desirable for some containerized infrastructure. But rather than add a specific stdout implementation, we thought it better to make this extensible to write to arbitrary destinations (for example, a network log ingester). This additionally has the benefit of letting you customize how the writing happens; for example the tracing-appender crate uses an mpsc for writers, with a single thread performing all the actual I/O off of the queue.

Details

The PR has three commits:

• Rename FilesystemLogger to LdkNodeLogger. This renames the logger to an implementation-agnostic name. Hopefully this commit is easy to ignore when reviewing implementation.
• Make LdkNodeLogger technically customizable, retain default behavior. This sets up the configurable logger, and then statically configures it to retain the existing behavior.
• Add customizable logging to configuration. This adds configuration points to the Config struct itself. Users of the ::default() configuration should see no change in behavior.

[tnull]

Thanks for looking into this and I def. agree we need to add the capability for custom loggers (see #309) but I'm not sure we want to go with the approach chosen here.

As described over at #309, we probably want to try using the log crate facade if possible (at least for the Rust side), to which end we will try to take an optional dependency upstream. Moreover, the custom logger interface will need to be exposable via UniFFI bindings, which doesn't work with the approach chosen here. We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

Let me know if that makes sense and also what additional requirements you see for a custom logger interface. If you intend to keep working on this, you might want to coordinate with @enigbe which just recently indicated interest in working on it / #309, too.

[domZippilli]

Ah, I see. I was trying to avoid taking a dependency, but if you've already decided you want to use log, then that makes sense. We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

We originally did this in our local fork, as we can only see stdout logs right now. We can keep running with our fork until #309 is resolved.

As far as requirements I'd like to see for #309:

• I'd like to be able to do structured logging. JSON, in our case, but full control over formatting would be best.
• I'd like to be able to specify the device I'm writing to. Originally I did this with a dyn Writer, but for FFI you might need to do something else. At a minimum, I would need stdout.
• I'd like to be able to specify how the writing happens. A non-blocking writer like I mentioned is important for high volume logging.

eta:

We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

That would be very trivially reversed, and that should hold true for any implementation that's extensible enough. Maybe that's a good litmus test for customization, as well.

[tnull]

but if you've already decided you want to use log, then that makes sense.

Yes, we probably will go this way at least here in LDK Node, but possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log, i.e., especially in larger projects with multiple producers LDK stands out as the one logging to its own thing while all other logs are in a single stream. So IMO we should at least try to support it in LDK also.

We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

Right, you can also expose trait objects arguments via Uniffi, but they need to adhere to some rules, i.e., we'll likely need something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder or similar.

As far as requirements I'd like to see for #309:

Cool, thanks. Will keep these in mind! My goal for now is to end up with a slightly improved FilesystemLogger and a pluggable interface so that you can do whatever you want in your logger implementation.

[domZippilli]

something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder

Heh, I tried this as well, too bad I tossed the commit. I think that would work as well, although it left me writing some awkward documentation ... along the lines of, "If you specify this, these other logging fields from config are ignored."

[domZippilli]

possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

[tnull]

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

Yeah, that's a good point, maybe even more so regarding the upstream changes as we might need tracing to accommodate our structured logging macros there. That said, there also is tracing-log which should users that need it at least rudimentary compatibility if we use log.

[domZippilli]

Description

Adds logger configuration to ldk-node.

Motivation

The first impulse for this was to be able to write to stdout, as that's more desirable for some containerized infrastructure. But rather than add a specific stdout implementation, we thought it better to make this extensible to write to arbitrary destinations (for example, a network log ingester). This additionally has the benefit of letting you customize how the writing happens; for example the tracing-appender crate uses an mpsc for writers, with a single thread performing all the actual I/O off of the queue.

Details

The PR has three commits:

• Rename FilesystemLogger to LdkNodeLogger. This renames the logger to an implementation-agnostic name. Hopefully this commit is easy to ignore when reviewing implementation.
• Make LdkNodeLogger technically customizable, retain default behavior. This sets up the configurable logger, and then statically configures it to retain the existing behavior.
• Add customizable logging to configuration. This adds configuration points to the Config struct itself. Users of the ::default() configuration should see no change in behavior.

[tnull]

Thanks for looking into this and I def. agree we need to add the capability for custom loggers (see #309) but I'm not sure we want to go with the approach chosen here.

As described over at #309, we probably want to try using the log crate facade if possible (at least for the Rust side), to which end we will try to take an optional dependency upstream. Moreover, the custom logger interface will need to be exposable via UniFFI bindings, which doesn't work with the approach chosen here. We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

Let me know if that makes sense and also what additional requirements you see for a custom logger interface. If you intend to keep working on this, you might want to coordinate with @enigbe which just recently indicated interest in working on it / #309, too.

[domZippilli]

Ah, I see. I was trying to avoid taking a dependency, but if you've already decided you want to use log, then that makes sense. We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

We originally did this in our local fork, as we can only see stdout logs right now. We can keep running with our fork until #309 is resolved.

As far as requirements I'd like to see for #309:

• I'd like to be able to do structured logging. JSON, in our case, but full control over formatting would be best.
• I'd like to be able to specify the device I'm writing to. Originally I did this with a dyn Writer, but for FFI you might need to do something else. At a minimum, I would need stdout.
• I'd like to be able to specify how the writing happens. A non-blocking writer like I mentioned is important for high volume logging.

eta:

We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

That would be very trivially reversed, and that should hold true for any implementation that's extensible enough. Maybe that's a good litmus test for customization, as well.

[tnull]

but if you've already decided you want to use log, then that makes sense.

Yes, we probably will go this way at least here in LDK Node, but possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log, i.e., especially in larger projects with multiple producers LDK stands out as the one logging to its own thing while all other logs are in a single stream. So IMO we should at least try to support it in LDK also.

We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

Right, you can also expose trait objects arguments via Uniffi, but they need to adhere to some rules, i.e., we'll likely need something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder or similar.

As far as requirements I'd like to see for #309:

Cool, thanks. Will keep these in mind! My goal for now is to end up with a slightly improved FilesystemLogger and a pluggable interface so that you can do whatever you want in your logger implementation.

[domZippilli]

something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder

Heh, I tried this as well, too bad I tossed the commit. I think that would work as well, although it left me writing some awkward documentation ... along the lines of, "If you specify this, these other logging fields from config are ignored."

[domZippilli]

possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

[tnull]

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

Yeah, that's a good point, maybe even more so regarding the upstream changes as we might need tracing to accommodate our structured logging macros there. That said, there also is tracing-log which should users that need it at least rudimentary compatibility if we use log.

[domZippilli]

Description

Adds logger configuration to ldk-node.

Motivation

The first impulse for this was to be able to write to stdout, as that's more desirable for some containerized infrastructure. But rather than add a specific stdout implementation, we thought it better to make this extensible to write to arbitrary destinations (for example, a network log ingester). This additionally has the benefit of letting you customize how the writing happens; for example the tracing-appender crate uses an mpsc for writers, with a single thread performing all the actual I/O off of the queue.

Details

The PR has three commits:

• Rename FilesystemLogger to LdkNodeLogger. This renames the logger to an implementation-agnostic name. Hopefully this commit is easy to ignore when reviewing implementation.
• Make LdkNodeLogger technically customizable, retain default behavior. This sets up the configurable logger, and then statically configures it to retain the existing behavior.
• Add customizable logging to configuration. This adds configuration points to the Config struct itself. Users of the ::default() configuration should see no change in behavior.

[tnull]

Thanks for looking into this and I def. agree we need to add the capability for custom loggers (see #309) but I'm not sure we want to go with the approach chosen here.

As described over at #309, we probably want to try using the log crate facade if possible (at least for the Rust side), to which end we will try to take an optional dependency upstream. Moreover, the custom logger interface will need to be exposable via UniFFI bindings, which doesn't work with the approach chosen here. We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

Let me know if that makes sense and also what additional requirements you see for a custom logger interface. If you intend to keep working on this, you might want to coordinate with @enigbe which just recently indicated interest in working on it / #309, too.

[domZippilli]

Ah, I see. I was trying to avoid taking a dependency, but if you've already decided you want to use log, then that makes sense. We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

We originally did this in our local fork, as we can only see stdout logs right now. We can keep running with our fork until #309 is resolved.

As far as requirements I'd like to see for #309:

• I'd like to be able to do structured logging. JSON, in our case, but full control over formatting would be best.
• I'd like to be able to specify the device I'm writing to. Originally I did this with a dyn Writer, but for FFI you might need to do something else. At a minimum, I would need stdout.
• I'd like to be able to specify how the writing happens. A non-blocking writer like I mentioned is important for high volume logging.

eta:

We also want to make the FilesystemLogger play nicely with OS-level tooling such as logrotate as a first step, which would necessitate adding signal handling if we keep the open file handle around, which this PR also adds.

That would be very trivially reversed, and that should hold true for any implementation that's extensible enough. Maybe that's a good litmus test for customization, as well.

[tnull]

but if you've already decided you want to use log, then that makes sense.

Yes, we probably will go this way at least here in LDK Node, but possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log, i.e., especially in larger projects with multiple producers LDK stands out as the one logging to its own thing while all other logs are in a single stream. So IMO we should at least try to support it in LDK also.

We also wanted to avoid chasing generics all over the code, so the Fn trait objects were good, at least for the Rust side.

Right, you can also expose trait objects arguments via Uniffi, but they need to adhere to some rules, i.e., we'll likely need something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder or similar.

As far as requirements I'd like to see for #309:

Cool, thanks. Will keep these in mind! My goal for now is to end up with a slightly improved FilesystemLogger and a pluggable interface so that you can do whatever you want in your logger implementation.

[domZippilli]

something like set_custom_logger(&self, logger: Arc<dyn CustomLogger + Send + Sync>) method on the Builder

Heh, I tried this as well, too bad I tossed the commit. I think that would work as well, although it left me writing some awkward documentation ... along the lines of, "If you specify this, these other logging fields from config are ignored."

[domZippilli]

possibly also at least optionally upstream as users keep stumbling across the fact that LDK doesn't use log

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

[tnull]

P.S. -- If you do this, give some thought to the tracing user experience. Especially if you're targeting larger nodes, I think a lot of them would end up trying to drop-in tracing in place of log.

Yeah, that's a good point, maybe even more so regarding the upstream changes as we might need tracing to accommodate our structured logging macros there. That said, there also is tracing-log which should users that need it at least rudimentary compatibility if we use log.