diff --git a/client/cli/src/config.rs b/client/cli/src/config.rs index ae43e8f334c6d..5bc14d52a5f30 100644 --- a/client/cli/src/config.rs +++ b/client/cli/src/config.rs @@ -34,7 +34,7 @@ use sc_service::config::{ }; use sc_service::{ChainSpec, TracingReceiver, KeepBlocks, TransactionStorageMode}; use sc_telemetry::TelemetryHandle; -use sc_tracing::logging::GlobalLoggerBuilder; +use sc_tracing::logging::LoggerBuilder; use std::net::SocketAddr; use std::path::PathBuf; @@ -576,7 +576,7 @@ pub trait CliConfiguration: Sized { fn init(&self) -> Result { sp_panic_handler::set(&C::support_url(), &C::impl_version()); - let mut logger = GlobalLoggerBuilder::new(self.log_filters()?); + let mut logger = LoggerBuilder::new(self.log_filters()?); logger.with_log_reloading(!self.is_log_filter_reloading_disabled()?); if let Some(transport) = self.telemetry_external_transport()? { diff --git a/client/cli/src/lib.rs b/client/cli/src/lib.rs index a4b0bd45727e2..602c53272ea59 100644 --- a/client/cli/src/lib.rs +++ b/client/cli/src/lib.rs @@ -38,7 +38,7 @@ pub use runner::*; pub use sc_service::{ChainSpec, Role}; use sc_service::{Configuration, TaskExecutor}; use sc_telemetry::TelemetryHandle; -pub use sc_tracing::logging::GlobalLoggerBuilder; +pub use sc_tracing::logging::LoggerBuilder; pub use sp_version::RuntimeVersion; use std::io::Write; pub use structopt; diff --git a/client/cli/src/runner.rs b/client/cli/src/runner.rs index 06676655581b1..61a7fe9b01454 100644 --- a/client/cli/src/runner.rs +++ b/client/cli/src/runner.rs @@ -239,7 +239,7 @@ impl Runner { /// Get a new [`TelemetryHandle`]. /// - /// This is used when you want to register a new telemetry for a Substrate node. + /// This is used when you want to register with the [`TelemetryWorker`]. pub fn telemetry_handle(&self) -> TelemetryHandle { self.telemetry_worker.handle() } diff --git a/client/rpc/src/system/tests.rs b/client/rpc/src/system/tests.rs index 89676acae26bf..c196403501035 100644 --- a/client/rpc/src/system/tests.rs +++ b/client/rpc/src/system/tests.rs @@ -344,7 +344,7 @@ fn test_add_reset_log_filter() { // Enter log generation / filter reload if std::env::var("TEST_LOG_FILTER").is_ok() { - sc_tracing::logging::GlobalLoggerBuilder::new("test_before_add=debug").init().unwrap(); + sc_tracing::logging::LoggerBuilder::new("test_before_add=debug").init().unwrap(); for line in std::io::stdin().lock().lines() { let line = line.expect("Failed to read bytes"); if line.contains("add_reload") { diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 3dc716b4e1c9e..63abf8ca9576d 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -308,11 +308,7 @@ pub fn new_full_parts( { let keystore_container = KeystoreContainer::new(&config.keystore)?; - let telemetry_span = if config.telemetry_endpoints.is_some() { - Some(TelemetrySpan::new()) - } else { - None - }; + let telemetry_span = config.telemetry_endpoints.as_ref().map(|_| TelemetrySpan::new()); let task_manager = { let registry = config.prometheus_config.as_ref().map(|cfg| &cfg.registry); TaskManager::new(config.task_executor.clone(), registry, telemetry_span.clone())? @@ -383,11 +379,7 @@ pub fn new_light_parts( TExecDisp: NativeExecutionDispatch + 'static, { let keystore_container = KeystoreContainer::new(&config.keystore)?; - let telemetry_span = if config.telemetry_endpoints.is_some() { - Some(TelemetrySpan::new()) - } else { - None - }; + let telemetry_span = config.telemetry_endpoints.as_ref().map(|_| TelemetrySpan::new()); let task_manager = { let registry = config.prometheus_config.as_ref().map(|cfg| &cfg.registry); TaskManager::new(config.task_executor.clone(), registry, telemetry_span.clone())? diff --git a/client/service/src/config.rs b/client/service/src/config.rs index 74d15cb3fb922..71fbb3a2f2e4e 100644 --- a/client/service/src/config.rs +++ b/client/service/src/config.rs @@ -213,11 +213,8 @@ impl Configuration { return None; } - match self.telemetry_endpoints.as_ref() { - // Don't initialise telemetry if `telemetry_endpoints` == Some([]) - Some(endpoints) if !endpoints.is_empty() => Some(endpoints), - _ => None, - } + // Don't initialise telemetry if `telemetry_endpoints` == Some([]) + self.telemetry_endpoints.as_ref().filter(|x| !x.is_empty()) } /// Returns the network protocol id from the chain spec, or the default. diff --git a/client/service/src/task_manager/mod.rs b/client/service/src/task_manager/mod.rs index 4d9e16d900327..e910e2f3a32e2 100644 --- a/client/service/src/task_manager/mod.rs +++ b/client/service/src/task_manager/mod.rs @@ -233,7 +233,7 @@ pub struct TaskManager { /// terminates and gracefully shutdown. Also ends the parent `future()` if a child's essential /// task fails. children: Vec, - /// A telemetry handle used to enter the telemetry span when a task is spawned. + /// A `TelemetrySpan` used to enter the telemetry span when a task is spawned. telemetry_span: Option, } diff --git a/client/telemetry/README.md b/client/telemetry/README.md index a6b7b654508ae..2e3e19bd2f628 100644 --- a/client/telemetry/README.md +++ b/client/telemetry/README.md @@ -1,21 +1,21 @@ # sc-telemetry -Substrate's client telemetry is a part of substrate that allows logging telemetry information -with a [Polkadot telemetry](https://github.com/paritytech/substrate-telemetry). +Substrate's client telemetry is a part of substrate that allows ingesting telemetry data +with for example [Polkadot telemetry](https://github.com/paritytech/substrate-telemetry). -It works using Tokio's [tracing](https://github.com/tokio-rs/tracing/). The telemetry -information uses tracing's logging to report the telemetry which is then retrieved by a -tracing's `Layer`. This layer will then send the data through an asynchronous channel and to a -background task called [`TelemetryWorker`] which will send the information to the telemetry -server. +It works using Tokio's [tracing](https://github.com/tokio-rs/tracing/) library. The telemetry +information uses tracing's logging to report the telemetry data which is then retrieved by a +tracing `Layer`. This layer will then send the data through an asynchronous channel to a +background task called [`TelemetryWorker`] which will send the information to the configured +remote telemetry servers. -If multiple substrate nodes are running, it uses a tracing's `Span` to identify which substrate -node is reporting the telemetry. Every task spawned using sc-service's `TaskManager` -automatically inherit this span. +If multiple substrate nodes are running in the same process, it uses a `tracing::Span` to +identify which substrate node is reporting the telemetry. Every task spawned using sc-service's +`TaskManager` automatically inherit this span. -Substrate's nodes initialize/register to the [`TelemetryWorker`] using a [`TelemetryHandle`]. +Substrate's nodes initialize/register with the [`TelemetryWorker`] using a [`TelemetryHandle`]. This handle can be cloned and passed around. It uses an asynchronous channel to communicate with -the running [`TelemetryWorker`] dedicated to registration. Registering a telemetry can happen at -any point in time during the execution. +the running [`TelemetryWorker`] dedicated to registration. Registering can happen at any point +in time during the process execution. License: GPL-3.0-or-later WITH Classpath-exception-2.0 diff --git a/client/telemetry/src/endpoints.rs b/client/telemetry/src/endpoints.rs index 7d0338fb18e3c..fe4fa23974a64 100644 --- a/client/telemetry/src/endpoints.rs +++ b/client/telemetry/src/endpoints.rs @@ -25,7 +25,8 @@ use serde::{Deserialize, Deserializer, Serialize}; /// The URL string can be either a URL or a multiaddress. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Hash)] pub struct TelemetryEndpoints( - #[serde(deserialize_with = "url_or_multiaddr_deser")] pub(crate) Vec<(Multiaddr, u8)>, + #[serde(deserialize_with = "url_or_multiaddr_deser")] + pub(crate) Vec<(Multiaddr, u8)>, ); /// Custom deserializer for TelemetryEndpoints, used to convert urls or multiaddr to multiaddr. diff --git a/client/telemetry/src/layer.rs b/client/telemetry/src/layer.rs index eb5eee197770b..0ce3f97620da9 100644 --- a/client/telemetry/src/layer.rs +++ b/client/telemetry/src/layer.rs @@ -35,7 +35,7 @@ pub struct TelemetryLayer(Mutex>); impl TelemetryLayer { /// Create a new [`TelemetryLayer`] and [`TelemetryWorker`]. /// - /// If not provided, the `buffer_size` will be 16 by default. + /// The `buffer_size` defaults to 16. /// /// The [`ExtTransport`] is used in WASM contexts where we need some binding between the /// networking provided by the operating system or environment and libp2p. diff --git a/client/telemetry/src/lib.rs b/client/telemetry/src/lib.rs index 6a4533bb7bc40..7d50461bb9293 100644 --- a/client/telemetry/src/lib.rs +++ b/client/telemetry/src/lib.rs @@ -16,23 +16,23 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -//! Substrate's client telemetry is a part of substrate that allows logging telemetry information -//! with a [Polkadot telemetry](https://github.com/paritytech/substrate-telemetry). +//! Substrate's client telemetry is a part of substrate that allows ingesting telemetry data +//! with for example [Polkadot telemetry](https://github.com/paritytech/substrate-telemetry). //! -//! It works using Tokio's [tracing](https://github.com/tokio-rs/tracing/). The telemetry -//! information uses tracing's logging to report the telemetry which is then retrieved by a -//! tracing's `Layer`. This layer will then send the data through an asynchronous channel and to a -//! background task called [`TelemetryWorker`] which will send the information to the telemetry -//! server. +//! It works using Tokio's [tracing](https://github.com/tokio-rs/tracing/) library. The telemetry +//! information uses tracing's logging to report the telemetry data which is then retrieved by a +//! tracing `Layer`. This layer will then send the data through an asynchronous channel to a +//! background task called [`TelemetryWorker`] which will send the information to the configured +//! remote telemetry servers. //! -//! If multiple substrate nodes are running, it uses a tracing's `Span` to identify which substrate -//! node is reporting the telemetry. Every task spawned using sc-service's `TaskManager` -//! automatically inherit this span. +//! If multiple substrate nodes are running in the same process, it uses a `tracing::Span` to +//! identify which substrate node is reporting the telemetry. Every task spawned using sc-service's +//! `TaskManager` automatically inherit this span. //! -//! Substrate's nodes initialize/register to the [`TelemetryWorker`] using a [`TelemetryHandle`]. +//! Substrate's nodes initialize/register with the [`TelemetryWorker`] using a [`TelemetryHandle`]. //! This handle can be cloned and passed around. It uses an asynchronous channel to communicate with -//! the running [`TelemetryWorker`] dedicated to registration. Registering a telemetry can happen at -//! any point in time during the execution. +//! the running [`TelemetryWorker`] dedicated to registration. Registering can happen at any point +//! in time during the process execution. #![warn(missing_docs)] @@ -115,7 +115,7 @@ pub struct ConnectionMessage { /// Telemetry worker. /// -/// It should be ran as a background task using the [`TelemetryWorker::run`] method. This method +/// It should run as a background task using the [`TelemetryWorker::run`] method. This method /// will consume the object and any further attempts of initializing a new telemetry through its /// handle will fail (without being fatal). #[derive(Debug)] @@ -143,7 +143,7 @@ impl TelemetryWorker { /// Get a new [`TelemetryHandle`]. /// - /// This is used when you want to register a new telemetry for a Substrate node. + /// This is used when you want to register with the [`TelemetryWorker`]. pub fn handle(&self) -> TelemetryHandle { TelemetryHandle { message_sender: self.register_sender.clone(), diff --git a/client/tracing/src/logging/directives.rs b/client/tracing/src/logging/directives.rs index b108566bf2bce..39dee2b061f0a 100644 --- a/client/tracing/src/logging/directives.rs +++ b/client/tracing/src/logging/directives.rs @@ -114,7 +114,7 @@ pub(crate) fn set_reload_handle(handle: Handle) { let _ = FILTER_RELOAD_HANDLE.set(handle); } -// The layered Subscriber as built up in `init_logger()`. +// The layered Subscriber as built up in `LoggerBuilder::init()`. // Used in the reload `Handle`. type SCSubscriber< N = tracing_fmt::format::DefaultFields, diff --git a/client/tracing/src/logging/layers/prefix_layer.rs b/client/tracing/src/logging/layers/prefix_layer.rs index 6aa7e6d436e1a..0c8f25c24100c 100644 --- a/client/tracing/src/logging/layers/prefix_layer.rs +++ b/client/tracing/src/logging/layers/prefix_layer.rs @@ -33,9 +33,18 @@ where S: Subscriber + for<'a> LookupSpan<'a>, { fn new_span(&self, attrs: &Attributes<'_>, id: &Id, ctx: Context<'_, S>) { - let span = ctx - .span(id) - .expect("new_span has been called for this span; qed"); + let span = match ctx.span(id) { + Some(span) => span, + None => { + // this shouldn't happen! + debug_assert!( + false, + "newly created span with ID {:?} did not exist in the registry; this is a bug!", + id + ); + return; + } + }; if span.name() != PREFIX_LOG_SPAN { return; diff --git a/client/tracing/src/logging/mod.rs b/client/tracing/src/logging/mod.rs index ca4f74194bcc6..f74c3e6646073 100644 --- a/client/tracing/src/logging/mod.rs +++ b/client/tracing/src/logging/mod.rs @@ -53,21 +53,15 @@ pub type Result = std::result::Result; #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] #[non_exhaustive] +#[error(transparent)] pub enum Error { - #[error(transparent)] IoError(#[from] io::Error), - - #[error(transparent)] SetGlobalDefaultError(#[from] tracing::subscriber::SetGlobalDefaultError), - - #[error(transparent)] DirectiveParseError(#[from] tracing_subscriber::filter::ParseError), - - #[error(transparent)] SetLoggerError(#[from] tracing_log::log_tracer::SetLoggerError), } -macro_rules! disable_log_reloading { +macro_rules! enable_log_reloading { ($builder:expr) => {{ let builder = $builder.with_filter_reloading(); let handle = builder.reload_handle(); @@ -77,8 +71,8 @@ macro_rules! disable_log_reloading { } /// Common implementation to get the subscriber. -fn get_subscriber_internal( - pattern: &str, +fn prepare_subscriber( + directives: &str, max_level: Option, force_colors: Option, telemetry_buffer_size: Option, @@ -130,10 +124,10 @@ where } } - if pattern != "" { + if directives != "" { // We're not sure if log or tracing is available at this moment, so silently ignore the // parse error. - env_filter = parse_user_directives(env_filter, pattern)?; + env_filter = parse_user_directives(env_filter, directives)?; } let max_level_hint = Layer::::max_level_hint(&env_filter); @@ -196,24 +190,24 @@ where } /// A builder that is used to initialize the global logger. -pub struct GlobalLoggerBuilder { - pattern: String, +pub struct LoggerBuilder { + directives: String, profiling: Option<(crate::TracingReceiver, String)>, telemetry_buffer_size: Option, telemetry_external_transport: Option, - disable_log_reloading: bool, + log_reloading: bool, force_colors: Option, } -impl GlobalLoggerBuilder { - /// Create a new [`GlobalLoggerBuilder`] which can be used to initialize the global logger. - pub fn new>(pattern: S) -> Self { +impl LoggerBuilder { + /// Create a new [`LoggerBuilder`] which can be used to initialize the global logger. + pub fn new>(directives: S) -> Self { Self { - pattern: pattern.into(), + directives: directives.into(), profiling: None, telemetry_buffer_size: None, telemetry_external_transport: None, - disable_log_reloading: false, + log_reloading: true, force_colors: None, } } @@ -230,7 +224,7 @@ impl GlobalLoggerBuilder { /// Wether or not to disable log reloading. pub fn with_log_reloading(&mut self, enabled: bool) -> &mut Self { - self.disable_log_reloading = !enabled; + self.log_reloading = enabled; self } @@ -260,14 +254,14 @@ impl GlobalLoggerBuilder { // If profiling is activated, we require `trace` logging. let max_level = Some(log::LevelFilter::Trace); - if self.disable_log_reloading { - let (subscriber, telemetry_worker) = get_subscriber_internal( - &format!("{},{},sc_tracing=trace", self.pattern, profiling_targets), + if self.log_reloading { + let (subscriber, telemetry_worker) = prepare_subscriber( + &format!("{},{},sc_tracing=trace", self.directives, profiling_targets), max_level, self.force_colors, self.telemetry_buffer_size, self.telemetry_external_transport, - |builder| builder, + |builder| enable_log_reloading!(builder), )?; let profiling = crate::ProfilingLayer::new(tracing_receiver, &profiling_targets); @@ -275,13 +269,13 @@ impl GlobalLoggerBuilder { Ok(telemetry_worker) } else { - let (subscriber, telemetry_worker) = get_subscriber_internal( - &format!("{},{},sc_tracing=trace", self.pattern, profiling_targets), + let (subscriber, telemetry_worker) = prepare_subscriber( + &format!("{},{},sc_tracing=trace", self.directives, profiling_targets), max_level, self.force_colors, self.telemetry_buffer_size, self.telemetry_external_transport, - |builder| disable_log_reloading!(builder), + |builder| builder, )?; let profiling = crate::ProfilingLayer::new(tracing_receiver, &profiling_targets); @@ -290,27 +284,27 @@ impl GlobalLoggerBuilder { Ok(telemetry_worker) } } else { - if self.disable_log_reloading { - let (subscriber, telemetry_worker) = get_subscriber_internal( - &self.pattern, + if self.log_reloading { + let (subscriber, telemetry_worker) = prepare_subscriber( + &self.directives, None, self.force_colors, self.telemetry_buffer_size, self.telemetry_external_transport, - |builder| builder, + |builder| enable_log_reloading!(builder), )?; tracing::subscriber::set_global_default(subscriber)?; Ok(telemetry_worker) } else { - let (subscriber, telemetry_worker) = get_subscriber_internal( - &self.pattern, + let (subscriber, telemetry_worker) = prepare_subscriber( + &self.directives, None, self.force_colors, self.telemetry_buffer_size, self.telemetry_external_transport, - |builder| disable_log_reloading!(builder), + |builder| builder, )?; tracing::subscriber::set_global_default(subscriber)?; @@ -331,8 +325,8 @@ mod tests { const EXPECTED_LOG_MESSAGE: &'static str = "yeah logging works as expected"; const EXPECTED_NODE_NAME: &'static str = "THE_NODE"; - fn init_logger(pattern: &str) { - let _ = GlobalLoggerBuilder::new(pattern).init().unwrap(); + fn init_logger(directives: &str) { + let _ = LoggerBuilder::new(directives).init().unwrap(); } fn run_in_process(test_name: &str) { @@ -351,8 +345,8 @@ mod tests { fn test_logger_filters() { run_in_process("test_logger_filters"); - let test_pattern = "afg=debug,sync=trace,client=warn,telemetry,something-with-dash=error"; - init_logger(&test_pattern); + let test_directives = "afg=debug,sync=trace,client=warn,telemetry,something-with-dash=error"; + init_logger(&test_directives); tracing::dispatcher::get_default(|dispatcher| { let test_filter = |target, level| { @@ -410,8 +404,8 @@ mod tests { #[test] fn log_something_with_dash_target_name() { if env::var("ENABLE_LOGGING").is_ok() { - let test_pattern = "test-target=info"; - let _guard = init_logger(&test_pattern); + let test_directives = "test-target=info"; + let _guard = init_logger(&test_directives); log::info!(target: "test-target", "{}", EXPECTED_LOG_MESSAGE); } diff --git a/utils/browser/src/lib.rs b/utils/browser/src/lib.rs index 5e1e8db316688..b72f2e973b682 100644 --- a/utils/browser/src/lib.rs +++ b/utils/browser/src/lib.rs @@ -25,7 +25,7 @@ use sc_service::{ KeepBlocks, TransactionStorageMode, }; use sc_telemetry::TelemetryHandle; -use sc_tracing::logging::GlobalLoggerBuilder; +use sc_tracing::logging::LoggerBuilder; use wasm_bindgen::prelude::*; use futures::{ prelude::*, channel::{oneshot, mpsc}, compat::*, future::{ready, ok, select} @@ -41,7 +41,7 @@ pub fn init_logging_and_telemetry( pattern: &str, ) -> Result { let transport = ExtTransport::new(ffi::websocket_transport()); - let mut logger = GlobalLoggerBuilder::new(pattern); + let mut logger = LoggerBuilder::new(pattern); logger.with_transport(transport); logger.init() }