Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Allow adding a prefix to the informant #6174

Merged
merged 41 commits into from
Jun 9, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
6c3a2c0
Initial commit
cecton May 28, 2020
9c2e726
Add a Service Configuration's field + adapt informant + provide means…
cecton May 28, 2020
c966d4b
CLEANUP
cecton May 28, 2020
a3c306e
fix tests
cecton May 28, 2020
d3c1d52
fixed bad path to object
cecton May 28, 2020
cd86c58
Change OutputFormat enum to struct
cecton May 29, 2020
6c0029b
Add informant_prefix to builder and service
cecton Jun 2, 2020
ed89283
Revert "Change OutputFormat enum to struct"
cecton Jun 2, 2020
ad18c29
Revert "fix tests"
cecton Jun 2, 2020
ff258dc
Revert "Add a Service Configuration's field + adapt informant + provi…
cecton Jun 2, 2020
f5cf997
Implementation using the ServiceBuilder
cecton Jun 2, 2020
1b02761
reduce line length
cecton Jun 2, 2020
b88ed95
Merge commit f85fabd753c40041edcaac487f6f1ce259e8d11b (no conflict)
cecton Jun 2, 2020
b81f8ff
fix line width again
cecton Jun 2, 2020
a04e788
WIP
cecton Jun 2, 2020
cb8e402
WIP
cecton Jun 2, 2020
5dceb40
WIP
cecton Jun 2, 2020
bf7e144
use struct instead of enum
cecton Jun 2, 2020
d8daabb
WIP
cecton Jun 2, 2020
4133e3c
Update client/service/src/lib.rs
cecton Jun 3, 2020
23d775e
improve doc
cecton Jun 3, 2020
f006ccf
Update client/service/src/builder.rs
cecton Jun 3, 2020
b5dd8d8
Update client/service/src/builder.rs
cecton Jun 3, 2020
e32866a
change code
cecton Jun 3, 2020
1b38e33
Update client/informant/src/lib.rs
cecton Jun 3, 2020
5523518
enable_color
cecton Jun 3, 2020
36c4148
reorg log
cecton Jun 3, 2020
9728c25
remove macro
cecton Jun 3, 2020
036a1ea
Removed builder for informant prefix
cecton Jun 3, 2020
f494058
fix doc
cecton Jun 3, 2020
6ba7627
Update client/informant/src/lib.rs
cecton Jun 8, 2020
c1ec56a
Update client/informant/src/lib.rs
cecton Jun 8, 2020
d334904
Update client/informant/src/lib.rs
cecton Jun 8, 2020
f73afa4
Update client/informant/src/lib.rs
cecton Jun 8, 2020
89a9c2b
Update client/service/src/builder.rs
cecton Jun 8, 2020
a82f2cb
Update client/service/src/builder.rs
cecton Jun 8, 2020
f75b06a
Update client/service/src/builder.rs
cecton Jun 8, 2020
f1bcacb
Merge commit cc5b3a63fe5adb5100436ff43d5260a2ea18d893 (no conflict)
cecton Jun 9, 2020
38a4f27
Merge commit e287915eb37928491adbf4666e764ea0be5ac21a (conflicts)
cecton Jun 9, 2020
bb7c7c2
Merge commit feb334d87773331c23ef6560a55f43fcb5ef62a8 (no conflict)
cecton Jun 9, 2020
89b5df8
Merge branch 'cecton-informant-prefix' of github.com:paritytech/subst…
cecton Jun 9, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions client/cli/src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ pub use self::purge_chain_cmd::PurgeChainCmd;
pub use self::revert_cmd::RevertCmd;
pub use self::run_cmd::RunCmd;
pub use self::export_state_cmd::ExportStateCmd;
use crate::SubstrateCli;
use std::fmt::Debug;
use structopt::StructOpt;

Expand Down Expand Up @@ -402,6 +403,12 @@ macro_rules! substrate_cli_subcommands {
$($enum::$variant(cmd) => cmd.log_filters()),*
}
}

fn informant_prefix<C: SubstrateCli>(&self) -> $crate::Result<String> {
Copy link
Member

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.

Copy link
Contributor Author

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:

fn informant_prefix() -> &'static str {
    "BOO!"
}

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. 😃

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor

@gnunicorn gnunicorn May 29, 2020

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?

Copy link
Contributor Author

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?

But why would a validator or anyone else running the node care to replace it?

Sorry I don't know.

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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?

I understand @bkchr has an idea to structure this differently without involving the cli, which I'd appreciate.

I can move it to the service builder instead. Would that work?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah

match self {
$($enum::$variant(cmd) => cmd.informant_prefix::<C>()),*
}
}
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions client/cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,11 @@ pub trait CliConfiguration: Sized {
Ok(true)
}

/// A prefix for the informant's logs
fn informant_prefix<C: SubstrateCli>(&self) -> Result<String> {
Ok(C::informant_prefix().to_string())
}

/// Create a Configuration object from the current object
fn create_configuration<C: SubstrateCli>(
&self,
Expand Down Expand Up @@ -464,6 +469,7 @@ pub trait CliConfiguration: Sized {
max_runtime_instances,
announce_block: self.announce_block()?,
role,
informant_prefix: self.informant_prefix::<C>()?,
})
}

Expand Down
5 changes: 5 additions & 0 deletions client/cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ pub trait SubstrateCli: Sized {
/// Chain spec factory
fn load_spec(&self, id: &str) -> std::result::Result<Box<dyn ChainSpec>, String>;

/// A prefix for the informant's logs
fn informant_prefix() -> &'static str {
""
}

/// Helper function used to parse the command line arguments. This is the equivalent of
/// `structopt`'s `from_iter()` except that it takes a `VersionInfo` argument to provide the name of
/// the application, author, "about" and version. It will also set `AppSettings::GlobalVersion`.
Expand Down
5 changes: 4 additions & 1 deletion client/cli/src/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,12 @@ impl<C: SubstrateCli> Runner<C> {
F: FnOnce(Configuration) -> std::result::Result<T, sc_service::error::Error>,
T: AbstractService + Unpin,
{
let prefix = self.config.informant_prefix.clone();
let service = service_builder(self.config)?;

let informant_future = sc_informant::build(&service, sc_informant::OutputFormat::Coloured);
let informant_future = sc_informant::build(&service, sc_informant::OutputFormat::Coloured {
prefix,
});
let _informant_handle = self.tokio_runtime.spawn(informant_future);

// we eagerly drop the service so that the internal exit future is fired,
Expand Down
27 changes: 15 additions & 12 deletions client/informant/src/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

if self.format.colors

Copy link
Contributor Author

@cecton cecton Jun 3, 2020

Choose a reason for hiding this comment

The 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)),
Expand All @@ -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,
Expand All @@ -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),
);
),
}
}
}
Expand Down
25 changes: 20 additions & 5 deletions client/informant/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

Why is the prefix part of these variants?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok for the new struct.

Sounds like color stripping is a bug?

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?

Copy link
Member

Choose a reason for hiding this comment

The 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))
Expand Down Expand Up @@ -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,
),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I changed the format for plain, I removed entirely the color

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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(())
});

Expand Down
2 changes: 2 additions & 0 deletions client/service/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ pub struct Configuration {
pub max_runtime_instances: usize,
/// Announce block automatically after they have been imported
pub announce_block: bool,
/// A prefix for the informant's logs
pub informant_prefix: String,
}

/// Type for tasks spawned by the executor.
Expand Down