Skip to content

Conversation

@Kukoomomo
Copy link
Contributor

@Kukoomomo Kukoomomo commented Nov 27, 2025

Summary by CodeRabbit

  • New Features
    • Added metric to monitor pending finalize batches outside the challenge window, providing improved system observability.

✏️ Tip: You can customize this high-level summary in your review settings.

@Kukoomomo Kukoomomo requested a review from a team as a code owner November 27, 2025 10:12
@Kukoomomo Kukoomomo requested review from twcctop and removed request for a team November 27, 2025 10:12
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 27, 2025

Walkthrough

This PR introduces a new Prometheus gauge metric to track whether there are batches pending finalization. It adds the metric field and setter to the Metrics struct with full lifecycle management, then implements detection logic in the rollup service to identify when a finalize batch lies outside the challenge window and expose that state via the metric.

Changes

Cohort / File(s) Summary
Metrics Infrastructure
tx-submitter/metrics/metrics.go
Added HasPendingFinalizeBatch Prometheus gauge field to Metrics struct. Implemented SetHasPendingFinalizeBatch(hasPending bool) setter to update gauge value (1 or 0). Extended metric initialization and unregistration logic to handle the new gauge.
Rollup State Detection
tx-submitter/services/rollup.go
Added runtime logic in periodic status update loop to detect when a pending finalize batch exists outside the challenge window. Checks for unapplied committed batches and calls SetHasPendingFinalizeBatch accordingly, with error handling that logs warnings and leaves flag false on failures.

Sequence Diagram(s)

sequenceDiagram
    participant Rollup as Rollup Service
    participant Metrics as Metrics
    participant Prometheus as Prometheus Registry

    Note over Rollup: Periodic status update loop
    Rollup->>Rollup: Check for pending finalize batch
    Rollup->>Rollup: Verify batch outside challenge window
    alt Batch found and outside window
        Rollup->>Metrics: SetHasPendingFinalizeBatch(true)
        Metrics->>Prometheus: Update gauge to 1
    else No pending batch or inside window
        Rollup->>Metrics: SetHasPendingFinalizeBatch(false)
        Metrics->>Prometheus: Update gauge to 0
    else Error checking challenge window
        Note over Rollup: Log warning, leave as false
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Straightforward metric addition with consistent pattern (field + setter + init + cleanup)
  • Logic in rollup service is contained to one periodic check with clear error handling
  • All changes follow established patterns in the codebase

Possibly related PRs

Suggested reviewers

  • r3aker86
  • FletcherMan

Poem

🐰 A gauge so fine, now tracks the queue,
When batches linger in challenge's view,
Prometheus listens, metrics ring true,
Finalization pending—we're watching you! 📊

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a new metric (HasPendingFinalizeBatch) to track whether batches are pending finalization, which is the primary focus of both modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch submitter_metrics

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
tx-submitter/services/rollup.go (1)

205-219: Preserve previous metric value on challenge-window RPC errors

The detection logic for a finalizable batch (committed > finalized and !BatchInsideChallengeWindow(nextToFinalize)) looks correct and consistent with finalize(). The only nit is error handling: when BatchInsideChallengeWindow errors, you log a warning but still call SetHasPendingFinalizeBatch(false), which can incorrectly flip the metric from 1→0 on a transient RPC/revert issue, unlike the rest of this loop where you return on failures and keep existing values.

You can align behavior with the other metrics by bailing out on error so the gauge stays unchanged on failures:

-        inWindow, err := r.Rollup.BatchInsideChallengeWindow(nil, nextToFinalize)
-        if err != nil {
-            log.Warn("check challenge window error", "error", err, "batch_index", nextToFinalize.Uint64())
-        } else if !inWindow {
-            // Batch is outside challenge window and ready to finalize
-            hasPendingFinalizeBatch = true
-        }
+        inWindow, err := r.Rollup.BatchInsideChallengeWindow(nil, nextToFinalize)
+        if err != nil {
+            log.Warn("check challenge window error", "error", err, "batch_index", nextToFinalize.Uint64())
+            return
+        }
+        if !inWindow {
+            // Batch is outside challenge window and ready to finalize
+            hasPendingFinalizeBatch = true
+        }

This keeps the metric stable when you can’t reliably determine the state.

tx-submitter/metrics/metrics.go (2)

15-30: Gauge wiring is solid; tweak metric description for precision

The new HasPendingFinalizeBatch gauge is correctly added to the struct, initialized, registered, and unregistered — the plumbing looks good.

Given the actual logic in rollup.go only sets this to 1 when the next batch to finalize is outside the challenge window (i.e., ready to be finalized), you might want the Help text to reflect that more precisely:

-        HasPendingFinalizeBatch: prometheus.NewGauge(prometheus.GaugeOpts{
-            Name: "tx_submitter_has_pending_finalize_batch",
-            Help: "Whether there are batches pending finalization (1 = yes, 0 = no)",
-        }),
+        HasPendingFinalizeBatch: prometheus.NewGauge(prometheus.GaugeOpts{
+            Name: "tx_submitter_has_pending_finalize_batch",
+            Help: "Whether at least one committed batch is past its challenge window and pending finalization (1 = yes, 0 = no)",
+        }),

That keeps the metric semantics in sync with the comment and implementation in rollup.go.

Also applies to: 75-79, 104-104, 211-212


153-161: Setter semantics match usage; consider clarifying comment

SetHasPendingFinalizeBatch doing a simple bool{0,1} mapping is idiomatic for Prometheus gauges and matches how you use it from the rollup service.

To keep the exported API self-describing, you could mirror the more precise wording from the metric help string in the comment:

-// SetHasPendingFinalizeBatch sets whether there are batches pending finalization
-// hasPending should be true if there are pending batches, false otherwise
+// SetHasPendingFinalizeBatch sets whether at least one committed batch is past
+// its challenge window and pending finalization.
+// hasPending should be true if such a batch exists, false otherwise.

Implementation-wise this method is fine as-is.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cdfd03d and fa9369c.

⛔ Files ignored due to path filters (1)
  • go.work.sum is excluded by !**/*.sum
📒 Files selected for processing (2)
  • tx-submitter/metrics/metrics.go (5 hunks)
  • tx-submitter/services/rollup.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tx-submitter/services/rollup.go (1)
bindings/bindings/rollup.go (1)
  • Rollup (82-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: test
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (go)

@Kukoomomo Kukoomomo merged commit df65943 into main Nov 28, 2025
9 checks passed
@Kukoomomo Kukoomomo deleted the submitter_metrics branch November 28, 2025 01:58
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.

3 participants