diff --git a/Cargo.lock b/Cargo.lock index 4cd933929b..7815a81620 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10787,6 +10787,8 @@ dependencies = [ "futures", "indexmap 2.7.0", "mempool_test_utils", + "metrics 0.24.1", + "metrics-exporter-prometheus", "mockall", "papyrus_config", "papyrus_state_reader", @@ -10796,6 +10798,7 @@ dependencies = [ "starknet-types-core", "starknet_api", "starknet_batcher_types", + "starknet_infra_utils", "starknet_l1_provider_types", "starknet_mempool_types", "starknet_sequencer_infra", diff --git a/crates/starknet_batcher/Cargo.toml b/crates/starknet_batcher/Cargo.toml index 68d0c13c02..e3dc2aa98c 100644 --- a/crates/starknet_batcher/Cargo.toml +++ b/crates/starknet_batcher/Cargo.toml @@ -13,6 +13,7 @@ async-trait.workspace = true blockifier.workspace = true chrono.workspace = true indexmap.workspace = true +metrics.workspace = true papyrus_config.workspace = true papyrus_state_reader.workspace = true papyrus_storage.workspace = true @@ -33,9 +34,11 @@ assert_matches.workspace = true chrono = { workspace = true } futures.workspace = true mempool_test_utils.workspace = true +metrics-exporter-prometheus.workspace = true mockall.workspace = true rstest.workspace = true starknet-types-core.workspace = true starknet_api = { workspace = true, features = ["testing"] } +starknet_infra_utils.workspace = true starknet_l1_provider_types = { workspace = true, features = ["testing"] } starknet_mempool_types = { workspace = true, features = ["testing"] } diff --git a/crates/starknet_batcher/src/batcher.rs b/crates/starknet_batcher/src/batcher.rs index b6c45b4ae4..ee6f8698ea 100644 --- a/crates/starknet_batcher/src/batcher.rs +++ b/crates/starknet_batcher/src/batcher.rs @@ -2,6 +2,7 @@ use std::collections::{HashMap, HashSet}; use std::sync::Arc; use blockifier::state::contract_class_manager::ContractClassManager; +use metrics::counter; #[cfg(test)] use mockall::automock; use papyrus_storage::state::{StateStorageReader, StateStorageWriter}; @@ -48,6 +49,7 @@ use crate::block_builder::{ BlockMetadata, }; use crate::config::BatcherConfig; +use crate::metrics::register_metrics; use crate::transaction_provider::{ProposeTransactionProvider, ValidateTransactionProvider}; use crate::utils::{ deadline_as_instant, @@ -103,6 +105,10 @@ impl Batcher { mempool_client: SharedMempoolClient, block_builder_factory: Box, ) -> Self { + let storage_height = storage_reader + .height() + .expect("Failed to get height from storage during batcher creation."); + register_metrics(storage_height); Self { config: config.clone(), storage_reader, @@ -327,7 +333,7 @@ impl Batcher { Ok(SendProposalContentResponse { response: ProposalStatus::Aborted }) } - fn get_height_from_storage(&mut self) -> BatcherResult { + fn get_height_from_storage(&self) -> BatcherResult { self.storage_reader.height().map_err(|err| { error!("Failed to get height from storage: {}", err); BatcherError::InternalError @@ -335,7 +341,7 @@ impl Batcher { } #[instrument(skip(self), err)] - pub async fn get_height(&mut self) -> BatcherResult { + pub async fn get_height(&self) -> BatcherResult { let height = self.get_height_from_storage()?; Ok(GetHeightResponse { height }) } @@ -442,6 +448,7 @@ impl Batcher { error!("Failed to commit proposal to storage: {}", err); BatcherError::InternalError })?; + counter!(crate::metrics::STORAGE_HEIGHT.name).increment(1); let mempool_result = self .mempool_client .commit_block(CommitBlockArgs { address_to_nonce, rejected_tx_hashes }) @@ -543,7 +550,9 @@ impl Batcher { } } + #[instrument(skip(self), err)] pub async fn revert_block(&mut self, input: RevertBlockInput) -> BatcherResult<()> { + info!("Reverting block at height {}.", input.height); let height = self.get_height_from_storage()?.prev().ok_or( BatcherError::StorageHeightMarkerMismatch { marker_height: BlockNumber(0), @@ -567,6 +576,8 @@ impl Batcher { error!("Failed to revert block at height {}: {}", height, err); BatcherError::InternalError }) + // TODO(yair): Find a way to decrease the STORAGE_HEIGHT metric (turns out `absolute` only + // works if the new value is larger then the current). } } diff --git a/crates/starknet_batcher/src/batcher_test.rs b/crates/starknet_batcher/src/batcher_test.rs index 0e258b4703..b569c483e5 100644 --- a/crates/starknet_batcher/src/batcher_test.rs +++ b/crates/starknet_batcher/src/batcher_test.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use assert_matches::assert_matches; use blockifier::abi::constants; use indexmap::indexmap; +use metrics_exporter_prometheus::PrometheusBuilder; use mockall::predicate::eq; use rstest::rstest; use starknet_api::block::{BlockHeaderWithoutHash, BlockInfo, BlockNumber}; @@ -30,6 +31,7 @@ use starknet_batcher_types::batcher_types::{ ValidateBlockInput, }; use starknet_batcher_types::errors::BatcherError; +use starknet_infra_utils::metrics::parse_numeric_metric; use starknet_l1_provider_types::MockL1ProviderClient; use starknet_mempool_types::communication::MockMempoolClient; use starknet_mempool_types::mempool_types::CommitBlockArgs; @@ -162,6 +164,18 @@ async fn batcher_with_active_validate_block( batcher } +#[tokio::test] +async fn metrics_registered() { + let recorder = PrometheusBuilder::new().build_recorder(); + let _recorder_guard = metrics::set_default_local_recorder(&recorder); + let _batcher = create_batcher(MockDependencies::default()); + let metrics = recorder.handle().render(); + assert_eq!( + parse_numeric_metric::(&metrics, crate::metrics::STORAGE_HEIGHT.name), + Some(INITIAL_HEIGHT.0) + ); +} + #[rstest] #[tokio::test] async fn start_height_success() { @@ -218,8 +232,9 @@ async fn no_active_height() { #[tokio::test] async fn consecutive_heights_success() { let mut storage_reader = MockBatcherStorageReaderTrait::new(); - storage_reader.expect_height().times(1).returning(|| Ok(INITIAL_HEIGHT)); - storage_reader.expect_height().times(1).returning(|| Ok(INITIAL_HEIGHT.unchecked_next())); + storage_reader.expect_height().times(1).returning(|| Ok(INITIAL_HEIGHT)); // metrics registration + storage_reader.expect_height().times(1).returning(|| Ok(INITIAL_HEIGHT)); // first start_height + storage_reader.expect_height().times(1).returning(|| Ok(INITIAL_HEIGHT.unchecked_next())); // second start_height let mut block_builder_factory = MockBlockBuilderFactoryTrait::new(); for _ in 0..2 { @@ -403,7 +418,7 @@ async fn get_height() { let mut storage_reader = MockBatcherStorageReaderTrait::new(); storage_reader.expect_height().returning(|| Ok(INITIAL_HEIGHT)); - let mut batcher = create_batcher(MockDependencies { storage_reader, ..Default::default() }); + let batcher = create_batcher(MockDependencies { storage_reader, ..Default::default() }); let result = batcher.get_height().await.unwrap(); assert_eq!(result, GetHeightResponse { height: INITIAL_HEIGHT }); @@ -488,6 +503,8 @@ async fn concurrent_proposals_generation_fail() { #[rstest] #[tokio::test] async fn add_sync_block() { + let recorder = PrometheusBuilder::new().build_recorder(); + let _recorder_guard = metrics::set_default_local_recorder(&recorder); let mut mock_dependencies = MockDependencies::default(); mock_dependencies @@ -518,6 +535,11 @@ async fn add_sync_block() { transaction_hashes: test_tx_hashes().into_iter().collect(), }; batcher.add_sync_block(sync_block).await.unwrap(); + let metrics = recorder.handle().render(); + assert_eq!( + parse_numeric_metric::(&metrics, crate::metrics::STORAGE_HEIGHT.name), + Some(INITIAL_HEIGHT.unchecked_next().0) + ); } #[rstest] @@ -544,6 +566,7 @@ async fn add_sync_block_mismatch_block_number() { #[tokio::test] async fn revert_block() { + // TODO(yair): Test metrics. let mut mock_dependencies = MockDependencies::default(); mock_dependencies @@ -595,6 +618,8 @@ async fn revert_block_empty_storage() { #[rstest] #[tokio::test] async fn decision_reached() { + let recorder = PrometheusBuilder::new().build_recorder(); + let _recorder_guard = metrics::set_default_local_recorder(&recorder); let mut mock_dependencies = MockDependencies::default(); let expected_artifacts = BlockExecutionArtifacts::create_for_testing(); @@ -630,6 +655,12 @@ async fn decision_reached() { batcher.decision_reached(DecisionReachedInput { proposal_id: PROPOSAL_ID }).await.unwrap(); assert_eq!(response.state_diff, expected_artifacts.state_diff()); assert_eq!(response.l2_gas_used, expected_artifacts.l2_gas_used); + + let metrics = recorder.handle().render(); + assert_eq!( + parse_numeric_metric::(&metrics, crate::metrics::STORAGE_HEIGHT.name), + Some(INITIAL_HEIGHT.unchecked_next().0) + ); } #[rstest] diff --git a/crates/starknet_batcher/src/lib.rs b/crates/starknet_batcher/src/lib.rs index ab7472c08c..2d08e46e30 100644 --- a/crates/starknet_batcher/src/lib.rs +++ b/crates/starknet_batcher/src/lib.rs @@ -6,6 +6,7 @@ pub mod block_builder; mod block_builder_test; pub mod communication; pub mod config; +mod metrics; #[cfg(test)] mod test_utils; mod transaction_executor; diff --git a/crates/starknet_batcher/src/metrics.rs b/crates/starknet_batcher/src/metrics.rs new file mode 100644 index 0000000000..93dbec3aaf --- /dev/null +++ b/crates/starknet_batcher/src/metrics.rs @@ -0,0 +1,19 @@ +use metrics::{counter, describe_counter}; +use starknet_api::block::BlockNumber; + +pub const STORAGE_HEIGHT: Metric = + Metric { name: "batcher_storage_height", description: "The height of the batcher's storage" }; + +pub struct Metric { + pub name: &'static str, + pub description: &'static str, +} + +pub fn register_metrics(storage_height: BlockNumber) { + // Ideally, we would have a `Gauge` here because of reverts, but we can't because + // the value will need to implement `Into` and `BlockNumber` doesn't. + // In case of reverts, consider calling `absolute`. + let storage_height_metric = counter!(STORAGE_HEIGHT.name); + describe_counter!(STORAGE_HEIGHT.name, STORAGE_HEIGHT.description); + storage_height_metric.absolute(storage_height.0); +}