metrics: stabilize RuntimeMetrics::worker_count
#6556
Conversation
github-actions
bot
added
R-loom-current-thread
rcoh
force-pushed
the
min-stable-metrics
branch
2 times, most recently
from
5f2070b
to
68daec0
Compare
Darksonn
added
A-tokio
|
Remove the draft marker when this is ready for review. |
rcoh
force-pushed
the
min-stable-metrics
branch
4 times, most recently
from
59202e0
to
2ec8720
Compare
rcoh
force-pushed
the
min-stable-metrics
branch
5 times, most recently
from
519cd54
to
30a3576
Compare
rcoh
force-pushed
the
min-stable-metrics
branch
2 times, most recently
from
1a9bcc0
to
e74e836
Compare
tokio/src/macros/cfg.rs
Outdated
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
This was addressed by #6574
tokio/src/runtime/handle.rs
Outdated
| /// let metrics = Handle::current().metrics(); | ||
| /// println!("The runtime has {} workers", metrics.num_workers()); | ||
| /// } | ||
| /// ``` | ||
| pub fn metrics(&self) -> RuntimeMetrics { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Well, I do think it's nice if just for the purpose of grouping the methods together away from the other Handle methods.
tokio/src/runtime/handle.rs
Outdated
| @@ -22,6 +22,8 @@ use std::future::Future; | |||
| use std::marker::PhantomData; | |||
| use std::{error, fmt}; | |||
|
|
|||
| use super::RuntimeMetrics; | |||
There was a problem hiding this comment.
You can just add this to the imports on line 3.
| use std::sync::atomic::AtomicUsize; | ||
| use std::sync::atomic::Ordering::Relaxed; |
There was a problem hiding this comment.
Please add a comment about this not being the loom atomic.
There was a problem hiding this comment.
done — for clarity, could do a followup to create MetricAtomicUsize if we want
There was a problem hiding this comment.
That would make sense. But yes, let's do it in a follow-up.
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
metricsfeature 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:
cfg_metrics=>cfg_unstable_metrics: This behaves likecfg_metricsbehaved previously and is gated ontokio_unstablecfg_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.