-
Notifications
You must be signed in to change notification settings - Fork 7
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
3e772db
f8a42f4
e7f150d
83517bc
d941f8b
d0a867c
cb6a18c
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
Member
Author
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 understand it is: We don't enable the empty block building normally, so |
||
| 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.
I think this function doesn't do anything because you already take the
.block_building_start.take()infinish_all_block_building_recording.: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.
rollup-node/crates/chain-orchestrator/src/metrics.rs
Lines 22 to 27 in 3e772db
I think no issue here, because it assign value here, calculates twice I think it's ok