diff --git a/polkadot/node/core/backing/src/lib.rs b/polkadot/node/core/backing/src/lib.rs index 4551c4feaa..5a83bd5e6d 100644 --- a/polkadot/node/core/backing/src/lib.rs +++ b/polkadot/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 { - 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 { 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() }) } diff --git a/polkadot/node/jaeger/src/lib.rs b/polkadot/node/jaeger/src/lib.rs index 73e0fb8c17..cc264609f7 100644 --- a/polkadot/node/jaeger/src/lib.rs +++ b/polkadot/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, name: impl Into) -> Self { + pub fn new(leaf_span: Arc, 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) -> 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 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) -> 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) -> 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) -> JaegerSpan { /// /// This span automatically has the `relay-parent` tag set. #[inline(always)] -pub fn hash_span(hash: &Hash, span_name: impl Into) -> 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(&self, lazy_hash: F, span_name: impl Into) -> Option + fn span(&self, lazy_hash: F, span_name: &'static str) -> Option where F: Fn() -> Hash, { diff --git a/polkadot/node/network/availability-distribution/src/lib.rs b/polkadot/node/network/availability-distribution/src/lib.rs index 93f52b5b98..e8caf0371f 100644 --- a/polkadot/node/network/availability-distribution/src/lib.rs +++ b/polkadot/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, diff --git a/polkadot/node/network/bitfield-distribution/src/lib.rs b/polkadot/node/network/bitfield-distribution/src/lib.rs index f1949139a4..9413d691e7 100644 --- a/polkadot/node/network/bitfield-distribution/src/lib.rs +++ b/polkadot/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() { diff --git a/polkadot/node/network/statement-distribution/src/lib.rs b/polkadot/node/network/statement-distribution/src/lib.rs index 3f969e6600..1f9c90c5aa 100644 --- a/polkadot/node/network/statement-distribution/src/lib.rs +++ b/polkadot/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) {