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

Count total requests sent to dex API #102

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

MartinquaXD
Copy link
Contributor

Context

The oneinch solver on arbitrum currently causes a lot of noisy alerts which aren't really actionable.
When looking at the alert it appears as if ~40% of the request to the 1inch API return unexpected errors. However, after closer investigation it seems like this alert actually counts how many of the errors are unexpected errors.
Specifically for the arbitrum 1inch case <1% of all requests result in an unexpected error but 40% of all errors are unexpected.

Since 99% of the requests actually result in reasonable responses (albeit not good enough to satisfy the order's limit price) the 1inch solver should actually be considered healthy.

Solution

The error handling logic is surprisingly messy: Errors are grouped in weird ways, handled in multiple places and used in conjunction with Option.
To resolve the noisy alert ASAP using a minimal patch without having to rethink all the error handling logic I decided to just introduce another metric which simply counts all requests sent to the API.
After this gets merged the alert should be updated to alert based on the ratio unexpected_errors / total_requests instead of unexpected_errors / total_errors

Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

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

Looks reasonable, thanks 👍

@MartinquaXD MartinquaXD merged commit b7b251e into main Jan 21, 2025
3 checks passed
@MartinquaXD MartinquaXD deleted the fix-1inch-noisy-alerts branch January 21, 2025 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants