[jaeger] unify all used tags, introduce builder pattern, additional… (p…

…aritytech#2473)

* feat/jaeger: unify all used tags, introduce builder pattern, additional candidate annotations

* chores

* fixes, incomplete fn rename

* another fix

* more fixes

* silly doctests

node/core/backing/src/lib.rs

@@ -302,7 +302,9 @@ async fn make_pov_available(
 	};
 
 	{
-		let _span = span.as_ref().map(|s| s.child("erasure-coding"));
+		let _span = span.as_ref().map(|s| {
+			s.child_with_candidate("erasure-coding", &candidate_hash)
+		});
 
 		let chunks = erasure_coding::obtain_chunks_v1(
 			n_validators,
@@ -318,7 +320,10 @@ async fn make_pov_available(
 	}
 
 	{
-		let _span = span.as_ref().map(|s| s.child("store-data"));
+		let _span = span.as_ref().map(|s|
+			s.child_with_candidate("store-data", &candidate_hash)
+		);
+
 		store_available_data(
 			tx_from,
 			validator_index,
@@ -895,9 +900,7 @@ impl CandidateBackingJob {
 		if !self.backed.contains(&hash) {
 			// only add if we don't consider this backed.
 			let span = self.unbacked_candidates.entry(hash).or_insert_with(|| {
-				let mut span = parent_span.child("unbacked-candidate");
-				span.add_string_tag("candidate-hash", &format!("{:?}", hash.0));
-				span
+				parent_span.child_with_candidate("unbacked-candidate", &hash)
 			});
 			Some(span)
 		} else {
@@ -906,7 +909,7 @@ impl CandidateBackingJob {
 	}
 
 	fn get_unbacked_validation_child(&mut self, parent_span: &JaegerSpan, hash: CandidateHash) -> Option<JaegerSpan> {
-		self.insert_or_get_unbacked_span(parent_span, hash).map(|span| span.child("validation"))
+		self.insert_or_get_unbacked_span(parent_span, hash).map(|span| span.child_with_candidate("validation", &hash))
 	}
 
 	fn get_unbacked_statement_child(
@@ -916,9 +919,10 @@ impl CandidateBackingJob {
 		validator: ValidatorIndex,
 	) -> Option<JaegerSpan> {
 		self.insert_or_get_unbacked_span(parent_span, hash).map(|span| {
-			let mut span = span.child("import-statement");
-			span.add_string_tag("validator-index", &format!("{}", validator));
-			span
+			span.child_builder("import-statement")
+				.with_candidate(&hash)
+				.with_validator_index(validator)
+				.build()
 		})
 	}
 

node/jaeger/src/lib.rs

@@ -45,7 +45,9 @@
 //! ```
 
 use sp_core::traits::SpawnNamed;
-use polkadot_primitives::v1::{Hash, PoV, CandidateHash};
+use polkadot_primitives::v1::{CandidateHash, Hash, PoV, ValidatorIndex};
+use sc_network::PeerId;
+
 use parking_lot::RwLock;
 use std::{sync::Arc, result};
 
@@ -133,7 +135,7 @@ impl PerLeafSpan {
 	/// Takes the `leaf_span` that is created by the overseer per leaf and a name for a child span.
 	/// Both will be stored in this object, while the child span is implicitly accessible by using the
 	/// [`Deref`](std::ops::Deref) implementation.
-	pub fn new(leaf_span: Arc<JaegerSpan>, name: impl Into<String>) -> Self {
+	pub fn new(leaf_span: Arc<JaegerSpan>, name: &'static str) -> Self {
 		let span = leaf_span.child(name);
 
 		Self {
@@ -157,6 +159,83 @@ impl std::ops::Deref for PerLeafSpan {
 	}
 }
 
+
+/// A helper to annotate the stage with a numerical value
+/// to ease the life of the tooling team creating viable
+/// statistical metrics for which stage of the inclusion
+/// pipeline drops a significant amount of candidates,
+/// statistically speaking.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[repr(u8)]
+#[non_exhaustive]
+pub enum Stage {
+	Backing = 1,
+	Availability = 2,
+	// TODO expand this
+}
+
+/// Builder pattern for children and root spans to unify
+/// information annotations.
+pub struct SpanBuilder {
+	span: JaegerSpan,
+}
+
+impl SpanBuilder {
+	/// Attach a peer id to the span.
+	#[inline(always)]
+	pub fn with_peer_id(mut self, peer: &PeerId) -> Self {
+		self.span.add_string_tag("peer-id", &peer.to_base58());
+		self
+	}
+	/// Attach a candidate hash to the span.
+	#[inline(always)]
+	pub fn with_candidate(mut self, candidate_hash: &CandidateHash) -> Self  {
+		self.span.add_string_tag("candidate-hash", &format!("{:?}", candidate_hash.0));
+		self
+	}
+	/// Attach a candidate stage.
+	/// Should always come with a `CandidateHash`.
+	#[inline(always)]
+	pub fn with_candidate_stage(mut self, stage: Stage) -> Self {
+		self.span.add_string_tag("candidate-stage", &format!("{}", stage as u8));
+		self
+	}
+
+	#[inline(always)]
+	pub fn with_validator_index(mut self, validator: ValidatorIndex) -> Self {
+		self.span.add_string_tag("validator-index", &validator.to_string());
+		self
+	}
+
+	#[inline(always)]
+	pub fn with_chunk_index(mut self, chunk_index: u32) -> Self {
+		self.span.add_string_tag("chunk-index", &format!("{}", chunk_index));
+		self
+	}
+
+	#[inline(always)]
+	pub fn with_relay_parent(mut self, relay_parent: &Hash) -> Self {
+		self.span.add_string_tag("relay-parent", &format!("{:?}", relay_parent));
+		self
+	}
+
+	#[inline(always)]
+	pub fn with_claimed_validator_index(mut self, claimed_validator_index: ValidatorIndex) -> Self {
+		self.span.add_string_tag(
+			"claimed-validator",
+			&claimed_validator_index.to_string(),
+		);
+		self
+	}
+
+	/// Complete construction.
+	#[inline(always)]
+	pub fn build(self) -> JaegerSpan {
+		self.span
+	}
+}
+
+
 /// A wrapper type for a span.
 ///
 /// Handles running with and without jaeger.
@@ -169,13 +248,34 @@ pub enum JaegerSpan {
 
 impl JaegerSpan {
 	/// Derive a child span from `self`.
-	pub fn child(&self, name: impl Into<String>) -> Self {
+	pub fn child(&self, name: &'static str) -> Self {
 		match self {
 			Self::Enabled(inner) => Self::Enabled(inner.child(name)),
 			Self::Disabled => Self::Disabled,
 		}
 	}
 
+	pub fn child_builder(&self, name: &'static str) -> SpanBuilder {
+		SpanBuilder {
+			span: self.child(name),
+		}
+	}
+
+	/// Derive a child span from `self` but add a candidate annotation.
+	/// A shortcut for
+	///
+	/// ```rust,no_run
+	/// # use polkadot_primitives::v1::CandidateHash;
+	/// # use polkadot_node_jaeger::candidate_hash_span;
+	/// # let hash = CandidateHash::default();
+	/// # let span = candidate_hash_span(&hash, "foo");
+	/// let _span = span.child_builder("name").with_candidate(&hash).build();
+	/// ```
+	#[inline(always)]
+	pub fn child_with_candidate(&self, name: &'static str, candidate_hash: &CandidateHash) -> Self {
+		self.child_builder(name).with_candidate(candidate_hash).build()
+	}
+
 	/// Add an additional tag to the span.
 	pub fn add_string_tag(&mut self, tag: &str, value: &str) {
 		match self {
@@ -215,8 +315,8 @@ impl From<mick_jaeger::Span> for JaegerSpan {
 	}
 }
 
-/// Shortcut for [`candidate_hash_span`] with the hash of the `Candidate` block.
-pub fn candidate_hash_span(candidate_hash: &CandidateHash, span_name: impl Into<String>) -> JaegerSpan {
+/// Shortcut for [`hash_span`] with the hash of the `Candidate` block.
+pub fn candidate_hash_span(candidate_hash: &CandidateHash, span_name: &'static str) -> JaegerSpan {
 	let mut span: JaegerSpan = INSTANCE.read_recursive()
 		.span(|| { candidate_hash.0 }, span_name).into();
 
@@ -226,7 +326,7 @@ pub fn candidate_hash_span(candidate_hash: &CandidateHash, span_name: impl Into<
 
 /// Shortcut for [`hash_span`] with the hash of the `PoV`.
 #[inline(always)]
-pub fn pov_span(pov: &PoV, span_name: impl Into<String>) -> JaegerSpan {
+pub fn pov_span(pov: &PoV, span_name: &'static str) -> JaegerSpan {
 	INSTANCE.read_recursive().span(|| { pov.hash() }, span_name).into()
 }
 
@@ -235,7 +335,7 @@ pub fn pov_span(pov: &PoV, span_name: impl Into<String>) -> JaegerSpan {
 ///
 /// This span automatically has the `relay-parent` tag set.
 #[inline(always)]
-pub fn hash_span(hash: &Hash, span_name: impl Into<String>) -> JaegerSpan {
+pub fn hash_span(hash: &Hash, span_name: &'static str) -> JaegerSpan {
 	let mut span: JaegerSpan = INSTANCE.read_recursive().span(|| { *hash }, span_name).into();
 	span.add_string_tag("relay-parent", &format!("{:?}", hash));
 	span
@@ -307,7 +407,7 @@ impl Jaeger {
 		Ok(())
 	}
 
-	fn span<F>(&self, lazy_hash: F, span_name: impl Into<String>) -> Option<mick_jaeger::Span>
+	fn span<F>(&self, lazy_hash: F, span_name: &'static str) -> Option<mick_jaeger::Span>
 	where
 		F: Fn() -> Hash,
 	{

node/network/availability-distribution/src/lib.rs

@@ -262,9 +262,7 @@ impl ProtocolState {
 			};
 
 			// Create some span that will make it able to switch between the candidate and relay parent span.
-			let mut span = per_relay_parent.span.child("live-candidate");
-			span.add_string_tag("candidate-hash", &format!("{:?}", receipt_hash));
-
+			let span = per_relay_parent.span.child_with_candidate("live-candidate", &receipt_hash);
 			candidate_entry.span.add_follows_from(&span);
 			candidate_entry.live_in.insert(relay_parent);
 		}
@@ -429,11 +427,12 @@ where
 
 		// distribute all erasure messages to interested peers
 		for chunk_index in 0u32..(validator_count as u32) {
-			let _span = {
-				let mut span = per_candidate.span.child("load-and-distribute");
-				span.add_string_tag("chunk-index", &format!("{}", chunk_index));
-				span
-			};
+			let _span = per_candidate.span
+				.child_builder("load-and-distribute")
+				.with_candidate(&candidate_hash)
+				.with_chunk_index(chunk_index)
+				.build();
+
 			let message = if let Some(message) = per_candidate.message_vault.get(&chunk_index) {
 				tracing::trace!(
 					target: LOG_TARGET,
@@ -624,14 +623,15 @@ where
 	let live_candidates = state.cached_live_candidates_unioned(state.view.heads.iter());
 
 	// check if the candidate is of interest
-	let candidate_entry = if live_candidates.contains(&message.candidate_hash) {
+	let candidate_hash = message.candidate_hash;
+	let candidate_entry = if live_candidates.contains(&candidate_hash) {
 		state.per_candidate
-			.get_mut(&message.candidate_hash)
+			.get_mut(&candidate_hash)
 			.expect("All live candidates are contained in per_candidate; qed")
 	} else {
 		tracing::trace!(
 			target: LOG_TARGET,
-			candidate_hash = ?message.candidate_hash,
+			candidate_hash = ?candidate_hash,
 			peer = %origin,
 			"Peer send not live candidate",
 		);
@@ -641,10 +641,10 @@ where
 
 	// Handle a duplicate before doing expensive checks.
 	if let Some(existing) = candidate_entry.message_vault.get(&message.erasure_chunk.index) {
-		let span = candidate_entry.span.child("handle-duplicate");
+		let span = candidate_entry.span.child_with_candidate("handle-duplicate", &candidate_hash);
 		// check if this particular erasure chunk was already sent by that peer before
 		{
-			let _span = span.child("check-entry");
+			let _span = span.child_with_candidate("check-entry", &candidate_hash);
 			let received_set = candidate_entry
 				.received_messages
 				.entry(origin.clone())
@@ -659,7 +659,7 @@ where
 		// check that the message content matches what we have already before rewarding
 		// the peer.
 		{
-			let _span = span.child("check-accurate");
+			let _span = span.child_with_candidate("check-accurate", &candidate_hash);
 			if existing == &message {
 				modify_reputation(ctx, origin, BENEFIT_VALID_MESSAGE).await;
 			} else {
@@ -670,15 +670,15 @@ where
 		return Ok(());
 	}
 
-	let span = {
-		let mut span = candidate_entry.span.child("process-new-chunk");
-		span.add_string_tag("peer-id", &origin.to_base58());
-		span
-	};
+	let span = candidate_entry.span
+			.child_builder("process-new-chunk")
+			.with_candidate(&candidate_hash)
+			.with_peer_id(&origin)
+			.build();
 
 	// check the merkle proof against the erasure root in the candidate descriptor.
 	let anticipated_hash = {
-		let _span = span.child("check-merkle-root");
+		let _span = span.child_with_candidate("check-merkle-root", &candidate_hash);
 		match branch_hash(
 			&candidate_entry.descriptor.erasure_root,
 			&message.erasure_chunk.proof,
@@ -845,7 +845,7 @@ impl AvailabilityDistributionSubsystem {
 				}
 				FromOverseer::Communication {
 					msg: AvailabilityDistributionMessage::AvailabilityFetchingRequest(_),
-				} => { 
+				} => {
 					// TODO: Implement issue 2306:
 					tracing::warn!(
 						target: LOG_TARGET,

node/network/bitfield-distribution/src/lib.rs

@@ -398,15 +398,11 @@ where
 		return;
 	};
 
-	let mut _span = {
-		let mut span = job_data.span.child("msg-received");
-		span.add_string_tag("peer-id", &origin.to_base58());
-		span.add_string_tag(
-			"claimed-validator",
-			&message.signed_availability.validator_index().to_string(),
-		);
-		span
-	};
+	let mut _span = job_data.span
+			.child_builder("msg-received")
+			.with_peer_id(&origin)
+			.with_claimed_validator_index(message.signed_availability.validator_index())
+			.build();
 
 	let validator_set = &job_data.validator_set;
 	if validator_set.is_empty() {

node/network/statement-distribution/src/lib.rs

@@ -516,14 +516,10 @@ async fn circulate_statement_and_dependents(
 		None => return,
 	};
 
-	let _span = {
-		let mut span = active_head.span.child("circulate-statement");
-		span.add_string_tag(
-			"candidate-hash",
-			&format!("{:?}", statement.payload().candidate_hash().0),
-		);
-		span
-	};
+	let _span = active_head.span.child_with_candidate(
+		"circulate-statement",
+		&statement.payload().candidate_hash()
+	);
 
 	// First circulate the statement directly to all peers needing it.
 	// The borrow of `active_head` needs to encompass only this (Rust) statement.
@@ -701,18 +697,10 @@ async fn handle_incoming_message<'a>(
 	};
 
 	let candidate_hash = statement.payload().candidate_hash();
-	let handle_incoming_span = {
-		let mut span = active_head.span.child("handle-incoming");
-		span.add_string_tag(
-			"candidate-hash",
-			&format!("{:?}", candidate_hash.0),
-		);
-		span.add_string_tag(
-			"peer-id",
-			&peer.to_base58(),
-		);
-		span
-	};
+	let handle_incoming_span = active_head.span.child_builder("handle-incoming")
+		.with_candidate(&candidate_hash)
+		.with_peer_id(&peer)
+		.build();
 
 	// check the signature on the statement.
 	if let Err(()) = check_statement_signature(&active_head, relay_parent, &statement) {