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.

Sign up for GitHub

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

Jump to bottom

/health endpoint maybe be queued causing false positive restart for operators #2319

Closed
xgreenx opened this issue Oct 8, 2024 · 1 comment · Fixed by #2320
Closed

/health endpoint maybe be queued causing false positive restart for operators #2319

xgreenx opened this issue Oct 8, 2024 · 1 comment · Fixed by #2320
Assignees
@rymnc

Comments

@xgreenx
Copy link
Collaborator

xgreenx commented Oct 8, 2024

The /health endpoint now is also limited by concurrency limit after #2309

image

If the node has a lot of queries it can be terminated by failing liveness check for node operators.
@Voxelot
Copy link
Member

Voxelot commented Oct 8, 2024

We should only apply the concurrency limit on the graphql schema route if possible

@rymnc rymnc self-assigned this Oct 8, 2024
@rymnc rymnc mentioned this issue Oct 8, 2024
Merged
9 tasks
@rymnc rymnc linked a pull request Oct 8, 2024 that will close this issue
fix(api_service): exclude health checks from throttling with ConcurrencyLimitLayer #2320
Merged
9 tasks
rymnc added a commit that referenced this issue Oct 10, 2024
@rymnc @netrome @rafal-ch
fix(api_service): exclude health checks from throttling with Concurre…
f87f59a 
…ncyLimitLayer (#2320)

## Linked Issues/PRs
<!-- List of related issues/PRs -->
closes #2319

## Description
<!-- List of detailed changes -->
Uses axum's `merge` chaining operator to add the `ConcurrencyLimit
Layer` only on subsets of the routes we serve, and exclude `/health`
from it.

We retain `/metrics` to be throttled, because it acquires a lock on the
`GLOBAL_REGISTER`, which may prevent Prometheus from reading metrics.
This is a double-edged sword though i think.

## Checklist
- [x] Breaking changes are clearly marked as such in the PR description
and changelog
- [x] New behavior is reflected in tests
- [x] [The specification](https://github.com/FuelLabs/fuel-specs/)
matches the implemented behavior (link update PR if changes are needed)

### Before requesting review
- [x] I have reviewed the code myself
- [ ] I have created follow-up issues caused by this PR and linked them
here

### After merging, notify other teams

[Add or remove entries as needed]

- [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/)
- [ ] [Sway compiler](https://github.com/FuelLabs/sway/)
- [ ] [Platform
documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+)
(for out-of-organization contributors, the person merging the PR will do
this)
- [ ] Someone else?

---------

Co-authored-by: Mårten Blankfors <[email protected]>
Co-authored-by: Rafał Chabowski <[email protected]>
@netrome netrome closed this as completed Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rymnc rymnc

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@Voxelot @xgreenx @netrome @rymnc