Txpool metrics update (#2385)

## Linked Issues/PRs This PR addresses the outstanding comments to the TxPool metrics PR: #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
FuelLabs · Oct 31, 2024 · 676ca8f · 676ca8f
1 parent 7a80407
commit 676ca8f
Show file tree

Hide file tree

Showing 8 changed files with 137 additions and 120 deletions.
diff --git 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
@@ -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<HashMap<Buckets, Vec<f64>>> = OnceLock::new();
 pub(crate) fn buckets(b: Buckets) -> impl Iterator<Item = f64> {
@@ -36,22 +38,6 @@ fn initialize_buckets() -> HashMap<Buckets, Vec<f64>> {
                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<Buckets, Vec<f64>> {
                 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
@@ -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
@@ -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<S, SI, CM, SA> Pool<S, SI, CM, SA> {
             && 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<S: Storage, CM, SA> Pool<S, S::StorageIndex, CM, SA>
@@ -106,6 +112,16 @@ where
         tx: ArcPoolTx,
         persistent_storage: &impl TxPoolPersistentStorage,
     ) -> Result<Vec<ArcPoolTx>, 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<PoolTransaction>,
+        persistent_storage: &impl TxPoolPersistentStorage,
+    ) -> Result<Vec<std::sync::Arc<PoolTransaction>>, 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,20 +253,35 @@ 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 {
             tracing::warn!("Failed to calculate transaction time in txpool");
         }
     }
 
-    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<ArcPoolTx> {
-        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::<Vec<_>>()
@@ -525,6 +559,7 @@ where
             self.selection_algorithm
                 .on_removed_transaction(storage_entry);
         }
+        self.register_transaction_counts();
     }
 }