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

chore: bump metrics #4265

Merged
merged 4 commits into from
Aug 18, 2023
Merged

chore: bump metrics #4265

merged 4 commits into from
Aug 18, 2023

Conversation

shekhirin
Copy link
Collaborator

Problem

metrics-process metrics doesn't have a reth_ prefix.

Root cause

metrics-process dependency was updated to latest version in #3988, and with it the metrics dependency made a bump

reth/Cargo.lock

Line 3966 in efab153

"metrics 0.21.1",

while other crates depending on metrics continued to use older version

reth/Cargo.lock

Line 5692 in efab153

"metrics 0.20.1",

The problem with this is that metrics-macro uses the global RECORDER variable which we configure using older version of metrics crate

// Build metrics stack
Stack::new(recorder)
.push(PrefixLayer::new("reth"))
.install()
.wrap_err("Couldn't set metrics recorder.")?;
but newer version (which metrics-process uses) doesn't see it.

Solution

Use the same version of metrics dependency everywhere, so bump it to latest 0.21.1. The tricky part is this change metrics-rs/metrics#358, which required defining the metrics dependency on the crate level (https://doc.rust-lang.org/reference/paths.html#path-qualifiers).

@shekhirin shekhirin changed the title Alexey/process metrics prefix fix(metrics): bump metrics Aug 18, 2023
@shekhirin shekhirin changed the title fix(metrics): bump metrics chore: bump metrics Aug 18, 2023
@shekhirin shekhirin marked this pull request as ready for review August 18, 2023 12:30
@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #4265 (7a32e44) into main (efab153) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

Files Changed Coverage Δ
bin/reth/src/prometheus_exporter.rs 0.00% <ø> (ø)
crates/blockchain-tree/src/metrics.rs 100.00% <ø> (ø)
...tes/consensus/beacon/src/engine/invalid_headers.rs 73.68% <ø> (ø)
crates/consensus/beacon/src/engine/metrics.rs 100.00% <ø> (ø)
crates/net/downloaders/src/metrics.rs 33.33% <ø> (ø)
crates/net/eth-wire/src/p2pstream.rs 80.47% <ø> (+0.16%) ⬆️
crates/net/network/src/metrics.rs 61.53% <ø> (ø)
crates/payload/basic/src/metrics.rs 0.00% <ø> (ø)
crates/payload/builder/src/metrics.rs 18.18% <ø> (ø)
crates/rpc/rpc-builder/src/metrics.rs 100.00% <ø> (+2.00%) ⬆️
... and 5 more

... and 4 files with indirect coverage changes

Flag Coverage Δ
integration-tests 16.90% <ø> (-0.01%) ⬇️
unit-tests 63.75% <ø> (-0.01%) ⬇️

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

Components Coverage Δ
reth binary 26.13% <ø> (ø)
blockchain tree 82.56% <ø> (ø)
pipeline 90.07% <ø> (ø)
storage (db) 74.71% <ø> (ø)
trie 94.71% <ø> (ø)
txpool 49.05% <ø> (ø)
networking 77.51% <ø> (-0.02%) ⬇️
rpc 58.64% <ø> (+<0.01%) ⬆️
consensus 63.53% <ø> (ø)
revm 32.15% <ø> (ø)
payload builder 6.82% <ø> (ø)
primitives 86.31% <ø> (ø)

@shekhirin shekhirin force-pushed the alexey/process-metrics-prefix branch 2 times, most recently from b03ad73 to 7a32e44 Compare August 18, 2023 14:44
@shekhirin shekhirin added this pull request to the merge queue Aug 18, 2023
Merged via the queue into main with commit 2904745 Aug 18, 2023
48 checks passed
@shekhirin shekhirin deleted the alexey/process-metrics-prefix branch August 18, 2023 15:07
@shekhirin shekhirin added C-bug An unexpected or incorrect behavior A-observability Related to tracing, metrics, logs and other observability tools labels Aug 18, 2023
PlamenHristov pushed a commit to PlamenHristov/reth that referenced this pull request Aug 19, 2023
alessandromazza98 pushed a commit to alessandromazza98/reth that referenced this pull request Aug 19, 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.

2 participants