Skip to content

[consensus] Fix BATCH_NUM_PER_BLOCK histogram buckets #19473

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

Merged
danielxiangzl merged 3 commits into from
Apr 21, 2026
Merged

[consensus] Fix BATCH_NUM_PER_BLOCK histogram buckets #19473

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc419c3
[consensus] Fix BATCH_NUM_PER_BLOCK histogram buckets
danielxiangzl Apr 16, 2026
15fbda3
[consensus] Fix buckets for proof/batch count histograms
danielxiangzl Apr 20, 2026
9c62399
Merge branch 'main' into daniel/fix-batch-num-buckets
danielxiangzl Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions consensus/src/quorum_store/counters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ pub static BATCH_NUM_PER_BLOCK: Lazy<HistogramVec> = Lazy::new(|| {
"quorum_store_batch_num_per_block",
"Histogram for the number of batches per (committed) blocks.",
&["type"],
TRANSACTION_COUNT_BUCKETS.clone(),
PROOF_COUNT_BUCKETS.clone(),
Copy link
Copy Markdown
Contributor

@cursor cursor Bot Apr 16, 2026

Choose a reason for hiding this comment

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

Bucket fix is correct. BATCH_NUM_PER_BLOCK measures proof/batch counts per block (observed values of proof_num, inline_batch_num, opt_batch_num — all small integers, typically < 500). PROOF_COUNT_BUCKETS (max 500) is the right scale; TRANSACTION_COUNT_BUCKETS (exponential to ~25 000+) was semantically mismatched.

Two adjacent metrics with the same scale mismatch remain: PROOF_SIZE_WHEN_PULL (line ~408) and NUM_BATCHES_WITHOUT_PROOF_OF_STORE (line ~380) — both measure proof/batch counts but still use TRANSACTION_COUNT_BUCKETS.

Copy link
Copy Markdown
Contributor

@cursor cursor Bot Apr 16, 2026

Choose a reason for hiding this comment

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

[INFORMATIONAL] The bucket swap from TRANSACTION_COUNT_BUCKETS (exponential, ~1.5 to ~25,251) to PROOF_COUNT_BUCKETS (hand-tuned, 1–500) is semantically correct: this metric counts batches/proofs per committed block, which in practice stays in the tens-to-low-hundreds range. The fix also makes this metric consistent with PROOFS_WITHOUT_BATCH_SUMMARY, PROOFS_WITH_BATCH_SUMMARY, and NUM_PROOFS_IN_PROOF_QUEUE_AFTER_PULL, which already use PROOF_COUNT_BUCKETS.

Note for follow-up: PROOF_SIZE_WHEN_PULL (line 407) and NUM_BATCHES_WITHOUT_PROOF_OF_STORE (line 378) are semantically equivalent (probe counts, not tx counts) but still use TRANSACTION_COUNT_BUCKETS. Aligning those in a separate PR would complete the consistency story.

)
.unwrap()
});
Expand Down Expand Up @@ -389,7 +389,7 @@ pub static NUM_BATCHES_WITHOUT_PROOF_OF_STORE: Lazy<Histogram> = Lazy::new(|| {
register_histogram!(
"num_batches_without_proof_of_store",
"Histogram for the number of batches without proof of store in proof manager",
TRANSACTION_COUNT_BUCKETS.clone(),
PROOF_COUNT_BUCKETS.clone(),
)
.unwrap()
});
Expand Down Expand Up @@ -418,7 +418,7 @@ pub static PROOF_SIZE_WHEN_PULL: Lazy<Histogram> = Lazy::new(|| {
register_histogram!(
"quorum_store_proof_size_when_pull",
"Histogram for the number of proof-of-store per block when pulled for consensus.",
TRANSACTION_COUNT_BUCKETS.clone(),
PROOF_COUNT_BUCKETS.clone(),
)
.unwrap()
});
Expand Down
Loading