-
Notifications
You must be signed in to change notification settings - Fork 784
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
Correct the metrics for topic subscriptions #5344
Correct the metrics for topic subscriptions #5344
Conversation
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.
I think gossipsub.register_topics_for_metrics
should be called not only at the time of node startup but also when a fork occurs, to ensure the metrics remain accurate for a topic with the new fork digest. If I understand the code correctly, the appropriate place for this additional call would be around here:
lighthouse/beacon_node/network/src/service.rs
Lines 480 to 491 in ee6f667
Some(_) = &mut self.next_fork_subscriptions => { | |
if let Some((fork_name, _)) = self.beacon_chain.duration_to_next_fork() { | |
let fork_version = self.beacon_chain.spec.fork_version_for_name(fork_name); | |
let fork_digest = ChainSpec::compute_fork_digest(fork_version, self.beacon_chain.genesis_validators_root); | |
info!(self.log, "Subscribing to new fork topics"); | |
self.libp2p.subscribe_new_fork_topics(fork_name, fork_digest); | |
self.next_fork_subscriptions = Box::pin(None.into()); | |
} | |
else { | |
error!(self.log, "Fork subscription scheduled but no fork scheduled"); | |
} | |
} |
I did consider this. I originally wanted to avoid the complexity, but now that you raise it, we probably should handle this case too. I'll update this PR. Thanks @ackintosh |
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.
Thanks for the update. Looks good to me. ✨
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 85c3204 |
Some topic subscription metrics were not being registered as a safety mechanism to prevent us from recording metrics for unbounded number of topics.
The safety mechanism was too strict.
This PR increases the number of topics we store metrics for, and registers all the subnets and sync committees specifically to ensure these topics are always correctly accounted for.