-
Notifications
You must be signed in to change notification settings - Fork 682
Backport vote extension metrics #471
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
Backport vote extension metrics #471
Conversation
…ta (#8480) This pull request adds an additional set of metrics targeted at providing more visibility into `abci++`. The following set of metrics are added and exposed through the `metrics` endpoint: ``` tendermint_consensus_proposal_receive_count{chain_id="test-chain-IrF74Y",status="accepted"} 34 tendermint_consensus_proposal_create_count{chain_id="test-chain-IrF74Y"} 34 tendermint_consensus_vote_extension_receive_count{chain_id="test-chain-IrF74Y",status="accepted"} 34 tendermint_consensus_round_voting_power_percent{chain_id="test-chain-IrF74Y",vote_type="precommit"} 1 tendermint_consensus_round_voting_power_percent{chain_id="test-chain-IrF74Y",vote_type="prevote"} 1 tendermint_state_consensus_param_updates{chain_id="test-chain-IrF74Y"} 0 tendermint_state_validator_set_updates{chain_id="test-chain-IrF74Y"} 0 tendermint_consensus_late_votes{chain_id="test-chain-IrF74Y",vote_type="precommit"} 16 ``` This pull request also updates the `metrics.md` file to include some metrics that were previously missed. My hope is to generate the `metrics.md` file with a future version of the tool being architected in #8479
@@ -44,6 +44,7 @@ The following metrics are available: | |||
| consensus\_block\_gossip\_parts\_received | Counter | matches\_current | Number of block parts received by the node | | |||
| consensus\_quorum\_prevote\_delay | Gauge | | Interval in seconds between the proposal timestamp and the timestamp of the earliest prevote that achieved a quorum | | |||
| consensus\_full\_prevote\_delay | Gauge | | Interval in seconds between the proposal timestamp and the timestamp of the latest prevote in a round where all validators voted | | |||
| consensus\_vote\_extension\_receive\_count | Counter | status | Number of vote extensions received | |
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.
This is just a backport so it's fine but to mee somehow this metric is showing two things (number and status) and I am not sure whether one could extract stats on the number of rejects or accepts. Maybe it is not even relevant, but was a bit counterintuitive at first.
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.
My interpretation is that you want to count both, accepted and rejected, as two "sub-metrics" (that's my understanding of MarkVoteExtensionReceived
)
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.
You should definitely count both towards the number of VE seen. I was just wondering whether someone could further count the number within each sub-metric. But not really a major issue, just discussing.
|
||
pct := cmtproto.PrecommitType | ||
pcn := strings.ToLower(strings.TrimPrefix(pct.String(), "SIGNED_MSG_TYPE_")) | ||
m.RoundVotingPowerPercent.With("vote_type", pcn).Set(0) |
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.
How are these changes related to vote extensions and why is the count here set to 0 - because we start a new round?
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.
The original patch (the one I'm cherry-picking from) was not limited to vote extensions. This hunk is a left-over that hadn't been backported from the original patch.
This is the typical example why we're following this process (i.e., cherry-picking patches that are supposed to have already been backported manually)
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.
Actually, your comment makes me think that this might be missing for v0.37.x
. I'll take a look and submit a PR if that's the case
Contributes to #10
Partial cherry-pick of #8480
The rest of #8480 was backported for
v0.37.x
.PR checklist
.changelog
(we use unclog to manage our changelog)docs/
orspec/
) and code comments