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

fix(bin): process metrics version mismatch #5985

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

shekhirin
Copy link
Collaborator

@shekhirin shekhirin commented Jan 9, 2024

// Dependencies using different version of the `metrics` crate (to be exact, 0.21 vs 0.22)
// may not be able to communicate with each other through the global recorder.

@shekhirin shekhirin added C-bug An unexpected or incorrect behavior A-observability Related to tracing, metrics, logs and other observability tools A-cli Related to the reth CLI labels Jan 9, 2024
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.

I like the test, but this should probably be used to fix non-nextest test runs:

/// The default prometheus recorder handle. We use a global static to ensure that it is only
/// installed once.
pub static PROMETHEUS_RECORDER_HANDLE: Lazy<PrometheusHandle> =
Lazy::new(|| prometheus_exporter::install_recorder().unwrap());

Comment on lines 253 to 255
fn process_metrics() {
let handle = install_recorder().unwrap();

Copy link
Member

@Rjected Rjected Jan 9, 2024

Choose a reason for hiding this comment

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

fails if running with cargo test --workspace --tests:

failures:
    cli::tests::parse_env_filter_directives
    prometheus_exporter::tests::process_metrics

reason: recorder installed twice

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.

lgtm, unblocking for release - other test failures are unrelated to this PR

@shekhirin shekhirin added this pull request to the merge queue Jan 9, 2024
Merged via the queue into main with commit a1225be Jan 9, 2024
28 checks passed
@shekhirin shekhirin deleted the alexey/process-metrics-fix branch January 9, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Related to the reth CLI 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.

3 participants