From 3d8a2a72989b9892d171411f7f48ec9524e14c95 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Mon, 2 Sep 2024 17:31:37 +1000 Subject: [PATCH 1/2] Add blob count label to `DATA_COLUMN_SIDECAR_COMPUTATION` metric, and move metrics into the compute function, recording only successful computation. --- .../beacon_chain/src/block_verification.rs | 2 -- beacon_node/beacon_chain/src/kzg_utils.rs | 18 ++++++++++-- beacon_node/beacon_chain/src/metrics.rs | 7 +++-- common/lighthouse_metrics/src/lib.rs | 28 +++++++++++++++++++ 4 files changed, 48 insertions(+), 7 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index d9662d59f9e..1e301f110f4 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -802,9 +802,7 @@ fn build_gossip_verified_data_columns( GossipDataColumnError::KzgNotInitialized, ))?; - let timer = metrics::start_timer(&metrics::DATA_COLUMN_SIDECAR_COMPUTATION); let sidecars = blobs_to_data_column_sidecars(&blobs, block, kzg, &chain.spec)?; - drop(timer); let mut gossip_verified_data_columns = vec![]; for sidecar in sidecars { let subnet = DataColumnSubnetId::from_column_index::( diff --git a/beacon_node/beacon_chain/src/kzg_utils.rs b/beacon_node/beacon_chain/src/kzg_utils.rs index 55c1ee9e980..39c2db9b834 100644 --- a/beacon_node/beacon_chain/src/kzg_utils.rs +++ b/beacon_node/beacon_chain/src/kzg_utils.rs @@ -1,6 +1,8 @@ +use crate::metrics; use kzg::{ Blob as KzgBlob, Bytes48, CellRef as KzgCellRef, CellsAndKzgProofs, Error as KzgError, Kzg, }; +use lighthouse_metrics::TryExt; use rayon::prelude::*; use ssz_types::FixedVector; use std::sync::Arc; @@ -150,12 +152,22 @@ pub fn blobs_to_data_column_sidecars( if blobs.is_empty() { return Ok(vec![]); } + + let mut timer = metrics::start_timer_vec( + &metrics::DATA_COLUMN_SIDECAR_COMPUTATION, + &[&blobs.len().to_string()], + ); + let kzg_commitments = block .message() .body() .blob_kzg_commitments() .map_err(|_err| DataColumnSidecarError::PreDeneb)?; - let kzg_commitments_inclusion_proof = block.message().body().kzg_commitments_merkle_proof()?; + let kzg_commitments_inclusion_proof = block + .message() + .body() + .kzg_commitments_merkle_proof() + .discard_timer_on_break(&mut timer)?; let signed_block_header = block.signed_block_header(); // NOTE: assumes blob sidecars are ordered by index @@ -168,7 +180,8 @@ pub fn blobs_to_data_column_sidecars( .expect("blob should have a guaranteed size due to FixedVector"); kzg.compute_cells_and_proofs(blob) }) - .collect::, KzgError>>()?; + .collect::, KzgError>>() + .discard_timer_on_break(&mut timer)?; build_data_column_sidecars( kzg_commitments.clone(), @@ -177,6 +190,7 @@ pub fn blobs_to_data_column_sidecars( blob_cells_and_proofs_vec, spec, ) + .discard_timer_on_break(&mut timer) .map_err(DataColumnSidecarError::BuildSidecarFailed) } diff --git a/beacon_node/beacon_chain/src/metrics.rs b/beacon_node/beacon_chain/src/metrics.rs index 3394946255f..b241538cd11 100644 --- a/beacon_node/beacon_chain/src/metrics.rs +++ b/beacon_node/beacon_chain/src/metrics.rs @@ -1646,11 +1646,12 @@ pub static BLOB_SIDECAR_INCLUSION_PROOF_COMPUTATION: LazyLock> "Time taken to compute blob sidecar inclusion proof", ) }); -pub static DATA_COLUMN_SIDECAR_COMPUTATION: LazyLock> = LazyLock::new(|| { - try_create_histogram_with_buckets( +pub static DATA_COLUMN_SIDECAR_COMPUTATION: LazyLock> = LazyLock::new(|| { + try_create_histogram_vec_with_buckets( "data_column_sidecar_computation_seconds", "Time taken to compute data column sidecar, including cells, proofs and inclusion proof", - Ok(vec![0.04, 0.05, 0.1, 0.2, 0.3, 0.5, 0.7, 1.0]), + Ok(vec![0.1, 0.25, 0.5, 1.0, 2.5, 5.0, 10.0]), + &["blob_count"], ) }); pub static DATA_COLUMN_SIDECAR_INCLUSION_PROOF_VERIFICATION: LazyLock> = diff --git a/common/lighthouse_metrics/src/lib.rs b/common/lighthouse_metrics/src/lib.rs index fa8f47e364a..f52913dd001 100644 --- a/common/lighthouse_metrics/src/lib.rs +++ b/common/lighthouse_metrics/src/lib.rs @@ -400,3 +400,31 @@ pub fn decimal_buckets(min_power: i32, max_power: i32) -> Result> { } Ok(buckets) } + +/// Would be nice to use the `Try` trait bound and have a single implementation, but try_trait_v2 +/// is not a stable feature yet. +pub trait TryExt { + fn discard_timer_on_break(self, timer: &mut Option) -> Self; +} + +impl TryExt for std::result::Result { + fn discard_timer_on_break(self, timer_opt: &mut Option) -> Self { + if self.is_err() { + if let Some(timer) = timer_opt.take() { + timer.stop_and_discard(); + } + } + self + } +} + +impl TryExt for Option { + fn discard_timer_on_break(self, timer_opt: &mut Option) -> Self { + if self.is_none() { + if let Some(timer) = timer_opt.take() { + timer.stop_and_discard(); + } + } + self + } +} From 7186d2817f31b7dacc2087df22caf6cd31695bfb Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Tue, 3 Sep 2024 10:07:21 +1000 Subject: [PATCH 2/2] Move `discard_timer_on_break` usage to caller site. --- .../beacon_chain/src/block_verification.rs | 9 ++++++++- beacon_node/beacon_chain/src/kzg_utils.rs | 18 ++---------------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 1e301f110f4..737bbcec4bc 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -74,6 +74,7 @@ use derivative::Derivative; use eth2::types::{BlockGossip, EventKind, PublishBlockRequest}; use execution_layer::PayloadStatus; pub use fork_choice::{AttestationFromBlock, PayloadVerificationStatus}; +use lighthouse_metrics::TryExt; use parking_lot::RwLockReadGuard; use proto_array::Block as ProtoBlock; use safe_arith::ArithError; @@ -802,7 +803,13 @@ fn build_gossip_verified_data_columns( GossipDataColumnError::KzgNotInitialized, ))?; - let sidecars = blobs_to_data_column_sidecars(&blobs, block, kzg, &chain.spec)?; + let mut timer = metrics::start_timer_vec( + &metrics::DATA_COLUMN_SIDECAR_COMPUTATION, + &[&blobs.len().to_string()], + ); + let sidecars = blobs_to_data_column_sidecars(&blobs, block, kzg, &chain.spec) + .discard_timer_on_break(&mut timer)?; + drop(timer); let mut gossip_verified_data_columns = vec![]; for sidecar in sidecars { let subnet = DataColumnSubnetId::from_column_index::( diff --git a/beacon_node/beacon_chain/src/kzg_utils.rs b/beacon_node/beacon_chain/src/kzg_utils.rs index 39c2db9b834..55c1ee9e980 100644 --- a/beacon_node/beacon_chain/src/kzg_utils.rs +++ b/beacon_node/beacon_chain/src/kzg_utils.rs @@ -1,8 +1,6 @@ -use crate::metrics; use kzg::{ Blob as KzgBlob, Bytes48, CellRef as KzgCellRef, CellsAndKzgProofs, Error as KzgError, Kzg, }; -use lighthouse_metrics::TryExt; use rayon::prelude::*; use ssz_types::FixedVector; use std::sync::Arc; @@ -152,22 +150,12 @@ pub fn blobs_to_data_column_sidecars( if blobs.is_empty() { return Ok(vec![]); } - - let mut timer = metrics::start_timer_vec( - &metrics::DATA_COLUMN_SIDECAR_COMPUTATION, - &[&blobs.len().to_string()], - ); - let kzg_commitments = block .message() .body() .blob_kzg_commitments() .map_err(|_err| DataColumnSidecarError::PreDeneb)?; - let kzg_commitments_inclusion_proof = block - .message() - .body() - .kzg_commitments_merkle_proof() - .discard_timer_on_break(&mut timer)?; + let kzg_commitments_inclusion_proof = block.message().body().kzg_commitments_merkle_proof()?; let signed_block_header = block.signed_block_header(); // NOTE: assumes blob sidecars are ordered by index @@ -180,8 +168,7 @@ pub fn blobs_to_data_column_sidecars( .expect("blob should have a guaranteed size due to FixedVector"); kzg.compute_cells_and_proofs(blob) }) - .collect::, KzgError>>() - .discard_timer_on_break(&mut timer)?; + .collect::, KzgError>>()?; build_data_column_sidecars( kzg_commitments.clone(), @@ -190,7 +177,6 @@ pub fn blobs_to_data_column_sidecars( blob_cells_and_proofs_vec, spec, ) - .discard_timer_on_break(&mut timer) .map_err(DataColumnSidecarError::BuildSidecarFailed) }