-
-
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?
stabilize worker_total_busy_duration #6899
Conversation
…ch and Histogram out of unstable flag
/// Whether the histogram used to aggregate a metric uses a linear or | ||
/// logarithmic scale. | ||
#[derive(Debug, Copy, Clone, Eq, PartialEq)] | ||
#[non_exhaustive] | ||
#[allow(unreachable_pub)] | ||
pub enum HistogramScale { | ||
/// Linear bucket scale | ||
Linear, | ||
|
||
/// Logarithmic bucket scale | ||
#[allow(dead_code)] |
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.
is this change required for the mean time stabilization or just a coincidental change?
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.
I believe this change is required.
At master we have implemented another struct WorkerMetrics
at tokio/src/runtime/metrics/mock.rs
which didn't use struct Histogram
defined at tokio/src/runtime/metrics/histogram.rs
.
In order to stabilize the API, I have to removed this mock WorkerMetrics and instead point to the "real" WorkerMetrics
at tokio/src/runtime/metrics/worker.rs
The "real" WorkerMetrics
has a field poll_count_histogram
which is Option<Histogram>
, and thus will attempt to parse tokio/src/runtime/metrics/histogram.rs
. From there, you can see Histogram and HistogramBuilder both refers to HistogramScale
which is hidden inside tokio_unstable
. I think this wouldn't compile.
(I might be wrong though so feel free to correct me)
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.
I need to read through this PR in more detail so bare with me, but we shouldn't be making this stable and public if it isn't actually needed to to stabilize worker_total_busy_duration
(and I'm mostly sure that it isn't).
In general, all the #[allow(dead_code)]
that are required for this chnage give me the impression that we're exposing something that is only really used in tokio_unstable
and so we should find a way to expose it only unstable tokio_unstable
.
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.
Thanks for your comment!
I think the core issue here is that by exposing worker_total_busy_duration
, we are also exposing the "real" WorkerMetrics
at tokio/src/runtime/metrics/worker.rs
, which in turn will attempt to parse HistogramScale
.
Currently at master branch, when no tokio_unstable
flag is passed in, the WorkerMetrics
at tokio/src/runtime/metrics/mock.rs
will be parsed (which is just an empty struct). And if the flag is passed in, the WorkerMetrics
at tokio/src/runtime/metrics/worker.rs
will be parsed
https://github.com/tokio-rs/tokio/blob/master/tokio/src/runtime/metrics/mod.rs#L27-L40
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.
it's not actually public, it's still behind config_unstable. (Just verified via the autogenerated docs)
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.
Yeah becoz HistogramScale
is still behind cfg_unstable_metrics
:
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.
OK, now I believe I understand.
I would suggest that instead of compiling with the entire worker metrics in order to access only the busy_duration_total
field, we should gate all the fields that we won't be using on cfg_unstable_metrics
. Otherwise users will still be paying the price for metrics which they don't have access to - and that is something that we would really like to avoid.
Have a look at the runtime Builder for an example of how we have fields and implementations that touch them gated on a cfg flag:
tokio/tokio/src/runtime/builder.rs
Lines 125 to 126 in 512e9de
#[cfg(tokio_unstable)] | |
pub(super) unhandled_panic: UnhandledPanic, |
As a general rule, if you need #[allow(dead_code)]
then there is probably something that should be gated on a cfg flag instead.
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.
Thanks @hds . I've reverted changes to histogram.rs
. Also the real Histogram
, HistogramBuilder
and HistogramBatch
are gated behind unstable flag now, and instead I've created a mocked version of these for compilation.
For WorkerMetrics
, as we target to stabilize more metrics, I'd suggest exposing all fields instead of stabilizing only busy_duration_total
and putting other fields behind unstable.
Let me know what you think
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.
@Owen-CH-Leung the problem with stabilizing all of WorkerMetrics
is that when tokio_unstable
is not enabled, the runtime will pay the price for all those metrics, but there will be no way to access them. For this reason I think that it would be better to only stabilize what we're exposing.
tokio/src/runtime/metrics/runtime.rs
Outdated
pub fn worker_total_busy_duration(&self, worker: usize) -> Duration { | ||
let nanos = self | ||
.handle | ||
.inner | ||
.worker_metrics(worker) | ||
.busy_duration_total | ||
.load(Relaxed); | ||
Duration::from_nanos(nanos) | ||
} |
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.
formatting issues in here—autoformat doesn't run because of the parent macro
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.
Thanks. Fixed the formatting
tokio/src/runtime/scheduler/mod.rs
Outdated
@@ -9,6 +9,8 @@ cfg_rt! { | |||
pub(crate) use inject::Inject; | |||
|
|||
use crate::runtime::TaskHooks; | |||
|
|||
use crate::runtime::{WorkerMetrics}; |
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.
use crate::runtime::{WorkerMetrics}; | |
use crate::runtime::WorkerMetrics; |
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.
Thanks & fixed.
tokio/src/runtime/scheduler/mod.rs
Outdated
} | ||
|
||
cfg_unstable_metrics! { | ||
use crate::runtime::{SchedulerMetrics, WorkerMetrics}; | ||
use crate::runtime::{SchedulerMetrics}; |
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.
use crate::runtime::{SchedulerMetrics}; | |
use crate::runtime::SchedulerMetrics; |
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.
Thanks & fixed.
|
||
cfg_unstable_metrics! { | ||
use crate::runtime::{SchedulerMetrics, WorkerMetrics}; | ||
use crate::runtime::{SchedulerMetrics}; |
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.
use crate::runtime::{SchedulerMetrics}; | |
use crate::runtime::SchedulerMetrics; |
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.
Thanks & fixed.
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.
Please bare with me as I haven't finished the review, but my first impression is that this PR is making more things public than is strictly necessary for the stabilization of worker_total_busy_duration
and we should try to avoid that.
This is especially true since #6897 is also touching the histogram (although not taking it out of tokio_unstable
) and that would be a breaking change if this PR is released first.
/// Whether the histogram used to aggregate a metric uses a linear or | ||
/// logarithmic scale. | ||
#[derive(Debug, Copy, Clone, Eq, PartialEq)] | ||
#[non_exhaustive] | ||
#[allow(unreachable_pub)] | ||
pub enum HistogramScale { | ||
/// Linear bucket scale | ||
Linear, | ||
|
||
/// Logarithmic bucket scale | ||
#[allow(dead_code)] |
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.
I need to read through this PR in more detail so bare with me, but we shouldn't be making this stable and public if it isn't actually needed to to stabilize worker_total_busy_duration
(and I'm mostly sure that it isn't).
In general, all the #[allow(dead_code)]
that are required for this chnage give me the impression that we're exposing something that is only really used in tokio_unstable
and so we should find a way to expose it only unstable tokio_unstable
.
…nges on histogram.rs and guard it behind unstable flag
Motivation
Stabilize worker_total_busy_duration so it can be used outside of --cfg tokio_unstable
Solution
Move the API impl out of tokio_unstable flag
Ref: #6546