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

Add appropriate prefixes to the Edge metrics #1043

Merged

Conversation

Stefan-Ethernal
Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal commented Dec 13, 2022

Description

This PR accommodates prefixes to the existing Edge metrics in order to categorize and align them with the documentation: https://wiki.polygon.technology/docs/edge/configuration/prometheus-metrics/.

Existing metrics fall into 3 categories: consensus, network and tx pool.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Breaking changes

Datadog dashboards need to be changed in order to include renamed metrics.

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

@Stefan-Ethernal Stefan-Ethernal added the breaking change Functionality that contains breaking changes label Dec 13, 2022
@Stefan-Ethernal Stefan-Ethernal self-assigned this Dec 13, 2022
@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Merging #1043 (b6ee1bb) into develop (9cde1e3) will increase coverage by 0.00%.
The diff coverage is 75.00%.

@@           Coverage Diff            @@
##           develop    #1043   +/-   ##
========================================
  Coverage    53.88%   53.89%           
========================================
  Files          132      132           
  Lines        17564    17566    +2     
========================================
+ Hits          9465     9467    +2     
  Misses        7451     7451           
  Partials       648      648           
Impacted Files Coverage Δ
consensus/ibft/ibft.go 1.62% <0.00%> (ø)
network/server.go 75.00% <100.00%> (+0.10%) ⬆️
network/server_identity.go 94.11% <100.00%> (ø)
txpool/txpool.go 73.48% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ZeljkoBenovic ZeljkoBenovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

After we merge this PR, we'll need to update our docs as well as our own Prometheus scrapers.
What's changed:

  • txpool_pending_transactions => edge_txpool_pending_transactions
  • consensus_validators => edge_consensus_validators
  • consensus_rounds => does not exist anymore -- we should restore this metric asap
  • consensus_num_txs => edge_consensus_num_txs
  • consensus_block_interval => edge_consensus_block_interval
  • network_peers => edge_network_peers
  • network_outbound_connections_count => edge_network_outbound_connections_count
  • network_inbound_connections_count => edge_network_inbound_connections_count
  • network_pending_outbound_connections_count => edge_network_pending_outbound_connections_count
  • network_pending_inbound_connections_count => edge_network_pending_inbound_connections_count

@Stefan-Ethernal
Copy link
Collaborator Author

Stefan-Ethernal commented Dec 14, 2022

consensus_rounds => does not exist anymore -- we should restore this metric asap

Not sure where it has vanished, but good point. 🙂 Let me try find it in git history and re-introduce it.

EDIT:
I did some research and it turns out that consensus_rounds has been removed with introduction of IBFT 2.0 (#650, https://github.com/0xPolygon/polygon-edge/pull/650/files#diff-496bebdd7de5ae02eb3ea22e95ac9270069798437f3477475ee59fb60dca0492L503). Due to the fact that consensus engine has changed quite a lot, not sure how we can re-establish this metric back.

We can consider maybe doing something similar like we did for the polybft (although in the polybft it denotes how many rounds did it take to seal a block and is available only after block is finalized and inserted): https://github.com/0xPolygon/polygon-edge/pull/650/files#diff-496bebdd7de5ae02eb3ea22e95ac9270069798437f3477475ee59fb60dca0492L503.

@ZeljkoBenovic
Copy link
Contributor

With the introduction of go-ibft, consensus_rounds metric is "buried" in the go-ibft module itself and needs to be exposed in that package.

Restoring this metric should probably be a dedicated task...

@ZeljkoBenovic
Copy link
Contributor

Just to clarify.
We already had these prefixes in place, as we can clearly see in our public docs, but it looks like they vanished when edge prefix was added.
This is how these metrics look like currently (before this PR):

edge_block_interval
edge_inbound_connections_count
edge_num_txs
edge_outbound_connections_count
edge_peers
edge_pending_inbound_connections_count
edge_pending_outbound_connections_count
edge_pending_transactions
edge_validators

@Stefan-Ethernal Stefan-Ethernal merged commit 785ccd8 into develop Dec 16, 2022
@Stefan-Ethernal Stefan-Ethernal deleted the EVM-311-adapt-naming-of-edge-existing-metrics branch December 16, 2022 06:15
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change Functionality that contains breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants