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

Refactor Warp Filter Initialization For Metrics Reset #789

Merged
merged 2 commits into from
Nov 17, 2022

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Nov 16, 2022

This PR refactors how we initialize our warp router and ensures that metrics are initialized for all requests.

This is a QOL change to improve Grafana metrics around API request responses. This is because, if we don't reset metrics to 0 they don't exist at all until the first time we increment their counter. This causes no-data alerts in Grafana alerts and can make panels weird to look at.

Test Plan

Run some sample queries and see they still work:

% curl -s http://localhost:8080/api/v1/quote -X POST -H 'content-type: application/json' --data '@-' <<JSON | jq
{
  "from": "0x0000000000000000000000000000000000000000",
  "sellToken": "0xdac17f958d2ee523a2206206994597c13d831ec7",
  "buyToken": "0x6b175474e89094c44da98b954eedeac495271d0f",
  "kind": "sell",
  "sellAmountBeforeFee": "1000000000"
}
JSON
{
  "quote": {
    "sellToken": "0xdac17f958d2ee523a2206206994597c13d831ec7",
    "buyToken": "0x6b175474e89094c44da98b954eedeac495271d0f",
    "receiver": null,
    "sellAmount": "996326028",
    "buyAmount": "992459269806446405282",
    "validTo": 1668605853,
    "appData": "0x0000000000000000000000000000000000000000000000000000000000000000",
    "feeAmount": "3673972",
    "kind": "sell",
    "partiallyFillable": false,
    "sellTokenBalance": "erc20",
    "buyTokenBalance": "erc20",
    "signingScheme": "eip712"
  },
  "from": "0x0000000000000000000000000000000000000000",
  "expiration": "2022-11-16T13:08:33.276651Z",
  "id": 4
}

Also, fetch metrics and see we initialized a bunch of request metrics:

% curl -s http://localhost:9586/metrics
...
gp_v2_api_api_requests_complete{method="v1/version",status_code="200"} 0
gp_v2_api_api_requests_complete{method="v1/version",status_code="201"} 0
gp_v2_api_api_requests_complete{method="v1/version",status_code="400"} 0
gp_v2_api_api_requests_complete{method="v1/version",status_code="401"} 0
gp_v2_api_api_requests_complete{method="v1/version",status_code="403"} 0
gp_v2_api_api_requests_complete{method="v1/version",status_code="404"} 0
gp_v2_api_api_requests_complete{method="v1/version",status_code="500"} 0
gp_v2_api_api_requests_complete{method="v1/version",status_code="503"} 0
...

@nlordell nlordell requested a review from a team as a code owner November 16, 2022 13:14
@nlordell nlordell added the oncall Issue/PR for consideration during oncall rotation label Nov 16, 2022
@vkgnosis
Copy link
Contributor

For context, the problem is that if we don't reset metrics to 0 they don't exist at all until the first time we increment their counter. This causes no-data alerts in Grafana alerts and can make panels weird to look at.

@nlordell
Copy link
Contributor Author

For context, the problem is that if we don't reset metrics to 0 they don't exist at all until the first time we increment their counter. This causes no-data alerts in Grafana alerts and can make panels weird to look at.

Added this to the PR description.

Copy link
Contributor

@sistemd sistemd left a comment

Choose a reason for hiding this comment

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

I'm not thrilled with the proliferation of v1 prefixes, but if there's no other way it's not that big of a deal anyway.

@nlordell
Copy link
Contributor Author

I'm not thrilled with the proliferation of v1 prefixes

Interesting, I actually thought it was "more correct". For example, the get_solvable_orders_v2 module clearly expects to be a v2 route, but required the caller to install it in the right place (instead of having the caller just chose the prefix (api in this case). WDYT?

Otherwise, we can easily change the router definition to insert v1 and v2 when creating the routes.

@nlordell nlordell enabled auto-merge (squash) November 17, 2022 16:09
@nlordell nlordell merged commit 11f4fdc into main Nov 17, 2022
@nlordell nlordell deleted the initialize-request-metrics branch November 17, 2022 16:14
@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
oncall Issue/PR for consideration during oncall rotation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants