paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

rpc server: add `/health/readiness` endpoint #14314

Open niklasad1 opened 1 year ago

niklasad1 commented 1 year ago

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
stale[bot] commented 1 year ago

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.

bkchr commented 1 year ago

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 commented 1 year ago

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

paritytech-cicd-pr commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

bot rebase

paritytech-processbot[bot] commented 1 year ago

Rebased