chore: log enhancement for message reliability analysis #2640

Ivansete-status · 2024-04-28T16:38:58Z

Description

The next modules are touched:

waku_node.nim
archive.nim
waku_filter_v2/protocol.nim
~~waku_lightpush/protocol.nim~~
waku_relay/protocol.nim

Issue

Add logging of hashes to all nodes - #2474

github-actions · 2024-04-28T16:43:39Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2640-rln-v1

Built from c1793ad

github-actions · 2024-04-28T16:43:39Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2640-rln-v2

Built from c1793ad

gabrielmer

Uff amazing! This will help a lot!
Thanks so much 😍

waku/waku_relay/protocol.nim

NagyZoltanPeter

LGTM thank you for it.

NagyZoltanPeter · 2024-04-29T08:09:56Z

waku/waku_filter_v2/protocol.nim

-    hash = messagePush.pubsubTopic.computeMessageHash(messagePush.wakuMessage).to0xHex()
+    target_peer_ids = peers.mapIt(shortLog(it)),
+    msg_hash =
+      messagePush.pubsubTopic.computeMessageHash(messagePush.wakuMessage).to0xHex()


It might be called twice per logging. There is a processing flow in chronicles that evaluates twice the message arguments. It is better as its in line 214 in this file.

NagyZoltanPeter · 2024-04-29T08:11:33Z

waku/waku_filter_v2/protocol.nim

@@ -228,16 +232,20 @@ proc handleMessage*(
    if not await wf.pushToPeers(subscribedPeers, messagePush).withTimeout(
      MessagePushTimeout
    ):
-      debug "timed out pushing message to peers",
+      info "timed out pushing message to peers",


Isnt it an error? Or do we just want to notice this?

Isnt it an error? Or do we just want to notice this?

ah yes good point! I will change it to error

SionoiS · 2024-04-29T13:02:12Z

I understand the request since Nwaku logs are unusable BUT this "fix" will make it worst, we need to improve the logs not accommodate specific readers.

I disagree with this PR. We need clear guidelines and a concrete plan to improve the logs. Lets discuss this please!

AlbertoSoutullo · 2024-04-30T12:44:29Z

waku/node/waku_node.nim

        pubsubTopic = pubsubTopic,
        contentTopic = message.contentTopic,
-        peer = peer.peerId
+        target_peer_id = peer.peerId,
+        msg_hash = pubsubTopic.computeMessageHash(message).to0xHex()
      return await node.wakuLightpushClient.publish(pubsubTopic, message, peer)

    if not node.wakuLightPush.isNil():
      debug "publishing message with self hosted lightpush",


Is this supposed to be debug?

Because in the "publishing message with lightpush" case, it is being logged at info level.

As per previous conversations we had in PM meeting, I will move back all info to debug

The next modules are touched: - waku_node.nim - archive.nim - waku_filter_v2/protocol.nim - waku_lightpush/protocol.nim - waku_relay/protocol.nim

Co-authored-by: gabrielmer <101006718+gabrielmer@users.noreply.github.com>

chaitanyaprem

Few comments

chaitanyaprem · 2024-04-30T14:27:03Z

waku/node/waku_node.nim

@@ -914,7 +914,7 @@ proc mountLightPush*(

      if publishedCount == 0:
        ## Agreed change expected to the lightpush protocol to better handle such case. https://github.com/waku-org/pm/issues/93
-        debug "Lightpush request has not been published to any peers"
+        info "Lightpush request has not been published to any peers"


Shouldn't we log the hash here? It would be hard to trace otherwise with many messages going in parallel.

Thanks I will submit a change with that

chaitanyaprem · 2024-04-30T14:29:22Z

waku/waku_archive/archive.nim


  let insertStartTime = getTime().toUnixFloat()

  (await self.driver.put(pubsubTopic, msg, msgDigest, msgHash, msgTimestamp)).isOkOr:
    waku_archive_errors.inc(labelValues = [insertFailure])
    debug "failed to insert message", err = error
+
+  info "message archived",


One suggestion i have is rather than making all these as info logs? Can we make it as part of compile flag or some config to log such messages for debugging/tracing? I remember Jakubs mentioning something similar being already there in nwaku which they enable in status fleets for tracing.

Ivansete-status · 2024-05-01T08:26:38Z

@NagyZoltanPeter - for any unknown reason I couldn't change any single line of waku_lightpush/protocol.nim because that caused a SIGSEGV crash on macos tests. Did you face a similar issue?

NagyZoltanPeter · 2024-05-02T02:55:47Z

@NagyZoltanPeter - for any unknown reason I couldn't change any single line of waku_lightpush/protocol.nim because that caused a SIGSEGV crash on macos tests. Did you face a similar issue?

Hm... issue I had with MacOs tests I found in my PR and fixed. Nothing particular. But what change do you mean here?

Ivansete-status · 2024-05-06T15:06:29Z

@NagyZoltanPeter - for any unknown reason I couldn't change any single line of waku_lightpush/protocol.nim because that caused a SIGSEGV crash on macos tests. Did you face a similar issue?

Hm... issue I had with MacOs tests I found in my PR and fixed. Nothing particular. But what change do you mean here?

Sorry for late response. In the following PR it can be seen how the test fails on MacOS due to the changes in lighpush/protocol.nim: #2655
In this case there are many changes but it fail even with few log lines :)

github-actions bot assigned Ivansete-status Apr 28, 2024

Ivansete-status marked this pull request as ready for review April 28, 2024 19:04

Ivansete-status requested review from NagyZoltanPeter, SionoiS and gabrielmer April 28, 2024 19:04

gabrielmer approved these changes Apr 29, 2024

View reviewed changes

waku/waku_relay/protocol.nim Outdated Show resolved Hide resolved

Ivansete-status requested a review from AlbertoSoutullo April 29, 2024 08:17

NagyZoltanPeter approved these changes Apr 29, 2024

View reviewed changes

Ivansete-status force-pushed the enhance-logs branch from f8339db to ef32371 Compare April 30, 2024 10:53

AlbertoSoutullo reviewed Apr 30, 2024

View reviewed changes

Ivansete-status and others added 5 commits April 30, 2024 15:11

log enhancement for message reliability analysis

e558f47

The next modules are touched: - waku_node.nim - archive.nim - waku_filter_v2/protocol.nim - waku_lightpush/protocol.nim - waku_relay/protocol.nim

waku_relay/protocol.nim: restore back upper case

642bc6f

Update waku/waku_relay/protocol.nim

45e4665

Co-authored-by: gabrielmer <101006718+gabrielmer@users.noreply.github.com>

waku_filter_v2/protocol.nim: apply Zoltán's optimization suggestions

aebe405

waku_filter_v2/protocol: log info -> error when timed out push

a29db36

chaitanyaprem reviewed Apr 30, 2024

View reviewed changes

Ivansete-status added 5 commits April 30, 2024 20:59

waku_node.nim: only calculate msgHash once in lightpushPublish

b86fc11

change info to debug the log lines that were previously set to info

ef72b2b

revert changes in waku_lightpush/protocol

e3ce42c

d

3d400a4

revert lightpush changes

679544d

Ivansete-status force-pushed the enhance-logs branch from ef32371 to 679544d Compare May 1, 2024 07:48

Ivansete-status merged commit d5e0e4a into master May 1, 2024
26 of 30 checks passed

Ivansete-status deleted the enhance-logs branch May 1, 2024 08:25

This was referenced May 1, 2024

Add logging of hashes to all nodes #2474

Closed

chore: enhance logs for status #2495

Closed

chair28980 mentioned this pull request Aug 20, 2024

[Epic: nwaku] Telemetry: fleet logging #3080

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: log enhancement for message reliability analysis #2640

chore: log enhancement for message reliability analysis #2640

Ivansete-status commented Apr 28, 2024 •

edited

Loading

github-actions bot commented Apr 28, 2024 •

edited

Loading

github-actions bot commented Apr 28, 2024 •

edited

Loading

gabrielmer left a comment

NagyZoltanPeter left a comment

NagyZoltanPeter Apr 29, 2024

NagyZoltanPeter Apr 29, 2024

Ivansete-status Apr 29, 2024

SionoiS commented Apr 29, 2024

AlbertoSoutullo Apr 30, 2024

Ivansete-status Apr 30, 2024

chaitanyaprem left a comment

chaitanyaprem Apr 30, 2024

Ivansete-status Apr 30, 2024

chaitanyaprem Apr 30, 2024

Ivansete-status commented May 1, 2024

NagyZoltanPeter commented May 2, 2024

Ivansete-status commented May 6, 2024

chore: log enhancement for message reliability analysis #2640

chore: log enhancement for message reliability analysis #2640

Conversation

Ivansete-status commented Apr 28, 2024 • edited Loading

Description

Issue

github-actions bot commented Apr 28, 2024 • edited Loading

github-actions bot commented Apr 28, 2024 • edited Loading

gabrielmer left a comment

Choose a reason for hiding this comment

NagyZoltanPeter 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

SionoiS commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaitanyaprem 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

Ivansete-status commented May 1, 2024

NagyZoltanPeter commented May 2, 2024

Ivansete-status commented May 6, 2024

Ivansete-status commented Apr 28, 2024 •

edited

Loading

github-actions bot commented Apr 28, 2024 •

edited

Loading

github-actions bot commented Apr 28, 2024 •

edited

Loading