Skip to content
This repository has been archived by the owner on Apr 28, 2022. It is now read-only.

Demote Paraswap Errors to warn! + success/failure metrics #1248

Merged
merged 6 commits into from
Oct 14, 2021

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Oct 13, 2021

Based on the amount of noise produced by Paraswap errors in our alerts channel, we deemed it appropriate to demote these errors to warnings and instead record success/failure counts of the single order solver grouped by solver name.

Note that this change required passing Arc<dyn SolverMetrics> through the trait method Solver::solve which remains unused in many places (apart from the single order solver). However, this PR serves as a good preparation for recording other metrics to come (refined time counters on different stages of solving, etc...).

Test Plan

I recall that @fleupold had done some sort of manual test for metrics at one point and provided the result data in some strange prometheus file format. I could do that if it seems appropriate.

  1. Run the solver locally pointed at the production orderbook with just the Paraswap solver enabled

  2. Connect to the metrics api and find something like this in the logs

# HELP gp_v2_solver_single_order_solver Success/Failure counts
# TYPE gp_v2_solver_single_order_solver counter
gp_v2_solver_single_order_solver{result="success",solver_type="ParaSwap"} 4

@bh2smith bh2smith requested a review from a team October 13, 2021 06:46
@bh2smith bh2smith changed the title Demote Paraswap Errors to warn! + add success/failure metrics Demote Paraswap Errors to warn! + success/failure metrics Oct 13, 2021
solver/src/driver.rs Outdated Show resolved Hide resolved
solver/src/solver.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

Not a fan of passing metrics with the Solver::solve method, I would instead recommend passing the metrics in when constructing the solvers.

@nlordell
Copy link
Contributor

I could do that if it seems appropriate.

Please do. You can run the solver and connect to http://localhost:9586 to get the metrics.

@nlordell
Copy link
Contributor

I believe this PR closes #1015

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2021

Codecov Report

Merging #1248 (ef319bf) into main (e021831) will decrease coverage by 0.01%.
The diff coverage is 42.10%.

@@            Coverage Diff             @@
##             main    #1248      +/-   ##
==========================================
- Coverage   67.43%   67.42%   -0.02%     
==========================================
  Files         135      135              
  Lines       28344    28372      +28     
==========================================
+ Hits        19113    19129      +16     
- Misses       9231     9243      +12     

Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for fixing this 😄.

As a follow up task, could you add panels in our Grafana dashboard for this as well?

@bh2smith
Copy link
Contributor Author

As a follow up task, could you add panels in our Grafana dashboard

Yes absolutely, although I am still trying to figure out how.

@bh2smith bh2smith merged commit 60e9079 into main Oct 14, 2021
@bh2smith bh2smith deleted the paraswap-errors branch October 14, 2021 08:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants