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

MERC-5943 Surface EA error fields and track bid/ask violations #13785

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

martin-cll
Copy link
Contributor

@martin-cll martin-cll commented Jul 9, 2024

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:

{
    "status": "errored",
    "statusCode": 500,
    "error": {
        "name": "AdapterLWBAError",
        "message": "Invariant violation. Mid price must be between bid and ask prices. Got: (mid: 3051.493596362398, bid: 3051.360434937159, ask: 3051.4270156497787)"
    }
}

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 a eautils.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.

@martin-cll martin-cll requested a review from a team July 9, 2024 05:46
@martin-cll martin-cll requested review from a team as code owners July 9, 2024 05:46
Copy link
Contributor

@mjk90 mjk90 left a 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)"
    }
}

Copy link
Collaborator

@samsondav samsondav left a 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

@@ -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)
Copy link
Contributor Author

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.

Copy link
Contributor

@mjk90 mjk90 left a 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 👍

@martin-cll martin-cll enabled auto-merge July 16, 2024 05:24
@martin-cll martin-cll added this pull request to the merge queue Jul 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 16, 2024
@martin-cll martin-cll added this pull request to the merge queue Jul 16, 2024
Merged via the queue into develop with commit 873abac Jul 16, 2024
115 checks passed
@martin-cll martin-cll deleted the ml/bid-ask-violation branch July 16, 2024 07:23
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.

3 participants