-
Notifications
You must be signed in to change notification settings - Fork 3
chore: add Health Status to Waku API #92
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -30,6 +30,8 @@ contributors: | |||||||||||||||||||||||||||||||||||
| * [Predefined values](#predefined-values) | ||||||||||||||||||||||||||||||||||||
| * [Extended definitions](#extended-definitions) | ||||||||||||||||||||||||||||||||||||
| * [The Validation API](#the-validation-api) | ||||||||||||||||||||||||||||||||||||
| * [Health Status](#health-status) | ||||||||||||||||||||||||||||||||||||
| * [Event Source](#event-source) | ||||||||||||||||||||||||||||||||||||
| * [Security/Privacy Considerations](#securityprivacy-considerations) | ||||||||||||||||||||||||||||||||||||
| * [Copyright](#copyright) | ||||||||||||||||||||||||||||||||||||
| <!-- TOC --> | ||||||||||||||||||||||||||||||||||||
|
|
@@ -313,6 +315,58 @@ that would contain all validation parameters including RLN. | |||||||||||||||||||||||||||||||||||
| In the time being, parameters specific to RLN are accepted for the message validation. | ||||||||||||||||||||||||||||||||||||
| RLN can also be disabled. | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| ## Health Status | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| #### Type definitions | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| ```yml | ||||||||||||||||||||||||||||||||||||
| types: | ||||||||||||||||||||||||||||||||||||
| HealthStatus: | ||||||||||||||||||||||||||||||||||||
| type: enum | ||||||||||||||||||||||||||||||||||||
| values: [Unhealthy, MinimallyHealthy, Healthy] | ||||||||||||||||||||||||||||||||||||
| description: "Used to identify health of the operating node" | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+324
to
+327
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think is very important/compulsory to allow precise detail about why a node is unhealthy, f.e. For now, it is enough returning this detail in a string so that the API consumer can better understand why the node is not getting healthy. That will also help us bring a better assistance (in case someone asks.) The |
||||||||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| #### Extended definitions | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| `Unhealthy` indicates that the node has lost connectivity for message reception, | ||||||||||||||||||||||||||||||||||||
| sending, or both, and as a result, it cannot reliably process or transmit messages. | ||||||||||||||||||||||||||||||||||||
weboko marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| `MinimallyHealthy` indicates that the node meets the minimum operational requirements, | ||||||||||||||||||||||||||||||||||||
weboko marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
| although performance or reliability may be impacted. | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| `Healthy` indicates that the node is operating optimally, | ||||||||||||||||||||||||||||||||||||
| with full support for message processing and transmission. | ||||||||||||||||||||||||||||||||||||
weboko marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| ## Event source | ||||||||||||||||||||||||||||||||||||
weboko marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| #### Type definitions | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| ```yaml | ||||||||||||||||||||||||||||||||||||
| types: | ||||||||||||||||||||||||||||||||||||
| HealthStatus: | ||||||||||||||||||||||||||||||||||||
| type: object | ||||||||||||||||||||||||||||||||||||
| fields: | ||||||||||||||||||||||||||||||||||||
| eventType: | ||||||||||||||||||||||||||||||||||||
| type: string | ||||||||||||||||||||||||||||||||||||
| default: "health" | ||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||||||||||||||||||
| description: "Event type identifier" | ||||||||||||||||||||||||||||||||||||
| status: | ||||||||||||||||||||||||||||||||||||
| type: HealthStatus | ||||||||||||||||||||||||||||||||||||
| description: "Node health status emitted on state change" | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| EventSource: | ||||||||||||||||||||||||||||||||||||
| type: object | ||||||||||||||||||||||||||||||||||||
| description: "Event source for Waku API events" | ||||||||||||||||||||||||||||||||||||
| fields: | ||||||||||||||||||||||||||||||||||||
| onEvent: | ||||||||||||||||||||||||||||||||||||
| type: function | ||||||||||||||||||||||||||||||||||||
| description: "Callback for captured events" | ||||||||||||||||||||||||||||||||||||
| parameters: | ||||||||||||||||||||||||||||||||||||
| - name: event | ||||||||||||||||||||||||||||||||||||
| type: HealthStatus | ||||||||||||||||||||||||||||||||||||
weboko marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| ## Security/Privacy Considerations | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| See [WAKU2-ADVERSARIAL-MODELS](https://github.com/waku-org/specs/blob/master/informational/adversarial-models.md). | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
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.
One of the things i have initially considered and while thinking again recently on this topic is to consider HealthStatus not just at node level but also at a more fine-grained level.
In case of relay health status makes more sense at shard level because a node can be healthily connected to 1 shard and not on the other.
When it comes to edge/light clients this becomes even more complicated as there are even finer grain concept of content-topics, i.e a node can have peers or subscriptions to certain content-topics and loose subscriptions to others.
If so, does it make more sense to consider health to be notified to a user based on content-topics? Ref discussion thread:
https://discord.com/channels/1110799176264056863/1414975481438011392/1428769536139857989
We can always aggregate the health of all shards or content-topics and depict it as node health, but i do think we will require granular health events to be reported to app devs. Consider status for example, if a user is part of 2 communities (which is nothing but 2 different content-topics) but has at some point only subscriptions to 1 content-topic then we can't indicate node is unhealthy rather reception on 1 community is unhealthy whereas on the other it is fine.
WDYT @fryorcraken @weboko @Ivansete-status
cc @igor-sirotin
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.
really good idea, @chaitanyaprem
I think this comes to the realm of Waku App API as now we are communicating about application concerns of fragmented network and it's application of content topics in an app
but we definitely can implement it in Waku API
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.
I agree that health per content topic may make sense.
But at this point in time, I would KISS it, and then review whether an application developer would want to expose this information to their user. Aka, not do per content topic just yet.
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.
iirc status app already has this requirement unless there is some priority change. hence was suggesting this.
But i would leave it upto based on priorities.
Uh oh!
There was an error while loading. Please reload this page.
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.
Please provide more context. What do they do with this information? How does the per content-topic information impacts application behaviour?
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.
I remember they wanted to show connectivity health at community level , but not sure if priority has changed now. Maybe @igor-sirotin or someone from status team can give more clarity on this
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 to know, If that's something they have/want to keep then yes, we should do it from the get go.
cc @osmaczko @jrainville to confirm if this feature still exists
cc @plopezlpz @jazzz FYI, there is a request to show connectivity at conversation level (can be done later in chat sdk)
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.
UI
I only know about this feature from this ticket, but I don't know about our desire to implement this on Status UI level.
It makes sense on paper, but in reality this might unnecessarily complicate UI. And I don't think this is of any priority at the moment.
cc @jrainville
Backend
In backend we manually trigger discovery (or peer exchange) when we don't have enough peers.
messaging/waku/gowaku.go#L1773-L1776
messaging/waku/gowaku.go#L605-L612
Instead, we want to:
cc @weboko @chaitanyaprem @fryorcraken