From d8ef3566b1dd7854ab39a72e4c3f347eafa5771a Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Wed, 31 May 2023 18:55:35 +0300 Subject: [PATCH 1/3] approval-distribution: Add approvals/assignments spans on all paths The approval and assignment logic gets called from multiple paths, so make sure we create a tracing span on all paths to make debugging easier and be able and correlate with the spans from approval-voting. Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index 82f62502e474..a7f1b884215d 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -725,6 +725,14 @@ impl State { ) where R: CryptoRng + Rng, { + let _span = self + .spans + .get(&assignment.block_hash) + .map(|span| span.child("import-and-distribute-assignment")) + .unwrap_or_else(|| jaeger::Span::new(&assignment.block_hash, "distribute-assignment")) + .with_string_tag("block-hash", format!("{:?}", assignment.block_hash)) + .with_stage(jaeger::Stage::ApprovalDistribution); + let block_hash = assignment.block_hash; let validator_index = assignment.validator; @@ -985,6 +993,14 @@ impl State { source: MessageSource, vote: IndirectSignedApprovalVote, ) { + let _span = self + .spans + .get(&vote.block_hash) + .map(|span| span.child("import-and-distribute-approval")) + .unwrap_or_else(|| jaeger::Span::new(&vote.block_hash, "distribute-approval")) + .with_string_tag("block-hash", format!("{:?}", vote.block_hash)) + .with_stage(jaeger::Stage::ApprovalDistribution); + let block_hash = vote.block_hash; let validator_index = vote.validator; let candidate_index = vote.candidate_index; @@ -1724,14 +1740,6 @@ impl ApprovalDistribution { state.handle_new_blocks(ctx, metrics, metas, rng).await; }, ApprovalDistributionMessage::DistributeAssignment(cert, candidate_index) => { - let _span = state - .spans - .get(&cert.block_hash) - .map(|span| span.child("import-and-distribute-assignment")) - .unwrap_or_else(|| jaeger::Span::new(&cert.block_hash, "distribute-assignment")) - .with_string_tag("block-hash", format!("{:?}", cert.block_hash)) - .with_stage(jaeger::Stage::ApprovalDistribution); - gum::debug!( target: LOG_TARGET, "Distributing our assignment on candidate (block={}, index={})", @@ -1751,14 +1759,6 @@ impl ApprovalDistribution { .await; }, ApprovalDistributionMessage::DistributeApproval(vote) => { - let _span = state - .spans - .get(&vote.block_hash) - .map(|span| span.child("import-and-distribute-approval")) - .unwrap_or_else(|| jaeger::Span::new(&vote.block_hash, "distribute-approval")) - .with_string_tag("block-hash", format!("{:?}", vote.block_hash)) - .with_stage(jaeger::Stage::ApprovalDistribution); - gum::debug!( target: LOG_TARGET, "Distributing our approval vote on candidate (block={}, index={})", From 3e13595bffb77156b583490b8a7d40dbfd6b92df Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Wed, 31 May 2023 19:26:22 +0300 Subject: [PATCH 2/3] Tag each label with a difference tracing name Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index a7f1b884215d..64ee1440dbdb 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -496,6 +496,7 @@ impl State { assignment, claimed_index, rng, + "new-blocks-import-and-distribute-assignment", ) .await; }, @@ -505,6 +506,7 @@ impl State { metrics, MessageSource::Peer(peer_id), approval_vote, + "new-blocks-import-and-distribute-approval", ) .await; }, @@ -593,6 +595,7 @@ impl State { assignment, claimed_index, rng, + "peer-message-import-and-distribute-assignment", ) .await; } @@ -629,6 +632,7 @@ impl State { metrics, MessageSource::Peer(peer_id), approval_vote, + "peer-message-import-and-distribute-approval", ) .await; } @@ -722,13 +726,14 @@ impl State { assignment: IndirectAssignmentCert, claimed_candidate_index: CandidateIndex, rng: &mut R, + tracing_label: &'static str, ) where R: CryptoRng + Rng, { let _span = self .spans .get(&assignment.block_hash) - .map(|span| span.child("import-and-distribute-assignment")) + .map(|span| span.child(tracing_label)) .unwrap_or_else(|| jaeger::Span::new(&assignment.block_hash, "distribute-assignment")) .with_string_tag("block-hash", format!("{:?}", assignment.block_hash)) .with_stage(jaeger::Stage::ApprovalDistribution); @@ -992,11 +997,12 @@ impl State { metrics: &Metrics, source: MessageSource, vote: IndirectSignedApprovalVote, + tracing_label: &'static str, ) { let _span = self .spans .get(&vote.block_hash) - .map(|span| span.child("import-and-distribute-approval")) + .map(|span| span.child(tracing_label)) .unwrap_or_else(|| jaeger::Span::new(&vote.block_hash, "distribute-approval")) .with_string_tag("block-hash", format!("{:?}", vote.block_hash)) .with_stage(jaeger::Stage::ApprovalDistribution); @@ -1755,6 +1761,7 @@ impl ApprovalDistribution { cert, candidate_index, rng, + "import-and-distribute-assignment", ) .await; }, @@ -1767,7 +1774,13 @@ impl ApprovalDistribution { ); state - .import_and_circulate_approval(ctx, metrics, MessageSource::Local, vote) + .import_and_circulate_approval( + ctx, + metrics, + MessageSource::Local, + vote, + "import-and-distribute-approval", + ) .await; }, ApprovalDistributionMessage::GetApprovalSignatures(indices, tx) => { From bd3ec415d42fc35dde4ecc513724fe1fec148559 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Thu, 1 Jun 2023 13:49:13 +0300 Subject: [PATCH 3/3] Address review feedback Use the source to determine the tag name Signed-off-by: Alexandru Gheorghe --- node/jaeger/src/spans.rs | 13 +++++++- node/network/approval-distribution/src/lib.rs | 33 ++++++++++--------- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/node/jaeger/src/spans.rs b/node/jaeger/src/spans.rs index b17a78410949..be8bf9cd5ddc 100644 --- a/node/jaeger/src/spans.rs +++ b/node/jaeger/src/spans.rs @@ -277,7 +277,7 @@ impl Span { } /// Derive a child span from `self`. - pub fn child(&self, name: &'static str) -> Self { + pub fn child(&self, name: &str) -> Self { match self { Self::Enabled(inner) => Self::Enabled(inner.child(name)), Self::Disabled => Self::Disabled, @@ -297,11 +297,22 @@ impl Span { self } + /// Attach a peer-id tag to the span. #[inline(always)] pub fn with_peer_id(self, peer: &PeerId) -> Self { self.with_string_tag("peer-id", &peer.to_base58()) } + /// Attach a `peer-id` tag to the span when peer is present. + #[inline(always)] + pub fn with_optional_peer_id(self, peer: Option<&PeerId>) -> Self { + if let Some(peer) = peer { + self.with_peer_id(peer) + } else { + self + } + } + /// Attach a candidate hash to the span. #[inline(always)] pub fn with_candidate(self, candidate_hash: CandidateHash) -> Self { diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index 64ee1440dbdb..0707716b33f0 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -496,7 +496,6 @@ impl State { assignment, claimed_index, rng, - "new-blocks-import-and-distribute-assignment", ) .await; }, @@ -506,7 +505,6 @@ impl State { metrics, MessageSource::Peer(peer_id), approval_vote, - "new-blocks-import-and-distribute-approval", ) .await; }, @@ -595,7 +593,6 @@ impl State { assignment, claimed_index, rng, - "peer-message-import-and-distribute-assignment", ) .await; } @@ -632,7 +629,6 @@ impl State { metrics, MessageSource::Peer(peer_id), approval_vote, - "peer-message-import-and-distribute-approval", ) .await; } @@ -726,16 +722,22 @@ impl State { assignment: IndirectAssignmentCert, claimed_candidate_index: CandidateIndex, rng: &mut R, - tracing_label: &'static str, ) where R: CryptoRng + Rng, { let _span = self .spans .get(&assignment.block_hash) - .map(|span| span.child(tracing_label)) + .map(|span| { + span.child(if source.peer_id().is_some() { + "peer-import-and-distribute-assignment" + } else { + "local-import-and-distribute-assignment" + }) + }) .unwrap_or_else(|| jaeger::Span::new(&assignment.block_hash, "distribute-assignment")) .with_string_tag("block-hash", format!("{:?}", assignment.block_hash)) + .with_optional_peer_id(source.peer_id().as_ref()) .with_stage(jaeger::Stage::ApprovalDistribution); let block_hash = assignment.block_hash; @@ -997,14 +999,20 @@ impl State { metrics: &Metrics, source: MessageSource, vote: IndirectSignedApprovalVote, - tracing_label: &'static str, ) { let _span = self .spans .get(&vote.block_hash) - .map(|span| span.child(tracing_label)) + .map(|span| { + span.child(if source.peer_id().is_some() { + "peer-import-and-distribute-approval" + } else { + "local-import-and-distribute-approval" + }) + }) .unwrap_or_else(|| jaeger::Span::new(&vote.block_hash, "distribute-approval")) .with_string_tag("block-hash", format!("{:?}", vote.block_hash)) + .with_optional_peer_id(source.peer_id().as_ref()) .with_stage(jaeger::Stage::ApprovalDistribution); let block_hash = vote.block_hash; @@ -1761,7 +1769,6 @@ impl ApprovalDistribution { cert, candidate_index, rng, - "import-and-distribute-assignment", ) .await; }, @@ -1774,13 +1781,7 @@ impl ApprovalDistribution { ); state - .import_and_circulate_approval( - ctx, - metrics, - MessageSource::Local, - vote, - "import-and-distribute-approval", - ) + .import_and_circulate_approval(ctx, metrics, MessageSource::Local, vote) .await; }, ApprovalDistributionMessage::GetApprovalSignatures(indices, tx) => {