Add structured logging context fields to LogRecord #751
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
joostjager
force-pushed
the
structured-logging
branch
from
262db08 to
c74331f
Compare
joostjager
force-pushed
the
structured-logging
branch
2 times, most recently
from
59d9bb1 to
22c37aa
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
One comment, otherwise looks good.
While you're at it, mind also making the change to ldk-server as well? See https://github.com/lightningdevkit/ldk-server/blob/f3eaacd327d40fc8ee3fd7f6fbaccb04fa077434/ldk-server/src/util/logger.rs#L86-L129
|
|
||
| let log = format!( | ||
| "{} {:<5} [{}:{}] {}\n", | ||
| "{} {:<5} [{}:{}] {}{}\n", |
There was a problem hiding this comment.
Just appending it plain looks a bit odd for some messages:
Persistence of ChannelMonitorUpdate id 13 completed ch:d6487d p:02d87e
Can we maybe wrap the context in parentheses or brackets?
There was a problem hiding this comment.
Parentheses added. Also made the implementation a bit more efficient by avoiding some heap allocs.
joostjager
force-pushed
the
structured-logging
branch
from
22c37aa to
3a73173
Compare
| /// Implements `Display` to format context fields (channel_id, peer_id, payment_hash) directly | ||
| /// into a formatter, avoiding intermediate heap allocations when used with `format_args!` or | ||
| /// `write!` macros. | ||
| pub struct LogContext<'a> { |
There was a problem hiding this comment.
Why does this need to be pub? Seems it's an internal utility only? When you drop it, please also drop the redundant doc comments here and on the Display impl.
There was a problem hiding this comment.
For ldk-node users that have their own logger implementation, but also want to reuse the context fmting?
There was a problem hiding this comment.
For ldk-node users that have their own logger implementation, but also want to reuse the context fmting?
Hmm, so do you plan to use it for LDK Server? If not, I'd say we can make it private as other users implementation might look differently. If we want to keep it pub for reuse in Server, let's still drop the docs on impl Display and also the entire pub fn new() which is just unnecessary (and to a degree an anti-pattern) if you have only pub struct fields anyways.
There was a problem hiding this comment.
Yes, plan to use in server. Dropped new and moved Display docs.
joostjager
force-pushed
the
structured-logging
branch
2 times, most recently
from
db45dff to
877bb46
Compare
Extend LogRecord with peer_id, channel_id, and payment_hash fields from LDK's Record struct. These structured fields are now available to custom LogWriter implementations and are automatically appended to log messages by the built-in FileWriter and LogFacadeWriter. - Add peer_id, channel_id, payment_hash fields to LogRecord (both uniffi and non-uniffi versions) - Add LogContext struct with Display impl to format fields with truncated hex values, avoiding intermediate heap allocations - Update FileWriter and LogFacadeWriter to append context to messages - Update UDL bindings with new LogRecord fields - Add unit tests for LogContext and LogFacadeWriter Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
joostjager
force-pushed
the
structured-logging
branch
from
877bb46 to
4f86a64
Compare
3 participants
Extend LogRecord with peer_id, channel_id, and payment_hash fields from
LDK's Record struct. These structured fields are now available to custom
LogWriter implementations and are automatically appended to log messages
by the built-in FileWriter and LogFacadeWriter.
and non-uniffi versions)
hex values, avoiding intermediate heap allocations
Closes #712