Add load-based throttling to search endpoints #116
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emmanueldenloye
approved these changes
Dec 23, 2022
This PR removes the yet unreleased `fromheight` parameter of the `/txs/accounts` endpoint. The original purpose of that parameter was to facilitate efficient pagination of the `/txs/accounts` results, but it has since been obsoleted for that purpose by the `Chainweb-Next`-token-based pagination workflow. The `Chainweb-Next`-based pagination is superior, because it doesn't require the client to figure out how to stitch together the result chunks produced by each request and it is easier to reuse across endpoints independent of whether specifying `height` limits is a reasonable or efficient way to paginate for a given endpoint. Admittedly, `fromheight` could still be useful independent of pagination, but I'm not convinced that adding query parameters to `chainweb-data` endpoints for hypothetical use cases is a good idea at this stage, especially considering the backward compatibility burden that entails. * Remove the fromheight parameter of /txs/accounts * Update the chainweb-api pin
This PR adds a hidden `--serve-swagger-ui` CLI option to the `serve` subcommand. When `--serve-swagger-ui` is enabled, `chainweb-data` serves an OpenAPI v3 spec for the `chainweb-data` API. The spec that's currently served is automatically generated from the Servant API definition using [`servant-openapi3`](https://hackage.haskell.org/package/servant-openapi3) and this PR makes no effort to refine the documentation generated by `servant-openapi3` which lacks any explanations for the endpoints and the data schemas and it produces unhelpful details like integral values having meaningless upper and lower bounds. So, despite being helpful as it is, the currently generated spec is only rudimentary, that's why this PR hides the new `--serve-swagger-ui` argument from the CLI `--help` output keeping it unofficial for now. * Generate and serve OpenAPI specs * Make swagger UI optional with a hidden CLI flag * Remove unused declaration * Add special 404 message when cwd-spec not enabled
enobayram
changed the title
|
@emmanueldenloye, I've made significant changes to this PR since your approval (see the PR description), so I've asked for another review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds instrumentation around the connection pool used by the endpoint handlers to keep track of the number of requests currently pending or being served. It then uses this information in order to scale down the amount of work that's performed per request by the search endpoints in order to make it easier to maintain a fair resource allocation under load across multiple clients.
This PR still doesn't stop a single client from making a large number of search requests in parallel, but
chainweb-dataexpects that this would be handled via rate limiting at the API gateway layer. In theory, it could be argued that it would be better for the gateway to handle such throttling as well, but it seems harder to implement there in practice.This PR also increases the maximum
scanLimitof the search endpoints from 20000 to 50000 (but subject to load throttling) in order to reduce network roundtrips. It used to take us around 50-100ms to scan 20000 rows, so now each request should take between 100-250ms, which is still low enough and now each request should scan around 10 hours worth of blockchain history.