feat: Rest endoint /health for rln #2011
Conversation
|
You can find the image built from this PR at |
|
You can find the experimental image built from this PR at |
waku/node/rest/health/handlers.nim
Outdated
| var status = Http200 | ||
|
|
||
| if not isReadyStateFut.read(): | ||
| msg = "Node is not inititialized" |
There was a problem hiding this comment.
perhaps node is not ready?
There was a problem hiding this comment.
If you wish so?! :-) I will change!
| msg = "Node is not inititialized" | ||
| status = Http503 | ||
|
|
||
| return RestApiResponse.textResponse(msg, status) |
There was a problem hiding this comment.
unsure about the best practices of health endpoints, but would it make sense to return a json that we can extend in the future? In case we want more granularity on the state of the services. For example, we may want to say that is not healthy, but provide a list on which services are not healthy.
No need to define it right now but allow for future extension. return json instead of textResponse?
There was a problem hiding this comment.
I can change it but in this case http status holds the information rather.
For meaningful message, text can be enough. Not sure what information we can structure into json later.
For checking status of different submodules or protocols sounds more like an admin interface than a health. Wdyt?
There was a problem hiding this comment.
yep indeed. http status code has the information.
i was suggesting to return a json in case we want to add more information in the future. so if for example its failing, you have information such as (example taken from prysm node): so you know which service is not healthy
/healthz
*rpc.Service: OK
*prometheus.Service: OK
*p2p.Server: OK
*powchain.Web3Service: OK
*attestation.Service: OK
*operations.Service: OK
*blockchain.ChainService: OK
*sync.Service: ERROR not initially synced
no need to do it now, but be prepared to extend it.
There was a problem hiding this comment.
I was gonna add the same comment - let's use JSON rather than text - it is extensible and easier to process with tools than plain text
There was a problem hiding this comment.
I'm ok with it, but will do in a separate PR is possible.
Also I think we need to enhance a bit client decoders (not just here) to accept json/text both due error cases returns text type, mixing them can cause exception. It's on my list already, but will do in all rest endpoints where necessary.
There was a problem hiding this comment.
Unrelated to this issue, obviously, but since we are talking about it - would it make more sense to always return json (even for errors)?
Co-authored-by: Ivan Folgueira Bande <128452529+Ivansete-status@users.noreply.github.com>
|
|
||
| router.api(MethodGet, ROUTE_HEALTH) do () -> RestApiResponse: | ||
|
|
||
| let isReadyStateFut = node.isReady() |
There was a problem hiding this comment.
Maybe a comment to indicate what isReady() indicates? Does it mean the node has completed setup procedures and is listening for new connections?
There was a problem hiding this comment.
I will add as there is one little stuff to fix in test.
Description
For RLN readiness /health rest endpoint is added.
It checks if wakunode is ready - currently it means rln is mounted correctly. returns proper http status upon.
Changes
How to test
or
Issue
feat(rest): Add /health endpoint to rest api