fix: fix and cleanup metrics (sequencers + block) #2904
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task —— View job Changelog Entry Already Present ✓I've checked the repository and the changelog entry has already been successfully added to this PR! Current Status:
Findings: The changelog entry for PR #2904 is already present in ### Removed
- **BREAKING:** Removed unused and confusing metrics from sequencers and block processing, including sequencer-specific metrics (gas price, blob size, transaction status, pending blocks), channel buffer metrics, overly granular error metrics, block production categorization metrics, and sync lag metrics. Essential metrics for DA submission health, block production, and performance monitoring are retained. [#2904](https://github.com/evstack/ev-node/pull/2904)The PR has been successfully merged into |
Summary of ChangesHello @julienrbrt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a comprehensive cleanup and refactoring of the system's metrics. It systematically removes a significant number of metrics identified as either confusing or unused, particularly those related to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does a great job of cleaning up and simplifying the metrics, especially by removing the now-unused metrics from the single sequencer. The changes in the block package also help consolidate metric reporting. I have one suggestion to further reduce potential confusion by removing a redundant metric.
| func (e *Executor) recordBlockMetrics(newState types.State, data *types.Data) { | ||
| e.metrics.Height.Set(float64(newState.LastBlockHeight)) | ||
|
|
||
| if data == nil || data.Metadata == nil { | ||
| return | ||
| } | ||
|
|
||
| e.metrics.NumTxs.Set(float64(len(data.Txs))) | ||
| e.metrics.TotalTxs.Add(float64(len(data.Txs))) | ||
| e.metrics.TxsPerBlock.Observe(float64(len(data.Txs))) | ||
| e.metrics.BlockSizeBytes.Set(float64(data.Size())) | ||
| e.metrics.CommittedHeight.Set(float64(data.Metadata.Height)) | ||
| } |
There was a problem hiding this comment.
The Height metric appears to be redundant with the CommittedHeight metric. Both are set to the same value, as newState.LastBlockHeight and data.Metadata.Height will be identical for a newly produced block. The Prometheus name for CommittedHeight is latest_block_height, which is quite descriptive.
To align with the goal of this PR to clean up metrics and reduce confusion, I suggest removing the Height metric altogether. This would also simplify the code changes in this PR.
This suggestion removes the Height metric update and reverts the function signature. You would also need to:
- Remove the
Heightfield from thecommon.Metricsstruct inblock/internal/common/metrics.go. - Remove its initialization in
PrometheusMetricsandNopMetricsin the same file. - Update the call at line 429 to
e.recordBlockMetrics(data). - Update tests in
block/internal/common/metrics_test.goto remove checks for theHeightmetric.
func (e *Executor) recordBlockMetrics(data *types.Data) {
if data == nil || data.Metadata == nil {
return
}
e.metrics.NumTxs.Set(float64(len(data.Txs)))
e.metrics.TotalTxs.Add(float64(len(data.Txs)))
e.metrics.TxsPerBlock.Observe(float64(len(data.Txs)))
e.metrics.BlockSizeBytes.Set(float64(data.Size()))
e.metrics.CommittedHeight.Set(float64(data.Metadata.Height))
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2904 +/- ##
==========================================
- Coverage 64.63% 64.53% -0.10%
==========================================
Files 81 81
Lines 7382 7370 -12
==========================================
- Hits 4771 4756 -15
- Misses 2069 2072 +3
Partials 542 542
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
we should add a changelog as well |
|
@claude can you add a changelog to this pr |
|
Claude finished @tac0turtle's task —— View job Changelog Entry Added ✓I've successfully created a changelog entry for this metrics cleanup PR. However, I encountered a permissions issue when trying to push the changes to the remote repository. What I did:
Changelog Entry Added: ### Removed
- Removed unused and confusing metrics from sequencers and block processing, including sequencer-specific metrics (gas price, blob size, transaction status, pending blocks), channel buffer metrics, overly granular error metrics, block production categorization metrics, and sync lag metrics. Essential metrics for DA submission health, block production, and performance monitoring are retained. [#2904](https://github.com/evstack/ev-node/pull/2904)Issue: Next Steps:
The changelog entry summarizes the removal of unused metrics while highlighting that essential observability metrics are retained, which aligns with the PR's goal of cleaning up confusing and unused metrics. |
|
Oops, didn't see that notification. |
* main: refactor(sequencers): persist prepended batch (#2907) feat(evm): add force inclusion command (#2888) feat: DA client, remove interface part 1: copy subset of types needed for the client using blob rpc. (#2905) feat: forced inclusion (#2797) fix: fix and cleanup metrics (sequencers + block) (#2904) build(deps): Bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /docs in the npm_and_yarn group across 1 directory (#2900) refactor(block): centralize timeout in client (#2903) build(deps): Bump the all-go group across 2 directories with 3 updates (#2898) chore: bump default timeout (#2902) fix: revert default db (#2897) refactor: remove obsolete // +build tag (#2899) fix:da visualiser namespace (#2895)
Overview
ref: #2896 (comment)
Cleanup and remove confusing (and unused) metrics