feat(starknet_batcher): emit proposal metrics

[yair-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[yair-starkware]

• feat(starknet_batcher): emit transaction metrics #3315
• feat(starknet_batcher): emit proposal metrics #3305 👈 (View in Graphite)
• feat(starknet_batcher): storage height metric #3272
• main

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

[graphite-app]

Graphite Automations

"Yair - Auto-assign" took an action on this PR • (01/14/25)

1 assignee was added to this PR based on Yair's automation.

[alonh5]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dafnamatsry and @yair-starkware)

---

crates/starknet_batcher/src/batcher_test.rs line 275 at r1 (raw file):

async fn validate_block_full_flow() {
    let recorder = PrometheusBuilder::new().build_recorder();
    let _g = metrics::set_default_local_recorder(&recorder);

Same comments from the previous PR.

---

crates/starknet_batcher/src/batcher_test.rs line 283 at r1 (raw file):

        parse_numeric_metric::<u64>(&metrics, crate::metrics::PROPOSAL_STARTED.name),
        Some(1)
    );

Why not use this?

Suggestion:

    assert_proposal_metrics(&metrics, 1, 0, 0, 0);

---

crates/starknet_batcher/src/batcher_test.rs line 384 at r1 (raw file):

    // The block builder is running in a separate task, and the proposal metrics are emitted from
    // that task, so we need to wait for them.

This is specifically because of the abort, right? Can you reflect that in the comment?

Code quote:

    // The block builder is running in a separate task, and the proposal metrics are emitted from
    // that task, so we need to wait for them.

---

crates/starknet_batcher/src/batcher_test.rs line 385 at r1 (raw file):

    // The block builder is running in a separate task, and the proposal metrics are emitted from
    // that task, so we need to wait for them.
    // TODO: Find a way to wait for the metrics to be emitted.

We decided on no more nameless TODOs

Suggestion:

// TODO(Yair): Find a way to wait for the metrics to be emitted.

---

crates/starknet_batcher/src/batcher_test.rs line 702 at r1 (raw file):

}

fn assert_proposal_metrics(

Please move this and other utility functions to the top of the file.

---

crates/starknet_batcher/src/batcher_test.rs line 741 at r1 (raw file):

        expected_aborted,
        aborted,
    );

Should we add an assertion that the sum makes sense? (0 <= started - (succeeded + failed + aborted) <= 1)

---

crates/starknet_batcher/src/batcher.rs line 642 at r1 (raw file):

impl ComponentStarter for Batcher {}

/// A handle to update the proposal metrics when the proposal is created and dropped.

Move all this to the metrics file?

---

crates/starknet_batcher/src/batcher.rs line 655 at r1 (raw file):

    pub fn set_succeeded(&mut self) {
        debug!("Proposal succeeded.");

Move these two debug logs to the drop and add for Failed also.

Code quote:

debug!("Proposal succeeded.");

---

crates/starknet_batcher/src/batcher.rs line 672 at r1 (raw file):

}

impl Drop for ProposalMetricsHandle {

Very nice

[yair-starkware]

Reviewable status: 0 of 3 files reviewed, 8 unresolved discussions (waiting on @alonh5 and @dafnamatsry)

---

crates/starknet_batcher/src/batcher.rs line 642 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Move all this to the metrics file?

Done.

---

crates/starknet_batcher/src/batcher.rs line 655 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Move these two debug logs to the drop and add for Failed also.

I left the logs by mistake, I don't think we need them

---

crates/starknet_batcher/src/batcher_test.rs line 275 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Same comments from the previous PR.

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 283 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why not use this?

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 384 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This is specifically because of the abort, right? Can you reflect that in the comment?

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 385 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

We decided on no more nameless TODOs

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 702 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Please move this and other utility functions to the top of the file.

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 741 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Should we add an assertion that the sum makes sense? (0 <= started - (succeeded + failed + aborted) <= 1)

I think with mocks not all metrics will always be emitted

[alonh5]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @yair-starkware)

---

crates/starknet_batcher/src/batcher_test.rs line 741 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

I think with mocks not all metrics will always be emitted

Here in the batcher test (where the batcher shouldn't be mocked) it will always be emitted, no?
Because the handle is created in the batcher, and the metrics are emitted when it drops.

---

crates/starknet_batcher/src/batcher_test.rs line 443 at r2 (raw file):

    // that task, so we need to wait for them (we don't have a way to wait for the completion of the
    // abort).
    // TODO(Alon): Find a way to wait for the metrics to be emitted.

Suggestion:

TODO(AlonH)

[yair-starkware]

crates/starknet_batcher/src/batcher_test.rs line 741 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Here in the batcher test (where the batcher shouldn't be mocked) it will always be emitted, no?
Because the handle is created in the batcher, and the metrics are emitted when it drops.

There is the issue with aborts, but other than that you are probably right.

[graphite-app]

Merge activity

• Jan 19, 7:38 AM EST: Graphite disabled "merge when ready" on this PR due to: We hit a GitHub rate limit while trying to merge. If you are attempting to merge a large stack, breaking it up into smaller chunks may help..

[alonh5]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @yair-starkware)

---

crates/starknet_batcher/src/batcher_test.rs line 741 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

There is the issue with aborts, but other than that you are probably right.

So you'll add it?

[yair-starkware]

crates/starknet_batcher/src/batcher_test.rs line 741 at r1 (raw file):
Done

=0 is enforced by u64 type (and issues a warning when asserted)

[alonh5]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @yair-starkware)

---

crates/blockifier/cairo_native line 1 at r4 (raw file):

Subproject commit 76e83965d3bf1252eb6c68200a3accd5fd1ec004

Revert.

[alonh5]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @yair-starkware)

[yair-starkware]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @dafnamatsry)

---

crates/blockifier/cairo_native line 1 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Revert.

Done.

[yair-starkware]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @dafnamatsry)

[alonh5]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)