Skip to content

apollo_batcher: refactor commit metrics to read the label from the task output#14384

Open
yoavGrs wants to merge 1 commit into
apollo_batcher_task_timer_per_label_mapfrom
apollo_batcher_commit_metrics_refactor
Open

apollo_batcher: refactor commit metrics to read the label from the task output#14384
yoavGrs wants to merge 1 commit into
apollo_batcher_task_timer_per_label_mapfrom
apollo_batcher_commit_metrics_refactor

Conversation

@yoavGrs

@yoavGrs yoavGrs commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

The result-collection loops use the new CommitterTaskOutput::task_label()
instead of hardcoding CommitBlock, and both commit labels share
record_commit_block_metric ahead of adding a second commit endpoint.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

@yoavGrs Graphite App

yoavGrs commented Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@reviewable-StarkWare

reviewable-StarkWare commented Jun 4, 2026

Copy link
Copy Markdown

This change is Reviewable

This was referenced Jun 4, 2026
Open
Open
Open
Open
Open
Open
@yoavGrs yoavGrs marked this pull request as ready for review June 8, 2026 07:14
@cursor

cursor Bot commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Summary

Low Risk
Observability-only refactor in the batcher commitment manager; commit/revert behavior is unchanged and task_label() still maps commits to CommitBlock today.

Overview
Refactors how commitment-manager task duration metrics are recorded when results arrive from the state committer.

get_commitment_results and wait_for_revert_result now call CommitterTaskOutput::task_label() (new on the output enum) instead of hardcoding CommitBlock / inlining revert handling. wait_for_revert_result also records metrics once per message via result.height() before branching on commit vs revert.

update_task_duration_metric pulls shared commit latency/count increments into record_commit_block_metric, used for both CommitBlock and ReadPathsAndCommitBlock (feature-gated), with debug logging parameterized by the label.

height() on CommitterTaskOutput is simplified to match on inner outputs without destructuring in the pattern.

Reviewed by Cursor Bugbot for commit 6354afd. Bugbot is set up for automated code reviews on this repo. Configure here.

@yoavGrs yoavGrs requested a review from itamar-starkware June 8, 2026 07:15
@yoavGrs yoavGrs self-assigned this Jun 8, 2026
@yoavGrs @claude
apollo_batcher: refactor commit metrics to read the label from the ta…
6354afd 
…sk output

The result-collection loops use the new CommitterTaskOutput::task_label()
instead of hardcoding CommitBlock, and both commit labels share
record_commit_block_metric ahead of adding a second commit endpoint.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yoavGrs yoavGrs force-pushed the apollo_batcher_task_timer_per_label_map branch from fdc71fe to 8fb9782 Compare June 9, 2026 10:31
@yoavGrs yoavGrs force-pushed the apollo_batcher_commit_metrics_refactor branch from daf5ca3 to 6354afd Compare June 9, 2026 10:31
itamar-starkware
itamar-starkware approved these changes Jun 9, 2026
View reviewed changes

@itamar-starkware itamar-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One non blocking comment

Comment thread crates/apollo_batcher/src/commitment_manager/commitment_manager_impl.rs
// TODO(Ariel): Add dedicated metrics once we use os_input in prod.
COMMITMENT_MANAGER_COMMIT_BLOCK_LATENCY.increment(task_duration);
COMMITMENT_MANAGER_COMMIT_BLOCK_COUNT.increment(1);
record_commit_block_metric(task_duration, height, task_type)

@itamar-starkware itamar-starkware Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non blocking - we should consider create another metric for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@itamar-starkware itamar-starkware itamar-starkware approved these changes

Assignees

@yoavGrs yoavGrs

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yoavGrs @reviewable-StarkWare @itamar-starkware