-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Allow adding a prefix to the informant #6174
Changes from 4 commits
6c3a2c0
9c2e726
c966d4b
a3c306e
d3c1d52
cd86c58
6c0029b
ed89283
ad18c29
ff258dc
f5cf997
1b02761
b88ed95
b81f8ff
a04e788
cb8e402
5dceb40
bf7e144
d8daabb
4133e3c
23d775e
f006ccf
b5dd8d8
e32866a
1b38e33
5523518
36c4148
9728c25
036a1ea
f494058
6ba7627
c1ec56a
d334904
f73afa4
89a9c2b
a82f2cb
f75b06a
f1bcacb
38a4f27
bb7c7c2
89b5df8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,16 +67,18 @@ impl<B: BlockT> InformantDisplay<B> { | |
self.last_update = Instant::now(); | ||
self.last_number = Some(best_number); | ||
|
||
let (status, target) = match (net_status.sync_state, net_status.best_seen_block) { | ||
(SyncState::Idle, _) => ("💤 Idle".into(), "".into()), | ||
(SyncState::Downloading, None) => (format!("⚙️ Preparing{}", speed), "".into()), | ||
(SyncState::Downloading, Some(n)) => (format!("⚙️ Syncing{}", speed), format!(", target=#{}", n)), | ||
let (level, status, target) = match (net_status.sync_state, net_status.best_seen_block) { | ||
(SyncState::Idle, _) => ("💤", "Idle".into(), "".into()), | ||
(SyncState::Downloading, None) => ("⚙️ ", format!("Preparing{}", speed), "".into()), | ||
(SyncState::Downloading, Some(n)) => ("⚙️ ", format!("Syncing{}", speed), format!(", target=#{}", n)), | ||
}; | ||
|
||
if self.format == OutputFormat::Coloured { | ||
info!( | ||
match &self.format { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in e32866a |
||
OutputFormat::Coloured { prefix } => info!( | ||
target: "substrate", | ||
"{}{} ({} peers), best: #{} ({}), finalized #{} ({}), {} {}", | ||
"{} {}{}{} ({} peers), best: #{} ({}), finalized #{} ({}), {} {}", | ||
level, | ||
prefix, | ||
Colour::White.bold().paint(&status), | ||
target, | ||
Colour::White.bold().paint(format!("{}", num_connected_peers)), | ||
|
@@ -86,11 +88,12 @@ impl<B: BlockT> InformantDisplay<B> { | |
info.chain.finalized_hash, | ||
Colour::Green.paint(format!("⬇ {}", TransferRateFormat(net_status.average_download_per_sec))), | ||
Colour::Red.paint(format!("⬆ {}", TransferRateFormat(net_status.average_upload_per_sec))), | ||
); | ||
} else { | ||
info!( | ||
), | ||
OutputFormat::Plain { prefix } => info!( | ||
target: "substrate", | ||
"{}{} ({} peers), best: #{} ({}), finalized #{} ({}), ⬇ {} ⬆ {}", | ||
"{} {}{}{} ({} peers), best: #{} ({}), finalized #{} ({}), ⬇ {} ⬆ {}", | ||
level, | ||
prefix, | ||
status, | ||
target, | ||
num_connected_peers, | ||
|
@@ -100,7 +103,7 @@ impl<B: BlockT> InformantDisplay<B> { | |
info.chain.finalized_hash, | ||
TransferRateFormat(net_status.average_download_per_sec), | ||
TransferRateFormat(net_status.average_upload_per_sec), | ||
); | ||
), | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,18 +29,22 @@ use std::time::Duration; | |
mod display; | ||
|
||
/// The format to print telemetry output in. | ||
#[derive(PartialEq)] | ||
#[derive(Clone)] | ||
pub enum OutputFormat { | ||
Coloured, | ||
Plain, | ||
Coloured { | ||
prefix: String, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the prefix part of these variants? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense to me. Maybe it would make more sense to have this: struct Format {
color: bool,
prefix: String,
} But I noticed that we don't really need the distinction between coloured and plain at all because the colors are strips anyway. (As I said here: #6174 (comment) ) I think we should delete it and keep only the prefix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like color stripping is a bug? The new struct looks more reasonable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok for the new struct.
No I think it is made on purpose: f77b3e3#diff-4b08e5c0745ff570ad2a3be5fdaf2b5aR477 Plus it kinda makes sense to format everything with colors and strip them automatically so you don't have to maintain 2 pathways. @arkpar do you have any comment on this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Color is stripped to make sure it is not written to non-terminal outputs, such as file. You can have the same log string be printed in the console with color, and duplicated to stderr with color stripped. |
||
}, | ||
Plain { | ||
prefix: String, | ||
}, | ||
} | ||
|
||
/// Creates an informant in the form of a `Future` that must be polled regularly. | ||
cecton marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pub fn build(service: &impl AbstractService, format: OutputFormat) -> impl futures::Future<Output = ()> { | ||
let client = service.client(); | ||
let pool = service.transaction_pool(); | ||
|
||
let mut display = display::InformantDisplay::new(format); | ||
let mut display = display::InformantDisplay::new(format.clone()); | ||
|
||
let display_notifications = service | ||
.network_status(Duration::from_millis(5000)) | ||
|
@@ -97,7 +101,18 @@ pub fn build(service: &impl AbstractService, format: OutputFormat) -> impl futur | |
last_best = Some((n.header.number().clone(), n.hash.clone())); | ||
} | ||
|
||
info!(target: "substrate", "✨ Imported #{} ({})", Colour::White.bold().paint(format!("{}", n.header.number())), n.hash); | ||
match &format { | ||
OutputFormat::Coloured { prefix } => info!( | ||
target: "substrate", | ||
"✨ {}Imported #{} ({})", | ||
prefix, Colour::White.bold().paint(n.header.number().to_string()), n.hash, | ||
), | ||
OutputFormat::Plain { prefix } => info!( | ||
target: "substrate", "✨ {}Imported #{} ({})", | ||
prefix, n.header.number(), n.hash, | ||
), | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: I changed the format for plain, I removed entirely the color There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is already something that strips the colors. Maybe it's not worth keeping the arms like that (here and in the display.rs) and just display the things with colors. I don't want to go to far in a refactoring... but that's reasonable |
||
|
||
future::ready(()) | ||
}); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that a CLI feature?
I think we should move the informant out of the CLI. Aka being activated by the service or being done completely "manual".
If we move it into the service, we could probably enable it by default as it is done currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already in the Service's Configuration! But that struct is generated by the trait ConfigurationCli. Here it's kinda easy to customize because you can add:
In here: https://github.com/paritytech/substrate/blob/master/bin/node/cli/src/command.rs#L24
And it works easily. It will be as easy in cumulus. 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(So the answer is that it is a sc-service feature (makes sense) + a sc-cli feature (for convenience))
Do you have another idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not convinced that this should be done in CLI. We should move it entirely to the service. Have it enabled by default and provide a function to set the prefix in the service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reasoning to have it as a
cli
flag? I can see that for running a custom node, you might want to customize it so – as a developer. But why would a validator or anyone else running the node care to replace it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's first a Service configuration setting as you can see in client/service/src/config.rs: https://github.com/paritytech/substrate/pull/6174/files#diff-fc7dfe9c2ea7011575c8b225844cd030R105
Then to set this setting you can use the CliConfiguration trait OR the SubstrateCli trait.
The only known use case for this feature for now is when you have multiple services running and you want to distinguish them in the logs.
The informant is built there: https://github.com/paritytech/substrate/pull/6174/files#diff-8f956a9810426119c8963e0eb8bd2b74R215 There aren't many alternatives to the current implementation. Maybe I could pass an extra argument to the runner... @bkchr can you detail a bit how you would do?
Sorry I don't know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you understand this now after the DM's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have any use case (we can come up with) I'd rather not pollute the cli with params. From the comment, I understand @bkchr has an idea to structure this differently without involving the cli, which I'd appreciate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move it to the service builder instead. Would that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah