-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
MERC-5943 Surface EA error fields and track bid/ask violations #13785
Conversation
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.
This looks good to me generally but we should also get approval from someone with more core experience. The EA framework has now been updated with a more specific error type to make it easier to pick up in the node:
{
"status": "errored",
"statusCode": 500,
"error": {
"name": "AdapterLWBAError",
"message": "Invariant violation. LWBA response must contain mid, bid and ask prices. Got: (mid: 3064.866547022821, bid: 3064.8327375532613, ask: null)"
}
}
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.
LGTM just need a regression test
91dc1be
to
2f5d7d9
Compare
6e52e58
to
dfab69f
Compare
@@ -64,7 +64,7 @@ func makeHTTPRequest( | |||
|
|||
if statusCode >= 400 { | |||
maybeErr := bestEffortExtractError(responseBytes) | |||
return nil, statusCode, respHeaders, 0, errors.Errorf("got error from %s: (status code %v) %s", url.String(), statusCode, maybeErr) | |||
return responseBytes, statusCode, respHeaders, 0, errors.Errorf("got error from %s: (status code %v) %s", url.String(), statusCode, maybeErr) |
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.
This helper is only used by the http and bridge tasks and in both cases it's fine to return the response on error.
dfab69f
to
b2da7aa
Compare
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.
The new telemetry change looks good to me but I'll leave it to @samsondav to give the final approval 👍
Quality Gate passedIssues Measures |
Context
Per @mjk90, EAs will track if the bid/ask invariant is violated and will send through a specific error message with a 500 status if so:
We want to capture the EA's error, propagate it through the pipeline, catch it in the Mercury observer, and track it. We also want to make sure that the observation is not used in producing a report.
Changes
The EA is called in the bridge task. Currently, for a 500 response, the
makeHTTPRequest
helper returns a generic error which isn't the most helpful. I've added aeautils.BestEffortExtractEAError
helper to build a structured EA error that captures the error name and message.Then, in the Mercury observer, we can look for this error (it will be a wrapped error from parsing one of the pipeline's 3 pricing fields). If this error is found then we will send an enhanced telemetry record for it.
Note that previously we would not send telemetry if there is any error so this is new behaviour cc @mjk90. I've also added a new
EnhancedEAMercury.dp_invariant_violation_detected
protobuf field to track this.