Skip to content

feat: add canonicalization latency metric#3865

Merged
Rjected merged 3 commits intoparadigmxyz:mainfrom
cjeva10:make-canonical-latency
Jul 24, 2023
Merged

feat: add canonicalization latency metric#3865
Rjected merged 3 commits intoparadigmxyz:mainfrom
cjeva10:make-canonical-latency

Conversation

@cjeva10
Copy link
Contributor

@cjeva10 cjeva10 commented Jul 21, 2023

Closes #3815

@onbjerg onbjerg changed the title added metric to track latency for making a block canonical feat: add canonicalization latency metric Jul 24, 2023
@onbjerg onbjerg added C-enhancement New feature or request M-changelog This change should be included in the changelog A-observability Related to tracing, metrics, logs and other observability tools A-engine Related to the engine implementation labels Jul 24, 2023
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

Logic wise this looks great! I wonder if we can do something like the following though:

        let start = Instant::now();
        let make_canonical_result = self.blockchain.make_canonical(&state.head_block_hash);
        self.record_make_canonical_latency(start, make_canonical_result);
        let status = match make_canonical_result {
        // ... rest

where record_make_canonical_latency does a match:

let elapsed = start.elapsed();
self.metrics.make_canonical_latency.record(elapsed);
match outcome {
    Ok(MakeCanonicalOutcome::AlreadyCanonical) => self.metrics.make_canonical_already_canonical_latency.record(elapsed),
    // the AlreadyCommitted case
    Err(_) => // the err case
}

does that make sense? I think then, we don't have to intersperse the metrics record calls in the match for other logic.

@Rjected
Copy link
Member

Rjected commented Jul 24, 2023

this also needs to be rebased / merged with the latest main since we made some fixes to CI

@cjeva10
Copy link
Contributor Author

cjeva10 commented Jul 24, 2023

Perfect, yes makes sense - I wasn't sure if I should restructure the flow on the main match statement but what you have there makes sense. Will push shortly

@cjeva10
Copy link
Contributor Author

cjeva10 commented Jul 24, 2023

@Rjected I put in the changes you asked for - please let me know if the branch looks ok.

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #3865 (5d7c03c) into main (9b9ae82) will decrease coverage by 0.02%.
The diff coverage is 93.75%.

❗ Current head 5d7c03c differs from pull request most recent head 6a70991. Consider uploading reports for the commit 6a70991 to get more accurate results

Impacted file tree graph

Files Changed Coverage Δ
crates/consensus/beacon/src/engine/metrics.rs 100.00% <ø> (ø)
crates/consensus/beacon/src/engine/mod.rs 75.48% <93.75%> (+0.20%) ⬆️

... and 9 files with indirect coverage changes

Flag Coverage Δ
integration-tests 15.54% <0.00%> (-0.02%) ⬇️
unit-tests 64.41% <93.75%> (-0.01%) ⬇️

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

Components Coverage Δ
reth binary 26.44% <ø> (ø)
blockchain tree 83.01% <ø> (ø)
pipeline 89.66% <ø> (ø)
storage (db) 74.19% <ø> (ø)
trie 94.65% <ø> (ø)
txpool 46.00% <ø> (ø)
networking 77.64% <ø> (-0.05%) ⬇️
rpc 58.25% <ø> (-0.03%) ⬇️
consensus 64.46% <93.75%> (+0.13%) ⬆️
revm 33.68% <ø> (ø)
payload builder 6.61% <ø> (ø)
primitives 87.99% <ø> (-0.02%) ⬇️

@Rjected
Copy link
Member

Rjected commented Jul 24, 2023

cool - just going to clean up the merge a little bit

@Rjected Rjected force-pushed the make-canonical-latency branch from 5d7c03c to 6a70991 Compare July 24, 2023 20:49
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

great work - thank you!

@Rjected Rjected added this pull request to the merge queue Jul 24, 2023
Merged via the queue into paradigmxyz:main with commit 993b844 Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-engine Related to the engine implementation A-observability Related to tracing, metrics, logs and other observability tools C-enhancement New feature or request M-changelog This change should be included in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Metrics for make_canonical latency when processing forkchoice updates

3 participants