-
Notifications
You must be signed in to change notification settings - Fork 124
feat(l1): upgrade execution breakdown pie chart and add new panels #5220
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
Conversation
| } | ||
| } | ||
|
|
||
| #[instrument(level = "trace", name = "Block execution", skip_all)] |
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 are not instrumenting other non-pipeline related functions, if we want to do so we might need to do it in a following PR.
|
|
||
| /// Applies account updates based on the block's latest storage state | ||
| /// and returns the new state root after the updates have been applied. | ||
| #[instrument(level = "trace", name = "Trie update", skip_all)] |
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.
This was already instrumented at handle_merkleization, if it was intended to be part of the non-pipeline instrumentation, we need to do it in a follow-up.
Lines of code reportTotal lines added: Detailed view |
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.
Pull Request Overview
This PR reorganizes tracing instrumentation for performance monitoring by moving the "Block DB update" trace span from the storage layer to the blockchain layer, and removes redundant instrumentation from the VM execution path. Additionally, it includes significant Grafana dashboard enhancements for better block execution breakdown visualization.
- Relocated "Block DB update" instrumentation from
rocksdb.rstoblockchain.rs::store_blockfor more accurate monitoring - Removed instrumentation from
execute_blockandapply_account_updates_batchto reduce overhead - Enhanced Grafana dashboard with collapsed block execution breakdown section containing multiple detailed panels
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/vm/backends/mod.rs | Removed #[instrument] from execute_block method |
| crates/storage/store_db/rocksdb.rs | Removed inline tracing span for "Block DB update" |
| crates/storage/store.rs | Removed #[instrument] from apply_account_updates_batch and cleaned up unused instrument import |
| crates/blockchain/blockchain.rs | Added #[instrument] to store_block method to track "Block DB update" |
| metrics/provisioning/grafana/dashboards/common_dashboards/ethrex_l1_perf.json | Enhanced dashboard with collapsed block execution breakdown section, improved queries for execution vs merkleization analysis, and added deaggregated block-by-block execution visualization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
metrics/provisioning/grafana/dashboards/common_dashboards/ethrex_l1_perf.json
Outdated
Show resolved
Hide resolved
metrics/provisioning/grafana/dashboards/common_dashboards/ethrex_l1_perf.json
Outdated
Show resolved
Hide resolved
metrics/provisioning/grafana/dashboards/common_dashboards/ethrex_l1_perf.json
Outdated
Show resolved
Hide resolved
…5220) **Motivation** We needed more granular panels to analyze the breakdown of execution for performance decisions. **Description** This PR use the current instrumentation to: - Update the pie char of execution - Add a diff measurement between execution and merkleization to understand the work of the `execute_block_pipeline` function - Add a deagregated series by block showing the different instrumentations available today - Maintain all of them in the previous Block Execution Breakdown row and repeat them vertically across instances <img width="1910" height="705" alt="image" src="https://github.com/user-attachments/assets/0af6dfda-41b3-46f0-8463-1494c6ea0c8b" /> **Note for running it locally:** _After #5088 we needed to re-labeling our local instances to avoid duplicated panels that repeat across instances. This requires prometheus to be restarted to pick up the new labeling, one way to check this out is make look at intances, if `ethereum-metrics-exporte` appears there, we need to restart prometheus, only `localhost` should be there locally._ | Bad (needs restarting prometheus)| Good | | --- | --- | | <img width="359" height="156" alt="image" src="https://github.com/user-attachments/assets/7c4bfd32-f565-4a79-bea9-86094128bfcf" /> | <img width="453" height="176" alt="image" src="https://github.com/user-attachments/assets/8e8423ec-3830-461f-94ab-697bd5967968" /> | **Next steps:** - Naming is not optimal, but changing it would require breaking the panel for instances not updated to this PR. We want first to test it with all instances and make sure the panels are useful and change the names afterwards, here is an issue for tracking that: #5219 Closes #5218 --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
**Motivation** Document the new execution breakdown and RPC panels in the dashboard as well as other metric related docs **Description** This PR documents both #5220 and #5311 additions, so we might want to merge #5311 first. It updates the gap analysis, the actual dashboard doc and some metrics related docs. It also sets prometheus scrape to 5s as already done today on our centralized grafana instance. Quick [link](https://github.com/lambdaclass/ethrex/blob/update-dashboard-documentation/docs/developers/l1/dashboards.md) to check the updated dashboard doc out with the viewer --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Motivation
We needed more granular panels to analyze the breakdown of execution for performance decisions.
Description
This PR use the current instrumentation to:
execute_block_pipelinefunctionNote for running it locally:
After #5088 we needed to re-labeling our local instances to avoid duplicated panels that repeat across instances. This requires prometheus to be restarted to pick up the new labeling, one way to check this out is make look at intances, if
ethereum-metrics-exporteappears there, we need to restart prometheus, onlylocalhostshould be there locally.Next steps:
Closes #5218