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

Chain validation metric #363

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

rus-alex
Copy link
Contributor

@rus-alex rus-alex commented Sep 6, 2022

Here is the set of geth compatible metrics:

  • there are no uncle blocks in Opera, so "chain/reorg/*" are always zero;
  • "chain/validation" shows event validation time;

( it is an adaptation of #290 after #358 )

// aBFT processing
return s.engine.Process(e)
err = s.engine.Process(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why we need to do this refactoring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need it )

Copy link
Contributor

@hadv hadv left a comment

Choose a reason for hiding this comment

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

LGTM

_ = metrics.GetOrRegisterMeter("chain/reorg/executes", nil)
_ = metrics.GetOrRegisterMeter("chain/reorg/add", nil)
_ = metrics.GetOrRegisterMeter("chain/reorg/drop", nil)
_ = metrics.GetOrRegisterMeter("chain/reorg/invalidTx", nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

@rus-alex what is the point in adding metrics, which will be unused?

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 point is to show the same set of metrics like geth. As I remember it was a one of #290 goals.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we should add as much metrics from geth as we can, however if we cannot fill the metric with any data and we create it unused, it seems useless to me - is there some usecase which needs them to be present, even through they will have no meaningful value?

Copy link
Contributor Author

@rus-alex rus-alex Sep 13, 2022

Choose a reason for hiding this comment

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

It is just method to say: we didn't forget implemented such metrics and they better than geth's.
Agreed, it is overmach.

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.

None yet

3 participants