-
Notifications
You must be signed in to change notification settings - Fork 57
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
chore: adding observers for message logging #2800
Conversation
You can find the image built from this PR at
Built from e29af21 |
You can find the image built from this PR at
Built from e29af21 |
dbe4134
to
8f65319
Compare
8f65319
to
399758c
Compare
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.
LGTM! Thanks! A very clean PR 🥳
I just added some comments that I hope you find useful.
On the other hand, we might need to review the following snippet because I have the feeling that we will duplicate some logs when a message is rx.
I think we could potentially move that logic to the new one, when a message is received.
Lines 227 to 238 in 15d578a
proc traceHandler(topic: PubsubTopic, msg: WakuMessage) {.async, gcsafe.} = | |
notice "waku.relay received", | |
my_peer_id = node.peerId, | |
pubsubTopic = topic, | |
msg_hash = topic.computeMessageHash(msg).to0xHex(), | |
receivedTime = getNowInNanosecondTime(), | |
payloadSizeBytes = msg.payload.len | |
let msgSizeKB = msg.payload.len / 1000 | |
waku_node_messages.inc(labelValues = ["relay"]) | |
waku_histogram_message_size.observe(msgSizeKB) |
Allow me to jump here and give some personal point of view c: I did some analysis on
I think a mix between something like the previous line and what you did would be perfect. Because the only thing that I was missing from the previous line was something like On the other hand, I don't know your motivation regarding logging So if you are concerned about the amount of logging, and if you don't think about any benefit of this, I would recommend just stick to |
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.
It is super cool!
About the compilation flag question. I'm on the side not to have such logs when out to production as it is not just overwhelming amount of logs but reveals way toooo much information about the network and messages.
I see your point. Nevertheless, I think for now it is interesting to have it in that way because it is much simpler than having a compilation flag and we are just disclosing a bit of information about the neighbors but doesn't bring a picture of the whole network. We can revisit that in the future if needed but for now is fine IMHO. |
I think I'll merge it as it is now, and if needed we can add the compilation flag anytime :) |
This reverts commit b522865.
Description
Adding observers so the hash and origin/destination of every message sent and received is logged. This will help with our reliability simulation efforts.
This adds lots of logs, so there's always the option to have it under a compilation flag. Let me know if that's preferable or if it's ok for now and we'll make them optional at a later stage.
Note
We're finally adding observers only for messages received, as no additional valuable information is currently added by logging sent messages. If necessary at any point, they can be easily activated.
Changes
nim-libp2p
observers for incoming and outgoing messages to log message dataExample output: