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.

Sign up for GitHub

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

Jump to bottom

Additional mempool metrics #2878

Merged
merged 13 commits into from
Oct 19, 2021
Merged

Additional mempool metrics #2878

merged 13 commits into from
Oct 19, 2021

Conversation

conradoplg
Copy link
Collaborator

Motivation

We need mempool metrics.

Specifications

Designs

Which metrics and their names were discussed in #2626

Solution

  • Add new metrics.
  • Rename old ones.
  • Expand grafana dashboard with new metrics.
  • Remove old "transaction verification" dashboard that was incorporated in the mempool one.

Closes #2626
Closes #2627 (please check. I think it does, since inbound = downloader, outbound = gossiped)

Review

Required for beta release. Anyone can review.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

@conradoplg conradoplg requested a review from a team October 14, 2021 18:54
@teor2345
Copy link
Contributor 

* Remove old "transaction verification" dashboard that was incorporated in the mempool one.

What about transactions that are verified as part of blocks?

@conradoplg
Copy link
Collaborator Author 

* Remove old "transaction verification" dashboard that was incorporated in the mempool one.

What about transactions that are verified as part of blocks?

We don't have metrics for those yet. That dashboard only showed mempool transactions

@teor2345 teor2345 added the P-Low label Oct 18, 2021
@oxarbitrage oxarbitrage self-requested a review October 18, 2021 19:30
oxarbitrage
oxarbitrage previously approved these changes Oct 18, 2021
View reviewed changes
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I think the dashboard looks great, the code is also ok for me. I am not very familiar with the PinnedDrop part and but i guess this is fine.

Great work.

teor2345
teor2345 reviewed Oct 19, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I have some optional suggestions about counter names.

The PinnedDrop looks fine, we're not doing anything unsafe or even risky in the drop impl.

zebrad/src/components/mempool/downloads.rs Show resolved Hide resolved
zebrad/src/components/mempool/downloads.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool/storage.rs Outdated Show resolved Hide resolved
@oxarbitrage oxarbitrage enabled auto-merge (squash) October 19, 2021 15:41
@oxarbitrage oxarbitrage merged commit a5d1467 into main Oct 19, 2021
@oxarbitrage oxarbitrage deleted the mempool-metrics-more branch October 19, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 left review comments

@oxarbitrage oxarbitrage oxarbitrage approved these changes

Assignees

@conradoplg conradoplg

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@conradoplg @teor2345 @oxarbitrage