Skip to content
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

fix(tree): update metrics only on canonical/side chain changes #3671

Merged
merged 1 commit into from
Jul 9, 2023

Conversation

shekhirin
Copy link
Collaborator

@shekhirin shekhirin commented Jul 8, 2023

The bug this PR fixes was introduced in #3507.

During the live sync on every new FCU or payload, blockchain tree processes new update, modifies the canonical or side chain, and updates metrics (which are common for both blockchain tree and pipeline syncs).

But during the pipeline sync, blockchain tree can't do any updates, so it buffers the block for further processing when the pipeline finishes. However, it still updates metrics:

fn buffer_block(&self, block: SealedBlockWithSenders) -> Result<(), InsertBlockError> {
let mut tree = self.tree.write();
let res = tree.buffer_block(block);
tree.update_metrics();
res
}

Since we didn't modify the actual tree and only buffered a block, the metrics will get updated with the current state of blockchain tree:

pub(crate) fn update_metrics(&mut self) {
let height = self.canonical_chain().tip().number;
self.metrics.sidechains.set(self.chains.len() as f64);
self.metrics.canonical_chain_height.set(height as f64);
if let Some(metrics_tx) = self.sync_metrics_tx.as_mut() {
let _ = metrics_tx.send(MetricEvent::SyncHeight { height });
}
}

The problem arises when we do an update to metrics during the pipeline sync (initial or on large range of missing blocks):

  1. Pipeline sync updates metrics with new checkpoint reached by the stage.
  2. New FCU or payload arrives, blockchain tree can't process it and buffers the block, and STILL updates metrics with the current blockchain tree, overwriting the updates to metrics made by pipeline. If it's an initial sync, metrics get updated with 0, because we don't have any canonical chain in the blockchain tree yet.
  3. Pipeline sync updates metrics again, overwriting the previously set 0 by blockchain tree.

This produces such dashboard (notice spiky chart which demonstrates the behaviour with 0 -> pipeline sync metric -> 0 -> pipeline sync metric metric value):


This PR fixes this behaviour by updating the blockchain tree metrics ONLY during the actual modification of the blockchain tree, i.e. doesn't update metrics during the block buffering.

@shekhirin shekhirin force-pushed the alexey/blockchain-tree-metrics branch from dd560e2 to 6401dc2 Compare July 8, 2023 11:50
@shekhirin shekhirin added C-bug An unexpected or incorrect behavior A-blockchain-tree Related to sidechains, reorgs and pending blocks A-observability Related to tracing, metrics, logs and other observability tools and removed A-blockchain-tree Related to sidechains, reorgs and pending blocks labels Jul 8, 2023
@shekhirin shekhirin marked this pull request as ready for review July 8, 2023 12:09
@shekhirin shekhirin requested review from mattsse and onbjerg and removed request for rakita and gakonst July 8, 2023 12:09
@codecov
Copy link

codecov bot commented Jul 8, 2023

Codecov Report

Merging #3671 (8427564) into main (42a824c) will decrease coverage by 19.34%.
The diff coverage is 33.33%.

Impacted file tree graph

Impacted Files Coverage Δ
crates/blockchain-tree/src/shareable.rs 22.95% <25.00%> (-15.58%) ⬇️
crates/blockchain-tree/src/blockchain_tree.rs 81.58% <100.00%> (-0.26%) ⬇️

... and 226 files with indirect coverage changes

Flag Coverage Δ
integration-tests 13.58% <0.00%> (-2.34%) ⬇️
unit-tests 43.91% <33.33%> (-20.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 16.06% <ø> (-10.35%) ⬇️
blockchain tree 70.31% <33.33%> (-10.95%) ⬇️
pipeline 68.71% <ø> (-17.98%) ⬇️
storage (db) 59.85% <ø> (-13.64%) ⬇️
trie 67.15% <ø> (-27.51%) ⬇️
txpool 34.18% <ø> (-15.17%) ⬇️
networking 50.65% <ø> (-27.21%) ⬇️
rpc 42.08% <ø> (-16.07%) ⬇️
consensus 40.33% <ø> (-23.02%) ⬇️
revm 21.18% <ø> (-13.68%) ⬇️
payload builder 6.83% <ø> (ø)
primitives 63.98% <ø> (-24.26%) ⬇️

@shekhirin shekhirin force-pushed the alexey/blockchain-tree-metrics branch from 6401dc2 to 8427564 Compare July 8, 2023 12:10
@shekhirin shekhirin changed the title fix(tree): update metrics only on canon/side chain changes fix(tree): update metrics only on canonical/side chain changes Jul 8, 2023
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, it still updates metrics

hehe

nice find!

@onbjerg onbjerg added this pull request to the merge queue Jul 9, 2023
Merged via the queue into main with commit b68116c Jul 9, 2023
@onbjerg onbjerg deleted the alexey/blockchain-tree-metrics branch July 9, 2023 17:50
@shekhirin shekhirin added this to the 0.1.0-alpha.3 milestone Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-observability Related to tracing, metrics, logs and other observability tools C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants