feat(metrics): add sequencer metrics & grafana dashboard generation tool - #651
feat(metrics): add sequencer metrics & grafana dashboard generation tool#651Arjentix wants to merge 15 commits into
Conversation
…s in dashboard generation
erhant
left a comment
erhant
left a comment
There was a problem hiding this comment.
LGTM nice work! few small comments
3dc0414 to
2856c6e
Compare
moudyellaz
left a comment
moudyellaz
left a comment
There was a problem hiding this comment.
lgtm, thanks! Left some minor comments/questions.
| ); | ||
|
|
||
| sequencer_core_metrics::record_block_creation_time(now.elapsed()); | ||
| sequencer_core_metrics::increment_blocks_total(); |
There was a problem hiding this comment.
increment_blocks_total() fires in build_block_from_mempool before publish, so it counts build attempts, parked blocks included, and misses peer-adopted ones. Since startup seeds it from chain_height(), "Chain height" drifts once a second sequencer produces. Could we move it to record_produced_block's Applied arm plus the follow path, or rename it blocks_produced_total?
| #[must_use] | ||
| pub fn rate_per_min(metric: &str, legend: &str) -> Target { | ||
| Target::new(format!("rate({metric}[1m]) * 60")).legend(legend) | ||
| } |
There was a problem hiding this comment.
Maybe rate({metric}[{RATE_WINDOW}]) * 60 instead?
| Deserialize, | ||
| BorshSerialize, | ||
| BorshDeserialize, | ||
| strum::IntoStaticStr, |
There was a problem hiding this comment.
Maybe drop IntoStaticStr and EnumIter since they are not used?
| )?; | ||
| sequencer_core_metrics::record_mempool_transaction_application_time( | ||
| origin.into(), | ||
| tx.kind().into(), |
There was a problem hiding this comment.
Maybe record this only when applied, or add a status label, so failed applications do not skew the histogram?
| /// A percentile as its `histogram_quantile` argument, derived without float | ||
| /// math: zero-pad to two digits then drop trailing zeros (50 → `0.5`). | ||
| fn quantile(percentile: u32) -> String { | ||
| let quantile = format!("0.{percentile:02}"); |
There was a problem hiding this comment.
quantile(100) gives 0.1 and quantile(0) gives 0., maybe assert 1..=99 like the default check above?
Pravdyvy
left a comment
Pravdyvy
left a comment
There was a problem hiding this comment.
lgtm. One comment, probably a future work if it is not trivial.
| pub async fn start_from_config( | ||
| config: SequencerConfig, | ||
| ) -> (Self, MemPoolHandle<(TransactionOrigin, LeeTransaction)>) { | ||
| sequencer_core_metrics::init(); |
There was a problem hiding this comment.
It looks like we make metrics collection mandatory for every sequencer. For this PR it is fine, but it would be good to make it configurable. Not providing metrics_address in config should be a sufficient trigger.
4 participants
🎯 Purpose
This PR introduces metrics to the Sequencer as well as a tool to generate Grafana dashboards with code-first approach.
⚙️ Approach
metriccrates: one forsequencer_coreand one forsequencer_servicetool/dashboard_genwhich generates Grafana dashboard from rust specificationdocker composeandjustsetup to run Prometheus + Grafana setup locallymetrics.mdScreenshot (not full):
🧪 How to Test
justhttp://localhost:3000and go to theSequencerdashboard🔗 Dependencies
None
🔜 Future Work
dashboard_gentool only supports things we need right now, so on every new non-supported Grafana json syntax we need to expand it📋 PR Completion Checklist
- [ ] Add/update tests