From 676ca8f6830503bf9466ee5ceedcfa37ee0013c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= <88321181+rafal-ch@users.noreply.github.com> Date: Thu, 31 Oct 2024 13:12:38 +0100 Subject: [PATCH] Txpool metrics update (#2385) ## Linked Issues/PRs This PR addresses the outstanding comments to the TxPool metrics PR: https://github.com/FuelLabs/fuel-core/pull/2321 ## Description There are a couple of changes to how and when metrics are registered and some histogram buckets are redefined. Additionally there is also a small clean-up of the changelog file. ### Before requesting review - [X] I have reviewed the code myself --- CHANGELOG.md | 23 ++++--- crates/metrics/src/buckets.rs | 58 ++++++++++++----- crates/metrics/src/txpool_metrics.rs | 31 ++++----- crates/services/txpool_v2/src/pool.rs | 63 ++++++++++++++----- crates/services/txpool_v2/src/service.rs | 62 +++++------------- .../services/txpool_v2/src/storage/graph.rs | 4 -- crates/services/txpool_v2/src/storage/mod.rs | 3 - crates/types/src/services/txpool.rs | 13 ---- 8 files changed, 137 insertions(+), 120 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3060675e042..053d3258a87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,22 +6,29 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] +### Added +- [2321](https://github.com/FuelLabs/fuel-core/pull/2321): New metrics for the TxPool: + - The size of transactions in the txpool (`txpool_tx_size`) + - The time spent by a transaction in the txpool in seconds (`txpool_tx_time_in_txpool_seconds`) + - The number of transactions in the txpool (`txpool_number_of_transactions`) + - The number of transactions pending verification before entering the txpool (`txpool_number_of_transactions_pending_verification`) + - The number of executable transactions in the txpool (`txpool_number_of_executable_transactions`) + - The time it took to select transactions for inclusion in a block in microseconds (`txpool_select_transactions_time_microseconds`) + - The time it took to insert a transaction in the txpool in microseconds (`transaction_insertion_time_in_thread_pool_microseconds`) +- [2385](https://github.com/FuelLabs/fuel-core/pull/2385): Added new histogram buckets for some of the TxPool metrics, optimize the way they are collected. +- [2347](https://github.com/FuelLabs/fuel-core/pull/2364): Add activity concept in order to protect against infinitely increasing DA gas price scenarios +- [2362](https://github.com/FuelLabs/fuel-core/pull/2362): Added a new request_response protocol version `/fuel/req_res/0.0.2`. In comparison with `/fuel/req/0.0.1`, which returns an empty response when a request cannot be fulfilled, this version returns more meaningful error codes. Nodes still support the version `0.0.1` of the protocol to guarantee backward compatibility with fuel-core nodes. Empty responses received from nodes using the old protocol `/fuel/req/0.0.1` are automatically converted into an error `ProtocolV1EmptyResponse` with error code 0, which is also the only error code implemented. More specific error codes will be added in the future. +- [2386](https://github.com/FuelLabs/fuel-core/pull/2386): Add a flag to define the maximum number of file descriptors that RocksDB can use. By default it's half of the OS limit. +- [2376](https://github.com/FuelLabs/fuel-core/pull/2376): Add a way to fetch transactions in P2P without specifying a peer. + ### Fixed - [2366](https://github.com/FuelLabs/fuel-core/pull/2366): The `importer_gas_price_for_block` metric is properly collected. - [2369](https://github.com/FuelLabs/fuel-core/pull/2369): The `transaction_insertion_time_in_thread_pool_milliseconds` metric is properly collected. - [2413](https://github.com/FuelLabs/fuel-core/issues/2413): block production immediately errors if unable to lock the mutex. ### Changed - - [2378](https://github.com/FuelLabs/fuel-core/pull/2378): Use cached hash of the topic instead of calculating it on each publishing gossip message. -### Added -- [2321](https://github.com/FuelLabs/fuel-core/pull/2321): New metrics for the txpool: "The size of transactions in the txpool" (`txpool_tx_size`), "The time spent by a transaction in the txpool in seconds" (`txpool_tx_time_in_txpool_seconds`), The number of transactions in the txpool (`txpool_number_of_transactions`), "The number of transactions pending verification before entering the txpool" (`txpool_number_of_transactions_pending_verification`), "The number of executable transactions in the txpool" (`txpool_number_of_executable_transactions`), "The time it took to select transactions for inclusion in a block in nanoseconds" (`txpool_select_transaction_time_nanoseconds`), The time it took to insert a transaction in the txpool in milliseconds (`txpool_insert_transaction_time_milliseconds`). -- [2347](https://github.com/FuelLabs/fuel-core/pull/2364): Add activity concept in order to protect against infinitely increasing DA gas price scenarios -- [2362](https://github.com/FuelLabs/fuel-core/pull/2362): Added a new request_response protocol version `/fuel/req_res/0.0.2`. In comparison with `/fuel/req/0.0.1`, which returns an empty response when a request cannot be fulfilled, this version returns more meaningful error codes. Nodes still support the version `0.0.1` of the protocol to guarantee backward compatibility with fuel-core nodes. Empty responses received from nodes using the old protocol `/fuel/req/0.0.1` are automatically converted into an error `ProtocolV1EmptyResponse` with error code 0, which is also the only error code implemented. More specific error codes will be added in the future. -- [2386](https://github.com/FuelLabs/fuel-core/pull/2386): Add a flag to define the maximum number of file descriptors that RocksDB can use. By default it's half of the OS limit. -- [2376](https://github.com/FuelLabs/fuel-core/pull/2376): Add a way to fetch transactions in P2P without specifying a peer. - ## [Version 0.40.0] ### Added diff --git a/crates/metrics/src/buckets.rs b/crates/metrics/src/buckets.rs index ccec479e85b..c4fcb110556 100644 --- a/crates/metrics/src/buckets.rs +++ b/crates/metrics/src/buckets.rs @@ -9,8 +9,10 @@ use strum_macros::EnumIter; #[cfg_attr(test, derive(EnumIter))] pub(crate) enum Buckets { Timing, - TimingCoarseGrained, TransactionSize, + TransactionInsertionTimeInThreadPool, + SelectTransactionsTime, + TransactionTimeInTxpool, } static BUCKETS: OnceLock>> = OnceLock::new(); pub(crate) fn buckets(b: Buckets) -> impl Iterator { @@ -36,22 +38,6 @@ fn initialize_buckets() -> HashMap> { 10.000, ], ), - ( - Buckets::TimingCoarseGrained, - vec![ - 5.0, - 10.0, - 25.0, - 50.0, - 100.0, - 250.0, - 500.0, - 1000.0, - 2500.0, - 5000.0, - 10000.0, - ], - ), ( // We consider blocks up to 256kb in size and single transaction can take any of this space. Buckets::TransactionSize, @@ -76,6 +62,44 @@ fn initialize_buckets() -> HashMap> { 256.0 * 1024.0, ] ), + ( + Buckets::TransactionInsertionTimeInThreadPool, + vec![ + 50.0, + 250.0, + 1000.0, + 10000.0, + 100000.0, + 300000.0, + 1_000_000.0, + 5_000_000.0, + ] + ), + ( + Buckets::SelectTransactionsTime, + vec![ + 50.0, + 250.0, + 1000.0, + 10000.0, + 100000.0, + 300000.0, + 1_000_000.0, + 5_000_000.0, + ] + ), + ( + Buckets::TransactionTimeInTxpool, + vec![ + 1.0, + 2.0, + 5.0, + 10.0, + 100.0, + 250.0, + 600.0 + ] + ), ] .into_iter() .collect() diff --git a/crates/metrics/src/txpool_metrics.rs b/crates/metrics/src/txpool_metrics.rs index 2978825560f..28ac4d46288 100644 --- a/crates/metrics/src/txpool_metrics.rs +++ b/crates/metrics/src/txpool_metrics.rs @@ -23,19 +23,20 @@ pub struct TxPoolMetrics { /// Time of transactions in the txpool in seconds pub transaction_time_in_txpool_secs: Histogram, /// Time actively spent by transaction insertion in the thread pool - pub transaction_insertion_time_in_thread_pool_milliseconds: Histogram, + pub transaction_insertion_time_in_thread_pool_microseconds: Histogram, /// How long it took for the selection algorithm to select transactions - pub select_transaction_time_nanoseconds: Histogram, + pub select_transactions_time_microseconds: Histogram, } impl Default for TxPoolMetrics { fn default() -> Self { let tx_size = Histogram::new(buckets(Buckets::TransactionSize)); - let transaction_time_in_txpool_secs = Histogram::new(buckets(Buckets::Timing)); - let select_transaction_time_nanoseconds = - Histogram::new(buckets(Buckets::TimingCoarseGrained)); - let transaction_insertion_time_in_thread_pool_milliseconds = - Histogram::new(buckets(Buckets::TimingCoarseGrained)); + let transaction_time_in_txpool_secs = + Histogram::new(buckets(Buckets::TransactionTimeInTxpool)); + let select_transactions_time_microseconds = + Histogram::new(buckets(Buckets::SelectTransactionsTime)); + let transaction_insertion_time_in_thread_pool_microseconds = + Histogram::new(buckets(Buckets::TransactionInsertionTimeInThreadPool)); let number_of_transactions = Gauge::default(); let number_of_transactions_pending_verification = Gauge::default(); @@ -47,8 +48,8 @@ impl Default for TxPoolMetrics { number_of_transactions_pending_verification, number_of_executable_transactions, transaction_time_in_txpool_secs, - transaction_insertion_time_in_thread_pool_milliseconds, - select_transaction_time_nanoseconds, + transaction_insertion_time_in_thread_pool_microseconds, + select_transactions_time_microseconds, }; let mut registry = global_registry().registry.lock(); @@ -83,16 +84,16 @@ impl Default for TxPoolMetrics { ); registry.register( - "txpool_select_transaction_time_nanoseconds", - "The time it took to select transactions for inclusion in a block in nanoseconds", - metrics.select_transaction_time_nanoseconds.clone(), + "txpool_select_transactions_time_microseconds", + "The time it took to select transactions for inclusion in a block in microseconds", + metrics.select_transactions_time_microseconds.clone(), ); registry.register( - "txpool_insert_transaction_time_milliseconds", - "The time it took to insert a transaction in the txpool in milliseconds", + "txpool_transaction_insertion_time_in_thread_pool_microseconds", + "The time it took to insert a transaction in the txpool in microseconds", metrics - .transaction_insertion_time_in_thread_pool_milliseconds + .transaction_insertion_time_in_thread_pool_microseconds .clone(), ); diff --git a/crates/services/txpool_v2/src/pool.rs b/crates/services/txpool_v2/src/pool.rs index d82faa72bc6..be417dce5f4 100644 --- a/crates/services/txpool_v2/src/pool.rs +++ b/crates/services/txpool_v2/src/pool.rs @@ -10,6 +10,7 @@ use std::{ }; use collisions::CollisionsExt; +use fuel_core_metrics::txpool_metrics::txpool_metrics; use fuel_core_types::{ fuel_tx::{ field::BlobId, @@ -89,6 +90,11 @@ impl Pool { && self.current_gas == 0 && self.current_bytes_size == 0 } + + /// Returns the number of transactions in the pool. + pub fn tx_count(&self) -> usize { + self.tx_id_to_storage_id.len() + } } impl Pool @@ -106,6 +112,16 @@ where tx: ArcPoolTx, persistent_storage: &impl TxPoolPersistentStorage, ) -> Result, Error> { + let insertion_result = self.insert_inner(tx, persistent_storage); + self.register_transaction_counts(); + insertion_result + } + + fn insert_inner( + &mut self, + tx: std::sync::Arc, + persistent_storage: &impl TxPoolPersistentStorage, + ) -> Result>, Error> { let CanStoreTransaction { checked_transaction, transactions_to_remove, @@ -146,6 +162,10 @@ where debug_assert!(!self.tx_id_to_storage_id.contains_key(&tx_id)); self.tx_id_to_storage_id.insert(tx_id, storage_id); + if self.config.metrics { + txpool_metrics().tx_size.observe(bytes_size as f64); + }; + let tx = Storage::get(&self.storage, &storage_id).expect("Transaction is set above"); self.collision_manager.on_stored_transaction(storage_id, tx); @@ -233,7 +253,7 @@ where fn record_transaction_time_in_txpool(tx: &StorageData) { if let Ok(elapsed) = tx.creation_instant.elapsed() { - fuel_core_metrics::txpool_metrics::txpool_metrics() + txpool_metrics() .transaction_time_in_txpool_secs .observe(elapsed.as_secs_f64()); } else { @@ -241,12 +261,27 @@ where } } - fn record_select_transaction_time_in_nanoseconds(start: Instant) { - let elapsed = start.elapsed().as_nanos() as f64; - fuel_core_metrics::txpool_metrics::txpool_metrics() - .select_transaction_time_nanoseconds + fn record_select_transaction_time(start: Instant) { + let elapsed = start.elapsed().as_micros() as f64; + txpool_metrics() + .select_transactions_time_microseconds .observe(elapsed); } + + fn register_transaction_counts(&self) { + if self.config.metrics { + let num_transactions = self.tx_count(); + let executable_txs = + self.selection_algorithm.number_of_executable_transactions(); + txpool_metrics() + .number_of_transactions + .set(num_transactions as i64); + txpool_metrics() + .number_of_executable_transactions + .set(executable_txs as i64); + } + } + // TODO: Use block space also (https://github.com/FuelLabs/fuel-core/issues/2133) /// Extract transactions for a block. /// Returns a list of transactions that were selected for the block @@ -255,26 +290,25 @@ where &mut self, constraints: Constraints, ) -> Vec { - let start = std::time::Instant::now(); let metrics = self.config.metrics; + let maybe_start = metrics.then(std::time::Instant::now); let best_txs = self .selection_algorithm .gather_best_txs(constraints, &mut self.storage); + if let Some(start) = maybe_start { + Self::record_select_transaction_time(start) + }; if metrics { - Self::record_select_transaction_time_in_nanoseconds(start) - }; + best_txs.iter().for_each(|storage_data| { + Self::record_transaction_time_in_txpool(storage_data) + }); + } best_txs .into_iter() - .inspect(|storage_data| { - if metrics { - Self::record_transaction_time_in_txpool(storage_data) - } - }) .map(|storage_entry| { self.update_components_and_caches_on_removal(iter::once(&storage_entry)); - storage_entry.transaction }) .collect::>() @@ -525,6 +559,7 @@ where self.selection_algorithm .on_removed_transaction(storage_entry); } + self.register_transaction_counts(); } } diff --git a/crates/services/txpool_v2/src/service.rs b/crates/services/txpool_v2/src/service.rs index 28a5aae3d93..c80952f7885 100644 --- a/crates/services/txpool_v2/src/service.rs +++ b/crates/services/txpool_v2/src/service.rs @@ -1,6 +1,5 @@ use crate::{ self as fuel_core_txpool, - selection_algorithms::SelectionAlgorithm, }; use fuel_core_metrics::txpool_metrics::txpool_metrics; @@ -71,7 +70,6 @@ use fuel_core_types::{ }, txpool::{ ArcPoolTx, - PoolTransaction, TransactionStatus, }, }, @@ -227,18 +225,6 @@ where View: TxPoolPersistentStorage, { async fn run(&mut self, watcher: &mut StateWatcher) -> anyhow::Result { - // TODO: move this to the Task struct - if self.metrics { - let pool = self.pool.read(); - let num_transactions = pool.storage.tx_count(); - - let executable_txs = - pool.selection_algorithm.number_of_executable_transactions(); - - record_number_of_transactions_in_txpool(num_transactions); - record_number_of_executable_transactions_in_txpool(executable_txs); - } - tokio::select! { biased; @@ -392,6 +378,13 @@ where from_peer_info: Option, response_channel: Option>>, ) -> impl FnOnce() + Send + 'static { + let metrics = self.metrics; + if metrics { + txpool_metrics() + .number_of_transactions_pending_verification + .inc(); + } + let verification = self.verification.clone(); let pool = self.pool.clone(); let p2p = self.p2p.clone(); @@ -400,7 +393,6 @@ where let time_txs_submitted = self.pruner.time_txs_submitted.clone(); let tx_id = transaction.id(&self.chain_id); let utxo_validation = self.utxo_validation; - let metrics = self.metrics; let insert_transaction_thread_pool_op = move || { let current_height = *current_height.read(); @@ -417,6 +409,12 @@ where utxo_validation, ); + if metrics { + txpool_metrics() + .number_of_transactions_pending_verification + .dec(); + } + p2p.process_insertion_result(from_peer_info, &result); let checked_tx = match result { @@ -431,10 +429,6 @@ where } }; - if metrics { - record_tx_size(&checked_tx) - }; - let tx = Arc::new(checked_tx); let result = { @@ -489,19 +483,12 @@ where }; move || { if metrics { - let txpool_metrics = txpool_metrics(); - txpool_metrics - .number_of_transactions_pending_verification - .inc(); let start_time = tokio::time::Instant::now(); insert_transaction_thread_pool_op(); - let time_for_task_to_complete = start_time.elapsed().as_millis(); - txpool_metrics - .transaction_insertion_time_in_thread_pool_milliseconds + let time_for_task_to_complete = start_time.elapsed().as_micros(); + txpool_metrics() + .transaction_insertion_time_in_thread_pool_microseconds .observe(time_for_task_to_complete as f64); - txpool_metrics - .number_of_transactions_pending_verification - .dec(); } else { insert_transaction_thread_pool_op(); } @@ -693,23 +680,6 @@ where } } -fn record_tx_size(tx: &PoolTransaction) { - let size = tx.metered_bytes_size(); - let txpool_metrics = txpool_metrics(); - txpool_metrics.tx_size.observe(size as f64); -} - -fn record_number_of_transactions_in_txpool(num_transactions: usize) { - txpool_metrics() - .number_of_transactions - .set(num_transactions as i64); -} -fn record_number_of_executable_transactions_in_txpool(executable_txs: usize) { - txpool_metrics() - .number_of_executable_transactions - .set(executable_txs as i64); -} - #[allow(clippy::too_many_arguments)] pub fn new_service< P2P, diff --git a/crates/services/txpool_v2/src/storage/graph.rs b/crates/services/txpool_v2/src/storage/graph.rs index ace99937427..b3b40575331 100644 --- a/crates/services/txpool_v2/src/storage/graph.rs +++ b/crates/services/txpool_v2/src/storage/graph.rs @@ -619,10 +619,6 @@ impl Storage for GraphStorage { self.clear_cache(storage_entry); }) } - - fn tx_count(&self) -> usize { - self.graph.node_count() - } } impl RatioTipGasSelectionAlgorithmStorage for GraphStorage { diff --git a/crates/services/txpool_v2/src/storage/mod.rs b/crates/services/txpool_v2/src/storage/mod.rs index 949bea30698..38686eb6b9b 100644 --- a/crates/services/txpool_v2/src/storage/mod.rs +++ b/crates/services/txpool_v2/src/storage/mod.rs @@ -102,7 +102,4 @@ pub trait Storage { /// Remove a transaction from the storage. fn remove_transaction(&mut self, index: Self::StorageIndex) -> Option; - - /// Returns the number of transactions in the storage. - fn tx_count(&self) -> usize; } diff --git a/crates/types/src/services/txpool.rs b/crates/types/src/services/txpool.rs index 55f0d6f96ca..c684a78b092 100644 --- a/crates/types/src/services/txpool.rs +++ b/crates/types/src/services/txpool.rs @@ -32,7 +32,6 @@ use crate::{ }, services::executor::TransactionExecutionResult, }; -use core::time::Duration; use fuel_vm_private::{ checked_transaction::CheckedTransaction, fuel_types::BlockHeight, @@ -319,18 +318,6 @@ impl From<&PoolTransaction> for CheckedTransaction { } } -/// The `removed` field contains the list of removed transactions during the insertion -/// of the `inserted` transaction. -#[derive(Debug, PartialEq, Eq)] -pub struct InsertionResult { - /// This was inserted - pub inserted: ArcPoolTx, - /// The time the transaction was inserted. - pub submitted_time: Duration, - /// These were removed during the insertion - pub removed: Vec, -} - /// The status of the transaction during its life from the tx pool until the block. #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[derive(Clone, Debug, PartialEq, Eq)]