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

feat: Rest endoint /health for rln #2011

Merged
merged 5 commits into from
Sep 8, 2023
Merged

feat: Rest endoint /health for rln #2011

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
beb3cf5
Rest endoint /health added
NagyZoltanPeter Sep 7, 2023
b95c998
Remove dev-debug echo
NagyZoltanPeter Sep 7, 2023
18a5a3f
Changed not ready message
NagyZoltanPeter Sep 7, 2023
72b4639
Update waku/node/rest/health/handlers.nim
NagyZoltanPeter Sep 7, 2023
e1f37c4
Various fixes on PR foundings, added comments
NagyZoltanPeter Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions apps/wakunode2/app.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import
../../waku/node/rest/relay/topic_cache,
../../waku/node/rest/filter/handlers as rest_filter_api,
../../waku/node/rest/store/handlers as rest_store_api,
../../waku/node/rest/health/handlers as rest_health_api,
../../waku/node/jsonrpc/admin/handlers as rpc_admin_api,
../../waku/node/jsonrpc/debug/handlers as rpc_debug_api,
../../waku/node/jsonrpc/filter/handlers as rpc_filter_api,
Expand Down Expand Up @@ -569,6 +570,9 @@ proc startRestServer(app: App, address: ValidIpAddress, port: Port, conf: WakuNo
## Debug REST API
installDebugApiHandlers(server.router, app.node)

## Health REST API
installHealthApiHandler(server.router, app.node)

## Relay REST API
if conf.relay:
let relayCache = TopicCache.init(capacity=conf.restRelayCacheCapacity)
Expand Down
1 change: 1 addition & 0 deletions tests/all_tests_waku.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,4 @@ when defined(rln):
./waku_rln_relay/test_wakunode_rln_relay,
./waku_rln_relay/test_rln_group_manager_onchain,
./waku_rln_relay/test_rln_group_manager_static
./wakunode_rest/test_rest_health
77 changes: 77 additions & 0 deletions tests/wakunode_rest/test_rest_health.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
{.used.}

import
std/tempfiles,
stew/shims/net,
testutils/unittests,
presto,
presto/client as presto_client,
libp2p/peerinfo,
libp2p/multiaddress,
libp2p/crypto/crypto
import
../../waku/waku_node,
../../waku/node/waku_node as waku_node2, # TODO: Remove after moving `git_version` to the app code.
../../waku/node/rest/server,
../../waku/node/rest/client,
../../waku/node/rest/responses,
../../waku/node/rest/health/handlers as health_api,
../../waku/node/rest/health/client as health_api_client,
../../waku/waku_rln_relay,
../testlib/common,
../testlib/wakucore,
../testlib/wakunode


proc testWakuNode(): WakuNode =
let
privkey = crypto.PrivateKey.random(Secp256k1, rng[]).tryGet()
bindIp = ValidIpAddress.init("0.0.0.0")
extIp = ValidIpAddress.init("127.0.0.1")
port = Port(58000)
NagyZoltanPeter marked this conversation as resolved.
Show resolved Hide resolved

newTestWakuNode(privkey, bindIp, port, some(extIp), some(port))


suite "Waku v2 REST API - health":
asyncTest "Get node health info - GET /health":
# Given
let node = testWakuNode()
await node.start()
await node.mountRelay()

let restPort = Port(58001)
let restAddress = ValidIpAddress.init("0.0.0.0")
let restServer = RestServerRef.init(restAddress, restPort).tryGet()

installHealthApiHandler(restServer.router, node)
restServer.start()
let client = newRestHttpClient(initTAddress(restAddress, restPort))

# When
var response = await client.healthCheck()

# Then
check:
response.status == 503
$response.contentType == $MIMETYPE_TEXT
response.data == "Node is not inititialized"

# now kick in rln (currently the only check for health)
await node.mountRlnRelay(WakuRlnConfig(rlnRelayDynamic: false,
rlnRelayCredIndex: some(1.uint),
rlnRelayTreePath: genTempPath("rln_tree", "wakunode"),
))

# When
response = await client.healthCheck()
alrevuelta marked this conversation as resolved.
Show resolved Hide resolved

# Then
check:
response.status == 200
$response.contentType == $MIMETYPE_TEXT
response.data == "Node is healthy"

await restServer.stop()
await restServer.closeWait()
await node.stop()
30 changes: 30 additions & 0 deletions waku/node/rest/health/client.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
when (NimMajor, NimMinor) < (1, 4):
alrevuelta marked this conversation as resolved.
Show resolved Hide resolved
{.push raises: [Defect].}
else:
{.push raises: [].}

import
chronicles,
json_serialization,
json_serialization/std/options,
presto/[route, client]
import
../serdes,
../responses

logScope:
topics = "waku node rest health_api"

proc decodeBytes*(t: typedesc[string], value: openArray[byte],
contentType: Opt[ContentTypeData]): RestResult[string] =
if MediaType.init($contentType) != MIMETYPE_TEXT:
error "Unsupported contentType value", contentType = contentType
return err("Unsupported contentType")

var res: string
if len(value) > 0:
res = newString(len(value))
copyMem(addr res[0], unsafeAddr value[0], len(value))
return ok(res)

proc healthCheck*(): RestResponse[string] {.rest, endpoint: "/health", meth: HttpMethod.MethodGet.}
37 changes: 37 additions & 0 deletions waku/node/rest/health/handlers.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
when (NimMajor, NimMinor) < (1, 4):
{.push raises: [Defect].}
else:
{.push raises: [].}

import
chronicles,
json_serialization,
presto/route
import
../../waku_node,
../responses,
../serdes

logScope:
topics = "waku node rest health_api"

const ROUTE_HEALTH* = "/health"
NagyZoltanPeter marked this conversation as resolved.
Show resolved Hide resolved

const futIsReadyTimout = 5.seconds
NagyZoltanPeter marked this conversation as resolved.
Show resolved Hide resolved

proc installHealthApiHandler*(router: var RestRouter, node: WakuNode) =

router.api(MethodGet, ROUTE_HEALTH) do () -> RestApiResponse:

let isReadyStateFut = node.isReady()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment to indicate what isReady() indicates? Does it mean the node has completed setup procedures and is listening for new connections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add as there is one little stuff to fix in test.

if not await isReadyStateFut.withTimeout(futIsReadyTimout):
return RestApiResponse.internalServerError("Health check timed out")

var msg = "Node is healthy"
var status = Http200

if not isReadyStateFut.read():
msg = "Node is not inititialized"
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps node is not ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you wish so?! :-) I will change!

status = Http503

return RestApiResponse.textResponse(msg, status)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this issue, obviously, but since we are talking about it - would it make more sense to always return json (even for errors)?

41 changes: 41 additions & 0 deletions waku/node/rest/health/openapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
openapi: 3.0.3
alrevuelta marked this conversation as resolved.
Show resolved Hide resolved
info:
title: Waku V2 node REST API
version: 1.0.0
contact:
name: VAC Team
url: https://forum.vac.dev/

tags:
- name: health
description: Healt check REST API for WakuV2 node

paths:
/health:
get:
summary: Get node health status
description: Retrieve readiness of a Waku v2 node.
operationId: healthcheck
tags:
- health
responses:
'200':
description: Waku v2 node is up and running.
content:
text/plain:
schema:
type: string
example: Node is healty
'500':
description: Internal server error
content:
text/plain:
schema:
type: string
'503':
description: Node not initialized or having issues
content:
text/plain:
schema:
type: string
example: Node is not initialized
1 change: 1 addition & 0 deletions waku/node/waku_node.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -910,4 +910,5 @@ proc isReady*(node: WakuNode): Future[bool] {.async.} =
return false
return await node.wakuRlnRelay.isReady()
## TODO: add other protocol `isReady` checks
echo "***NZP*** rln not defined!"
NagyZoltanPeter marked this conversation as resolved.
Show resolved Hide resolved
return true
Loading