Skip to content

Conversation

@iurii-ssv
Copy link
Contributor

@iurii-ssv iurii-ssv commented Nov 26, 2025

This PR adds the missing logging for every stage of QBFT instance to complement the metrics we record (while metrics show a general picture of how things look, they don't help when debugging a specific QBFT instance misbehavior).

Also, round_change stage was added so we can track it as well (instead of "bundling" it with the rest of the stages).

@iurii-ssv iurii-ssv requested review from a team as code owners November 26, 2025 12:26
Comment on lines 26 to 27
if inst == nil {
i := instance.NewInstance(c.GetConfig(), c.CommitteeMember, c.Identifier, msg.QBFTMessage.Height, c.OperatorSigner)
i := instance.NewInstance(zap.NewNop(), c.GetConfig(), c.CommitteeMember, c.Identifier, msg.QBFTMessage.Height, c.OperatorSigner)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are going to get rid of "decided" messages anyway I'm just passing the zap.NewNop() here for simplicity.

Copy link
Contributor

Choose a reason for hiding this comment

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

@iurii-ssv I think it needs to be commented, because after merging it won't be clear why we decided to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the comment 👍

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 26, 2025

Greptile Overview

Greptile Summary

added comprehensive stage duration logging for QBFT instances to complement existing metrics, enabling better debugging of specific instance misbehavior. The changes introduce a new round_change stage to separately track round change durations, and refactor the metrics recording system to maintain internal stage state. Each stage transition (proposal → prepare → commit, or any stage → round_change) now logs its duration with round and stage information.

Key Changes:

  • Added logger field to Instance struct and passed through all constructors
  • Refactored metricsRecorder to track current stage internally instead of passing it as parameter
  • Added stageRoundChange and stageUndefined constants for better stage lifecycle management
  • Updated all stage transitions to explicitly call EndStage() followed by StartStage()
  • Added debug logging in EndStage() to log stage completion with duration

Technical Details:

  • Stage transitions are now explicit at every protocol phase boundary
  • The prevRound variable was introduced in round_change.go to correctly record metrics before bumping to new round
  • All tests updated to pass logger parameter to NewInstance()

Confidence Score: 5/5

  • This PR is safe to merge - changes are well-structured observability improvements with no functional behavior changes
  • The changes are purely additive for observability purposes, adding logging without modifying QBFT consensus logic. All stage transitions are correctly paired (EndStage + StartStage), and the refactoring improves code clarity by tracking stage state internally. Tests have been updated appropriately.
  • No files require special attention - all changes follow consistent patterns

Important Files Changed

File Analysis

Filename Score Overview
protocol/v2/qbft/instance/instance.go 5/5 added logger field to Instance struct, modified constructor to accept logger, refactored Start() to use instance logger
protocol/v2/qbft/instance/metrics.go 5/5 added stage tracking to metricsRecorder, added logging to EndStage(), changed EndStage signature to use internal stage state
protocol/v2/qbft/instance/observability.go 5/5 added stageRoundChange and stageUndefined stage constants to track round change as separate stage
protocol/v2/qbft/instance/proposal.go 5/5 updated to use new metrics API with explicit stage transitions, renamed variable for clarity
protocol/v2/qbft/instance/prepare.go 5/5 updated to use new metrics API with explicit stage transitions from prepare to commit
protocol/v2/qbft/instance/commit.go 5/5 updated EndStage call to use new API signature without explicit stage parameter
protocol/v2/qbft/instance/round_change.go 5/5 added prevRound tracking, added stage transitions for round changes (both justified and partial quorum paths)
protocol/v2/qbft/instance/timeout.go 5/5 added stage transition when timeout occurs, ending current stage and starting round_change stage

Sequence Diagram

sequenceDiagram
    participant C as Controller
    participant I as Instance
    participant M as MetricsRecorder
    participant T as Timer

    Note over C,T: Happy Path: Normal Consensus Flow
    C->>I: StartNewInstance(logger, height, value)
    I->>M: StartStage(stageProposal)
    I->>T: TimeoutForRound(height, FirstRound)
    
    alt Leader for round
        I->>I: CreateProposal()
        I->>I: Broadcast(proposal)
    end

    Note over I,M: Proposal Stage → Prepare Stage
    I->>I: uponProposal(msg)
    I->>M: EndStage(round)
    I->>M: StartStage(stagePrepare)
    I->>I: Broadcast(prepare)

    Note over I,M: Prepare Stage → Commit Stage
    I->>I: uponPrepare(msg)
    alt Prepare Quorum Reached
        I->>M: EndStage(round)
        I->>M: StartStage(stageCommit)
        I->>I: Broadcast(commit)
    end

    Note over I,M: Commit Stage → Decided
    I->>I: UponCommit(msg)
    alt Commit Quorum Reached
        I->>M: EndStage(round)
        Note over I: Instance Decided
    end

    Note over C,T: Timeout Path: Round Change Flow
    T->>I: UponRoundTimeout()
    I->>M: EndStage(round)
    I->>M: StartStage(stageRoundChange)
    I->>M: RecordRoundChange(round, reasonTimeout)
    I->>I: CreateRoundChange(newRound)
    I->>I: Broadcast(roundChange)

    Note over I,M: Round Change Paths
    I->>I: uponRoundChange(msg)
    
    alt Justified Round Change (Quorum)
        I->>M: EndStage(prevRound)
        I->>M: StartStage(stageProposal)
        I->>M: RecordRoundChange(prevRound, reasonJustified)
        I->>I: CreateProposal()
        I->>I: Broadcast(proposal)
    else Partial Quorum
        I->>M: EndStage(prevRound)
        I->>M: StartStage(stageRoundChange)
        I->>M: RecordRoundChange(prevRound, reasonPartialQuorum)
        I->>I: CreateRoundChange(newRound)
        I->>I: Broadcast(roundChange)
    end
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.2%. Comparing base (b24f7dc) to head (0d99985).
⚠️ Report is 9 commits behind head on stage.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@iurii-ssv iurii-ssv marked this pull request as draft November 26, 2025 15:12
@iurii-ssv iurii-ssv marked this pull request as ready for review November 27, 2025 14:49
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

15 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

y0sher
y0sher previously approved these changes Dec 3, 2025
nkryuchkov
nkryuchkov previously approved these changes Dec 3, 2025
@iurii-ssv iurii-ssv dismissed stale reviews from nkryuchkov and y0sher via 0d99985 December 3, 2025 15:52
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.

4 participants