-
Notifications
You must be signed in to change notification settings - Fork 67
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
Less error debugs for 0x api call failure #184
Conversation
Codecov Report
@@ Coverage Diff @@
## main #184 +/- ##
==========================================
- Coverage 64.59% 64.57% -0.02%
==========================================
Files 185 185
Lines 38596 38602 +6
==========================================
- Hits 24931 24928 -3
- Misses 13665 13674 +9 |
.flat_map(|result| match result { | ||
Ok(order_record_vec) => order_record_vec, | ||
Err(err) => { | ||
tracing::warn!("ZeroExResponse error during liqudity fetching: {}", err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, its good to have metrics for measuring errors that we don't alert on.
That being said, I think we already count the number of 0x liquidity orders with another metric, so we can use that to see if we aren't getting any 0x liquidity through.
Also, are these errors intermittent? Is there some underlying issue that is causing them to happen (such as being rate limited? - if so maybe we can consider using the new RateLimiter
that @MartinquaXD introduced?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, I think we already count the number of 0x liquidity orders with another metric, so we can use that to see if we aren't getting any 0x liquidity through.
There is currently only a metric for the number of LimitOrder
liquidity items. At the moment we only get LimitOrder
liquidity from 0x but this might change in the future. This metric also only shows the number of usable 0x orders we got. I believe we could theoretically get 0 usable 0x orders without errors while collecting liquidity.
if so maybe we can consider using the new RateLimiter
The underlying error is some generic reqwest::Error
which happened during RequestBuilder::send()
. The docs say the following about that:
/// # Errors
///
/// This method fails if there was an error while sending request,
/// redirect loop was detected or redirect limit was exhausted.
pub fn execute(
&self,
request: Request,
) -> impl Future<Output = Result<Response, crate::Error>> {
So I don't think there is anything we can do about that. To make use of the RateLimiter
we need to actually get a response with status code 429
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Martin, for the investigation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it generally makes sense to overhaul the way we fetch liquidity.
At the moment an error while fetching uniswap-like
, balancer
or 0x
liquidity will cause all the successfully collected liquidity to be discarded or not fetched in the first place.
I believe the PR offers a good enough temporary work around such that 0x errors no longer affect all the other liquidity but we should keep this issue in mind to have a proper solution for all liquidity types moving forward.
During oncall, it occured to me that the 0x liquidity fetching was generating noisy error messages like this:
https://cowservices.slack.com/archives/C0371SB243E/p1651405247972289
Tried to fix it with a different error handling.
Test Plan