feat: topic health tracking #3212

gabrielmer · 2024-12-17T10:11:40Z

Description

Tracking topic health in the Peer Manager in the same way as in go-waku, and integrating the feature into libwaku

Changes

defining a topicHealthChangeHandler app callback and setting it in libwaku
adding a topic health change event in libwaku
computing topic health in the peer manager and defining a callback to run when a topic changes its health state
creating topic_health.nim file for topic health definitions

Issue

closes #3196

github-actions · 2024-12-17T10:16:48Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3212

Built from fe2a18c

gabrielmer · 2024-12-17T10:18:53Z

waku/node/peer_manager/peer_manager.nim

+  topicsHealth*: Table[string, TopicHealth]
+  onTopicHealthChange*: TopicHealthChangeHandler


Added these two parameters + wakuRelay for feature-parity with go-waku's peer manager

https://github.com/waku-org/go-waku/blob/78b522db5063c6bb178dc5c16c41e1c308c4d7c0/waku/v2/peermanager/peer_manager.go#L70-L91

relay -> wakuRelay
subRelayTopics -> topicsHealth
TopicHealthNotifCh -> onTopicHealthChange

I think onTopicHealthChange should also belong only to WakuRelay

gabrielmer · 2024-12-17T10:24:42Z

waku/node/peer_manager/peer_manager.nim

+  if futs.len() > 0:
+    proc waiter(): Future[void] {.async.} =
+      # slow path - we have to wait for the handlers to complete
+      try:
+        futs = await allFinished(futs)
+      except CancelledError:
+        # propagate cancellation
+        for fut in futs:
+          if not (fut.finished):
+            await fut.cancelAndWait()
+
+      # check for errors in futures
+      for fut in futs:
+        if fut.failed:
+          let err = fut.readError()
+          warn "Error in health change handler", description = err.msg
+
+    return waiter()


This part looks horrible honestly - copied it from nim-libp2p's handling of Relay callback events

https://github.com/vacp2p/nim-libp2p/blob/fd26f93b80cfeecd4e6f9c84eb7a15a3be26c1ee/libp2p/protocols/pubsub/pubsub.nim#L379-L408

It's in part necessary because the callbacks configured can be async

gabrielmer · 2024-12-17T10:25:50Z

waku/node/peer_manager/peer_manager.nim

@@ -914,6 +967,7 @@ proc relayConnectivityLoop*(pm: PeerManager) {.async.} =
      await pm.manageRelayPeers()
    else:
      await pm.connectToRelayPeers()
+    discard pm.updateTopicsHealth() # we don't want to await for the callbacks to finish


discarding because the completion of the app callbacks shouldn't stop the peer manager from establishing new connections

In fact, I think we can have a separate async loop but running within the topic_healt.nim module

gabrielmer · 2024-12-17T10:26:43Z

waku/node/peer_manager/topic_health.nim

+proc `$`*(t: TopicHealth): string =
+  result =
+    case t
+    of UNHEALTHY: "UnHealthy"
+    of MINIMALLY_HEALTHY: "MinimallyHealthy"
+    of SUFFICIENTLY_HEALTHY: "SufficientlyHealthy"


Using same strings as go-waku: https://github.com/waku-org/go-waku/blob/78b522db5063c6bb178dc5c16c41e1c308c4d7c0/waku/v2/peermanager/peer_manager.go#L38-L49

richard-ramos

Good work, sir!

Ivansete-status

Thanks for the PR!
I've added some comments that I hope you find useful. IMO, we can have a simpler solution without even touching PeerManager :)
Great work so far 🙌

library/libwaku.nim

Ivansete-status · 2024-12-23T10:19:13Z

waku/node/peer_manager/peer_manager.nim

+  topicsHealth*: Table[string, TopicHealth]
+  onTopicHealthChange*: TopicHealthChangeHandler


I think onTopicHealthChange should also belong only to WakuRelay

waku/node/peer_manager/peer_manager.nim

Ivansete-status · 2024-12-23T10:40:37Z

waku/node/peer_manager/peer_manager.nim

+      let fut = pm.onTopicHealthChange(topic, currentHealth)
+      if not fut.completed(): # Fast path for successful sync handlers
+        futs.add(fut)
+
+  if futs.len() > 0:
+    proc waiter(): Future[void] {.async.} =
+      # slow path - we have to wait for the handlers to complete
+      try:
+        futs = await allFinished(futs)
+      except CancelledError:
+        # propagate cancellation
+        for fut in futs:
+          if not (fut.finished):
+            await fut.cancelAndWait()
+
+      # check for errors in futures
+      for fut in futs:
+        if fut.failed:
+          let err = fut.readError()
+          warn "Error in health change handler", description = err.msg
+
+    return waiter()


Why don't just invoke the callbacks in this point? Sorry but I can't quite see the need of this part

Yeah I updated it, took it from the callback handling of nim-libp2p but I agree we can just await for the callbacks to be finished and not define this other waiter proc

Ivansete-status · 2024-12-23T10:41:57Z

waku/node/peer_manager/peer_manager.nim

@@ -914,6 +967,7 @@ proc relayConnectivityLoop*(pm: PeerManager) {.async.} =
      await pm.manageRelayPeers()
    else:
      await pm.connectToRelayPeers()
+    discard pm.updateTopicsHealth() # we don't want to await for the callbacks to finish


In fact, I think we can have a separate async loop but running within the topic_healt.nim module

waku/node/peer_manager/topic_health.nim

Ivansete-status

LGTM! Thanks for it! 🙌

waku/factory/waku.nim

waku/waku_relay/protocol.nim

github-actions bot assigned gabrielmer Dec 17, 2024

gabrielmer commented Dec 17, 2024

View reviewed changes

gabrielmer requested review from richard-ramos, chaitanyaprem, darshankabariya, NagyZoltanPeter and Ivansete-status December 17, 2024 10:35

gabrielmer marked this pull request as ready for review December 17, 2024 10:35

gabrielmer mentioned this pull request Dec 17, 2024

feat: integrate topic health change event waku-org/waku-go-bindings#10

Open

richard-ramos approved these changes Dec 19, 2024

View reviewed changes

Ivansete-status reviewed Dec 23, 2024

View reviewed changes

gabrielmer requested a review from Ivansete-status December 23, 2024 16:31

Ivansete-status approved these changes Dec 23, 2024

View reviewed changes

waku/factory/waku.nim Outdated Show resolved Hide resolved

waku/waku_relay/protocol.nim Show resolved Hide resolved

gabrielmer added 13 commits December 23, 2024 19:34

initial definitions

558b04f

refactor and fixing compilation errors

c7a74de

updated version that compiles

5eed954

discarding future

3b32bf4

configuring onTopicHealthChange

55e089a

fixing bugs and adding debug logs

8b3de3f

don't return error if no peers in mesh

f96333c

removing debug logs

aed66f6

applying suggestions

d38a74d

moving topic health to relay module

766d3ad

fixing futures handling

ccb0c59

undo comment

23b05be

remove comment

640f6d3

remove unnecessary condition

55e4019

gabrielmer force-pushed the feat-topic-health-tracking branch from c9977e4 to 55e4019 Compare December 23, 2024 18:35

gabrielmer merged commit 6020a67 into master Dec 24, 2024
9 of 11 checks passed

gabrielmer deleted the feat-topic-health-tracking branch December 24, 2024 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: topic health tracking #3212

feat: topic health tracking #3212

gabrielmer commented Dec 17, 2024 •

edited

Loading

github-actions bot commented Dec 17, 2024 •

edited

Loading

gabrielmer Dec 17, 2024 •

edited

Loading

Ivansete-status Dec 23, 2024

gabrielmer Dec 17, 2024

gabrielmer Dec 17, 2024

Ivansete-status Dec 23, 2024

gabrielmer Dec 17, 2024

richard-ramos left a comment

Ivansete-status left a comment

Ivansete-status Dec 23, 2024

Ivansete-status Dec 23, 2024

gabrielmer Dec 23, 2024

Ivansete-status Dec 23, 2024

Ivansete-status left a comment

		topicsHealth*: Table[string, TopicHealth]
		onTopicHealthChange*: TopicHealthChangeHandler

feat: topic health tracking #3212

feat: topic health tracking #3212

Conversation

gabrielmer commented Dec 17, 2024 • edited Loading

Description

Changes

Issue

github-actions bot commented Dec 17, 2024 • edited Loading

gabrielmer Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richard-ramos left a comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

gabrielmer commented Dec 17, 2024 •

edited

Loading

github-actions bot commented Dec 17, 2024 •

edited

Loading

gabrielmer Dec 17, 2024 •

edited

Loading