Update price estimate categories #3256
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
What is the main purpose of this PR? Do you want a better observation on Grafana panels? I think this PR doesn't change anything else. |
Co-authored-by: ilya <[email protected]>
Yes, we're setting up a Grafana dashboard for the quoting competition. Hopefully this will help us to visualize the failing quotes a bit better. I think we also want to show the driver errors, but I'll do that in a follow up PR. |
| } else { | ||
| "failure" | ||
| match estimate { | ||
| Ok(_) | Err(PriceEstimationError::NoLiquidity) => "success", |
There was a problem hiding this comment.
Given that the intention of this PR is to expose a helpful dashboard for solver about their quoting performance I think it makes more sense to have a separate label for NoLiquidity and adjust grafana queries that currently use success to use success|no_liqudity going forward.
| Err(PriceEstimationError::EstimatorInternal(_)) => "estimator_internal_error", | ||
| Err(PriceEstimationError::ProtocolInternal(_)) => "protocol_internal_error", |
There was a problem hiding this comment.
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.
| let _ = instrumented.estimate(queries[0].clone()).await; | ||
| let _ = instrumented.estimate(queries[1].clone()).await; | ||
| for i in 0..7 { | ||
| let query = if i % 2 == 0 { &queries[0] } else { &queries[1] }; |
There was a problem hiding this comment.
Alternating between the sell and the buy order query seems odd and leads to clunky code. I think we could also just have 1 query we use for all these cases instead of alternating between 2.
Co-authored-by: Martin Magnus <[email protected]>
Description
This PR updates the price estimate categories to return the error reason on a failing price estimate.
Changes
PriceEstimationErrorto the result for increased visibility on price estimation error reasonsHow to test