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: stabilize RuntimeMetrics::worker_count #6556

Merged
merged 3 commits into from
May 28, 2024

Conversation

rcoh
Copy link
Contributor

@rcoh rcoh commented May 13, 2024

Motivation

This PR stabilizes a single metric API to start the process of stabilizing metrics. Future work will continue to stabilize more metrics.

We decided against adding a metrics feature and instead metrics will always be available.

I audited the generated docs and the feature gating from docs.rs appears to be correct.

Solution

After a bit of fiddling, I ended up restructuring the metrics macros:

  • Rename cfg_metrics => cfg_unstable_metrics: This behaves like cfg_metrics behaved previously and is gated on tokio_unstable
  • cfg_not_unstable_metrics: Since metrics needs to swap out with a mock module, I needed a final flag to ensure that I was able to do one or the other.

@github-actions github-actions bot added R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR labels May 13, 2024
@rcoh rcoh force-pushed the min-stable-metrics branch 2 times, most recently from 5f2070b to 68daec0 Compare May 13, 2024 19:52
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-metrics Module: tokio/runtime/metrics labels May 14, 2024
@Darksonn
Copy link
Contributor

Remove the draft marker when this is ready for review.

@rcoh rcoh force-pushed the min-stable-metrics branch 4 times, most recently from 59202e0 to 2ec8720 Compare May 14, 2024 17:25
tokio/src/lib.rs Outdated Show resolved Hide resolved
@rcoh rcoh force-pushed the min-stable-metrics branch 5 times, most recently from 519cd54 to 30a3576 Compare May 16, 2024 15:14
@rcoh rcoh marked this pull request as ready for review May 16, 2024 15:16
@rcoh rcoh force-pushed the min-stable-metrics branch 2 times, most recently from 1a9bcc0 to e74e836 Compare May 17, 2024 13:48
Comment on lines 218 to 223
// When running tests in loom, mocks are used instead of metrics.
macro_rules! cfg_metrics {
($($item:item)*) => {
$(
// For now, metrics is only disabled in loom tests.
// When stabilized, it might have a dedicated feature flag.
#[cfg(not(loom))]
$item
Copy link
Contributor

Choose a reason for hiding this comment

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

To what extent can we get rid of this? It's okay if we can't easily do so, but if we can, that would simplify the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I have no idea. I had figured that a bunch of extra atomics was making loom run more slowly or something? I can see what happens if we don't do that. @carllerche do you know why metrics are disabled under loom?

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember. If we don't want metrics to impact loom (which we probably don't) we can just use std atomics unconditionally.

Copy link
Contributor Author

@rcoh rcoh May 20, 2024

Choose a reason for hiding this comment

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

so the reason (?) it was using the loom types is that the loom types work even when AtomicU64 doesn't exist on the platform 🤔 (fallback to Mutex)

I guess the best solution would be to make an AtomicCounter type that fell back to U32 when U64 support wasn't available use AtomicUsize?

I don't think we'd want to stabilize & take a lock for all the metrics on platforms without AtomicU64 support 🤔

Maybe we should keep this as-is in this CR and follow up to remove the loom-gating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was addressed by #6574

/// let metrics = Handle::current().metrics();
/// println!("The runtime has {} workers", metrics.num_workers());
/// }
/// ```
pub fn metrics(&self) -> RuntimeMetrics {
Copy link
Member

Choose a reason for hiding this comment

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

I will raise a question and risk starting a bike shed conversation. Is RuntimeMetrics necessary? It provides no additional functional benefit over just having the methods on Handle it introduces a clone.

The clone isn't the end of the world as we expect that clone to be rare, so I can go either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I do think it's nice if just for the purpose of grouping the methods together away from the other Handle methods.

@@ -22,6 +22,8 @@ use std::future::Future;
use std::marker::PhantomData;
use std::{error, fmt};

use super::RuntimeMetrics;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just add this to the imports on line 3.

Comment on lines +4 to +5
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering::Relaxed;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment about this not being the loom atomic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done — for clarity, could do a followup to create MetricAtomicUsize if we want

Copy link
Contributor

Choose a reason for hiding this comment

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

That would make sense. But yes, let's do it in a follow-up.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thank you.

@Darksonn Darksonn merged commit 86658bd into tokio-rs:master May 28, 2024
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-metrics Module: tokio/runtime/metrics R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants