feat: improve sequencer metrics

[georgehao]

No description provided.

[codspeed-hq]

Merging this PR will not alter performance

✅ 2 untouched benchmarks

---

_{Comparing feat/improve_sequencer_metrics (0bbc7b9) with main (b88371e)}

[frisitano]

I've added some comments inline. On a high level I think we should record the following metrics:

• block_building_duration: the time it takes to build a non-empty block
• empty_block_building_duration: the time it takes to build an empty block
• consecutive_block_interval: the time between two consecutive blocks that are added to the canonical chain.

I'm not sure if you had a different idea about these metrics to please let me know if I have misunderstood something.

[georgehao]

• block_building_duration: the time it takes to build a non-empty block
• empty_block_building_duration: the time it takes to build an empty block

Oh, I understand it is that:

if I want to check the block building duration, I will check the whole block building duration, then check the no-empty block building duration, then I will get the conclusion for it ( because the empty block building will always have a constant value, no sense to display this alone, and can calculate it from the all_block_building_duration and block_building_duration)

What do you think?

[frisitano]

Added a comment inline. I think the description you provided does not match the code.

[frisitano]

I think this solution doesn't work as expected. Please let me know if I made a mistake in my understanding.

[frisitano]

I think this function doesn't do anything because you already take the .block_building_start.take() in finish_all_block_building_recording.:

                let block = self
                    .sequencer
                    .as_mut()
                    .expect("sequencer must be present")
                    .finalize_payload_building(payload_id, &mut self.engine)
                    .await?

                self.metric_handler.finish_all_block_building_recording(); # block_building_start is taken here
                self.metric_handler.finish_block_building_interval_recording();

                if let Some(block) = block {
                    let block_info: L2BlockInfoWithL1Messages = (&block).into();
                    self.database
                        .update_l1_messages_from_l2_blocks(vec![block_info.clone()])
                        .await?;
                    self.signer
                        .as_mut()
                        .expect("signer must be present")
                        .sign_block(block.clone())?;
                     # this doesn't do anything because block_building_start is now None.
                    self.metric_handler.finish_no_empty_block_building_recording();
                    return Ok(Some(ChainOrchestratorEvent::BlockSequenced(block)));
                }

[georgehao]

rollup-node/crates/chain-orchestrator/src/metrics.rs

Lines 22 to 27 in 3e772db

	pub(crate) fn start_block_building_recording(&mut self) {
	if self.block_building_meter.block_building_start.is_some() {
	tracing::warn!(target: "scroll::chain_orchestrator", "block building recording is already ongoing, overwriting");
	}
	self.block_building_meter.block_building_start = Some(Instant::now());
	}

I think no issue here, because it assign value here, calculates twice I think it's ok

[frisitano]

rollup-node/crates/chain-orchestrator/src/metrics.rs

Lines 41 to 53 in cb6a18c

	pub(crate) fn finish_all_block_building_recording(&mut self) {
	let duration = self
	.block_building_meter
	.block_building_start
	.take()
	.map(|block_building_start| block_building_start.elapsed());
	if let Some(duration) = duration {
	self.block_building_meter
	.metric
	.all_block_building_duration
	.record(duration.as_secs_f64());
	}
	}

You take self.block_building_meter.block_building_start.take() here.

You then try and take it again below which will always be None.

rollup-node/crates/chain-orchestrator/src/metrics.rs

Lines 29 to 39 in cb6a18c

	/// The duration of the current block building task if any.
	pub(crate) fn finish_no_empty_block_building_recording(&mut self) {
	let duration = self
	.block_building_meter
	.block_building_start
	.take()
	.map(|block_building_start| block_building_start.elapsed());
	if let Some(duration) = duration {
	self.block_building_meter.metric.block_building_duration.record(duration.as_secs_f64());
	}
	}

[georgehao]

yes, that's right

[georgehao]

struct Test {
    pub block_building_start: Option<Instant>,
}

impl Test {
    fn finish_no_empty_block_building_recording(&mut self) {
        if let Some(block_build_start) = self.block_building_start {
            let duration = block_build_start.elapsed();
            println!("Time taken: {:?}", duration);
        }
    }

    fn finish_all_block_building_recording(&mut self) {
        if let Some(block_build_start) = self.block_building_start {
            let duration = block_build_start.elapsed();
            println!("Time taken(2): {:?}", duration);
        }
    }
}
#[tokio::main]
async fn main() {
    let mut t = Test {
        block_building_start: Option::from(Instant::now()),
    };

    time::sleep(Duration::from_secs(2)).await;
    t.finish_no_empty_block_building_recording();

    time::sleep(Duration::from_secs(2)).await;
    t.finish_all_block_building_recording();
}

This is ok