Skip to content

Conversation

sergio-mena
Copy link
Contributor

Contributes to #10

Partial cherry-pick of #8480

The rest of #8480 was backported for v0.37.x.


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

williambanfield and others added 2 commits March 6, 2023 15:02
…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
@sergio-mena sergio-mena added the abci Application blockchain interface label Mar 6, 2023
@sergio-mena sergio-mena requested a review from a team as a code owner March 6, 2023 14:36
@sergio-mena sergio-mena self-assigned this Mar 6, 2023
@sergio-mena sergio-mena mentioned this pull request Mar 6, 2023
16 tasks
@@ -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 |
Copy link
Contributor

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.

Copy link
Contributor Author

@sergio-mena sergio-mena Mar 7, 2023

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)

Copy link
Contributor

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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)

Copy link
Contributor Author

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

@sergio-mena sergio-mena merged commit 4efe37c into feature/abci++vef Mar 7, 2023
@sergio-mena sergio-mena deleted the sergio/10-vote-extension-metrics branch March 7, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abci Application blockchain interface
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants