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

Add trait for configuring GasNow WebSocket error reporting #10

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Sep 7, 2021

This PR adds a trait for reporting errors in the gas now WebSocket-based gas price estimator. This way, we can configure custom error handling (such as adding metrics) in gp-v2-services as well as remove some of the pesky error alerts we were seeing.

Test Plan

Not sure what to run, just did cargo fmt -- --check && cargo clippy --all-features --all-targets && cargo test.

@nlordell nlordell requested a review from a team September 7, 2021 13:59
Copy link
Contributor

@vkgnosis vkgnosis left a comment

Choose a reason for hiding this comment

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

Seems fine although the fact that we need it is an indication that this component should move back into the main repo to me.

Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

I was looking around in gp-v2-services all day for some indication of what was causing either of these two alerts

[DFUSION][GP-V2-SERVICES] Error
ERROR (pod: dev-dfusion-v2-solver-mainnet-dc6549cf-chhzq):
2021-09-06T20:44:02.871Z ERROR gas_estimation::gasnow_websocket: websocket stream timed out

[DFUSION][GP-V2-SERVICES] Error
ERROR (pod: dev-dfusion-v2-solver-mainnet-dc6549cf-chhzq):
2021-09-07T07:11:19.768Z ERROR gas_estimation::gasnow_websocket: websocket stream failed err=Protocol(ResetWithoutClosingHandshake)

Looks like this PR intended to be a first step at dealing with these. I actually didn't even know this repo existed until I was notified about this PR.

@nlordell
Copy link
Contributor Author

nlordell commented Sep 8, 2021

Seems fine although the fact that we need it is an indication that this component should move back into the main repo to me.

Created #11.

I don't disagree. I also found that its not super general and specific to how we use it in services. Merging for now to silence alerts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants