-
Notifications
You must be signed in to change notification settings - Fork 6
feat: improve sequencer metrics #461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest we revert this function name back to |
||
| 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) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest we rename this function |
||
| 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) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should rename this function to |
||
| 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 { | ||
|
|
@@ -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, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be updated to be |
||
| /// The duration of the block interval include empty block | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on my other comments, I would update this description to be |
||
| consecutive_block_interval: Histogram, | ||
| } | ||
There was a problem hiding this comment.
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_recordinghere and instead invoke this function infinish_no_empty_block_building_recording.