Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update price estimate categories #3256

Merged
merged 12 commits into from
Feb 3, 2025
83 changes: 58 additions & 25 deletions crates/shared/src/price_estimation/instrumented.rs
Original file line number Diff line number Diff line change
@@ -37,15 +37,18 @@ impl<T> InstrumentedPriceEstimator<T> {
}
}

/// Determines the result of a price estimate as either "success" or
/// "failure".
/// Determines the result of a price estimate, returning either "success" or
/// the error reason
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"
match estimate {
Ok(_) | Err(PriceEstimationError::NoLiquidity) => "success",
bram-vdberg marked this conversation as resolved.
Show resolved Hide resolved
Err(PriceEstimationError::UnsupportedToken { .. }) => "unsupported_token",
Err(PriceEstimationError::UnsupportedOrderType(_)) => "unsupported_order_type",
Err(PriceEstimationError::RateLimited) => "rate_limited",
Err(PriceEstimationError::EstimatorInternal(_)) => "estimator_internal_error",
Err(PriceEstimationError::ProtocolInternal(_)) => "protocol_internal_error",
squadgazzz marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +51 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 errors should be adjusted in a follow up PR to contain specific error conditions and not a catch all error like anyhow::Error so that we can match on them here and plot more helpful data.

}
}
}
@@ -141,39 +144,69 @@ mod tests {
];

let mut estimator = MockPriceEstimating::new();
let expected_query = queries[0].clone();
estimator
.expect_estimate()
.times(1)
.withf(move |q| *q == expected_query)
.returning(|_| async { Ok(Estimate::default()) }.boxed());
let expected_query = queries[1].clone();
estimator
.expect_estimate()
.times(1)
.withf(move |q| *q == expected_query)
.returning(|_| {
async { Err(PriceEstimationError::EstimatorInternal(anyhow!(""))) }.boxed()
});
let expectations = vec![
(0, Ok(Estimate::default())),
(1, Err(PriceEstimationError::NoLiquidity)),
(
0,
Err(PriceEstimationError::UnsupportedToken {
token: H160([0; 20]),
reason: "".to_string(),
}),
),
(
1,
Err(PriceEstimationError::UnsupportedOrderType("".to_string())),
),
(0, Err(PriceEstimationError::RateLimited)),
(1, Err(PriceEstimationError::EstimatorInternal(anyhow!("")))),
(0, Err(PriceEstimationError::ProtocolInternal(anyhow!("")))),
];

for (query_index, result) in expectations {
let expected_query = queries[query_index].clone();
estimator
.expect_estimate()
.times(1)
.withf(move |q| *q == expected_query)
.returning(move |_| {
let result = result.clone();
async { result }.boxed()
});
}

let instrumented = InstrumentedPriceEstimator::new(estimator, "foo".to_string());

let _ = instrumented.estimate(queries[0].clone()).await;
let _ = instrumented.estimate(queries[1].clone()).await;
for i in 0..7 {
bram-vdberg marked this conversation as resolved.
Show resolved Hide resolved
let query = if i % 2 == 0 { &queries[0] } else { &queries[1] };
bram-vdberg marked this conversation as resolved.
Show resolved Hide resolved
let _ = instrumented.estimate(query.clone()).await;
}

for result in &["success", "failure"] {
for result in &[
"unsupported_token",
"unsupported_order_type",
"rate_limited",
"estimator_internal_error",
"protocol_internal_error",
] {
let observed = instrumented
.metrics
.price_estimates
.with_label_values(&["foo", result])
.get();
assert_eq!(observed, 1);
}
let observed = instrumented
let observed_success = instrumented
.metrics
.price_estimates
.with_label_values(&["foo", "success"])
.get();
assert_eq!(observed_success, 2);
let observed_count = instrumented
.metrics
.price_estimation_times
.with_label_values(&["foo"])
.get_sample_count();
assert_eq!(observed, 2);
assert_eq!(observed_count, 7);
}
}