-
-
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
metrics: stabilize num_alive_tasks #6619
Conversation
Since #6114 has been merged, we need to resolve conflict here. |
3542c59
to
0d087ef
Compare
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.
Looks good to me.
tokio/src/runtime/metrics/runtime.rs
Outdated
/// | ||
/// This value increases and decreases over time as tasks are spawned and as they are completed or cancelled. | ||
/// | ||
/// To see the total number of spawned tasks, see `spawned_tasks_count`. Note that this API currently requires using `--cfg 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.
If this is being stabilized, then the documentation should no longer say that it's unstable.
Ah that comment was attempting to refer to spawned_task_count
…On Sat, Jun 15, 2024, 3:06 PM Alice Ryhl ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tokio/src/runtime/metrics/runtime.rs
<#6619 (comment)>:
> @@ -47,6 +47,29 @@ impl RuntimeMetrics {
self.handle.inner.num_workers()
}
+ /// Returns the current number of active tasks in the runtime.
+ ///
+ /// This value increases and decreases over time as tasks are spawned and as they are completed or cancelled.
+ ///
+ /// To see the total number of spawned tasks, see `spawned_tasks_count`. Note that this API currently requires using `--cfg tokio_unstable`.
If this is being stabilized, then the documentation should no longer say
that it's unstable.
—
Reply to this email directly, view it on GitHub
<#6619 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADYKZZO5E3NIQNX465VC7LZHSGEFAVCNFSM6AAAAABI5FHB4SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMRQG44TGNZYHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Okay. It's a bit confusing like this. You don't need to say that it's unstable in the docs for other methods. |
0d087ef
to
c2b7d9a
Compare
I'm sorry for responding slowly here. I'm wondering whether the name should be something else. The word "active" is the opposite of "idle", but the count includes idle tasks that have not yet exited. How about a name like |
runnable? That has some issues too. I wonder if this would be helped with a
TaskMetrics struct that could give some docs to explain the different task
states & group the metrics.
…On Sun, Jun 30, 2024, 4:46 AM Alice Ryhl ***@***.***> wrote:
I'm sorry for responding slowly here.
I'm wondering whether the name should be something else. The word "active"
is the opposite of "idle", but the count includes idle tasks that have not
yet exited. How about a name like alive_tasks_count?
—
Reply to this email directly, view it on GitHub
<#6619 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADYKZ5ICVVOKM5II2OKRUTZJ7OXTAVCNFSM6AAAAABI5FHB4SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJYGUYTKNJRGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I ended up renaming it in #6667. |
I don't think runnable works either. That also implies that idle tasks shouldn't be counted, but they are. |
1. stabilize `num_active_tasks` 2. Rename internal counters to match `num_active_tasks` 3. Split `rt_metrics` into `rt_metrics` and `rt_unstable_metrics`.
c2b7d9a
to
0fd010f
Compare
0fd010f
to
fa322a0
Compare
Sorry for the delay on this. @Darksonn I've rebased. I noticed there was a mix between |
That makes sense. The publicly used name is |
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.
Motivation
Stabilize
num_active_tasks
num_alive_tasks
so it can be used outside of--cfg tokio_unstable
Solution
num_alive_tasks
num_alive_tasks
rt_metrics
intort_metrics
andrt_unstable_metrics
.Refs: #6546