feat: add metrics to differential state #8678
Conversation
Summary of ChangesHello @nazarhussain, 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 significantly enhances the observability of the differential state management system by introducing a comprehensive suite of metrics. These metrics cover various aspects of state regeneration, block replay, and differential storage, providing crucial insights into performance, success rates, and resource utilization. The goal is to enable better monitoring and understanding of the system's behavior, facilitating future optimizations and debugging. 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 introduces metrics for the differential state processing, enhancing observability into the regeneration, storage, and application of differential states. The changes involve adding new metric definitions, integrating these metrics into relevant functions, and updating function signatures to pass the metrics object. Overall, the implementation is well-structured and follows a consistent pattern for metric collection.
|
|
||
| return state; | ||
| } catch (err) { | ||
| ctx.metrics?.regenErrorCount.inc({reason: RegenErrorType.loadState}); |
There was a problem hiding this comment.
The regenErrorCount is incremented with RegenErrorType.loadState in the catch block. While loading state is a potential failure point, errors from applyStateRegenPlan could also be related to blockProcessing or invalidStateRoot if, for example, block replay fails or a state root mismatch occurs during block application. Consider a more granular error type mapping or a generic RegenErrorType.unknown if the specific error type cannot be determined from the caught exception.
There was a problem hiding this comment.
@nazarhussain Can you address this concern? regenErrorCount might count twice or thrice depending on where the error come from
| name: "lodestar_differential_state_load_diff_state_time_seconds", | ||
| help: "Time to load a differential snapshot state from the database in seconds", | ||
| // 30s, 1m, 2m, 4m | ||
| buckets: [30, 60, 120, 240], |
There was a problem hiding this comment.
The help string for loadDiffStateTime is currently identical to loadSnapshotStateTime. To improve clarity and accuracy, please update it to reflect that it measures the time to load a differential state, not a snapshot state.
loadDiffStateTime: metricsRegister.histogram({
name: "lodestar_differential_state_load_diff_state_time_seconds",
help: "Time to load a differential state from the database in seconds",
| } | ||
| } | ||
|
|
||
| export async function storeDifferentialState( |
There was a problem hiding this comment.
Where is this function used? If it is not related to metrics, please add it in another PR.
| const state = snapshotToBeaconState(ctx, finalState); | ||
|
|
||
| return snapshotToBeaconState(ctx, finalState); | ||
| ctx.metrics?.regenSuccessCount.inc(); |
There was a problem hiding this comment.
Incrementing regenSuccessCount should come after regenTimer has stopped
|
|
||
| return state; | ||
| } catch (err) { | ||
| ctx.metrics?.regenErrorCount.inc({reason: RegenErrorType.loadState}); |
There was a problem hiding this comment.
@nazarhussain Can you address this concern? regenErrorCount might count twice or thrice depending on where the error come from
| }), | ||
| blockReplayCount: metricsRegister.histogram({ | ||
| name: "lodestar_differential_state_block_replay_count", | ||
| help: "Count of blocks processed during state transition to differential historical state", |
There was a problem hiding this comment.
Note that this is count of slots not count of blocks. There could be empty slots
| name: "lodestar_differential_state_load_diff_state_time_seconds", | ||
| help: "Time to load a differential snapshot state from the database in seconds", | ||
| // 30s, 1m, 2m, 4m | ||
| buckets: [30, 60, 120, 240], |
Motivation
Add metrics to different processing of differential state.
Description
Note
Will add the dashboard later during the testing it on feature group.