[Networking] Adds metrics to GossipSub router #3501
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yhassanzadeh13
commented
Nov 3, 2022
| @@ -48,7 +48,7 @@ require ( | |||
| github.com/libp2p/go-libp2p v0.22.0 | |||
| github.com/libp2p/go-libp2p-kad-dht v0.18.0 | |||
| github.com/libp2p/go-libp2p-kbucket v0.4.7 | |||
| github.com/libp2p/go-libp2p-pubsub v0.8.1-0.20220908052023-8866ca88a105 | |||
| github.com/libp2p/go-libp2p-pubsub v0.8.2-0.20221102045350-1e161006c43f | |||
There was a problem hiding this comment.
We upgrade our libp2p-pubsub to enable the recent refactoring we upstreamed: libp2p/go-libp2p-pubsub#503
Codecov Report
@@ Coverage Diff @@
## master #3501 +/- ##
==========================================
- Coverage 55.27% 49.83% -5.45%
==========================================
Files 754 430 -324
Lines 69207 39553 -29654
==========================================
- Hits 38257 19711 -18546
+ Misses 27808 18060 -9748
+ Partials 3142 1782 -1360
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit a60e306 The command Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
yhassanzadeh13
requested review from
kc1116 and
peterargue
and removed request for
vishalchangrani
peterargue
approved these changes
Nov 18, 2022
There was a problem hiding this comment.
Looks good besides the comments about log level.
🚩 An important flag you noted is that the log message will not contain the peerID of the sender because it's not available in the router. That means operators will need to use other means to identify the source of these control messages.
| lg := o.logger.With().Str("peer", id.String()).Logger() | ||
| switch acceptStatus { | ||
| case pubsub.AcceptAll: | ||
| lg.Debug().Msg("accepting all messages from peer") |
There was a problem hiding this comment.
should these be Trace? If this method is called on all incoming messages, this could get spammy
| includedMessages := len(rpc.GetPublish()) | ||
|
|
||
| // TODO: add peer id of the sender to the log (currently unavailable in the RPC). | ||
| o.logger.Debug(). |
There was a problem hiding this comment.
this will result in one log event per gossip message received right? Probably should make it Trace level.
…bservability-to-gossipsub
yhassanzadeh13
deleted the
yahya/6422-adding-observability-to-gossipsub
branch
peterargue
pushed a commit
that referenced
this pull request
Nov 23, 2022
* [temporary] replaces libp2p gossipsub * make tidy * adds observable gossipsub router * implements gossipsub metrics * adds metrics * Revert "make tidy" This reverts commit c4906b3. * Revert "[temporary] replaces libp2p gossipsub" This reverts commit 60377e2. * tidy * upgrades pubsub version * small refactor * fmt * mocks * adds topic to constructor of metrics for gossipsub * regenerates mocks * wires in metrics * wip * adds with default tag tracer to pubsub router * adds metrics to libp2p node * adds documentation to gossipsub router * regenerates mocks * adds gossipsub received rpc count * refactors logging * adds metrics for tracking rpcs * adds accept rpc metrics * fixes build error * tidy * fixes noop collector * generates mocks * wires in accept from metrics * adds included publish messages * generates mocks * lint fix * make tidy * disables observable gossipsub * switches log level * switching log level
peterargue
pushed a commit
that referenced
this pull request
Dec 1, 2022
* [temporary] replaces libp2p gossipsub * make tidy * adds observable gossipsub router * implements gossipsub metrics * adds metrics * Revert "make tidy" This reverts commit c4906b3. * Revert "[temporary] replaces libp2p gossipsub" This reverts commit 60377e2. * tidy * upgrades pubsub version * small refactor * fmt * mocks * adds topic to constructor of metrics for gossipsub * regenerates mocks * wires in metrics * wip * adds with default tag tracer to pubsub router * adds metrics to libp2p node * adds documentation to gossipsub router * regenerates mocks * adds gossipsub received rpc count * refactors logging * adds metrics for tracking rpcs * adds accept rpc metrics * fixes build error * tidy * fixes noop collector * generates mocks * wires in accept from metrics * adds included publish messages * generates mocks * lint fix * make tidy * disables observable gossipsub * switches log level * switching log level
peterargue
pushed a commit
that referenced
this pull request
Dec 9, 2022
* [temporary] replaces libp2p gossipsub * make tidy * adds observable gossipsub router * implements gossipsub metrics * adds metrics * Revert "make tidy" This reverts commit c4906b3. * Revert "[temporary] replaces libp2p gossipsub" This reverts commit 60377e2. * tidy * upgrades pubsub version * small refactor * fmt * mocks * adds topic to constructor of metrics for gossipsub * regenerates mocks * wires in metrics * wip * adds with default tag tracer to pubsub router * adds metrics to libp2p node * adds documentation to gossipsub router * regenerates mocks * adds gossipsub received rpc count * refactors logging * adds metrics for tracking rpcs * adds accept rpc metrics * fixes build error * tidy * fixes noop collector * generates mocks * wires in accept from metrics * adds included publish messages * generates mocks * lint fix * make tidy * disables observable gossipsub * switches log level * switching log level
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements an
ObservableGossipSubRoutercomponent that wraps an actualGossipSubRouterand adds metrics and logging observability on it in terms of control message exchanges.