[EASY] Add metrics for logging native price requests (#3006)

# Description Add more metrics for native price requests. So we can properly assess how many requests are sent for each estimator.
cowprotocol · Sep 26, 2024 · 1b6ed89 · 1b6ed89
1 parent 9e488c3
commit 1b6ed89
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 45 deletions.
diff --git a/crates/shared/src/price_estimation/factory.rs b/crates/shared/src/price_estimation/factory.rs
@@ -189,25 +189,35 @@ impl<'a> PriceEstimatorFactory<'a> {
                 let estimator = self.get_estimator(driver)?.native.clone();
                 Ok((
                     driver.name.clone(),
-                    Arc::new(NativePriceEstimator::new(
-                        Arc::new(self.sanitized(estimator)),
-                        self.network.native_token,
-                        native_token_price_estimation_amount,
+                    Arc::new(InstrumentedPriceEstimator::new(
+                        NativePriceEstimator::new(
+                            Arc::new(self.sanitized(estimator)),
+                            self.network.native_token,
+                            native_token_price_estimation_amount,
+                        ),
+                        driver.name.to_string(),
+                    )),
+                ))
+            }
+            NativePriceEstimatorSource::OneInchSpotPriceApi => {
+                let name = "OneInchSpotPriceApi".to_string();
+                Ok((
+                    name.clone(),
+                    Arc::new(InstrumentedPriceEstimator::new(
+                        native::OneInch::new(
+                            self.components.http_factory.create(),
+                            self.args.one_inch_url.clone(),
+                            self.args.one_inch_api_key.clone(),
+                            self.network.chain_id,
+                            self.network.block_stream.clone(),
+                            self.components.tokens.clone(),
+                        ),
+                        name,
                     )),
                 ))
             }
-            NativePriceEstimatorSource::OneInchSpotPriceApi => Ok((
-                "OneInchSpotPriceApi".into(),
-                Arc::new(native::OneInch::new(
-                    self.components.http_factory.create(),
-                    self.args.one_inch_url.clone(),
-                    self.args.one_inch_api_key.clone(),
-                    self.network.chain_id,
-                    self.network.block_stream.clone(),
-                    self.components.tokens.clone(),
-                )),
-            )),
             NativePriceEstimatorSource::CoinGecko => {
+                let name = "CoinGecko".to_string();
                 let coin_gecko = native::CoinGecko::new(
                     self.components.http_factory.create(),
                     self.args.coin_gecko.coin_gecko_url.clone(),
@@ -240,12 +250,15 @@ impl<'a> PriceEstimatorFactory<'a> {
                                 .unwrap(),
                         };
 
-                        Arc::new(BufferedRequest::with_config(coin_gecko, configuration))
+                        Arc::new(InstrumentedPriceEstimator::new(
+                            BufferedRequest::with_config(coin_gecko, configuration),
+                            name.clone() + "Buffered",
+                        ))
                     } else {
-                        Arc::new(coin_gecko)
+                        Arc::new(InstrumentedPriceEstimator::new(coin_gecko, name.clone()))
                     };
 
-                Ok(("CoinGecko".into(), coin_gecko))
+                Ok((name, coin_gecko))
             }
         }
     }
@@ -395,12 +408,9 @@ impl PriceEstimatorCreating for ExternalPriceEstimator {
     }
 }
 
-fn instrument(
-    estimator: impl PriceEstimating,
+fn instrument<T: PriceEstimating>(
+    estimator: T,
     name: impl Into<String>,
-) -> Arc<InstrumentedPriceEstimator> {
-    Arc::new(InstrumentedPriceEstimator::new(
-        Box::new(estimator),
-        name.into(),
-    ))
+) -> Arc<InstrumentedPriceEstimator<T>> {
+    Arc::new(InstrumentedPriceEstimator::new(estimator, name.into()))
 }
diff --git a/crates/shared/src/price_estimation/instrumented.rs b/crates/shared/src/price_estimation/instrumented.rs
@@ -1,21 +1,28 @@
 use {
-    crate::price_estimation::{PriceEstimating, PriceEstimationError, Query},
+    crate::price_estimation::{
+        native::{NativePriceEstimateResult, NativePriceEstimating},
+        PriceEstimating,
+        PriceEstimationError,
+        Query,
+    },
+    ethcontract::jsonrpc::futures_util::future::BoxFuture,
     futures::future::FutureExt,
+    primitive_types::H160,
     prometheus::{HistogramVec, IntCounterVec},
     std::{sync::Arc, time::Instant},
     tracing::Instrument,
 };
 
 /// An instrumented price estimator.
-pub struct InstrumentedPriceEstimator {
-    inner: Box<dyn PriceEstimating>,
+pub struct InstrumentedPriceEstimator<T> {
+    inner: T,
     name: String,
     metrics: &'static Metrics,
 }
 
-impl InstrumentedPriceEstimator {
+impl<T> InstrumentedPriceEstimator<T> {
     /// Wraps an existing price estimator in an instrumented one.
-    pub fn new(inner: Box<dyn PriceEstimating>, name: String) -> Self {
+    pub fn new(inner: T, name: String) -> Self {
         let metrics = Metrics::instance(observe::metrics::get_storage_registry()).unwrap();
         for result in ["success", "failure"] {
             metrics
@@ -29,9 +36,21 @@ impl InstrumentedPriceEstimator {
             metrics,
         }
     }
+
+    /// Determines the result of a price estimate as either "success" or
+    /// "failure".
+    fn estimate_result<B>(&self, estimate: Result<&B, &PriceEstimationError>) -> &str {
+        // Count as a successful request if the answer is ok (no error) or if the error
+        // is No Liquidity
+        if estimate.is_ok() || matches!(estimate, Err(PriceEstimationError::NoLiquidity)) {
+            "success"
+        } else {
+            "failure"
+        }
+    }
 }
 
-impl PriceEstimating for InstrumentedPriceEstimator {
+impl<T: PriceEstimating> PriceEstimating for InstrumentedPriceEstimator<T> {
     fn estimate(
         &self,
         query: Arc<Query>,
@@ -43,9 +62,7 @@ impl PriceEstimating for InstrumentedPriceEstimator {
                 .price_estimation_times
                 .with_label_values(&[self.name.as_str()])
                 .observe(start.elapsed().as_secs_f64());
-
-            let success = !matches!(&estimate, Err(PriceEstimationError::EstimatorInternal(_)));
-            let result = if success { "success" } else { "failure" };
+            let result = self.estimate_result(estimate.as_ref());
             self.metrics
                 .price_estimates
                 .with_label_values(&[self.name.as_str(), result])
@@ -58,6 +75,28 @@ impl PriceEstimating for InstrumentedPriceEstimator {
     }
 }
 
+impl<T: NativePriceEstimating> NativePriceEstimating for InstrumentedPriceEstimator<T> {
+    fn estimate_native_price(&self, token: H160) -> BoxFuture<'_, NativePriceEstimateResult> {
+        async move {
+            let start = Instant::now();
+            let estimate = self.inner.estimate_native_price(token).await;
+            self.metrics
+                .price_estimation_times
+                .with_label_values(&[self.name.as_str()])
+                .observe(start.elapsed().as_secs_f64());
+            let result = self.estimate_result(estimate.as_ref());
+            self.metrics
+                .price_estimates
+                .with_label_values(&[self.name.as_str(), result])
+                .inc();
+
+            estimate
+        }
+        .instrument(tracing::info_span!("native estimator", name = &self.name,))
+        .boxed()
+    }
+}
+
 #[derive(prometheus_metric_storage::MetricStorage)]
 struct Metrics {
     /// price estimates
@@ -117,7 +156,7 @@ mod tests {
                 async { Err(PriceEstimationError::EstimatorInternal(anyhow!(""))) }.boxed()
             });
 
-        let instrumented = InstrumentedPriceEstimator::new(Box::new(estimator), "foo".to_string());
+        let instrumented = InstrumentedPriceEstimator::new(estimator, "foo".to_string());
 
         let _ = instrumented.estimate(queries[0].clone()).await;
         let _ = instrumented.estimate(queries[1].clone()).await;

diff --git a/crates/shared/src/price_estimation/trade_finder.rs b/crates/shared/src/price_estimation/trade_finder.rs
@@ -23,6 +23,7 @@ use {
 
 /// A `TradeFinding`-based price estimator with request sharing and rate
 /// limiting.
+#[derive(Clone)]
 pub struct TradeEstimator {
     inner: Arc<Inner>,
     sharing: RequestSharing<Arc<Query>, BoxFuture<'static, Result<Estimate, PriceEstimationError>>>,
@@ -104,16 +105,6 @@ impl Inner {
     }
 }
 
-impl Clone for TradeEstimator {
-    fn clone(&self) -> Self {
-        Self {
-            inner: self.inner.clone(),
-            sharing: self.sharing.clone(),
-            rate_limiter: self.rate_limiter.clone(),
-        }
-    }
-}
-
 impl PriceEstimating for TradeEstimator {
     fn estimate(&self, query: Arc<Query>) -> futures::future::BoxFuture<'_, PriceEstimateResult> {
         self.estimate(query).boxed()