feat: instruments proposer QBFT lifecycle - #1105
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #1105 +/- ##
============================================
+ Coverage 62.70% 63.09% +0.38%
============================================
Files 161 161
Lines 27000 27203 +203
============================================
+ Hits 16930 17163 +233
+ Misses 10070 10040 -30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ary-instrumentation
|
Claude finished @jnhsigmap's task in 4m 37s —— View job PR Review: feat: instruments proposer QBFT lifecycle
Part 1 — Review against #1066/#1067/#921 criteriaMapping to the four acceptance criteria stated in the PR body:
Things done well
Concerns (confidence-rated)1.
|
| Drift category | Recommendation |
|---|---|
| Span/event names and field names | Defer — internal consumers, low rename cost. Land #1104 after. |
Prom metric names (the anchor_proposer_qbft_* identifiers) |
Realign before merge — once these are scraped in any environment that anyone builds a dashboard against, renames become operator-visible breakage. The list is only 5 metrics; if their final names disagree with #919, fix the names in this PR. |
Prom label keys/values (e.g., outcome, reason, f_plus_1_rc) |
Realign before merge — same argument; label-name churn breaks queries. |
Internal Rust enum/variant names (ProposerOutcome, RoundAdvanceReason) |
Defer — no external surface. |
Concretely: if #1104 only catalogues span/field/enum drift, the deferral is sound and this PR can ship as-is. If #1104 names changes to the Prometheus metric or label identifiers introduced by #1066 and re-used here, those should land before this PR is scraped by any observability backend, otherwise reconciliation later is an operator-visible event. Worth a one-line "what is in scope for #1104" inventory on the issue so the freeze-point on metric names is unambiguous.
Approach for the realignment PR itself (when it lands):
- Treat metric renames as a deprecation cycle if any operator is already scraping: emit both old and new series for one release with the old marked deprecated in description, then drop. If no operators are scraping yet, hard-rename in a single PR is fine and far cleaner.
- Keep the realignment PR purely textual where possible (renames + doc tweaks + test-string updates), with zero behavioral change, so it can be reviewed and reverted as a unit.
Summary
Approve direction; consider addressing the six concerns above before merge. The implementation cleanly separates classification (pure) from emission (side-effecting), keeps the consensus crate read-only, and threads the handoff budget at the right layer. The remaining items are docstring/help-text accuracy, a low-severity sentinel concern, and a design question on the Option<u64> plumbing — none of which block merge on their own. The #1104 deferral is defensible for span/field/enum drift, but I'd push to realign Prometheus metric and label names in-flight if #1104 touches them, since those become operator-visible the moment scraping starts.
• branch: feat/1067/boundary-instrumentation
|
@claude, be1aefd addresses the following:
PROPOSER_QBFT_DECIDED_ROUND description is incomplete on the ChannelClosed outcome
*u64::try_from(get_instance_height()).unwrap_or(u64::MAX) saturates to a misleading sentinel
Additionally for test coverage
Items acknowledged without action: 5× None handoff-budget propagation is noisy
**OTLP span "built-in duration ≈ event moments, not lifetime" **
Buffered-replay window during initialize() is unobserved
Part 2 — Recommendation in #1104: taxonomy drift from #919
Review again given this angle, also provide a critique of my assessment here. |
|
Claude finished @jnhsigmap's task in 2m 0s —— View job Re-review of
|
| Item | Status | Evidence |
|---|---|---|
Doc comment for observe_state_transition clarifies dual-emit intent |
✅ | instrumentation.rs:195-199 — "Fires independently of round advances …" |
finish() gates decided-round + duration on outcome != ChannelClosed; outcome counter unaffected |
✅ | instrumentation.rs:236-247 — counter unconditional; histograms guarded |
Lossless as u64 cast replaces saturating try_from().unwrap_or(u64::MAX) |
✅ | instance.rs:335 |
compute_handoff_budget_ms() extracted with 4 unit tests |
✅ | validator_store/src/lib.rs:161-168, tests at :3408-3474 (0ms, 3000ms, 11999ms, pre-genesis→None) |
Notes:
PROPOSER_QBFT_DECIDED_ROUNDhelp text ("Round on which proposer QBFT decided or timed out") is now accurate — the histogram is no longer written onChannelClosed, so the help matches the population.finish()still writesdecided_roundandduration_msas span fields and into the completion log event forChannelClosed. That's per-instance context (log-side) and won't pollute Prometheus aggregates; semantically fine.- Pre-genesis test correctly exercises the
Option::Nonepath viaslot_clock.millis_from_current_slot_start()returningNone. The other three cover full-slot, mid-slot, and near-boundary. That's the right shape. - Correction on my earlier OTLP-span comment: you're right — a
tracing::Spanobject's exported duration is bounded by its construction/drop, not by.in_scopeentries.ProposerObserverholds theSpanuntilfinish()and drops with the observer at the end ofqbft_instance, so exported span duration ≈ instance lifetime. Withdraw that concern;duration_msas a field is redundant with span wall time, not compensating for it. Keeping it as a field is still fine (log-side consumers can read it without span-timing math).
One minor observation on the ChannelClosed gating (not blocking): finish() still takes decided_round as an argument in the RecvResult::Closed branch (instance.rs:442-445). The value is dead-for-metrics on that path but live-for-span-field. If you want to underline the semantic split, the observer could expose finish_terminal(outcome, decided_round) and finish_teardown() — but this is a design nit and the current shape is clear enough given the code comment.
Critique of your assessment on the #1104 deferral
Your framing is basically sound. Three sub-points where I'd sharpen it:
1. The "PR-body provisional note" is weaker than a HELP-string provisional note.
Operators consuming these metrics almost never read the PR body — they read # HELP lines exported alongside the metrics, which show up in Grafana metric descriptions, Prometheus UI, and any dashboard tooling. A PR-body note only reaches reviewers today; it won't reach the operator writing a dashboard next quarter. If you want the "provisional pending #1104" signal to survive contact with users, put it in the try_create_* help strings themselves, e.g.:
"Count of proposer QBFT round advances by reason (provisional; renaming tracked in #1104)"
That's a one-line change per affected metric, adds zero blast radius, and gets scraped into every downstream consumer for free. A PR-body-only note is invisible after the PR merges.
2. "It's going into unstable" is a weaker gate than it sounds.
unstable → stable promotion is a discrete event, not a continuous filter. If #1104 hasn't landed by the time the next release cuts, the provisional names ship to stable and become operator-visible. The unstable argument is only load-bearing if there is an explicit release-gate checklist that blocks promotion until #1104 is merged. If no such gate exists, "landing in unstable" is functionally equivalent to "landing", just with a lag. Worth stating explicitly: what mechanism prevents the provisional names from riding a release train past the point of no return?
3. The counter-split (round_advance_total{reason} → round_timeout_total{round}) is the highest-cost item to defer, not the lowest.
A counter rename with label re-parameterisation isn't just a name change — it changes the query semantics (label cardinality goes from ~4 reasons to ~12 rounds, aggregation formulas change, dashboards break, alert thresholds re-baseline). Splitting or reshaping a counter after operators have written queries against it is materially more expensive than renaming an unused span field. I agree that doing the split inside this PR would blur its objectives, but the deferral argument should acknowledge that this is the most expensive item to defer, not the least — and therefore deserves the strongest pre-merge signal (HELP-string note + explicit release-gate).
Where your argument holds up well:
- The claim that mixing transformational and additive work in one PR makes both harder to review and revert is correct and is exactly the reason this repo should keep them separate.
- The scope containment argument (touching feat(qbft_manager): add instrumentation taxonomy and metrics surface #1068 territory expands blast radius) is real and non-trivial — realignment PRs tend to sprawl because the taxonomy touches every consumer of it. Keeping the wiring PR small is the pragmatic call.
- Framing Re-align proposer QBFT instrumentation taxonomy with the original observability model #1104 as a separate follow-up with a linked issue is exactly what an audit trail wants to see, so the process is right even if the signal strength could be stronger.
Concrete recommendation:
- Ship this PR as-is on the four addressed items.
- Add one-line
"(provisional; tracked in #1104)"suffixes to the affected metric HELP strings before merge — cheap, durable, operator-visible. - Add an explicit release-cut checklist item on Re-align proposer QBFT instrumentation taxonomy with the original observability model #1104 (or wherever release notes are drafted): "resolve Re-align proposer QBFT instrumentation taxonomy with the original observability model #1104 before promoting
feat/1067metrics tostable" so the "going into unstable" argument has a mechanism backing it. - Defer everything else in Re-align proposer QBFT instrumentation taxonomy with the original observability model #1104 as you propose.
That gives you the smallest merge here, the strongest possible "these names will change" signal on the surface operators actually see, and a durable gate that prevents the provisional names from silently graduating.
Overall: the four addressed items are correctly implemented, the OTLP-duration point should be withdrawn on my side, and the taxonomy-deferral argument is right in shape but under-signals on operator-visible surfaces — a HELP-string note and a release-gate would close that gap without expanding this PR's blast radius.
• branch: feat/1067/boundary-instrumentation
|
fair enough, however, if #1104 is agreed upon then I disagree on the perceived severity of splitting the change between this PR and leaving renamed metrics and taxonomy changes to a future change. If this is direction is approved and agreed upon, it would be a priority to ensure #1104 follows promptly and that the general nature of Marking automated review as 'done' pending team consensus on actual direction and whether my beliefs outlined in #1104 are valid and worth pursuing or whether we remain on the current course with taxonomy shape 👍 |
|
regarding the
|
|
Tick the box to add this pull request to the merge queue (same as
|
|
thanks @shane-moore, I'm not really happy with it in its current form. I think it needs more thought before we go ahead. Once we're done sprinting on |
3 participants
Problem, Evidence, and Context (Required)
qbft_instanceloop.ssvlabs/ssv#2609(QBFT stage-duration logging); satisfies the four acceptance criteria of Instrument proposer QBFT lifecycle and round transitions #921.Note: I understand that #1066 drifted materially from the original agreed observability model in #919 and I think that we should explicitly aim to realign. This would obviously touch already-merged components and should sensibly land in a separate change that follows this one. See #1104 for detail.
Change Overview (Required)
tracingspan with lifecycle checkpoint events (start, proposal accepted, prepare quorum, round advance, decided/timed-out/channel-closed).qbft_manager::decide_instancefrom the duty slot for proposer instances only.instrumentation.rsmodule. Hot loop controls instrumentation semantically through methods offered by observer.Qbft::decided_round()and not the local round cursor. This is a better choice for cross-round decided commits.start_round/start_stateso round advances during buffered-message replay (if applicable) before the observer exists are at least traceable through recording the starting state.instrumentation.rsmodule (observer + taxonomy), thenhandoff_budgetcalculation inqbft_manager/src/lib.rsmethoddecide_instance, and thecommon/qbftdetermination ofdecided_round, then the instance loop call sites ininstance.rs.common/qbftconsensus logic (gains only a read-only state accessor from feat(qbft_manager): add instrumentation taxonomy and metrics surface #1068), timeout formulas, leader selection, and all non-proposer duties.Risks, Trade-offs, and Mitigations (Required)
qbft_manager(instance loop, instrumentation, decide_instance internals) and a read-only behavior-neutral addition tocommon/qbft(adecided_roundfield and getter.current_roundis never mutated).Validation (Required)
qbft_managertests and real-Qbftcorrespondence tests (from feat(qbft_manager): add instrumentation taxonomy and metrics surface #1068).decided_roundcoverage incommon/qbft(a discriminating certificate-round != cursor case and a commit-quorum case).compute_handoff_budget_mshelper.make cargo-fmt-check,make lint(-D warnings), andmake testall pass.Rollback (Required for behavior or runtime changes; optional otherwise)
Additional Info / Next Steps (Optional)
handoff_budgetcomputed inqbft_managerfrom the duty-slot boundary. IsNonewhen the slot clock cannot resolve start_of(duty_slot)/now.handoff_budget_mschanges in late-start scenarios