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

Correct the metrics for topic subscriptions #5344

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

AgeManning
Copy link
Member

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.

@AgeManning AgeManning added the ready-for-review The code is ready for review label Mar 4, 2024
Copy link
Member

@ackintosh ackintosh left a 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:

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");
}
}

@AgeManning
Copy link
Member Author

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:

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

@AgeManning AgeManning requested a review from jxs March 7, 2024 05:34
@AgeManning AgeManning added the v5.1.0 Q2 2024 label Mar 7, 2024
Copy link
Member

@ackintosh ackintosh left a 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. ✨

@pawanjay176
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Mar 7, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 85c3204

mergify bot added a commit that referenced this pull request Mar 7, 2024
mergify bot added a commit that referenced this pull request Mar 7, 2024
@mergify mergify bot merged commit 85c3204 into sigp:unstable Mar 7, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review The code is ready for review v5.1.0 Q2 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants