From b52151cb5774ff39492089ca18e3533f4b0ffa82 Mon Sep 17 00:00:00 2001 From: Ben Smith Date: Tue, 12 Oct 2021 16:33:23 +0200 Subject: [PATCH 1/4] add single order solver metrics in preparation for tracking them --- solver/src/metrics.rs | 24 ++++++++++++++++++++++++ solver/src/solver/oneinch_solver/api.rs | 2 +- solver/src/solver/paraswap_solver.rs | 2 +- solver/src/solver/single_order_solver.rs | 18 +++++++++++------- solver/src/solver/zeroex_solver.rs | 2 +- 5 files changed, 38 insertions(+), 10 deletions(-) diff --git a/solver/src/metrics.rs b/solver/src/metrics.rs index 5a7d4a0a3..1a5c43878 100644 --- a/solver/src/metrics.rs +++ b/solver/src/metrics.rs @@ -27,6 +27,8 @@ pub trait SolverMetrics { fn order_settled(&self, order: &Order, solver: &'static str); fn settlement_simulation_succeeded(&self, solver: &'static str); fn settlement_simulation_failed_on_latest(&self, solver: &'static str); + fn single_order_solver_succeeded(&self, solver: &'static str); + fn single_order_solver_failed(&self, solver: &'static str); fn settlement_simulation_failed(&self, solver: &'static str); fn settlement_submitted(&self, successful: bool, solver: &'static str); fn orders_matched_but_not_settled(&self, count: usize); @@ -41,6 +43,7 @@ pub struct Metrics { liquidity: IntGaugeVec, settlement_simulations: IntCounterVec, settlement_submissions: IntCounterVec, + single_order_solver_runs: IntCounterVec, matched_but_unsettled_orders: IntCounter, transport_requests: HistogramVec, pool_cache_hits: IntCounter, @@ -94,6 +97,12 @@ impl Metrics { )?; registry.register(Box::new(settlement_submissions.clone()))?; + let single_order_solver_runs = IntCounterVec::new( + Opts::new("single_order_solver", "Success/Failure counts"), + &["result", "solver_type"], + )?; + registry.register(Box::new(single_order_solver_runs.clone()))?; + let matched_but_unsettled_orders = IntCounter::new( "orders_matched_not_settled", "Counter for the number of orders for which at least one solver computed an execution which was not chosen in this run-loop", @@ -126,6 +135,7 @@ impl Metrics { liquidity, settlement_simulations, settlement_submissions, + single_order_solver_runs, matched_but_unsettled_orders, transport_requests, pool_cache_hits, @@ -189,6 +199,18 @@ impl SolverMetrics for Metrics { .inc() } + fn single_order_solver_succeeded(&self, solver: &'static str) { + self.single_order_solver_runs + .with_label_values(&["success", solver]) + .inc() + } + + fn single_order_solver_failed(&self, solver: &'static str) { + self.single_order_solver_runs + .with_label_values(&["failure", solver]) + .inc() + } + fn settlement_simulation_failed(&self, solver: &'static str) { self.settlement_simulations .with_label_values(&["failure", solver]) @@ -260,6 +282,8 @@ impl SolverMetrics for NoopMetrics { fn order_settled(&self, _: &Order, _: &'static str) {} fn settlement_simulation_succeeded(&self, _: &'static str) {} fn settlement_simulation_failed_on_latest(&self, _: &'static str) {} + fn single_order_solver_succeeded(&self, _: &'static str) {} + fn single_order_solver_failed(&self, _: &'static str) {} fn settlement_simulation_failed(&self, _: &'static str) {} fn settlement_submitted(&self, _: bool, _: &'static str) {} fn orders_matched_but_not_settled(&self, _: usize) {} diff --git a/solver/src/solver/oneinch_solver/api.rs b/solver/src/solver/oneinch_solver/api.rs index a71801189..dcd8af90e 100644 --- a/solver/src/solver/oneinch_solver/api.rs +++ b/solver/src/solver/oneinch_solver/api.rs @@ -143,7 +143,7 @@ impl From for SettlementError { SettlementError { inner: anyhow!(error.message), retryable: matches!(error.status_code, 500), - should_alert: true, + track_failure: true, } } } diff --git a/solver/src/solver/paraswap_solver.rs b/solver/src/solver/paraswap_solver.rs index 101b6f1d7..384d1f726 100644 --- a/solver/src/solver/paraswap_solver.rs +++ b/solver/src/solver/paraswap_solver.rs @@ -81,7 +81,7 @@ impl From for SettlementError { | ParaswapResponseError::ServerBusy | ParaswapResponseError::Send(_), ), - should_alert: !matches!( + track_failure: !matches!( err, ParaswapResponseError::PriceChange | ParaswapResponseError::BuildingTransaction(_) diff --git a/solver/src/solver/single_order_solver.rs b/solver/src/solver/single_order_solver.rs index ee6cba297..d54f7892a 100644 --- a/solver/src/solver/single_order_solver.rs +++ b/solver/src/solver/single_order_solver.rs @@ -54,14 +54,18 @@ impl Solver for SingleOrderSolver let settle = async { while let Some(order) = orders.pop_front() { match self.inner.try_settle_order(order.clone()).await { - Ok(settlement) => settlements.extend(settlement), + Ok(settlement) => { + // TODO - track success + settlements.extend(settlement) + }, Err(err) => { let name = self.inner.name(); if err.retryable { tracing::warn!("Solver {} retryable error: {:?}", name, &err); orders.push_back(order); - } else if err.should_alert { - tracing::error!("Solver {} hard error: {:?}", name, &err); + } else if err.track_failure { + tracing::warn!("Solver {} hard error: {:?}", name, &err); + // TODO - record metric (increment counter for name) } else { tracing::warn!("Solver {} soft error: {:?}", name, &err); } @@ -89,7 +93,7 @@ pub struct SettlementError { pub inner: anyhow::Error, pub retryable: bool, // Whether or not this error should be logged as an error - pub should_alert: bool, + pub track_failure: bool, } impl From for SettlementError { @@ -97,7 +101,7 @@ impl From for SettlementError { SettlementError { inner: err, retryable: false, - should_alert: true, + track_failure: true, } } } @@ -166,7 +170,7 @@ mod tests { 0 => Err(SettlementError { inner: anyhow!(""), retryable: true, - should_alert: true, + track_failure: true, }), 1 => Ok(None), _ => unreachable!(), @@ -206,7 +210,7 @@ mod tests { Err(SettlementError { inner: anyhow!(""), retryable: false, - should_alert: true, + track_failure: true, }) }); diff --git a/solver/src/solver/zeroex_solver.rs b/solver/src/solver/zeroex_solver.rs index 661a0b623..140b2301a 100644 --- a/solver/src/solver/zeroex_solver.rs +++ b/solver/src/solver/zeroex_solver.rs @@ -137,7 +137,7 @@ impl From for SettlementError { SettlementError { inner: anyhow!("0x Response Error {:?}", err), retryable: matches!(err, ZeroExResponseError::ServerError(_)), - should_alert: true, + track_failure: true, } } } From 200dcb5059c9a312fe93b4aeb000ff34d1ffd708 Mon Sep 17 00:00:00 2001 From: Ben Smith Date: Wed, 13 Oct 2021 08:40:10 +0200 Subject: [PATCH 2/4] pass metrics into solver.solve and record single order solver success and failures --- solver/src/driver.rs | 16 +++++---- solver/src/metrics.rs | 2 +- solver/src/solver.rs | 17 ++++++--- solver/src/solver/baseline_solver.rs | 2 ++ solver/src/solver/http_solver.rs | 2 ++ solver/src/solver/naive_solver.rs | 3 ++ solver/src/solver/single_order_solver.rs | 44 ++++++++++++++++-------- 7 files changed, 59 insertions(+), 27 deletions(-) diff --git a/solver/src/driver.rs b/solver/src/driver.rs index 9e119a614..5a0a7a497 100644 --- a/solver/src/driver.rs +++ b/solver/src/driver.rs @@ -119,13 +119,15 @@ impl Driver { let metrics = &self.metrics; async move { let start_time = Instant::now(); - let result = - match tokio::time::timeout_at(auction.deadline.into(), solver.solve(auction)) - .await - { - Ok(inner) => inner, - Err(_timeout) => Err(anyhow!("solver timed out")), - }; + let result = match tokio::time::timeout_at( + auction.deadline.into(), + solver.solve(auction, metrics.clone()), + ) + .await + { + Ok(inner) => inner, + Err(_timeout) => Err(anyhow!("solver timed out")), + }; metrics.settlement_computed(solver.name(), start_time); (solver.clone(), result) } diff --git a/solver/src/metrics.rs b/solver/src/metrics.rs index 1a5c43878..0ce271292 100644 --- a/solver/src/metrics.rs +++ b/solver/src/metrics.rs @@ -20,7 +20,7 @@ use strum::VariantNames; /// The maximum time between the completion of two run loops. If exceeded the service will be considered unhealthy. const MAX_RUNLOOP_DURATION: Duration = Duration::from_secs(7 * 60); -pub trait SolverMetrics { +pub trait SolverMetrics: Send + Sync { fn orders_fetched(&self, orders: &[LimitOrder]); fn liquidity_fetched(&self, liquidity: &[Liquidity]); fn settlement_computed(&self, solver_type: &str, start: Instant); diff --git a/solver/src/solver.rs b/solver/src/solver.rs index 765a667de..afe9966b5 100644 --- a/solver/src/solver.rs +++ b/solver/src/solver.rs @@ -1,3 +1,4 @@ +use crate::metrics::SolverMetrics; use crate::{ liquidity::{LimitOrder, Liquidity}, settlement::Settlement, @@ -47,7 +48,11 @@ pub trait Solver: 'static { /// order) so that they can be merged by the driver at its leisure. /// /// id identifies this instance of solving by the driver in which it invokes all solvers. - async fn solve(&self, auction: Auction) -> Result>; + async fn solve( + &self, + auction: Auction, + metrics: Arc, + ) -> Result>; /// Returns solver's account that should be used to submit settlements. fn account(&self) -> &Account; @@ -307,7 +312,11 @@ impl SellVolumeFilteringSolver { #[async_trait::async_trait] impl Solver for SellVolumeFilteringSolver { - async fn solve(&self, mut auction: Auction) -> Result> { + async fn solve( + &self, + mut auction: Auction, + metrics: Arc, + ) -> Result> { let original_length = auction.orders.len(); auction.orders = self .filter_orders(auction.orders, &auction.price_estimates) @@ -316,7 +325,7 @@ impl Solver for SellVolumeFilteringSolver { "Filtered {} orders because on insufficient volume", original_length - auction.orders.len() ); - self.inner.solve(auction).await + self.inner.solve(auction, metrics).await } fn account(&self) -> &Account { @@ -338,7 +347,7 @@ mod tests { pub struct NoopSolver(); #[async_trait::async_trait] impl Solver for NoopSolver { - async fn solve(&self, _: Auction) -> Result> { + async fn solve(&self, _: Auction, _: Arc) -> Result> { Ok(Vec::new()) } diff --git a/solver/src/solver/baseline_solver.rs b/solver/src/solver/baseline_solver.rs index 17f2c96d0..13b713bc3 100644 --- a/solver/src/solver/baseline_solver.rs +++ b/solver/src/solver/baseline_solver.rs @@ -1,3 +1,4 @@ +use crate::metrics::SolverMetrics; use crate::{ liquidity::{ token_pairs, AmmOrderExecution, ConstantProductOrder, LimitOrder, Liquidity, @@ -29,6 +30,7 @@ impl Solver for BaselineSolver { Auction { orders, liquidity, .. }: Auction, + _: Arc, ) -> Result> { Ok(self.solve_(orders, liquidity)) } diff --git a/solver/src/solver/http_solver.rs b/solver/src/solver/http_solver.rs index aa469feee..1e21d052c 100644 --- a/solver/src/solver/http_solver.rs +++ b/solver/src/solver/http_solver.rs @@ -3,6 +3,7 @@ pub mod model; mod settlement; use self::{model::*, settlement::SettlementContext}; +use crate::metrics::SolverMetrics; use crate::{ liquidity::{LimitOrder, Liquidity}, settlement::Settlement, @@ -559,6 +560,7 @@ impl Solver for HttpSolver { deadline, price_estimates, }: Auction, + _: Arc, ) -> Result> { if orders.is_empty() { return Ok(Vec::new()); diff --git a/solver/src/solver/naive_solver.rs b/solver/src/solver/naive_solver.rs index be9ecc0e2..4d8588971 100644 --- a/solver/src/solver/naive_solver.rs +++ b/solver/src/solver/naive_solver.rs @@ -1,5 +1,6 @@ mod multi_order_solver; +use crate::metrics::SolverMetrics; use crate::{ liquidity::{ConstantProductOrder, LimitOrder, Liquidity}, settlement::Settlement, @@ -9,6 +10,7 @@ use anyhow::Result; use ethcontract::Account; use model::TokenPair; use std::collections::HashMap; +use std::sync::Arc; pub struct NaiveSolver { account: Account, @@ -27,6 +29,7 @@ impl Solver for NaiveSolver { Auction { orders, liquidity, .. }: Auction, + _: Arc, ) -> Result> { let uniswaps = extract_deepest_amm_liquidity(&liquidity); Ok(settle(orders, uniswaps).await) diff --git a/solver/src/solver/single_order_solver.rs b/solver/src/solver/single_order_solver.rs index d54f7892a..2cfd54484 100644 --- a/solver/src/solver/single_order_solver.rs +++ b/solver/src/solver/single_order_solver.rs @@ -1,3 +1,4 @@ +use crate::metrics::SolverMetrics; use crate::{ liquidity::LimitOrder, settlement::Settlement, @@ -6,6 +7,7 @@ use crate::{ use anyhow::{Error, Result}; use ethcontract::Account; use rand::prelude::SliceRandom; +use std::sync::Arc; use std::{collections::VecDeque, time::Duration}; #[cfg_attr(test, mockall::automock)] @@ -45,6 +47,7 @@ impl Solver for SingleOrderSolver deadline, .. }: Auction, + metrics: Arc, ) -> Result> { // Randomize which orders we start with to prevent us getting stuck on bad orders. orders.shuffle(&mut rand::thread_rng()); @@ -55,17 +58,18 @@ impl Solver for SingleOrderSolver while let Some(order) = orders.pop_front() { match self.inner.try_settle_order(order.clone()).await { Ok(settlement) => { - // TODO - track success + metrics.single_order_solver_succeeded(self.inner.name()); settlements.extend(settlement) - }, + } Err(err) => { let name = self.inner.name(); + // TODO - determine if we should record failure on all or just some of these. if err.retryable { tracing::warn!("Solver {} retryable error: {:?}", name, &err); orders.push_back(order); } else if err.track_failure { tracing::warn!("Solver {} hard error: {:?}", name, &err); - // TODO - record metric (increment counter for name) + metrics.single_order_solver_failed(name); } else { tracing::warn!("Solver {} soft error: {:?}", name, &err); } @@ -110,6 +114,7 @@ impl From for SettlementError { mod tests { use super::*; use crate::liquidity::tests::CapturingSettlementHandler; + use crate::metrics::NoopMetrics; use anyhow::anyhow; use std::sync::Arc; @@ -147,10 +152,13 @@ mod tests { ]; let settlements = solver - .solve(Auction { - orders, - ..Default::default() - }) + .solve( + Auction { + orders, + ..Default::default() + }, + Arc::new(NoopMetrics::default()), + ) .await .unwrap(); assert_eq!(settlements.len(), 2); @@ -194,10 +202,13 @@ mod tests { is_liquidity_order: false, }; solver - .solve(Auction { - orders: vec![order], - ..Default::default() - }) + .solve( + Auction { + orders: vec![order], + ..Default::default() + }, + Arc::new(NoopMetrics::default()), + ) .await .unwrap(); } @@ -229,10 +240,13 @@ mod tests { is_liquidity_order: false, }; solver - .solve(Auction { - orders: vec![order], - ..Default::default() - }) + .solve( + Auction { + orders: vec![order], + ..Default::default() + }, + Arc::new(NoopMetrics::default()), + ) .await .unwrap(); } From e9512c8cbc58b50f5ae1c4eaf586a8c47655b576 Mon Sep 17 00:00:00 2001 From: Ben Smith Date: Wed, 13 Oct 2021 19:40:58 +0200 Subject: [PATCH 3/4] apply some very nice suggestions --- solver/src/driver.rs | 16 +++--- solver/src/main.rs | 1 + solver/src/solver.rs | 54 +++++++++---------- solver/src/solver/baseline_solver.rs | 2 - solver/src/solver/http_solver.rs | 2 - solver/src/solver/naive_solver.rs | 3 -- solver/src/solver/oneinch_solver/api.rs | 1 - solver/src/solver/paraswap_solver.rs | 9 ---- solver/src/solver/single_order_solver.rs | 68 ++++++++++-------------- solver/src/solver/zeroex_solver.rs | 1 - 10 files changed, 63 insertions(+), 94 deletions(-) diff --git a/solver/src/driver.rs b/solver/src/driver.rs index 5a0a7a497..9e119a614 100644 --- a/solver/src/driver.rs +++ b/solver/src/driver.rs @@ -119,15 +119,13 @@ impl Driver { let metrics = &self.metrics; async move { let start_time = Instant::now(); - let result = match tokio::time::timeout_at( - auction.deadline.into(), - solver.solve(auction, metrics.clone()), - ) - .await - { - Ok(inner) => inner, - Err(_timeout) => Err(anyhow!("solver timed out")), - }; + let result = + match tokio::time::timeout_at(auction.deadline.into(), solver.solve(auction)) + .await + { + Ok(inner) => inner, + Err(_timeout) => Err(anyhow!("solver timed out")), + }; metrics.settlement_computed(solver.name(), start_time); (solver.clone(), result) } diff --git a/solver/src/main.rs b/solver/src/main.rs index ef4a32eb8..cb424a6cc 100644 --- a/solver/src/main.rs +++ b/solver/src/main.rs @@ -444,6 +444,7 @@ async fn main() { args.shared.paraswap_partner, client.clone(), native_token_price_estimation_amount, + metrics.clone(), ) .expect("failure creating solvers"); let liquidity_collector = LiquidityCollector { diff --git a/solver/src/solver.rs b/solver/src/solver.rs index afe9966b5..259c3a64e 100644 --- a/solver/src/solver.rs +++ b/solver/src/solver.rs @@ -48,11 +48,7 @@ pub trait Solver: 'static { /// order) so that they can be merged by the driver at its leisure. /// /// id identifies this instance of solving by the driver in which it invokes all solvers. - async fn solve( - &self, - auction: Auction, - metrics: Arc, - ) -> Result>; + async fn solve(&self, auction: Auction) -> Result>; /// Returns solver's account that should be used to submit settlements. fn account(&self) -> &Account; @@ -66,7 +62,7 @@ pub trait Solver: 'static { /// A batch auction for a solver to produce a settlement for. #[derive(Clone, Debug)] pub struct Auction { - /// An ID that idetifies a batch within a `Driver` isntance. + /// An ID that identifies a batch within a `Driver` instance. /// /// Note that this ID is not unique across multiple instances of drivers, /// in particular it cannot be used to uniquely identify batches across @@ -158,6 +154,7 @@ pub fn create( paraswap_partner: Option, client: Client, native_token_amount_to_estimate_prices_with: U256, + solver_metrics: Arc, ) -> Result { // Tiny helper function to help out with type inference. Otherwise, all // `Box::new(...)` expressions would have to be cast `as Box`. @@ -217,7 +214,7 @@ pub fn create( }, )), SolverType::OneInch => { - let one_inch_solver: SingleOrderSolver<_> = + let one_inch_solver: SingleOrderSolver<_> = SingleOrderSolver::new( OneInchSolver::with_disabled_protocols( account, web3.clone(), @@ -225,8 +222,9 @@ pub fn create( chain_id, disabled_one_inch_protocols.clone(), client.clone(), - )? - .into(); + )?, + solver_metrics.clone(), + ); // We only want to use 1Inch for high value orders shared(SellVolumeFilteringSolver::new( Box::new(one_inch_solver), @@ -242,18 +240,24 @@ pub fn create( client.clone(), ) .unwrap(); - shared(SingleOrderSolver::from(zeroex_solver)) + shared(SingleOrderSolver::new( + zeroex_solver, + solver_metrics.clone(), + )) } - SolverType::Paraswap => shared(SingleOrderSolver::from(ParaswapSolver::new( - account, - web3.clone(), - settlement_contract.clone(), - token_info_fetcher.clone(), - paraswap_slippage_bps, - disabled_paraswap_dexs.clone(), - client.clone(), - paraswap_partner.clone(), - ))), + SolverType::Paraswap => shared(SingleOrderSolver::new( + ParaswapSolver::new( + account, + web3.clone(), + settlement_contract.clone(), + token_info_fetcher.clone(), + paraswap_slippage_bps, + disabled_paraswap_dexs.clone(), + client.clone(), + paraswap_partner.clone(), + ), + solver_metrics.clone(), + )), }; if let Ok(solver) = &solver { @@ -312,11 +316,7 @@ impl SellVolumeFilteringSolver { #[async_trait::async_trait] impl Solver for SellVolumeFilteringSolver { - async fn solve( - &self, - mut auction: Auction, - metrics: Arc, - ) -> Result> { + async fn solve(&self, mut auction: Auction) -> Result> { let original_length = auction.orders.len(); auction.orders = self .filter_orders(auction.orders, &auction.price_estimates) @@ -325,7 +325,7 @@ impl Solver for SellVolumeFilteringSolver { "Filtered {} orders because on insufficient volume", original_length - auction.orders.len() ); - self.inner.solve(auction, metrics).await + self.inner.solve(auction).await } fn account(&self) -> &Account { @@ -347,7 +347,7 @@ mod tests { pub struct NoopSolver(); #[async_trait::async_trait] impl Solver for NoopSolver { - async fn solve(&self, _: Auction, _: Arc) -> Result> { + async fn solve(&self, _: Auction) -> Result> { Ok(Vec::new()) } diff --git a/solver/src/solver/baseline_solver.rs b/solver/src/solver/baseline_solver.rs index 13b713bc3..17f2c96d0 100644 --- a/solver/src/solver/baseline_solver.rs +++ b/solver/src/solver/baseline_solver.rs @@ -1,4 +1,3 @@ -use crate::metrics::SolverMetrics; use crate::{ liquidity::{ token_pairs, AmmOrderExecution, ConstantProductOrder, LimitOrder, Liquidity, @@ -30,7 +29,6 @@ impl Solver for BaselineSolver { Auction { orders, liquidity, .. }: Auction, - _: Arc, ) -> Result> { Ok(self.solve_(orders, liquidity)) } diff --git a/solver/src/solver/http_solver.rs b/solver/src/solver/http_solver.rs index 1e21d052c..aa469feee 100644 --- a/solver/src/solver/http_solver.rs +++ b/solver/src/solver/http_solver.rs @@ -3,7 +3,6 @@ pub mod model; mod settlement; use self::{model::*, settlement::SettlementContext}; -use crate::metrics::SolverMetrics; use crate::{ liquidity::{LimitOrder, Liquidity}, settlement::Settlement, @@ -560,7 +559,6 @@ impl Solver for HttpSolver { deadline, price_estimates, }: Auction, - _: Arc, ) -> Result> { if orders.is_empty() { return Ok(Vec::new()); diff --git a/solver/src/solver/naive_solver.rs b/solver/src/solver/naive_solver.rs index 4d8588971..be9ecc0e2 100644 --- a/solver/src/solver/naive_solver.rs +++ b/solver/src/solver/naive_solver.rs @@ -1,6 +1,5 @@ mod multi_order_solver; -use crate::metrics::SolverMetrics; use crate::{ liquidity::{ConstantProductOrder, LimitOrder, Liquidity}, settlement::Settlement, @@ -10,7 +9,6 @@ use anyhow::Result; use ethcontract::Account; use model::TokenPair; use std::collections::HashMap; -use std::sync::Arc; pub struct NaiveSolver { account: Account, @@ -29,7 +27,6 @@ impl Solver for NaiveSolver { Auction { orders, liquidity, .. }: Auction, - _: Arc, ) -> Result> { let uniswaps = extract_deepest_amm_liquidity(&liquidity); Ok(settle(orders, uniswaps).await) diff --git a/solver/src/solver/oneinch_solver/api.rs b/solver/src/solver/oneinch_solver/api.rs index dcd8af90e..d85fc9c83 100644 --- a/solver/src/solver/oneinch_solver/api.rs +++ b/solver/src/solver/oneinch_solver/api.rs @@ -143,7 +143,6 @@ impl From for SettlementError { SettlementError { inner: anyhow!(error.message), retryable: matches!(error.status_code, 500), - track_failure: true, } } } diff --git a/solver/src/solver/paraswap_solver.rs b/solver/src/solver/paraswap_solver.rs index 384d1f726..fdb57af6d 100644 --- a/solver/src/solver/paraswap_solver.rs +++ b/solver/src/solver/paraswap_solver.rs @@ -81,15 +81,6 @@ impl From for SettlementError { | ParaswapResponseError::ServerBusy | ParaswapResponseError::Send(_), ), - track_failure: !matches!( - err, - ParaswapResponseError::PriceChange - | ParaswapResponseError::BuildingTransaction(_) - | ParaswapResponseError::ComputePrice(_) - | ParaswapResponseError::InsufficientLiquidity - | ParaswapResponseError::TooMuchSlippageOnQuote - | ParaswapResponseError::ServerBusy, - ), } } } diff --git a/solver/src/solver/single_order_solver.rs b/solver/src/solver/single_order_solver.rs index 2cfd54484..ac5e3a908 100644 --- a/solver/src/solver/single_order_solver.rs +++ b/solver/src/solver/single_order_solver.rs @@ -30,11 +30,12 @@ pub trait SingleOrderSolving { pub struct SingleOrderSolver { inner: I, + metrics: Arc, } -impl From for SingleOrderSolver { - fn from(inner: I) -> Self { - Self { inner } +impl SingleOrderSolver { + pub fn new(inner: I, metrics: Arc) -> Self { + Self { inner, metrics } } } @@ -47,7 +48,6 @@ impl Solver for SingleOrderSolver deadline, .. }: Auction, - metrics: Arc, ) -> Result> { // Randomize which orders we start with to prevent us getting stuck on bad orders. orders.shuffle(&mut rand::thread_rng()); @@ -58,20 +58,18 @@ impl Solver for SingleOrderSolver while let Some(order) = orders.pop_front() { match self.inner.try_settle_order(order.clone()).await { Ok(settlement) => { - metrics.single_order_solver_succeeded(self.inner.name()); + self.metrics + .single_order_solver_succeeded(self.inner.name()); settlements.extend(settlement) } Err(err) => { let name = self.inner.name(); - // TODO - determine if we should record failure on all or just some of these. + self.metrics.single_order_solver_failed(name); if err.retryable { - tracing::warn!("Solver {} retryable error: {:?}", name, &err); + tracing::warn!("Solver {} retryable error: {:?}", name, &err.inner); orders.push_back(order); - } else if err.track_failure { - tracing::warn!("Solver {} hard error: {:?}", name, &err); - metrics.single_order_solver_failed(name); } else { - tracing::warn!("Solver {} soft error: {:?}", name, &err); + tracing::warn!("Solver {} error: {:?}", name, &err.inner); } } } @@ -96,8 +94,6 @@ impl Solver for SingleOrderSolver pub struct SettlementError { pub inner: anyhow::Error, pub retryable: bool, - // Whether or not this error should be logged as an error - pub track_failure: bool, } impl From for SettlementError { @@ -105,7 +101,6 @@ impl From for SettlementError { SettlementError { inner: err, retryable: false, - track_failure: true, } } } @@ -125,8 +120,10 @@ mod tests { .expect_try_settle_order() .times(2) .returning(|_| Ok(Some(Settlement::new(Default::default())))); + inner.expect_name().returning(|| "Mock Solver"); - let solver: SingleOrderSolver<_> = inner.into(); + let solver: SingleOrderSolver<_> = + SingleOrderSolver::new(inner, Arc::new(NoopMetrics::default())); let handler = Arc::new(CapturingSettlementHandler::default()); let order = LimitOrder { id: Default::default(), @@ -152,13 +149,10 @@ mod tests { ]; let settlements = solver - .solve( - Auction { - orders, - ..Default::default() - }, - Arc::new(NoopMetrics::default()), - ) + .solve(Auction { + orders, + ..Default::default() + }) .await .unwrap(); assert_eq!(settlements.len(), 2); @@ -178,7 +172,6 @@ mod tests { 0 => Err(SettlementError { inner: anyhow!(""), retryable: true, - track_failure: true, }), 1 => Ok(None), _ => unreachable!(), @@ -187,7 +180,8 @@ mod tests { result }); - let solver: SingleOrderSolver<_> = inner.into(); + let solver: SingleOrderSolver<_> = + SingleOrderSolver::new(inner, Arc::new(NoopMetrics::default())); let handler = Arc::new(CapturingSettlementHandler::default()); let order = LimitOrder { id: Default::default(), @@ -202,13 +196,10 @@ mod tests { is_liquidity_order: false, }; solver - .solve( - Auction { - orders: vec![order], - ..Default::default() - }, - Arc::new(NoopMetrics::default()), - ) + .solve(Auction { + orders: vec![order], + ..Default::default() + }) .await .unwrap(); } @@ -221,11 +212,11 @@ mod tests { Err(SettlementError { inner: anyhow!(""), retryable: false, - track_failure: true, }) }); - let solver: SingleOrderSolver<_> = inner.into(); + let solver: SingleOrderSolver<_> = + SingleOrderSolver::new(inner, Arc::new(NoopMetrics::default())); let handler = Arc::new(CapturingSettlementHandler::default()); let order = LimitOrder { id: Default::default(), @@ -240,13 +231,10 @@ mod tests { is_liquidity_order: false, }; solver - .solve( - Auction { - orders: vec![order], - ..Default::default() - }, - Arc::new(NoopMetrics::default()), - ) + .solve(Auction { + orders: vec![order], + ..Default::default() + }) .await .unwrap(); } diff --git a/solver/src/solver/zeroex_solver.rs b/solver/src/solver/zeroex_solver.rs index 140b2301a..a0f85269f 100644 --- a/solver/src/solver/zeroex_solver.rs +++ b/solver/src/solver/zeroex_solver.rs @@ -137,7 +137,6 @@ impl From for SettlementError { SettlementError { inner: anyhow!("0x Response Error {:?}", err), retryable: matches!(err, ZeroExResponseError::ServerError(_)), - track_failure: true, } } } From ef319bf0ed212a8ed8047b13e9339eab05d63066 Mon Sep 17 00:00:00 2001 From: Ben Smith Date: Thu, 14 Oct 2021 09:34:06 +0200 Subject: [PATCH 4/4] cargo fmt --- solver/src/metrics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solver/src/metrics.rs b/solver/src/metrics.rs index ae93624c3..91e8f86ca 100644 --- a/solver/src/metrics.rs +++ b/solver/src/metrics.rs @@ -103,7 +103,7 @@ impl Metrics { Opts::new("single_order_solver", "Success/Failure counts"), &["result", "solver_type"], )?; - + registry.register(Box::new(single_order_solver_runs.clone()))?; let matched_but_liquidity = IntCounter::new( "orders_matched_liquidity",