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.

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

fix: add back waku discv5 metrics #2927

Merged
merged 3 commits into from
Jul 26, 2024
Merged

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Jul 23, 2024

I noticed that waku discv5 metrics were not used so I added them back.

@SionoiS SionoiS self-assigned this Jul 23, 2024
Copy link

github-actions bot commented Jul 23, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2927

Built from 2b0b7a3

@@ -211,6 +211,8 @@ proc findRandomPeers*(
elif wd.predicate.isSome():
discoveredRecords = discoveredRecords.filter(wd.predicate.get())

waku_discv5_discovered.inc(discoveredRecords.len)
Copy link
Contributor

Choose a reason for hiding this comment

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

The only concern that I have is if we receive the same peer twice, we will update this metric both times.

Another alternative is increasing it in addPeer() if the origin is PeerOrigin.Discv5 after passing the duplicate checks

Copy link
Contributor Author

@SionoiS SionoiS Jul 23, 2024

Choose a reason for hiding this comment

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

🤔 I thought we had another metric for that... I could add both.

This one say number of nodes discovered so maybe we don't filter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good :) Thank you!

@gabrielmer gabrielmer self-requested a review July 23, 2024 15:23
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much!

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for it 💯 !
I just added a minor suggestion

waku/discovery/waku_discv5.nim Outdated Show resolved Hide resolved
@SionoiS SionoiS merged commit e4e01fa into master Jul 26, 2024
10 of 11 checks passed
@SionoiS SionoiS deleted the chore--reenable-waku-discv5-metrics branch July 26, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants