Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion crates/chain-orchestrator/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,11 @@ impl<
.as_mut()
.expect("signer must be present")
.sign_block(block.clone())?;
self.metric_handler.finish_block_building_recording();
self.metric_handler.finish_no_empty_block_building_recording();
return Ok(Some(ChainOrchestratorEvent::BlockSequenced(block)));
}
self.metric_handler.finish_all_block_building_recording();
self.metric_handler.finish_block_building_interval_recording();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should remove the call to finish_block_building_interval_recording here and instead invoke this function in finish_no_empty_block_building_recording.

}
}

Expand Down
50 changes: 44 additions & 6 deletions crates/chain-orchestrator/src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,52 @@ impl MetricsHandler {

/// Starts tracking a new block building task.
pub(crate) fn start_block_building_recording(&mut self) {
if self.block_building_meter.start.is_some() {
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.start = Some(Instant::now());
self.block_building_meter.block_building_start = Some(Instant::now());
}

/// The duration of the current block building task if any.
pub(crate) fn finish_block_building_recording(&mut self) {
let duration = self.block_building_meter.start.take().map(|start| start.elapsed());
pub(crate) fn finish_no_empty_block_building_recording(&mut self) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we revert this function name back to finish_block_building_recording.

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());
}
}

pub(crate) fn finish_all_block_building_recording(&mut self) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we rename this function finish_empty_block_building_recording as it will only be triggered for empty blocks that are skipped.

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());
}
}

pub(crate) fn finish_block_building_interval_recording(&mut self) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should rename this function to finish_block_interval_recording and we should call this function in finish_no_empty_block_building_recording. This way it will record the time between two consecutive blocks are part of the canonical chain and gossiped to the network.

let interval = self
.block_building_meter
.last_block_building_time
.take()
.map(|last_block_building_time| last_block_building_time.elapsed());
if let Some(interval) = interval {
self.block_building_meter
.metric
.consecutive_block_interval
.record(interval.as_secs_f64());
}
self.block_building_meter.last_block_building_time = Some(Instant::now());
}
}

impl Default for MetricsHandler {
Expand Down Expand Up @@ -104,13 +137,18 @@ pub(crate) struct ChainOrchestratorMetrics {
#[derive(Debug, Default)]
pub(crate) struct BlockBuildingMeter {
metric: BlockBuildingMetric,
start: Option<Instant>,
block_building_start: Option<Instant>,
last_block_building_time: Option<Instant>,
}

/// Block building related metric.
#[derive(Metrics, Clone)]
#[metrics(scope = "chain_orchestrator")]
pub(crate) struct BlockBuildingMetric {
/// The duration of the block building task.
/// The duration of the block building task without empty block
block_building_duration: Histogram,
/// The duration of the block building task for all blocks include empty block
all_block_building_duration: Histogram,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be updated to be empty_block_building_duration as our finish_* functions will only update this for empty blocks.

/// The duration of the block interval include empty block
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on my other comments, I would update this description to be The duration between two consecutive blocks that become part of the canonical chain

consecutive_block_interval: Histogram,
}
Loading