Doc fixes for sc-telemetry & API struct rename (#7934)

* Doc fixes for sc-telemetry

* Fix flag to disable log reloading

* Forgot to reverse the conditions

* Apply suggestion

* Rename pattern to directives

* Rename GlobalLoggerBuilder to LoggerBuilder

* Return instead of expect

* Use transparent outside the enum

* Update client/tracing/src/logging/directives.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/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<DCV: DefaultConfigurationValues = ()>: Sized {
 	fn init<C: SubstrateCli>(&self) -> Result<sc_telemetry::TelemetryWorker> {
 		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()? {

substrate/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;

substrate/client/cli/src/runner.rs

@@ -239,7 +239,7 @@ impl<C: SubstrateCli> Runner<C> {
 
 	/// 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()
 	}

substrate/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") {

substrate/client/service/src/builder.rs

@@ -308,11 +308,7 @@ pub fn new_full_parts<TBl, TRtApi, TExecDisp>(
 {
 	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<TBl, TRtApi, TExecDisp>(
 	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())?

substrate/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.

substrate/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<TaskManager>,
-	/// 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<TelemetrySpan>,
 }
 

substrate/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

substrate/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.

substrate/client/telemetry/src/layer.rs

@@ -35,7 +35,7 @@ pub struct TelemetryLayer(Mutex<mpsc::Sender<(Id, u8, String)>>);
 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.

substrate/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 <https://www.gnu.org/licenses/>.
 
-//! 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(),

substrate/client/tracing/src/logging/directives.rs

@@ -114,7 +114,7 @@ pub(crate) fn set_reload_handle(handle: Handle<EnvFilter, SCSubscriber>) {
 	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,

substrate/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;

substrate/client/tracing/src/logging/mod.rs

@@ -53,21 +53,15 @@ pub type Result<T> = std::result::Result<T, Error>;
 #[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<N, E, F, W>(
-	pattern: &str,
+fn prepare_subscriber<N, E, F, W>(
+	directives: &str,
 	max_level: Option<log::LevelFilter>,
 	force_colors: Option<bool>,
 	telemetry_buffer_size: Option<usize>,
@@ -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::<FmtSubscriber>::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<usize>,
 	telemetry_external_transport: Option<ExtTransport>,
-	disable_log_reloading: bool,
+	log_reloading: bool,
 	force_colors: Option<bool>,
 }
 
-impl GlobalLoggerBuilder {
-	/// Create a new [`GlobalLoggerBuilder`] which can be used to initialize the global logger.
-	pub fn new<S: Into<String>>(pattern: S) -> Self {
+impl LoggerBuilder {
+	/// Create a new [`LoggerBuilder`] which can be used to initialize the global logger.
+	pub fn new<S: Into<String>>(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);
 		}

substrate/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<sc_telemetry::TelemetryWorker, sc_tracing::logging::Error> {
 	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()
 }