-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Allow adding a prefix to the informant #6174
Changes from 3 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 { | ||
|
||
| 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, | ||
|
||
| }, | ||
| Plain { | ||
| prefix: String, | ||
| }, | ||
| } | ||
|
|
||
| /// Creates an informant in the form of a `Future` that must be polled regularly. | ||
cecton marked this conversation as resolved.
Outdated
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, | ||
| ), | ||
| } | ||
|
||
|
|
||
| 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.
Uh oh!
There was an error while loading. Please reload this page.
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
cliflag? 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