[consensus] Update proposer metrics #19655
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
|
All of the changes to the metrics can be seen reflected on the left side of the graphs and the right side is "main". Let me know what you think.
|
consensus/core/src/core.rs
Outdated
| .add_blocks(accepted_blocks.iter().map(|b| b.reference()).collect()) | ||
| // Get max round of accepted blocks. This will be equal to the threshold | ||
| // clock round, either by advancing the threshold clock round by being | ||
| // greater than current clock round or by equaling the current clock round. |
There was a problem hiding this comment.
Is this the case? Blocks older than current threshold clock round can get accepted as well.
There was a problem hiding this comment.
I only added the case in the comment for greater and equal but blocks less than the clock round are essentially ignored by threshold clock
consensus/core/src/core.rs
Outdated
| self.context | ||
| .metrics | ||
| .node_metrics | ||
| .block_proposal_leader_wait_count |
There was a problem hiding this comment.
I think we should use a separate metric for counting the number of times leader is not found. block_proposal_leader_wait_count is tied to block_proposal_leader_wait_ms, so when the average wait is ~250ms, we know the leader is missing.
There was a problem hiding this comment.
I think the confusion for me with these metrics is that it doesn't just include leader wait time, it includes the quorum receive wait time which can make this metric a little misleading. Separating them brings more clarity. Though I guess we could always subtract this metric from quorum receive latency.
There was a problem hiding this comment.
Looking at ThresholdClock::add_block(), quorum_ts is updated when a quorum forms. Then the block_proposal_leader_wait_ms is calculated from quorum_ts. This does not include quorum receive latency. Also, leader timeout task is signaled when quorum_ts is updated, so block_proposal_leader_wait_ms should line up with actual leader wait.
There was a problem hiding this comment.
What I was trying to measure is the specific moment when the leader block is received versus when the round can advance due to quorum received. The wait time is anytime after the quorum is received as that is the minimum needed to advance the round. The leader block can be received and make up the quorum needed to advance the clock. So to accurately get the wait time for a leader block we should get the min(leader_received_ts - quorum_ts, 0).
What we currently have in theory should lead to the same results but after we add the block we go through the process of try_commit which then adds to the wait time calculation for the leader. I think this is why we see the difference in the graphs. (pasting here again)
[left is the new metric and right is main]
There was a problem hiding this comment.
Is the average is calculated with block_proposal_leader_wait_ms / block_proposal_leader_wait_count, can you compare the block_proposal_leader_wait_count rate as well? Originally the intention is to have the count similar to # of blocks proposed. With the new logic, I think it is double counting: if a block proposal has to wait for 250ms, and try_new_block() gets called after waiting for 50ms, 100ms, 150ms and 200ms, then the average leader wait seems to be skewed higher.
There was a problem hiding this comment.
Yeah you are correct using leader count was incorrect in the new metric but comparing it looks like the wait is indeed lower still
consensus/core/src/core.rs
Outdated
| self.context | ||
| .metrics | ||
| .node_metrics | ||
| .block_proposal_leader_wait_count |
There was a problem hiding this comment.
Looking at ThresholdClock::add_block(), quorum_ts is updated when a quorum forms. Then the block_proposal_leader_wait_ms is calculated from quorum_ts. This does not include quorum receive latency. Also, leader timeout task is signaled when quorum_ts is updated, so block_proposal_leader_wait_ms should line up with actual leader wait.
| &["authority"], | ||
| registry, | ||
| ).unwrap(), | ||
| block_proposal_interval: register_histogram_with_registry!( |
| Ordering::Greater => { | ||
| self.aggregator.clear(); | ||
| self.aggregator.add(block.author, &self.context.committee); | ||
| if proposal_leaders_exist { |
There was a problem hiding this comment.
I feel if we want to monitor the time between round N having quorum and round N leader exists, doing it in core is more natural, and it is almost equivalent to how block_proposal_leader_wait_ms is computed.
There was a problem hiding this comment.
I agree on that. Thresholdclock starts now having some responsibility which doesn't seem that coherent/related and probably Core might be a better place.
There was a problem hiding this comment.
Reverted the changes to threshold clock and block_proposal_leader_wait_ms. Will keep what we have for now
arun-koshy
force-pushed
the
ak/proposer-metrics
branch
from
9ce12a0 to
3752116
Compare
3 participants
Description
Add metric for the interval between propsals.
Test plan
private-testnet
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.