-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
stabilize worker_total_busy_duration #6899
base: master
Are you sure you want to change the base?
Changes from 12 commits
8f1fcb4
9b47cf9
e2f4f33
b6974ba
86f019f
64f626d
489003c
8a134a2
4bc00cf
7eb6b97
7239af5
57f6b9b
c8b2c7d
d7333cf
14543de
245d75c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ cfg_unstable_metrics! { | |
|
||
mod histogram; | ||
pub(crate) use histogram::{Histogram, HistogramBatch, HistogramBuilder}; | ||
|
||
#[allow(unreachable_pub)] // rust-lang/rust#57411 | ||
pub use histogram::{HistogramScale, HistogramConfiguration, LogHistogram, LogHistogramBuilder, InvalidHistogramConfiguration}; | ||
|
||
|
@@ -34,7 +35,9 @@ cfg_unstable_metrics! { | |
} | ||
|
||
cfg_not_unstable_metrics! { | ||
mod mock; | ||
mod worker; | ||
pub(crate) use worker::WorkerMetrics; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please move the modules and imports that are in both the macro blocks out above them, we don't need to gate them at all. |
||
|
||
pub(crate) use mock::{SchedulerMetrics, WorkerMetrics, MetricsBatch, HistogramBuilder}; | ||
mod mock; | ||
pub(crate) use mock::{SchedulerMetrics, MetricsBatch, HistogramBuilder}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,13 @@ | ||
use crate::runtime::metrics::Histogram; | ||
use crate::runtime::Config; | ||
use crate::util::metric_atomics::{MetricAtomicU64, MetricAtomicUsize}; | ||
use std::sync::atomic::Ordering::Relaxed; | ||
use std::sync::Mutex; | ||
use std::thread::ThreadId; | ||
|
||
cfg_unstable_metrics! { | ||
use crate::runtime::metrics::Histogram; | ||
} | ||
|
||
/// Retrieve runtime worker metrics. | ||
/// | ||
/// **Note**: This is an [unstable API][unstable]. The public API of this type | ||
|
@@ -15,40 +18,60 @@ use std::thread::ThreadId; | |
#[derive(Debug, Default)] | ||
#[repr(align(128))] | ||
pub(crate) struct WorkerMetrics { | ||
#[cfg(tokio_unstable)] | ||
#[cfg_attr(docsrs, doc(cfg(tokio_unstable)))] | ||
/// Number of times the worker parked. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this necessary? Since this isn't a public method, it won't appear in the documentation. |
||
pub(crate) park_count: MetricAtomicU64, | ||
|
||
#[cfg(tokio_unstable)] | ||
#[cfg_attr(docsrs, doc(cfg(tokio_unstable)))] | ||
/// Number of times the worker parked and unparked. | ||
pub(crate) park_unpark_count: MetricAtomicU64, | ||
|
||
#[cfg(tokio_unstable)] | ||
#[cfg_attr(docsrs, doc(cfg(tokio_unstable)))] | ||
/// Number of times the worker woke then parked again without doing work. | ||
pub(crate) noop_count: MetricAtomicU64, | ||
|
||
#[cfg(tokio_unstable)] | ||
#[cfg_attr(docsrs, doc(cfg(tokio_unstable)))] | ||
/// Number of tasks the worker stole. | ||
pub(crate) steal_count: MetricAtomicU64, | ||
|
||
#[cfg(tokio_unstable)] | ||
#[cfg_attr(docsrs, doc(cfg(tokio_unstable)))] | ||
/// Number of times the worker stole | ||
pub(crate) steal_operations: MetricAtomicU64, | ||
|
||
#[cfg(tokio_unstable)] | ||
#[cfg_attr(docsrs, doc(cfg(tokio_unstable)))] | ||
/// Number of tasks the worker polled. | ||
pub(crate) poll_count: MetricAtomicU64, | ||
|
||
#[cfg(tokio_unstable)] | ||
#[cfg_attr(docsrs, doc(cfg(tokio_unstable)))] | ||
/// EWMA task poll time, in nanoseconds. | ||
pub(crate) mean_poll_time: MetricAtomicU64, | ||
|
||
/// Amount of time the worker spent doing work vs. parking. | ||
pub(crate) busy_duration_total: MetricAtomicU64, | ||
|
||
#[cfg(tokio_unstable)] | ||
#[cfg_attr(docsrs, doc(cfg(tokio_unstable)))] | ||
/// Number of tasks scheduled for execution on the worker's local queue. | ||
pub(crate) local_schedule_count: MetricAtomicU64, | ||
|
||
#[cfg(tokio_unstable)] | ||
#[cfg_attr(docsrs, doc(cfg(tokio_unstable)))] | ||
/// Number of tasks moved from the local queue to the global queue to free space. | ||
pub(crate) overflow_count: MetricAtomicU64, | ||
|
||
/// Number of tasks currently in the local queue. Used only by the | ||
/// current-thread scheduler. | ||
pub(crate) queue_depth: MetricAtomicUsize, | ||
|
||
#[cfg(tokio_unstable)] | ||
#[cfg_attr(docsrs, doc(cfg(tokio_unstable)))] | ||
/// If `Some`, tracks the number of polls by duration range. | ||
pub(super) poll_count_histogram: Option<Histogram>, | ||
|
||
|
@@ -57,29 +80,41 @@ pub(crate) struct WorkerMetrics { | |
} | ||
|
||
impl WorkerMetrics { | ||
pub(crate) fn from_config(config: &Config) -> WorkerMetrics { | ||
let mut worker_metrics = WorkerMetrics::new(); | ||
worker_metrics.poll_count_histogram = config | ||
.metrics_poll_count_histogram | ||
.as_ref() | ||
.map(|histogram_builder| histogram_builder.build()); | ||
worker_metrics | ||
cfg_unstable_metrics! { | ||
pub(crate) fn from_config(config: &Config) -> WorkerMetrics { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be better if we grouped all the unstable functions together at the bottom of the |
||
let mut worker_metrics = WorkerMetrics::new(); | ||
worker_metrics.poll_count_histogram = config | ||
.metrics_poll_count_histogram | ||
.as_ref() | ||
.map(|histogram_builder| histogram_builder.build()); | ||
worker_metrics | ||
} | ||
} | ||
|
||
cfg_not_unstable_metrics! { | ||
pub(crate) fn from_config(_: &Config) -> WorkerMetrics { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's move this down to be right above the |
||
WorkerMetrics::new() | ||
} | ||
} | ||
|
||
pub(crate) fn new() -> WorkerMetrics { | ||
WorkerMetrics::default() | ||
} | ||
|
||
pub(crate) fn queue_depth(&self) -> usize { | ||
self.queue_depth.load(Relaxed) | ||
cfg_unstable_metrics! { | ||
pub(crate) fn queue_depth(&self) -> usize { | ||
self.queue_depth.load(Relaxed) | ||
} | ||
} | ||
|
||
pub(crate) fn set_queue_depth(&self, len: usize) { | ||
self.queue_depth.store(len, Relaxed); | ||
} | ||
|
||
pub(crate) fn thread_id(&self) -> Option<ThreadId> { | ||
*self.thread_id.lock().unwrap() | ||
cfg_unstable_metrics! { | ||
pub(crate) fn thread_id(&self) -> Option<ThreadId> { | ||
*self.thread_id.lock().unwrap() | ||
} | ||
} | ||
|
||
pub(crate) fn set_thread_id(&self, thread_id: ThreadId) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly that this function duplicates part of the
submit
function inbatch::MetricsBatch
?I think this is a problematic way of gradually stabilizing metrics, as it opens the possibility of having divirging implementations if a change is made to the "real"
MetricsBatch
by someone who doesn't realise that there is another one.This is additionally confusing because this effectively becomes the "stable" implementation, but it lives in a module called
mock
.I would propose that we instead split the
metrics::MetricsBatch
implementation into stable (always compiles) and unstable (gated bycfg
option), the same way we've done elsewhere in this PR. The same as with another comment, we would group all the unstable functions into a singlecfg_unstable_metrics!
block.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed spliting
metrics::MetricsBatch
is a much viable way of stabilising. I've adopted your suggestion and split it into stable & unstable (and group unstable functions into a single unstable block. Thanks a lot for reviewing!