-
Notifications
You must be signed in to change notification settings - Fork 680
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.
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
rpc server: add health/readiness endpoint
#4802
Conversation
prdoc/pr_4802.prdoc
Outdated
doc: | ||
- audience: Node Operator | ||
description: | | ||
Add health readiness endpoint to the rpc server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to mention the path to poll this?
"/health" if req.method() == http::Method::GET => InterceptRequest::Health, | ||
"/health/readiness" if req.method() == http::Method::GET => InterceptRequest::Readiness, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe could return an error if method()
is not Get
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, done :)
The CI pipeline was cancelled due to failure one of the required jobs. |
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Sorry I fucked up a merge commit and I had to rebase |
health/readiness endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Previous attempt paritytech/substrate#14314 Close paritytech#4443 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: ```bash ➜ polkadot-sdk (na-fix-4443) ✗ curl -v localhost:9944/health * Host localhost:9944 was resolved. * IPv6: ::1 * IPv4: 127.0.0.1 * Trying [::1]:9944... * connect to ::1 port 9944 from ::1 port 55024 failed: Connection refused * Trying 127.0.0.1:9944... * Connected to localhost (127.0.0.1) port 9944 > GET /health HTTP/1.1 > Host: localhost:9944 > User-Agent: curl/8.5.0 > Accept: */* > < HTTP/1.1 200 OK < content-type: application/json; charset=utf-8 < content-length: 53 < date: Fri, 14 Jun 2024 16:12:23 GMT < * Connection #0 to host localhost left intact {"peers":0,"isSyncing":false,"shouldHavePeers":false}% ➜ polkadot-sdk (na-fix-4443) ✗ curl -v localhost:9944/health/readiness * Host localhost:9944 was resolved. * IPv6: ::1 * IPv4: 127.0.0.1 * Trying [::1]:9944... * connect to ::1 port 9944 from ::1 port 54328 failed: Connection refused * Trying 127.0.0.1:9944... * Connected to localhost (127.0.0.1) port 9944 > GET /health/readiness HTTP/1.1 > Host: localhost:9944 > User-Agent: curl/8.5.0 > Accept: */* > < HTTP/1.1 500 Internal Server Error < content-type: application/json; charset=utf-8 < content-length: 0 < date: Fri, 14 Jun 2024 16:12:36 GMT < * Connection #0 to host localhost left intact ``` //cc @BulatSaif you may be interested in this.. --------- Co-authored-by: Bastian Köcher <[email protected]>
* change health checkendpoint dockerfiles elated to paritytech/polkadot-sdk#4802 * Add http:// prefix --------- Co-authored-by: Nazar Mokrynskyi <[email protected]>
Previous attempt paritytech/substrate#14314
Close #4443
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:
//cc @BulatSaif you may be interested in this..