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

change(state): Add block channel metrics, in preparation for block fork metrics #5327

Merged
merged 5 commits into from
Oct 20, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Oct 4, 2022

Motivation

These metrics are part of #5297, but we also needed them to diagnose bugs in PR #5257.

This is a low-risk change that is not a release blocker.
But we might want to merge it to improve sync diagnostics.

Solution

  • Add minimum queued block height to metrics
  • Add metrics to sent block cache
  • Add metrics for the last sent finalized height
  • update the grafana dashboard

Review

This PR can merge by itself, it is not required to merge PR #5257.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added C-enhancement Category: This is an improvement P-Medium ⚡ A-diagnostics Area: Diagnosing issues or monitoring performance A-state Area: State / database changes labels Oct 4, 2022
@teor2345 teor2345 self-assigned this Oct 4, 2022
@github-actions github-actions bot added C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Oct 4, 2022
@teor2345 teor2345 requested review from a team, dconnolly and arya2 and removed request for a team, dconnolly and arya2 October 4, 2022 01:35
@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Merging #5327 (80b8818) into main (a28350e) will increase coverage by 0.04%.
The diff coverage is 75.67%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5327      +/-   ##
==========================================
+ Coverage   79.16%   79.21%   +0.04%     
==========================================
  Files         308      308              
  Lines       39752    39787      +35     
==========================================
+ Hits        31470    31517      +47     
+ Misses       8282     8270      -12     

@teor2345
Copy link
Contributor Author

This is a low-risk change that is not a release blocker.
We might want to merge it to improve sync diagnostics.

@teor2345 teor2345 force-pushed the non-finalized-block-commit-channel branch from acd61c5 to 74b29be Compare October 11, 2022 01:05
Base automatically changed from non-finalized-block-commit-channel to main October 11, 2022 19:25
@teor2345 teor2345 force-pushed the block-commit-metrics branch from c3d859d to f9ea36b Compare October 12, 2022 00:32
@teor2345 teor2345 marked this pull request as ready for review October 12, 2022 00:33
@teor2345 teor2345 requested a review from a team as a code owner October 12, 2022 00:33
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team October 12, 2022 00:33
@teor2345 teor2345 changed the title change(state): Add block channel metrics change(state): Add block channel metrics, preparation for block fork metrics Oct 12, 2022
@teor2345 teor2345 changed the title change(state): Add block channel metrics, preparation for block fork metrics change(state): Add block channel metrics, in preparation for block fork metrics Oct 12, 2022
@arya2
Copy link
Contributor

arya2 commented Oct 12, 2022

mempool_requests_for_transactions test failed again (bug #5384):

Message: internal error: entered unreachable code: MempoolTransactionIds requests should always respond Ok(Vec<UnminedTxId>), got Ok(Nil)

https://github.com/ZcashFoundation/zebra/actions/runs/3231180904/jobs/5290771906#step:3:6780

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.

LGTM

mergify bot added a commit that referenced this pull request Oct 20, 2022
mergify bot added a commit that referenced this pull request Oct 20, 2022
@mergify mergify bot merged commit f31609e into main Oct 20, 2022
@mergify mergify bot deleted the block-commit-metrics branch October 20, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Diagnosing issues or monitoring performance A-state Area: State / database changes C-enhancement Category: This is an improvement C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants