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

Added EIP-2159 metrics #4887

Merged
merged 7 commits into from
Nov 16, 2022
Merged

Added EIP-2159 metrics #4887

merged 7 commits into from
Nov 16, 2022

Conversation

OlegJakushkin
Copy link
Contributor

following https://eips.ethereum.org/EIPS/eip-2159

Resolves #4842

Changes:

  • added 4 new metrics that follow eip-2159 (ethereum_blockchain_height ethereum_best_known_block_number ethereum_peer_count ethereum_peer_limit)
  • Added an attribute for developer defined metric name

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

  • [] Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

We need to see how ethereum_best_known_block_number changes over time, will ask the Angkor team for that.

@OlegJakushkin
Copy link
Contributor Author

OlegJakushkin commented Nov 11, 2022

Usage demo

Height:
image

Best known:
image

Compare block numbers:
image

peer_count vs limit:
image

@OlegJakushkin OlegJakushkin marked this pull request as ready for review November 11, 2022 14:25
@OlegJakushkin
Copy link
Contributor Author

@LukaszRozmej: please review,
Also wanted to ask: in many places currently we update metrics not checking if Metrics are Enabled. Shall we update it everywhere to only update if the flag Metrics Enabled is set to true?

@LukaszRozmej
Copy link
Member

@LukaszRozmej: please review, Also wanted to ask: in many places currently we update metrics not checking if Metrics are Enabled. Shall we update it everywhere to only update if the flag Metrics Enabled is set to true?

Would it be worth conditional jumps? Probably not.

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Fine overall, minor tweaks requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement EIP-2159 Common Prometheus Metrics Names for Clients
2 participants