Use log facade and log4rs for logging. #44
Conversation
G8XSU
commented
Feb 13, 2025
G8XSU
commented
- Pending Ldk-node upgrade, hence draft.
ldk-server/Cargo.toml
Outdated
| @@ -4,6 +4,8 @@ version = "0.1.0" | |||
| edition = "2021" | |||
|
|
|||
| [dependencies] | |||
| log = "0.4.25" | |||
| log4rs = "1.3.0" | |||
There was a problem hiding this comment.
This has a quite a big dependency tree, also single-author stuff like https://github.com/sfackler/rust-log-mdc.
Are we sure there is no feasible alternative? Or do we even want to implement a bare bones logger ourselves, it's really straightforward (see the MockLogger in lightningdevkit/ldk-node#463).
There was a problem hiding this comment.
Disabling all features does cut down the dep tree significantly.
And if we don't use rolling_file, we can cut down further dependencies. There are alternative implementations but log4rs seems pretty widely used and can serve our needs for now.
We will evaluate if we should replace everything with tracing but for now this is a good crate to depend on for basic logging. I don't feel we should invest too much effort into custom logging implementation as of now.
for [rust-log-mdc](https://github.com/sfackler/rust-log-mdc/tree/master), it looks like it is a simple thread local map for variables.
There was a problem hiding this comment.
Ugh, sorry for the delay here, this kinda fell of my radar for a bit.
Disabling all features does cut down the dep tree significantly.
Cool, it's still considerable, and for some of them I don't know why a simple logger would need to use them...
We will evaluate if we should replace everything with tracing but for now this is a good crate to depend on for basic logging. I don't feel we should invest too much effort into custom logging implementation as of now.
Okay, I'm just not sure what features we really need that would warrant taking on the downstream dependencies of log4rs vs. simpler solutions like simplelog for example. I guess if we may reconsider the choice in the future, we may be fine with reducing dependencies/features for now. But IMO we really should get back to it and replace it with our own implementation if we don't end up switching to tracing, for example.
That said, especially if we're considering this to be just the initial step to be revisited soon, I don't know why we don't opt for the simplest implementation, that doesn't introduce a considerable dependency tree?
for
[rust-log-mdc](https://github.com/sfackler/rust-log-mdc/tree/master), it looks like it is a simple thread local map for variables.
Well, I don't care so much about what it is, but that it is crate with no community besides a single author, which means they could of course unilaterally decide to publish a malicious version (not saying this particular author might ever do that, of course), and the chances of anybody catching that (in time) would be very very slim.
There was a problem hiding this comment.
simplelog doesn't work for us as an initial step.
Ldk server will be a long running process, hence i consider rolling file based logging a bare minimum for now.
Moreover, simplelog is also a single author crate and less widely used than log4rs.
I guess if we may reconsider the choice in the future, we may be fine with reducing dependencies/features for now. But IMO we really should get back to it and replace it with our own implementation if we don't end up switching to tracing, for example.
Yes, this is exactly the plan, we can either switch to custom impl which supports rolling file logging or go for tracing.
If you feel strongly about this, I could do a quick follow up to have rolling file based logging with tracing dependencies, without using any of the other tracing features if that helps.
There was a problem hiding this comment.
simplelogdoesn't work for us as an initial step.
Ldk server will be a long running process, hence i consider rolling file based logging a bare minimum for now.
IIUC the idea is to take the follow-up steps before this would ever run in production, no? Also, note that for making Server a good *IX citizen, we could at least consider actually integrating with syslog and logrotate rather than rolling our own?
Moreover,
simplelogis also a single author crate
It's not? Github lists 38 contributors? That said, simplelog was merely an example of taking a simpler approach as a fill-in until we land on the actual logger.
If you feel strongly about this, I could do a quick follow up to have rolling file based logging with tracing dependencies, without using any of the other tracing features if that helps.
That would be great, although I have no strong opinion on the timeline, if we were to start out with a real simple fill-in logger. If you would prefer to roll with log4rs for now, I think we should make sure to actually get around to drop the dependencies further (in general btw), before releasing the 'official' 0.1.
Given how often security incidents related to malicious dependencies keep coming up, we should be very deliberate with what we take on. Btw. I'm considering whether to finally introduce features for LDK Node (likely with 0.6), which would at least allow us to compartmentalize the risk from (e.g, Server could avoid the dependency tree stemming from Electrum / Esplora backends, if it only requires bitcoind RPC support.
That's not true. If you have a look at the linked LDK Node PR above, a bare-bones |
| @@ -36,6 +36,8 @@ const USAGE_GUIDE: &str = "Usage: ldk-server <config_path>"; | |||
| fn main() { | |||
| let args: Vec<String> = std::env::args().collect(); | |||
|
|
|||
| log4rs::init_file("./ldk-server/log4rs.toml", Default::default()).unwrap(); | |||
There was a problem hiding this comment.
It seems weird to hard-code this relative path in the binary. Do we expect to always start the server from the Git-workspace's directory, rather than the ldk-server directory, for example? Can we read this from the config at least?
