gui: display info-level logs when starting daemon #748
Conversation
|
Concept ACK |
jp1ac4
force-pushed
the
show-daemon-info-level-logs
branch
from
7516ba3 to
6f09708
Compare
|
I tested it on mainnet and it works fine. I'll let @edouardparis judge the approach (esp whether introducing this new dependency is necessary). I wanted to have this in v3, but i'm starting to lean toward having it in v4 instead given the bugs we've got to deal with already in v3. |
|
Is there anything left to do here? I don't know about the implementation, but i unloaded my lianad watchonly wallet from my local mainnet node for a couple days in order to test this and it worked like a charm. It displayed "Loading watchonly wallet." for around 10 seconds before starting up while before it would just appear to be freezing. |
|
That's great to hear. I don't have any further changes planned for now. I'll change it to ready for review. |
|
Just a self reminder to re-assess the approach here in light of the bugs with the way we were connecting the logs from bitcoind: #773. |
Since this is using an unbounded channel it appears it would never create a deadlock of the kind we had with bitcoind's logs? |
I suppose not, unless there's some external "global limit" imposed on its capacity, although we won't be logging as much as bitcoind's IBD so would be less likely to reach it in any case. But otherwise, as we won't be reading from the channel most of the time, is there a chance it might grow too large and consume lots of memory? Ideally there'd be a way to close the channel when we're done with it. |
| fn record_debug(&mut self, field: &tracing::field::Field, value: &dyn std::fmt::Debug) { | ||
| if field.name() == "message" { | ||
| let msg = format!("{:?}", value); | ||
| let _ = self.sender.send(msg); |
There was a problem hiding this comment.
We should check error indicating that the other side of the channel is disconnected in order to stop sending message in it.
There was a problem hiding this comment.
I haven't found a way to use the error to tell this layer to stop being triggered. I was trying to set a flag of some kind, but the on_event method only takes &self so I didn't manage to.
There was a problem hiding this comment.
With my latest changes, we may not need to do anything here as we only send messages to the channel while start_bitcoind_and_daemon is running.
gui/src/logger.rs
Outdated
| @@ -36,6 +39,7 @@ pub struct Logger { | |||
| Registry, | |||
| >, | |||
| level_handle: reload::Handle<filter::LevelFilter, Registry>, | |||
| receiver: crossbeam_channel::Receiver<String>, | |||
There was a problem hiding this comment.
I believe it should be removed.
the layer can be added in set_running_mode and the receiver returned as Result of the method
There was a problem hiding this comment.
If not possible, change it in Option<> and use take to pass it to loader.
The ideal would be that this Receiver is dropped once loader changed its state.
There was a problem hiding this comment.
When adding it in set_running_mode, I'm not sure how to make it active without calling init as is done in setup. The subscriber returns a message that it's trying to read from a disconnected channel (and this message is shown in the GUI on the loader screen).
The second approach using take seems to work in terms of making the channel become disconnected once the loader has completed (or shortly thereafter).
There was a problem hiding this comment.
We might be able to create an mpsc channel inside the iced subscription closure itself as in this example:
https://github.com/iced-rs/iced/blob/dd249a1d11c68b8fee1828d58bae158946ee2ebd/futures/src/subscription.rs#L286-L355
Then the channel should be dropped once the loader completes.
We would pass the sender back to the loader and would just need a way to subscribe to log events while the loader is running. Perhaps using the layer with https://docs.rs/tracing/latest/tracing/dispatcher/fn.set_default.html.
|
Thanks for the review. I'm trying to make those changes currently. Just wondering whether it would also be worth considering a similar approach as for the bitcoind logs and reading the GUI log file? |
|
Reading the logs from the file we write them to certainly sounds more roundabout and brittle (as in there is more moving parts, more could go wrong). Another avenue i thought of but don't know whether it's possible is to "hook" the |
jp1ac4
force-pushed
the
show-daemon-info-level-logs
branch
from
6f09708 to
8bb05df
Compare
|
I've made a change so that only the function I didn't manage yet to wrap the channel layer around the global default subscriber, which means that while |
|
I've set this back to draft for now as it needs further work to keep existing logging while the daemon is starting. |
This is to resolve #475.
I've tried to follow the approach suggested in #475 (comment).
I was having trouble using a
mpsc::channelin theiced::Subscriptionclosure as it can't be shared between threads, so for now I've created a channel usingcrossbeam_channel::unboundedinstead. Perhaps there's a different way of creating the subscription that would allow me to usempsc::channel.I'm using the same
Messagevariant for both Liana and bitcoind logs, and distinguishing between them inon_log()based on the step. I could also use a different message for each case.