Skip to content

Introduce Info and separate info/debug strings#328

Merged
rustaceanrob merged 1 commit intomasterfrom
log-info-4-11
Apr 13, 2025
Merged

Introduce Info and separate info/debug strings#328
rustaceanrob merged 1 commit intomasterfrom
log-info-4-11

Conversation

@rustaceanrob
Copy link
Copy Markdown
Collaborator

@rustaceanrob rustaceanrob commented Apr 11, 2025

Description

While it might be painful to have 4 different ways to handle messages, I think this design is worth the performance improvements if one would like to completely remove string allocations by omitting the debug messages. Informational messages can be useful but are far less chatty, and may potentially be expanded into the future (like positive matches for filters).

For a desktop application I don't see much harm in using the debug strings and writing to a log, but mobile users can't use the strings for much at all so they should have a way to omit them, but still get updates on the changes in state and such.

Implementation

  • Introduces a info! macro similar to log!, so informational messages aren't allocated with LogLevel::Warning set.
  • Adds a new receiver type and changes the Log enum to Info. The debug messages are emitted as strings and Info is an enum with the rest of the events

@rustaceanrob rustaceanrob force-pushed the log-info-4-11 branch 2 times, most recently from 59317e1 to f013f7d Compare April 12, 2025 11:14
@rustaceanrob rustaceanrob marked this pull request as ready for review April 12, 2025 11:43
};
}

macro_rules! info {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the structure and relatively exhaustive things sent over the info channel, should it just be detached from the LogLevel setting entirely? I might be missing a benefit, but decoupling it would simplify this code (could drop this macro?) and some conditionals.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing the 4 channels for the user interface at this point, I get where this change is coming from, but does feel a little pre-optimized. Does it make sense to combine the Info and Event channels to try and keep things simple?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an argument to be made for that, although I kind of envision the applications in 3 tiers:

  1. desktop or terminal app that writes to a debug log
  2. mobile or desktop application that adjusts UI components based on info/warnings
  3. a sort of "headless" node that just runs in the background unless it dies, like in ldk-node there's really not much of a use in having info messages because it would be so deeply nested in the syncing process

Maybe it's over kill, but the Progress variant of Info in particular might be a little wasted if emitted for all types of apps. The nice thing about the LogLevel construct is you get exactly the memory/CPU cycles you sign up for, so I'd like to leave these coupled for now

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to combine the Info and Event channels to try and keep things simple?

The original type of message was simply NodeMessage, and this caused havoc trying to match between things that were mostly unimportant and events like blocks. Doesn't feel so great to look at all these channels but the alternative is the inability to separate your program into "this part handles blocks" and "this part handles logs" type of thing

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm ok, I guess the "single receiver" constraint of channels kind of forces this opinionated separation of events.

Copy link
Copy Markdown
Collaborator Author

@rustaceanrob rustaceanrob Apr 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a real-world use of constraining channels to specific messages, check out what we do it bdk_kyoto to build updates for the wallet. If the Event included other things, it might be difficult to pass those on to the user. In that struct we really only care about blocks and reorgs anyway

Copy link
Copy Markdown
Collaborator

@nyonson nyonson Apr 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, if you stick to one channel you force the caller to make there own little downstream channels on their side. If the split is obvious enough that each caller would create the same 3 internal channels, probably makes sense to define them on the Kyoto side

Copy link
Copy Markdown
Collaborator

@nyonson nyonson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 31b35dd

@rustaceanrob rustaceanrob merged commit 311c5ec into master Apr 13, 2025
12 checks passed
@rustaceanrob rustaceanrob deleted the log-info-4-11 branch April 13, 2025 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants