fix(api_service): exclude health checks from throttling with ConcurrencyLimitLayer #2320
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
netrome
previously approved these changes
Oct 8, 2024
netrome
previously approved these changes
Oct 8, 2024
netrome
previously approved these changes
Oct 8, 2024
rafal-ch
reviewed
Oct 8, 2024
Co-authored-by: Rafał Chabowski <[email protected]>
rafal-ch
previously approved these changes
Oct 8, 2024
netrome
previously approved these changes
Oct 9, 2024
xgreenx
reviewed
Oct 9, 2024
Comment on lines
224
to
236
| let unthrottled_routes = Router::new() | ||
| .route("/v1/health", get(health)) | ||
| .route("/health", get(health)); | ||
|
|
||
| let throttled_routes = Router::new() | ||
| .route("/v1/playground", get(graphql_playground)) | ||
| .route("/v1/graphql", post(graphql_handler).options(ok)) | ||
| .route( | ||
| "/v1/graphql-sub", | ||
| post(graphql_subscription_handler).options(ok), | ||
| ) | ||
| .route("/v1/metrics", get(metrics)) | ||
| .route("/v1/health", get(health)) | ||
| .route("/health", get(health)) | ||
| .layer(ConcurrencyLimitLayer::new(concurrency_limit)) | ||
| .layer(ConcurrencyLimitLayer::new(concurrency_limit)); |
There was a problem hiding this comment.
I think we want something like. Because graph-sub also is limited by number of subscriptions, and playground is one time query that returns pre rendered html. Throteling metrics doesn't allow request data=)
There was a problem hiding this comment.
Throteling metrics doesn't allow request data=)
are you saying that because metrics are throttled, it affects how graphql and graphql-sub are throttled as well?
why shouldn't we throttle the metrics?
There was a problem hiding this comment.
Because if we have a lot of queries, it becomes impossible for us to track the state of the node from the Grafana=)
netrome
approved these changes
Oct 9, 2024
xgreenx
approved these changes
Oct 10, 2024
Merged
rafal-ch
added a commit
that referenced
this pull request
Oct 11, 2024
## Version v0.39.0 ### Added - [2324](#2324): Added metrics for sync, async processor and for all GraphQL queries. - [2320](#2320): Added new CLI flag `graphql-max-resolver-recursive-depth` to limit recursion within resolver. The default value it "1". ## Fixed - [2320](#2320): Prevent `/health` and `/v1/health` from being throttled by the concurrency limiter. - [2322](#2322): Set the salt of genesis contracts to zero on execution. - [2324](#2324): Ignore peer if we already are syncing transactions from it. #### Breaking - [2320](#2330): Reject queries that are recursive during the resolution of the query. ### Changed #### Breaking - [2311](#2311): Changed the text of the error returned by the executor if gas overflows. ## What's Changed * chore(executor): instrument errors to be more meaningful by @rymnc in #2311 * fix(dummy_da_block_costs): remove dependency on polling_interval and use channel instead by @rymnc in #2314 * fix(txpool): Error in GossipsubMessageAcceptance variant docstrings by @netrome in #2316 * refactor: Eager returns in txpool_v2::service::Task::run by @netrome in #2325 * fix(api_service): exclude health checks from throttling with ConcurrencyLimitLayer by @rymnc in #2320 * Remove the `normalize_rewards_and_costs()` function by @rafal-ch in #2293 * fix(genesis): set salt of contract on execution of genesis state configuration by @rymnc in #2322 * Fixing the issue with duplicate transaction synchronization processes by @xgreenx in #2324 * Reject queries that are recursive during the resolution by @xgreenx in #2330 **Full Changelog**: v0.38.0...v0.39.0
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.
Linked Issues/PRs
closes #2319
Description
Uses axum's
mergechaining operator to add theConcurrencyLimit Layeronly on subsets of the routes we serve, and exclude/healthfrom it.We retain
/metricsto be throttled, because it acquires a lock on theGLOBAL_REGISTER, which may prevent Prometheus from reading metrics. This is a double-edged sword though i think.Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]