Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

rpc server: add /health/readiness endpoint #14314

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Jun 7, 2023

Close #1017

Ideally, we should move /health and /health/readiness to the prometheus server but because it's was quite easy to implement on the RPC server and that RPC server already exposes /health.

Manual tests on a polkadot node syncing:

$ curl -v localhost:9944/health
*   Trying 127.0.0.1:9944...
* Connected to localhost (127.0.0.1) port 9944 (#0)
> GET /health HTTP/1.1
> Host: localhost:9944
> User-Agent: curl/8.1.2
> Accept: */*
>
< HTTP/1.1 200 OK
< content-type: application/json; charset=utf-8
< content-length: 52
< date: Wed, 07 Jun 2023 08:58:31 GMT
<
* Connection #0 to host localhost left intact
{"peers":15,"isSyncing":true,"shouldHavePeers":true}%

$ curl -v localhost:9944/health/readiness
*   Trying 127.0.0.1:9944...
* Connected to localhost (127.0.0.1) port 9944 (#0)
> GET /health/readiness HTTP/1.1
> Host: localhost:9944
> User-Agent: curl/8.1.2
> Accept: */*
>
< HTTP/1.1 200 OK
< content-type: application/json; charset=utf-8
< content-length: 0
< date: Wed, 07 Jun 2023 08:58:35 GMT
<
* Connection #0 to host localhost left intact

@niklasad1 niklasad1 added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. labels Jun 7, 2023
@niklasad1 niklasad1 changed the title add /health/readiness endpoint on the RPC server add /health/readiness endpoint to the RPC server Jun 7, 2023
@niklasad1 niklasad1 added the B1-note_worthy Changes should be noted in the release notes label Jun 7, 2023
@niklasad1 niklasad1 changed the title add /health/readiness endpoint to the RPC server rpc server: add /health/readiness endpoint Jun 7, 2023
@niklasad1 niklasad1 added the T0-node This PR/Issue is related to the topic “node”. label Jun 7, 2023
@niklasad1 niklasad1 requested review from lexnv and bkchr June 7, 2023 08:54
@stale
Copy link

stale bot commented Jul 9, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Jul 9, 2023
@bkchr
Copy link
Member

bkchr commented Jul 9, 2023

Wasn't there somewhere a discussion around this RPC? And that people should just prometheus to do this? If we want this pr, we should put it into the spec.

@xlc
Copy link
Contributor

xlc commented Jul 9, 2023

This is not a JSON RPC API and therefore don't belong to the new JSON RPC API spec.
I don't think we have another HTTP requests spec, which will only contain /health and this

@stale stale bot removed the A3-stale label Jul 9, 2023
client/rpc-servers/src/middleware.rs Outdated Show resolved Hide resolved
Comment on lines 374 to 377
/// Proxy `/health` to `system_health`.
Health,
/// Proxy `/health/readiness`.
Readiness,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe some docs on what they return and when.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3298764

@niklasad1
Copy link
Member Author

niklasad1 commented Jul 31, 2023

Yeah, I agree with @xlc that this isn't strictly related to the RPC API but it was so simple to add it to the JSON-RPC server as system_health already has the data which makes it trivial to implement.

Whether these health and readiness APIs should be on the RPC server or prometheus server is worth discussing though

EDIT: The new RPC spec will remove the system_health API, so maybe doing it on top of number of peers is the way forward..

@niklasad1
Copy link
Member Author

bot rebase

@paritytech-processbot
Copy link

Rebased

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

readiness and liveness endpoints for service health monitoring
5 participants