Skip to content

feat: metric of reorg depth of blockchain tree#3860

Merged
Rjected merged 1 commit intoparadigmxyz:mainfrom
int88:reorg-depth
Jul 26, 2023
Merged

feat: metric of reorg depth of blockchain tree#3860
Rjected merged 1 commit intoparadigmxyz:mainfrom
int88:reorg-depth

Conversation

@int88
Copy link
Contributor

@int88 int88 commented Jul 20, 2023

fixes part of #3817

@int88
Copy link
Contributor Author

int88 commented Jul 20, 2023

test result:

root@test:~# curl http://127.0.0.1:9001 | grep latest_reorg
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 27701  100 27701    0     0  13.2M      0 --:--:-- --:--:-- --:--:-- 13.2M
# HELP reth_blockchain_tree_latest_reorg_depth The latest reorg depth
# TYPE reth_blockchain_tree_latest_reorg_depth gauge
reth_blockchain_tree_latest_reorg_depth 0

@Rjected PTAL

@onbjerg onbjerg added C-enhancement New feature or request A-observability Related to tracing, metrics, logs and other observability tools A-engine Related to the engine implementation labels Jul 24, 2023
@onbjerg
Copy link
Collaborator

onbjerg commented Jul 24, 2023

cc @rkrasiuk / @Rjected / @rakita PTAL

@onbjerg onbjerg added the M-changelog This change should be included in the changelog label Jul 24, 2023
@rkrasiuk
Copy link
Contributor

hmm, i'd rather use a Histogram and plot it as a histogram over time on the dashboard. curious to hear your thoughts @onbjerg

@onbjerg
Copy link
Collaborator

onbjerg commented Jul 24, 2023

hmm, i'd rather use a Histogram and plot it as a histogram over time on the dashboard. curious to hear your thoughts @onbjerg

Not sure wym here, histogram support in the metrics crate we use is kind of not the best, I don't remember the exact issue, but the buckets are not configurable, so might not be worth? Up to you

@Rjected
Copy link
Member

Rjected commented Jul 24, 2023

It would be good to know the specific deficiencies with Histogram since it's also relevant to this PR: #3865

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.

nit: could we add a method on the tree for updating both reorg metrics? It could have depth as a parameter

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #3860 (3016091) into main (736de20) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

Files Changed Coverage Δ
crates/blockchain-tree/src/metrics.rs 100.00% <ø> (ø)
crates/blockchain-tree/src/blockchain_tree.rs 83.37% <100.00%> (+0.08%) ⬆️
crates/storage/provider/src/chain.rs 74.41% <100.00%> (+0.30%) ⬆️

... and 9 files with indirect coverage changes

Flag Coverage Δ
integration-tests 15.48% <0.00%> (-0.01%) ⬇️
unit-tests 64.50% <100.00%> (-0.01%) ⬇️

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

Components Coverage Δ
reth binary 27.24% <ø> (ø)
blockchain tree 83.04% <100.00%> (+0.03%) ⬆️
pipeline 89.68% <ø> (ø)
storage (db) 74.20% <100.00%> (+<0.01%) ⬆️
trie 94.70% <ø> (ø)
txpool 46.00% <ø> (ø)
networking 77.63% <ø> (-0.07%) ⬇️
rpc 58.38% <ø> (ø)
consensus 64.46% <ø> (ø)
revm 33.68% <ø> (ø)
payload builder 6.61% <ø> (ø)
primitives 88.04% <ø> (-0.02%) ⬇️

@int88
Copy link
Contributor Author

int88 commented Jul 26, 2023

nit: could we add a method on the tree for updating both reorg metrics? It could have depth as a parameter

@Rjected updated!

Copy link
Collaborator

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

lgtm, @Rjected ?

@onbjerg onbjerg requested a review from Rjected July 26, 2023 16:47
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.

nice! lgtm

@Rjected Rjected added this pull request to the merge queue Jul 26, 2023
Merged via the queue into paradigmxyz:main with commit b5a44ae Jul 26, 2023
This was referenced Nov 14, 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.

4 participants