-
Notifications
You must be signed in to change notification settings - Fork 842
Add block timeskew metric #4034
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
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 introduces a new metric to record the time skew between the current time and the block’s timestamp. The key changes include:
- Adding a new prometheus gauge (blockTimeSkew) in the metrics struct.
- Initializing and registering the blockTimeSkew metric in newMetrics.
- Capturing and recording the time skew in the buildBlocks function.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| snow/engine/snowman/metrics.go | New gauge added and registered for block time skew. |
| snow/engine/snowman/engine.go | Captures and adds the block time skew to the gauge. |
| blockTimeSkew: prometheus.NewGauge(prometheus.GaugeOpts{ | ||
| Name: "block_time_skew", | ||
| Help: "The differences between the current time and the block's timestamp", | ||
| }), |
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.
Should we include (either in the name or the help text) that this is specifically for blocks built by the node? Perhaps making the name blks_built_time_skew to align more the with the existing blks_built metric (which is used to actually get the time skew per block)
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.
Changed the name
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.
and changed the description too
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Why this should be merged
We have a metric that measures the time between building the block to accepting it. However it uses the block's timestamp. This value includes the time the VM builds a block as we need to compute the time and insert the time into the block before we build it.
To better compute the time between right before the block builder releases the block to the network and the block's acceptance it can be useful if we can estimate the time skew between the local time and the block's timestamp.
How this works
After the snowman engine builds a block, compute the difference between the current real time to the block's timestamp and publish it via a metrics gauge. Then we can use a monitor that divides this accumulated number (because it's a gauge that only adds) to the number of blocks built to get the average time skew.
How this was tested
I ran a modified local network and sent some Ethereum transactions to the C chain and then probed the metrics endpoint on the HTTP endpoint:
Need to be documented in RELEASES.md?