From acae36e411be275ec16bcc7a16882c3d1a76defd Mon Sep 17 00:00:00 2001 From: Nevo Agmon Date: Tue, 2 Apr 2024 14:30:46 +0300 Subject: [PATCH] feat: add ability to config profiling on/off --- Cargo.lock | 21 +++++----- config/default_config.json | 5 +++ crates/papyrus_common/src/metrics.rs | 7 ++++ crates/papyrus_node/src/config/mod.rs | 15 ++++--- ...fig__config_test__dump_default_config.snap | 5 +++ crates/papyrus_node/src/main.rs | 5 +++ crates/papyrus_proc_macros/Cargo.toml | 1 + crates/papyrus_proc_macros/src/lib.rs | 41 ++++++++++++++++--- .../tests/latency_histogram.rs | 14 ++++++- crates/papyrus_storage/Cargo.toml | 6 +-- crates/papyrus_storage/src/body/mod.rs | 2 +- crates/papyrus_storage/src/class.rs | 2 +- crates/papyrus_storage/src/compiled_class.rs | 2 +- crates/papyrus_storage/src/state/mod.rs | 2 +- crates/papyrus_sync/src/lib.rs | 6 +-- papyrus_utilities.Dockerfile | 14 +++---- 16 files changed, 110 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7f4f77239b..fc9156e236 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -75,9 +75,9 @@ dependencies = [ [[package]] name = "ahash" -version = "0.8.6" +version = "0.8.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "91429305e9f0a25f6205c5b8e0d2db09e0708a7a6df0f42212bb56c32c8ac97a" +checksum = "e89da841a80418a9b391ebaea17f5c112ffaaa96f621d2c285b5174da76b9011" dependencies = [ "cfg-if", "getrandom", @@ -3492,7 +3492,7 @@ version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "33ff8ae62cd3a9102e5637afc8452c55acf3844001bd5374e0b0bd7b6616c038" dependencies = [ - "ahash 0.8.6", + "ahash 0.8.11", ] [[package]] @@ -3501,7 +3501,7 @@ version = "0.14.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "290f1a1d9242c78d09ce40a5e87e7554ee637af1351968159f4952f028f75604" dependencies = [ - "ahash 0.8.6", + "ahash 0.8.11", "allocator-api2", "serde", ] @@ -4249,7 +4249,7 @@ version = "0.17.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2a071f4f7efc9a9118dfb627a0a94ef247986e1ab8606a4c806ae2b3aa3b6978" dependencies = [ - "ahash 0.8.6", + "ahash 0.8.11", "anyhow", "base64 0.21.5", "bytecount", @@ -5062,7 +5062,7 @@ version = "0.21.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fde3af1a009ed76a778cb84fdef9e7dbbdf5775ae3e4cc1f434a6a307f6f76c5" dependencies = [ - "ahash 0.8.6", + "ahash 0.8.11", "metrics-macros", "portable-atomic", ] @@ -6047,6 +6047,7 @@ version = "0.4.0-dev.2" dependencies = [ "metrics", "metrics-exporter-prometheus", + "papyrus_common", "prometheus-parse", "quote", "syn 2.0.39", @@ -9770,18 +9771,18 @@ dependencies = [ [[package]] name = "zerocopy" -version = "0.7.26" +version = "0.7.32" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e97e415490559a91254a2979b4829267a57d2fcd741a98eee8b722fb57289aa0" +checksum = "74d4d3961e53fa4c9a25a8637fc2bfaf2595b3d3ae34875568a5cf64787716be" dependencies = [ "zerocopy-derive", ] [[package]] name = "zerocopy-derive" -version = "0.7.26" +version = "0.7.32" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dd7e48ccf166952882ca8bd778a43502c64f33bf94c12ebe2a7f08e5a0f6689f" +checksum = "9ce1b18ccd8e73a9321186f97e46f9f04b778851177567b1975109d26a08d2a6" dependencies = [ "proc-macro2", "quote", diff --git a/config/default_config.json b/config/default_config.json index e913393b6c..e3582697f7 100644 --- a/config/default_config.json +++ b/config/default_config.json @@ -69,6 +69,11 @@ "privacy": "TemporaryValue", "value": false }, + "collect_profiling_metrics": { + "description": "If true, collect profiling metrics for the node.", + "privacy": "Public", + "value": false + }, "monitoring_gateway.collect_metrics": { "description": "If true, collect and return metrics in the monitoring gateway.", "pointer_target": "collect_metrics", diff --git a/crates/papyrus_common/src/metrics.rs b/crates/papyrus_common/src/metrics.rs index 336d168457..6431d78ca6 100644 --- a/crates/papyrus_common/src/metrics.rs +++ b/crates/papyrus_common/src/metrics.rs @@ -1,3 +1,5 @@ +use std::sync::OnceLock; + /// The central marker is the first block number that doesn't exist yet. pub const PAPYRUS_CENTRAL_BLOCK_MARKER: &str = "papyrus_central_block_marker"; @@ -30,3 +32,8 @@ pub const PAPYRUS_NUM_ACTIVE_INBOUND_SESSIONS: &str = "papyrus_num_active_inboun /// The number of active sessions this peer has in which it requests data. pub const PAPYRUS_NUM_ACTIVE_OUTBOUND_SESSIONS: &str = "papyrus_num_active_outbound_sessions"; + +// TODO: consider making this value non static and add a way to change this while the app is +// running. e.g via a monitoring endpoint. +/// Global variable set by the main config to enable collecting profiling metrics. +pub static COLLECT_PROFILING_METRICS: OnceLock = OnceLock::new(); diff --git a/crates/papyrus_node/src/config/mod.rs b/crates/papyrus_node/src/config/mod.rs index 0e0860397f..7237d3a759 100644 --- a/crates/papyrus_node/src/config/mod.rs +++ b/crates/papyrus_node/src/config/mod.rs @@ -16,18 +16,15 @@ use clap::{arg, value_parser, Arg, ArgMatches, Command}; use itertools::{chain, Itertools}; use lazy_static::lazy_static; use papyrus_base_layer::ethereum_base_layer_contract::EthereumBaseLayerConfig; -#[cfg(not(feature = "rpc"))] -use papyrus_config::dumping::ser_param; use papyrus_config::dumping::{ append_sub_config_name, ser_optional_sub_config, + ser_param, ser_pointer_target_param, SerializeConfig, }; use papyrus_config::loading::load_and_process_config; -#[cfg(not(feature = "rpc"))] -use papyrus_config::ParamPrivacyInput; -use papyrus_config::{ConfigError, ParamPath, SerializedParam}; +use papyrus_config::{ConfigError, ParamPath, ParamPrivacyInput, SerializedParam}; use papyrus_monitoring_gateway::MonitoringGatewayConfig; use papyrus_network::NetworkConfig; use papyrus_p2p_sync::{P2PSync, P2PSyncConfig}; @@ -67,6 +64,7 @@ pub struct NodeConfig { pub p2p_sync: Option, // TODO(shahak): Make network non-optional once it's developed enough. pub network: Option, + pub collect_profiling_metrics: bool, } // Default configuration values. @@ -82,6 +80,7 @@ impl Default for NodeConfig { sync: Some(SyncConfig::default()), p2p_sync: None, network: None, + collect_profiling_metrics: false, } } } @@ -98,6 +97,12 @@ impl SerializeConfig for NodeConfig { ser_optional_sub_config(&self.sync, "sync"), ser_optional_sub_config(&self.p2p_sync, "p2p_sync"), ser_optional_sub_config(&self.network, "network"), + BTreeMap::from_iter([ser_param( + "collect_profiling_metrics", + &self.collect_profiling_metrics, + "If true, collect profiling metrics for the node.", + ParamPrivacyInput::Public, + )]), ]; #[cfg(feature = "rpc")] sub_configs.push(append_sub_config_name(self.rpc.dump(), "rpc")); diff --git a/crates/papyrus_node/src/config/snapshots/papyrus_node__config__config_test__dump_default_config.snap b/crates/papyrus_node/src/config/snapshots/papyrus_node__config__config_test__dump_default_config.snap index 2273dad331..c16ded99ba 100644 --- a/crates/papyrus_node/src/config/snapshots/papyrus_node__config__config_test__dump_default_config.snap +++ b/crates/papyrus_node/src/config/snapshots/papyrus_node__config__config_test__dump_default_config.snap @@ -79,6 +79,11 @@ expression: dumped_default_config "value": "https://alpha-mainnet.starknet.io/", "privacy": "Public" }, + "collect_profiling_metrics": { + "description": "If true, collect profiling metrics for the node.", + "value": false, + "privacy": "Public" + }, "monitoring_gateway.collect_metrics": { "description": "If true, collect and return metrics in the monitoring gateway.", "value": false, diff --git a/crates/papyrus_node/src/main.rs b/crates/papyrus_node/src/main.rs index c0eacf2003..cb84bfdd03 100644 --- a/crates/papyrus_node/src/main.rs +++ b/crates/papyrus_node/src/main.rs @@ -11,6 +11,7 @@ use futures::channel::mpsc::Sender; use futures::future::BoxFuture; use futures::FutureExt; use papyrus_base_layer::ethereum_base_layer_contract::EthereumBaseLayerConfig; +use papyrus_common::metrics::COLLECT_PROFILING_METRICS; use papyrus_common::pending_classes::PendingClasses; use papyrus_common::BlockHashAndNumber; use papyrus_config::presentation::get_config_presentation; @@ -297,6 +298,10 @@ async fn main() -> anyhow::Result<()> { exit(1); } + COLLECT_PROFILING_METRICS + .set(config.collect_profiling_metrics) + .expect("This should be the first and only time we set this value."); + info!("Booting up."); let res = run_threads(config.clone()).await; if config.p2p_sync.is_some_and(|c| c.stop_sync_at_block_number.is_some()) { diff --git a/crates/papyrus_proc_macros/Cargo.toml b/crates/papyrus_proc_macros/Cargo.toml index 0bae1df495..b625328baf 100644 --- a/crates/papyrus_proc_macros/Cargo.toml +++ b/crates/papyrus_proc_macros/Cargo.toml @@ -13,6 +13,7 @@ quote = "1.0.26" [dev-dependencies] metrics.workspace = true metrics-exporter-prometheus.workspace = true +papyrus_common = { path = "../papyrus_common", version = "0.4.0-dev.1" } prometheus-parse.workspace = true test_utils = { path = "../test_utils" } diff --git a/crates/papyrus_proc_macros/src/lib.rs b/crates/papyrus_proc_macros/src/lib.rs index 2ffc973a9e..bec181d18f 100644 --- a/crates/papyrus_proc_macros/src/lib.rs +++ b/crates/papyrus_proc_macros/src/lib.rs @@ -1,6 +1,8 @@ +use std::str::FromStr; + use proc_macro::TokenStream; use quote::{quote, ToTokens}; -use syn::{parse_macro_input, ExprLit, ItemFn, ItemTrait, LitStr, Meta, TraitItem}; +use syn::{parse_macro_input, ExprLit, ItemFn, ItemTrait, LitBool, LitStr, Meta, TraitItem}; /// This macro is a wrapper around the "rpc" macro supplied by the jsonrpsee library that generates /// a server and client traits from a given trait definition. The wrapper gets a version id and @@ -98,30 +100,59 @@ pub fn versioned_rpc(attr: TokenStream, input: TokenStream) -> TokenStream { } /// This macro will emit a histogram metric with the given name and the latency of the function. +/// The macro also receives a boolean for whether it will be emitted only when +/// profiling is activated or at all times. /// /// # Example /// Given this code: /// /// ```rust,ignore -/// #[latency_histogram("metric_name")] +/// #[latency_histogram("metric_name", false)] /// fn foo() { /// // Some code ... /// } /// ``` /// Every call to foo will update the histogram metric with the name “metric_name” with the time it /// took to execute foo. +/// The metric will be emitted regardless of the value of the profiling configuration, +/// since the config value is false. #[proc_macro_attribute] pub fn latency_histogram(attr: TokenStream, input: TokenStream) -> TokenStream { let mut input_fn = parse_macro_input!(input as ItemFn); - let metric_name = parse_macro_input!(attr as ExprLit); + let parts = attr + .to_string() + .split(',') + .map(|s| { + TokenStream::from_str(s) + .expect("Expecting metric name and bool (is for profiling only)") + }) + .collect::>(); + let metric_name_as_tokenstream = parts + .first() + .expect("attribute should include metric name and controll with config boolean") + .clone(); + // TODO: consider naming the input value instead of providing a bool + // TODO: consider adding support for metrics levels (e.g. debug, info, warn, error) instead of + // boolean + let controll_with_config_as_tokenstream = parts + .get(1) + .expect("attribute should include metric name and controll with config boolean") + .clone(); + let metric_name = parse_macro_input!(metric_name_as_tokenstream as ExprLit); + let controll_with_config = parse_macro_input!(controll_with_config_as_tokenstream as LitBool); let origin_block = &mut input_fn.block; // Create a new block with the metric update. let expanded_block = quote! { { - let start_function_time=std::time::Instant::now(); + let mut start_function_time = None; + if !#controll_with_config || (#controll_with_config && *(papyrus_common::metrics::COLLECT_PROFILING_METRICS.get().unwrap_or(&false))) { + start_function_time=Some(std::time::Instant::now()); + } let return_value=#origin_block; - metrics::histogram!(#metric_name, start_function_time.elapsed().as_secs_f64()); + if let Some(start_time) = start_function_time { + metrics::histogram!(#metric_name, start_time.elapsed().as_secs_f64()); + } return_value } }; diff --git a/crates/papyrus_proc_macros/tests/latency_histogram.rs b/crates/papyrus_proc_macros/tests/latency_histogram.rs index e94c3ab90f..1af5f17078 100644 --- a/crates/papyrus_proc_macros/tests/latency_histogram.rs +++ b/crates/papyrus_proc_macros/tests/latency_histogram.rs @@ -1,19 +1,31 @@ use metrics_exporter_prometheus::PrometheusBuilder; +use papyrus_common::metrics::COLLECT_PROFILING_METRICS; use papyrus_proc_macros::latency_histogram; use prometheus_parse::Value::Untyped; use test_utils::prometheus_is_contained; #[test] fn latency_histogram_test() { - #[latency_histogram("foo_histogram")] + COLLECT_PROFILING_METRICS.set(false).unwrap(); + + #[latency_histogram("foo_histogram", false)] fn foo() -> usize { #[allow(clippy::let_and_return)] let start_function_time = 1000; start_function_time } + #[latency_histogram("bar_histogram", true)] + fn bar() -> usize { + #[allow(clippy::let_and_return)] + let start_function_time = 1000; + start_function_time + } + let handle = PrometheusBuilder::new().install_recorder().unwrap(); + assert!(handle.render().is_empty()); + assert_eq!(bar(), 1000); assert!(handle.render().is_empty()); assert_eq!(foo(), 1000); assert_eq!( diff --git a/crates/papyrus_storage/Cargo.toml b/crates/papyrus_storage/Cargo.toml index 182a2d0f65..be9f58cfff 100644 --- a/crates/papyrus_storage/Cargo.toml +++ b/crates/papyrus_storage/Cargo.toml @@ -8,7 +8,7 @@ description = "A storage implementation for a Starknet node." [features] testing = ["tempfile"] -document_calls = ["lazy_static", "papyrus_common"] +document_calls = ["lazy_static"] [[bin]] name = "dump_declared_classes" @@ -17,7 +17,7 @@ path = "src/bin/dump_declared_classes.rs" [[bin]] name = "storage_benchmark" -required-features = ["clap", "papyrus_common", "statistical"] +required-features = ["clap", "statistical"] path = "src/bin/storage_benchmark.rs" [dependencies] @@ -35,7 +35,7 @@ memmap2.workspace = true metrics.workspace = true num-bigint.workspace = true page_size.workspace = true -papyrus_common = { path = "../papyrus_common", version = "0.4.0-dev.2", optional = true } +papyrus_common = { path = "../papyrus_common", version = "0.4.0-dev.2" } papyrus_config = { path = "../papyrus_config", version = "0.4.0-dev.2" } papyrus_proc_macros = { path = "../papyrus_proc_macros", version = "0.4.0-dev.2" } parity-scale-codec.workspace = true diff --git a/crates/papyrus_storage/src/body/mod.rs b/crates/papyrus_storage/src/body/mod.rs index db7de53cda..a2c16481cd 100644 --- a/crates/papyrus_storage/src/body/mod.rs +++ b/crates/papyrus_storage/src/body/mod.rs @@ -315,7 +315,7 @@ impl<'env, Mode: TransactionKind> StorageTxn<'env, Mode> { } impl<'env> BodyStorageWriter for StorageTxn<'env, RW> { - #[latency_histogram("storage_append_body_latency_seconds")] + #[latency_histogram("storage_append_body_latency_seconds", false)] fn append_body(self, block_number: BlockNumber, block_body: BlockBody) -> StorageResult { let markers_table = self.open_table(&self.tables.markers)?; update_marker(&self.txn, &markers_table, block_number)?; diff --git a/crates/papyrus_storage/src/class.rs b/crates/papyrus_storage/src/class.rs index 4879cf901d..45f2e1137f 100644 --- a/crates/papyrus_storage/src/class.rs +++ b/crates/papyrus_storage/src/class.rs @@ -155,7 +155,7 @@ impl<'env, Mode: TransactionKind> ClassStorageReader for StorageTxn<'env, Mode> } impl<'env> ClassStorageWriter for StorageTxn<'env, RW> { - #[latency_histogram("storage_append_classes_latency_seconds")] + #[latency_histogram("storage_append_classes_latency_seconds", false)] fn append_classes( self, block_number: BlockNumber, diff --git a/crates/papyrus_storage/src/compiled_class.rs b/crates/papyrus_storage/src/compiled_class.rs index 36b5f17a3f..ea91f388ad 100644 --- a/crates/papyrus_storage/src/compiled_class.rs +++ b/crates/papyrus_storage/src/compiled_class.rs @@ -83,7 +83,7 @@ impl<'env, Mode: TransactionKind> CasmStorageReader for StorageTxn<'env, Mode> { } impl<'env> CasmStorageWriter for StorageTxn<'env, RW> { - #[latency_histogram("storage_append_casm_latency_seconds")] + #[latency_histogram("storage_append_casm_latency_seconds", false)] fn append_casm(self, class_hash: &ClassHash, casm: &CasmContractClass) -> StorageResult { let casm_table = self.open_table(&self.tables.casms)?; let markers_table = self.open_table(&self.tables.markers)?; diff --git a/crates/papyrus_storage/src/state/mod.rs b/crates/papyrus_storage/src/state/mod.rs index 5b299dd4a7..8eaaca7618 100644 --- a/crates/papyrus_storage/src/state/mod.rs +++ b/crates/papyrus_storage/src/state/mod.rs @@ -427,7 +427,7 @@ impl<'env, Mode: TransactionKind> StateReader<'env, Mode> { } impl<'env> StateStorageWriter for StorageTxn<'env, RW> { - #[latency_histogram("storage_append_thin_state_diff_latency_seconds")] + #[latency_histogram("storage_append_thin_state_diff_latency_seconds", false)] fn append_state_diff( self, block_number: BlockNumber, diff --git a/crates/papyrus_sync/src/lib.rs b/crates/papyrus_sync/src/lib.rs index 55de3510c6..7a08770f40 100644 --- a/crates/papyrus_sync/src/lib.rs +++ b/crates/papyrus_sync/src/lib.rs @@ -383,7 +383,7 @@ impl< } } - #[latency_histogram("sync_store_block_latency_seconds")] + #[latency_histogram("sync_store_block_latency_seconds", false)] #[instrument(skip(self, block), level = "debug", fields(block_hash = %block.header.block_hash), err)] fn store_block( &mut self, @@ -424,7 +424,7 @@ impl< Ok(()) } - #[latency_histogram("sync_store_state_diff_latency_seconds")] + #[latency_histogram("sync_store_state_diff_latency_seconds", false)] #[instrument(skip(self, state_diff, deployed_contract_class_definitions), level = "debug", err)] fn store_state_diff( &mut self, @@ -471,7 +471,7 @@ impl< Ok(()) } - #[latency_histogram("sync_store_compiled_class_latency_seconds")] + #[latency_histogram("sync_store_compiled_class_latency_seconds", false)] #[instrument(skip(self, compiled_class), level = "debug", err)] fn store_compiled_class( &mut self, diff --git a/papyrus_utilities.Dockerfile b/papyrus_utilities.Dockerfile index 7463ccfa57..d6d9471804 100644 --- a/papyrus_utilities.Dockerfile +++ b/papyrus_utilities.Dockerfile @@ -21,7 +21,7 @@ RUN CARGO_INCREMENTAL=0 cargo build --target x86_64-unknown-linux-musl --release # Build storage_benchmark. RUN CARGO_INCREMENTAL=0 cargo build --target x86_64-unknown-linux-musl --release --package papyrus_storage \ - --features "clap papyrus_common statistical" --bin storage_benchmark + --features "clap statistical" --bin storage_benchmark # Starting a new stage so that the final image will contain only the executables. @@ -44,9 +44,9 @@ COPY --from=utilities_builder /app/target/x86_64-unknown-linux-musl/release/stor ENV PATH="/app/target/release:${PATH}" ENTRYPOINT echo -e \ -"There is no default executable for this image. Run an executable using its name or path to it.\n\ -The available executables are:\n\ - - papyrus_load_test, performs a stress test on a node RPC gateway.\n\ - - dump_declared_classes, dumps the declared_classes table from the storage to a file.\n\ - - storage_benchmark, performs a benchmark on the storage.\n\ -For example, in a docker runtime: docker run --entrypoint papyrus_load_test " \ No newline at end of file + "There is no default executable for this image. Run an executable using its name or path to it.\n\ + The available executables are:\n\ + - papyrus_load_test, performs a stress test on a node RPC gateway.\n\ + - dump_declared_classes, dumps the declared_classes table from the storage to a file.\n\ + - storage_benchmark, performs a benchmark on the storage.\n\ + For example, in a docker runtime: docker run --entrypoint papyrus_load_test " \ No newline at end of file