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.

Sign up for GitHub

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

Jump to bottom

Update metrics to version 0.17 #2937

Closed
wants to merge 7 commits into from
Closed

Update metrics to version 0.17 #2937

wants to merge 7 commits into from

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Oct 21, 2021

Motivation

This is part of the update to use Tokio version 1 (#2200), and must be merged together with the other PRs.

The version of the metrics crate used by Zebra doesn't support the new version of Tokio. More specifically, metrics depends on Hyper 0.14, which has the same issue. Just like in #2936, either a new version of the runtime must run beside the old version of the runtime, or the dependency has to be updated.

Solution

Update metrics to 0.17.0. This also requires the metrics-exporter-prometheus to be updated to 0.6.1.

Review

Anyone from the team can review this, but it shouldn't be merged yet.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

@jvff jvff requested a review from a team October 21, 2021 22:16
@jvff jvff self-assigned this Oct 21, 2021
teor2345
teor2345 reviewed Oct 21, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Let's just use the new String support in metrics.

This is a blocking change, because we risk breaking some metrics by manually tweaking expanded macro code.

zebrad/src/components/metrics.rs Outdated Show resolved Hide resolved
@jvff jvff force-pushed the update-to-metrics-0.17 branch from 2dec967 to 997954b Compare October 22, 2021 21:52
@jvff jvff requested a review from teor2345 October 25, 2021 20:22
teor2345
teor2345 reviewed Oct 25, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Let's delete an outdated comment.

zebrad/src/components/metrics.rs Outdated Show resolved Hide resolved
@dconnolly dconnolly self-requested a review October 25, 2021 21:42
@jvff jvff force-pushed the update-to-metrics-0.17 branch from 997954b to ba31a5b Compare October 26, 2021 12:36
teor2345
teor2345 suggested changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This version update is incomplete - other crates also use metrics.

For example:

zebra/zebra-network/Cargo.toml

Line 31 in a166964

metrics = "0.13.0-alpha.8"

This is a merge blocker.

Please check for similar missed updates in other PRs.
You might find cargo deny check bans useful here:
https://embarkstudios.github.io/cargo-deny/checks/bans/index.html#use-case---duplicate-version-detection

zebrad/src/components/metrics.rs Outdated Show resolved Hide resolved
@mpguerra mpguerra mentioned this pull request Oct 29, 2021
Closed
10 tasks
jvff added 5 commits October 29, 2021 14:10
@jvff
Update metrics dependency to version 0.17
7ea866d 
And also update the `metrics-exporter-prometheus` to version 0.6.1.
These updates are to make sure Tokio 1 is supported.
@jvff
Remove unnecessary conversion
265d6e2 
Suggested by a Clippy lint.
@jvff
Use f64 as the histogram data type
3537c30 
`u64` isn't supported as the histogram data type in newer versions of
`metrics`.
@jvff
Simplify build version counter
a8725db 
Remove all constants and use the new `metrics::incement_counter!` macro.
@jvff
Change metrics output line to match on
5dd8d4d 
The snapshot string isn't included in the newer version of
`metrics-exporter-prometheus`.
@jvff jvff force-pushed the update-to-metrics-0.17 branch from ba31a5b to 5dd8d4d Compare October 29, 2021 14:10
teor2345
teor2345 suggested changes Nov 1, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This PR is missing metrics updates in some Zebra crates.
This is a merge blocker.

@jvff jvff mentioned this pull request Nov 1, 2021
Closed
3 tasks
@jvff
Update metrics dependency from other crates
ea71cec 
Update other `zebra` crates so that only one version of `metrics` is
used.
@jvff
Copy link
Contributor Author

jvff commented Nov 1, 2021

This PR is missing metrics updates in some Zebra crates. This is a merge blocker.

Oops, I had fixed this in my full-update-to-tokio-1.12.0 branch but forgot to cherry-pick into this PR :(

@jvff jvff requested a review from teor2345 November 1, 2021 17:46
@jvff jvff mentioned this pull request Nov 1, 2021
Merged
3 tasks
@jvff
Include patch version for metrics dependency
c706584 
Use a strict version specification.
teor2345
teor2345 approved these changes Nov 1, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

The dependencies seem fine here.

cargo deny check bans will find any duplicates as long as we put PR #2987 in the rollup PR.

dconnolly
dconnolly approved these changes Nov 2, 2021
View reviewed changes
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

🎉

@dconnolly
Copy link
Contributor

Once this is merged, we will be able to delete https://github.com/ZcashFoundation/metrics 🎉

@jvff
Copy link
Contributor Author

jvff commented Nov 2, 2021

Merged through #2994.

@jvff jvff closed this Nov 2, 2021
@teor2345
Copy link
Contributor

teor2345 commented Nov 3, 2021

Once this is merged, we will be able to delete https://github.com/ZcashFoundation/metrics 🎉

I archived the ZcashFoundation/metrics repository, so that builds of older Zebra releases would continue working.

If we need to use it again, we should rename the main branch, so we keep the commits that older Zebra releases depend on.

@jvff jvff deleted the update-to-metrics-0.17 branch November 24, 2021 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dconnolly dconnolly dconnolly approved these changes

@teor2345 teor2345 teor2345 approved these changes

Assignees

@jvff jvff

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jvff @dconnolly @teor2345