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

Metrics regression: some metrics don't appear in the exporter #1349

Closed
hdevalence opened this issue Nov 21, 2020 · 3 comments · Fixed by #1561
Closed

Metrics regression: some metrics don't appear in the exporter #1349

hdevalence opened this issue Nov 21, 2020 · 3 comments · Fixed by #1561
Assignees
Labels
C-bug Category: This is a bug S-needs-investigation Status: Needs further investigation

Comments

@hdevalence
Copy link
Contributor

Version

main

Description

After #1330 I noticed a problem where a bunch of metrics didn't show up. I'm not sure if it's connected, or a local misconfiguration on my part.

That change bumped the metrics version in order to pick up a new version of the exporter, because the old version used an old version of hyper that expected to run on the Tokio 0.2 runtime. The new version builds its own single-threaded runtime for the exporter and launches it on a worker thread.

If we have problems like this, we can roll back to the previous version of the metrics crate, and avoid the Tokio incompatibilities by copying the worker thread code from the current prometheus exporter but using it to launch a single-threaded Tokio 0.2 runtime for the old metrics exporter. This is probably faster than trynig to spend a lot of time debugging metrics issues right now.

@hdevalence hdevalence added C-bug Category: This is a bug S-needs-triage Status: A bug report needs triage labels Nov 21, 2020
@teor2345
Copy link
Contributor

I am also seeing these issues, they seem to affect (some) metrics which are updated within async blocks.

@hdevalence
Copy link
Contributor Author

I think the best fix for this would be to do the rollback sketched above, but I think it might be slightly lower priority than other bugs that block the sync process, since it only affects observability.

@mpguerra mpguerra added this to the v1.0.0-alpha.1 milestone Dec 9, 2020
@teor2345 teor2345 added the S-needs-investigation Status: Needs further investigation label Dec 17, 2020
@teor2345 teor2345 changed the title Potential metrics regression Metrics regression Jan 5, 2021
@teor2345 teor2345 changed the title Metrics regression Metrics regression: some metrics don't appear in the exporter Jan 5, 2021
@yaahc yaahc self-assigned this Jan 5, 2021
@yaahc
Copy link
Contributor

yaahc commented Jan 7, 2021

I'm betting good money on this being the culprit

$ cargo tree -d

metrics v0.12.1
├── zebra-consensus v1.0.0-alpha.0 (/home/jlusby/git/zfnd/zebra/zebra-consensus) (*)
├── zebra-network v1.0.0-alpha.0 (/home/jlusby/git/zfnd/zebra/zebra-network) (*)
└── zebra-state v1.0.0-alpha.0 (/home/jlusby/git/zfnd/zebra/zebra-state) (*)

metrics v0.13.0-alpha.8 (https://github.com/ZcashFoundation/metrics?rev=971133128e5aebe3ad177acffc6154449736cfa2#97113312)
├── metrics-exporter-prometheus v0.1.0-alpha.7 (https://github.com/ZcashFoundation/metrics?rev=971133128e5aebe3ad177acffc6154449736cfa2#97113312) (*)
├── metrics-util v0.4.0-alpha.6 (https://github.com/ZcashFoundation/metrics?rev=971133128e5aebe3ad177acffc6154449736cfa2#97113312) (*)
└── zebrad v1.0.0-alpha.0 (/home/jlusby/git/zfnd/zebra/zebrad)

There are actually quite a few duplicates in our dependency tree. I'm gonna start with trying to unify the metrics dependencies and then I'll probably go ahead and clean up as many duplicates as I possibly can.

yaahc added a commit that referenced this issue Jan 12, 2021
## Motivation

This PR is motivated by the regression identified in #1349. That PR notes that the metrics stopped working for most of the crates other than `zebrad`.

## Solution

This PR resolves the regression by deduplicating the `metrics` crate dependency. During a recent change we upgraded the metrics version in `zebrad` and a couple other of our crates, but we never updated the dependencies in `zebra-state`, `zebra-consensus`, or `zebra-network`. This caused the metrics macros to attempt to retrieve the current metrics exporter through the wrong function. We would install the metrics exporter in `0.13`, but then attempt to look it up through the `0.12` crate, which contains a different instance of the metrics exporter static variable which is unset. Doing this causes the metrics macros to return `None` for the current exporter after which they just silently give up.

## Related Issues

closes #1349

## Follow Up Work

I noticed we have quite a few duplicate dependencies in our tree. We might be able to save some compilation time by auditing those and deduplicating them as much as possible.

- #1582
Co-authored-by: teor <teor@riseup.net>
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug S-needs-investigation Status: Needs further investigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants