Optimize merge tracker and add merge metrics #4350
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
g11tech
force-pushed
the
g11tech/merge-tracker-opts-and-metrics
branch
from
8c9f932
to
a383d7c
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
reviewed
Aug 1, 2022
| @@ -71,10 +71,12 @@ export async function runNodeNotifier(modules: NodeNotifierModules): Promise<voi | |||
| // Notifier log lines must be kept at a reasonable max width otherwise it's very hard to read | |||
| const tdProgress = chain.eth1.getTDProgress(); | |||
| if (tdProgress !== null && !tdProgress.ttdHit) { | |||
| tdTimeSeries.addPoint(tdProgress.tdDiffScaled, tdProgress.timestamp); | |||
| // TimeSeries accept time in Ms while timestamp is in Sec | |||
| tdTimeSeries.addPoint(tdProgress.tdDiffScaled, tdProgress.timestamp * 1000); | |||
There was a problem hiding this comment.
Or TimeSeries.addPoint can take seconds instead and simplify the consumers since all want sec right?
There was a problem hiding this comment.
right! we can shift to seconds
dapplion
reviewed
Aug 1, 2022
| @@ -135,7 +135,7 @@ export class Eth1MergeBlockTracker { | |||
| return { | |||
| ttdHit: false, | |||
| tdFactor: this.safeTDFactor, | |||
| tdDiffScaled: Number((this.latestEth1Block.totalDifficulty / this.safeTDFactor) as bigint), | |||
| tdDiffScaled: Number((tdDiff / this.safeTDFactor) as bigint), | |||
dapplion
reviewed
Aug 1, 2022
| @@ -96,7 +96,7 @@ export class Eth1MergeBlockTracker { | |||
| // Set latestBlock stats | |||
| metrics.eth1.eth1LatestBlockNumber.set(this.latestEth1Block.number); | |||
| metrics.eth1.eth1LatestBlockTD.set(Number(this.latestEth1Block.totalDifficulty / this.safeTDFactor)); | |||
| metrics.eth1.eth1LatestBlockTimestamp.set(this.latestEth1Block.timestamp); | |||
| metrics.eth1.eth1LatestBlockTimestamp.set(this.latestEth1Block.timestamp * 1000); | |||
There was a problem hiding this comment.
Prometheus metrics always deal with IS units, so in case of time it must be seconds. See https://prometheus.io/docs/practices/naming/#metric-names
There was a problem hiding this comment.
moved collection in seconds, and update the dashboard expression (*1000) which displays with unit "From now" which seems to be using ms.
g11tech
force-pushed
the
g11tech/merge-tracker-opts-and-metrics
branch
from
3f377aa
to
c5ea976
Compare
dapplion
reviewed
Aug 2, 2022
| }); | ||
| } | ||
| // It is possible for status to be updated to merge complete or found or stopped | ||
| if (this.status.code != StatusCode.SEARCHING) { |
There was a problem hiding this comment.
This condition doesn't look right, should it be this?
Suggested change
| if (this.status.code != StatusCode.SEARCHING) { | |
| if (this.status.code === StatusCode.SEARCHING) { |
There was a problem hiding this comment.
no we want to clear intervals when its not searching, since this codeflow is called by the poll, if merge block has been found, the this.status has been transitioned to either FOUND or MERGE_COMPLETED (triggered from outside this module)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Optimize merge tracker and add merge metrics
Closes #4133
The merge logging,
curated for brevity: (activated post
bellatrix) (from local testnet testing ttd=50)first merge is tracked in %age, and then fully expanded form
expanded form activates at >99.99% merge complete or at
12 hoursbefore estimated merge (here in this log 10 seconds is the threshold for testing purposes)Metrics
Pre-merge
(local testnet TTD 50)
Post-merge
(local testnet TTD 50)
Merge search not started
(lodestar syncing, not near bellatrix, geth snap syncing fresh)
Post-merge
(kiln network)
Merge not set
(mainnet)
