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

Fix the description of steal_count #35

Merged
merged 2 commits into from Feb 5, 2023

Conversation

jschwe
Copy link
Contributor

@jschwe jschwe commented Dec 31, 2022

  • steal_count falsely claimed to be the number of times tasks were stolen, but was actually the number of tasks stolen. Adjust the documentation to reflect this fact
  • Expose the new steal_operations metric, which tracks what was previously documented to be steal_count. This requires #5330, so I guess it might make sense for me to split that part into a separate PR since it requires bumping the minimum supported tokio version. Edit: Removed this part and kept only the documentation changes.

@LucioFranco
Copy link
Member

Yeah probably makes sense to only do the stuff we can support right now aka the doc fixes and the rest can go in anothe PR.

`steal_count` falsely claimed to be the number of times tasks where
stolen, but was actually the number of tasks stolen.
The description is wrong for all versions of tokio since at least the
minimum tokio version tokio-metrics currently supports (v1.15).
See: https://github.com/tokio-rs/tokio/blob/f64673580dfc649954eb744eb2734f2f118baa47/tokio/src/runtime/queue.rs#L323
@jschwe
Copy link
Contributor Author

jschwe commented Jan 4, 2023

Updated the PR to only include the documentation changes.

@jschwe
Copy link
Contributor Author

jschwe commented Jan 27, 2023

The documentation fix was merged into tokio in tokio-rs/tokio#5330, so this PR should be ready now too.
I also added a fix for clippy, which is unrelated to my PR and probably from a new clippy lint.

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.

Thanks.

@Darksonn
Copy link
Contributor

Darksonn commented Feb 3, 2023

There are two test failures, but its not clear whether they're due to your change or just a flaky test. I will rerun the tests now, so we can see whether it's consistent.

failures:

---- src/task.rs - task::TaskMetrics::mean_scheduled_duration (line 1924) stdout ----
Test executable failed (exit status: 101).

stderr:
thread 'main' panicked at 'assertion failed: interval.mean_scheduled_duration() >= Duration::from_secs(1)', src/task.rs:34:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/panicking.rs:64:14
   2: core::panicking::panic
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/panicking.rs:111:5
   3: rust_out::main::{{closure}}
   4: <core::pin::Pin<P> as core::future::future::Future>::poll
   5: tokio::runtime::scheduler::current_thread::CoreGuard::block_on::{{closure}}::{{closure}}::{{closure}}
   6: tokio::runtime::scheduler::current_thread::CoreGuard::block_on::{{closure}}::{{closure}}
   7: tokio::runtime::scheduler::current_thread::Context::enter
   8: tokio::runtime::scheduler::current_thread::CoreGuard::block_on::{{closure}}
   9: tokio::runtime::scheduler::current_thread::CoreGuard::enter::{{closure}}
  10: tokio::macros::scoped_tls::ScopedKey<T>::set
  11: tokio::runtime::scheduler::current_thread::CoreGuard::enter
  12: tokio::runtime::scheduler::current_thread::CoreGuard::block_on
  13: tokio::runtime::scheduler::current_thread::CurrentThread::block_on
  14: tokio::runtime::runtime::Runtime::block_on
  15: rust_out::main
  16: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


---- src/task.rs - task::TaskMetrics::total_scheduled_duration (line 922) stdout ----
Test executable failed (exit status: 101).

stderr:
thread 'main' panicked at 'assertion failed: total_scheduled_duration >= Duration::from_millis(1000)', src/task.rs:30:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/panicking.rs:64:14
   2: core::panicking::panic
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/panicking.rs:111:5
   3: rust_out::main::{{closure}}
   4: <core::pin::Pin<P> as core::future::future::Future>::poll
   5: tokio::runtime::scheduler::current_thread::CoreGuard::block_on::{{closure}}::{{closure}}::{{closure}}
   6: tokio::runtime::scheduler::current_thread::CoreGuard::block_on::{{closure}}::{{closure}}
   7: tokio::runtime::scheduler::current_thread::Context::enter
   8: tokio::runtime::scheduler::current_thread::CoreGuard::block_on::{{closure}}
   9: tokio::runtime::scheduler::current_thread::CoreGuard::enter::{{closure}}
  10: tokio::macros::scoped_tls::ScopedKey<T>::set
  11: tokio::runtime::scheduler::current_thread::CoreGuard::enter
  12: tokio::runtime::scheduler::current_thread::CoreGuard::block_on
  13: tokio::runtime::scheduler::current_thread::CurrentThread::block_on
  14: tokio::runtime::runtime::Runtime::block_on
  15: rust_out::main
  16: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.



failures:
    src/task.rs - task::TaskMetrics::mean_scheduled_duration (line 1924)
    src/task.rs - task::TaskMetrics::total_scheduled_duration (line 922)

test result: FAILED. 56 passed; 2 failed; 2 ignored; 0 measured; 0 filtered out; finished in 35.93s

@jschwe
Copy link
Contributor Author

jschwe commented Feb 3, 2023

The test failure is consistent, but also happens on main for me. FYI, when testing with 1.60 even more doc tests fail on main.

@jschwe
Copy link
Contributor Author

jschwe commented Feb 4, 2023

There are two test failures,

The doc test failures are not present using tokio 1.22.0 and appear first with tokio 1.23.0. I bisected and it appears 22862739dddd49a94065aa7a917cde2dc8a3f6bc is the culprit.
@carllerche could you perhaps have a look?

@Darksonn
Copy link
Contributor

Darksonn commented Feb 5, 2023

I opened #36 to track this.

@Darksonn Darksonn merged commit 265851a into tokio-rs:main Feb 5, 2023
@jschwe jschwe deleted the steal_operations branch February 5, 2023 22:46
@jschwe jschwe mentioned this pull request Feb 6, 2023
Noah-Kennedy pushed a commit that referenced this pull request Mar 6, 2023
Follow-up to #35.

The new metric is available starting with tokio v1.25.0.
@Noah-Kennedy Noah-Kennedy mentioned this pull request Mar 6, 2023
Noah-Kennedy added a commit that referenced this pull request Mar 6, 2023
# 0.2.0 (March 6th, 2023)

### Added
- Add `Debug` implementations. ([#28])
- rt: add concrete `RuntimeIntervals` iterator type ([#26])
- rt: add budget_forced_yield_count metric ([#39])
- rt: add io_driver_ready_count metric ([#40])
- rt: add steal_operations metric ([#37])
- task: also instrument streams ([#31])

### Documented
- doc: fix count in `TaskMonitor` docstring ([#24])
- doc: the description of steal_count ([#35])

[#24]: #24
[#26]: #26
[#28]: #28
[#31]: #31
[#35]: #35
[#37]: #37
[#39]: #39
[#40]: #40
jschwe added a commit to jschwe/tokio-metrics that referenced this pull request Apr 13, 2023
jschwe added a commit to jschwe/tokio-metrics that referenced this pull request Apr 13, 2023
Follow-up to tokio-rs#35.

The new metric is available starting with tokio v1.25.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants