protocols/gossipsub: Add mesh metrics #2316
Conversation
There was a problem hiding this comment.
Sorry for the delay here @divagant-martian. I like the new approach of passing a registry at construction and keeping an Option<Metrics> at runtime.
protocols/gossipsub/src/metrics.rs
Outdated
|
|
||
| // Default value that limits how many topics for which there has been a previous or current | ||
| // subscription do we store metrics. | ||
| const DEFAULT_MAX_EXPLICITLY_SUBSCRIBED_TOPICS: usize = 300; |
There was a problem hiding this comment.
While a bit of a hack in my eyes, this seems like a practical solution to the problem of metric churn.
What is your experience with this constant limit? Do you ever hit it on your production nodes?
There was a problem hiding this comment.
For us the subscriptions that are really important occur at the beginning and are few, this is a very high upper limit. Still, that's why I left it as an application-configurable value
There was a problem hiding this comment.
We're currently bounded at about 250 topics (we can double our topic subscription around a fork boundary), most operating nodes would have around 125.
I guess we've set this value selfishly around our needs a little, but hopefully is sufficiently large for others.
| @@ -302,6 +304,9 @@ pub struct Gossipsub< | |||
| /// calculating the message-id and sending to the application. This is designed to allow the | |||
| /// user to implement arbitrary topic-based compression algorithms. | |||
| data_transform: D, | |||
|
|
|||
| /// Keep track of a set of internal metrics relating to gossipsub. | |||
| metrics: Option<Metrics>, | |||
|
Just solved conflicts with master, in case you can give this a review @mxinden Thx |
There was a problem hiding this comment.
@divagant-martian could you add a changelog entry in protocols/gossipsub/CHANGELOG.md and misc/metrics/CHANGELOG.md?
Other than that, this looks good to me and is ready to be merged.
|
I added the Changelog entries and bumped the version of the metrics toml. I also updated the style of this file to follow the other ones. Let me know if the last two steps were not needed / should be reverted. Thanks! |
Yes indeed :D |
Small part of #2235
I found it increasingly hard to reason about the correctness of the way we update each metrics with so many metrics at once, so this PR focus only on the mesh related metrics and topic_peer metrics. It also includes #2277