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(deps): bump metrics to 0.22 #8517

Merged
merged 1 commit into from
May 30, 2024
Merged

chore(deps): bump metrics to 0.22 #8517

merged 1 commit into from
May 30, 2024

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented May 30, 2024

Last hyper dupe in #7018

There's 0.23, but it's blocked by metrics-process not yet being up to date. Should be a trivial follow-up once they update it.

@DaniPopes DaniPopes added the A-dependencies Pull requests or issues that are about dependencies label May 30, 2024
@DaniPopes DaniPopes changed the title chore(deps): bump metrics chore(deps): bump metrics to 0.22 May 30, 2024
Copy link
Collaborator

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

IIRC there was an issue with the global static recorder variable that was changed between versions, and if some dependency uses the old one, we won't record anything from it.

0.21.1: https://github.com/metrics-rs/metrics/blob/0ffcc48cb219ea4664812d97bba8c07ec31515fa/metrics/src/recorder.rs#L87
0.22: https://github.com/metrics-rs/metrics/blob/da3edb3406ec99225c190f47ae09221869161399/metrics/src/recorder/mod.rs#L18

If you checked it works fine for all deps, then LGTM.

@DaniPopes DaniPopes force-pushed the dani/bump-metrics branch 2 times, most recently from 194e9dd to 5b8b9c5 Compare May 30, 2024 16:38
@@ -373,6 +371,12 @@ sha2 = { version = "0.10", default-features = false }
paste = "1.0"
url = "2.3"

# metrics
metrics = "0.22.0"
metrics-exporter-prometheus = { version = "0.14.0", default-features = false }
Copy link
Member Author

@DaniPopes DaniPopes May 30, 2024

Choose a reason for hiding this comment

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

default-features = false disables exporter (see https://docs.rs/metrics-exporter-prometheus/latest/metrics_exporter_prometheus/#features).
We need to disable this because it pulls in openssl through hyper-tls.

We only use the recorder, not the exporter, so this is fine:

let recorder = PrometheusBuilder::new().build_recorder();

@DaniPopes
Copy link
Member Author

DaniPopes commented May 30, 2024

IIRC there was an issue with the global static recorder variable that was changed between versions, and if some dependency uses the old one, we won't record anything from it.

Yeah there's no duplicate metrics dependencies

@DaniPopes DaniPopes enabled auto-merge May 30, 2024 16:46
@DaniPopes DaniPopes disabled auto-merge May 30, 2024 16:46
@DaniPopes DaniPopes force-pushed the dani/bump-metrics branch from 5b8b9c5 to b12703c Compare May 30, 2024 16:51
@DaniPopes DaniPopes enabled auto-merge May 30, 2024 16:52
@DaniPopes DaniPopes added this pull request to the merge queue May 30, 2024
Merged via the queue into main with commit e89e4c9 May 30, 2024
31 checks passed
@DaniPopes DaniPopes deleted the dani/bump-metrics branch May 30, 2024 17:12
mw2000 pushed a commit to mw2000/reth that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Pull requests or issues that are about dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants