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

Implement minimal (metrics-only) taskdumps. #5466

Closed
wants to merge 2 commits into from

Conversation

jswrenn
Copy link
Contributor

@jswrenn jswrenn commented Feb 17, 2023

This PR is the first in a series implementing the task dump feature proposal (#5457). This initial implementation includes only runtime metrics; e.g.:

{
  "flavor": "MultiThread",
  "metrics": {
    "num_workers": 72,
    "num_blocking_threads": 72,
    "num_idle_blocking_threads": 0,
    "remote_schedule_count": 1,
    "io_driver": {
      "fd_registered_count": 0,
      "fd_deregistered_count": 0
    }
  }
}

Implementation Details

This PR internally uses serde to provide flexible serialization of runtime internals. For testing, the schemars dev-dependency is used to generate a JSON schema against which task dumps can be validated (with jsonschema).

API

Before merging this PR, we should settle on an API for task dumps. Possibilities include:

/* this is what the PR implements at the time of creation */
impl fmt::Display for Handle {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        ...
    }
}

/* used like: */
println!("{}", Handle::current());

or

impl Handle {
    pub fn write_taskdump_json<W>(&self, mut writer: W) -> io::Result<()> 
    where
        W: io::Write,
    {
        ...
    }
}

/* used like: */
let handle = Handle::current();
let mut stdout = std::io::stdout().lock();
handle.write_taskdump_json(&mut stdout)?;
stdout.write_all(b"\n")?;
stdout.flush()?;

or

impl Handle {
    pub fn taskdump_json(&self) -> String {
        ...
    }
}

/* used like: */
println!("{}", Handle::current());

or something else? Suggestions welcome! My hope is we provide a streamlined API that can be used for serializing JSON task dumps to stdout or files. This streamlined API should co-exist with a possible future, more expansive API that provides options to skip traversing certain runtime structures and the option to serialize into other formats.

This is the first commit in a series that will bring "task dumps"
to Tokio. This commit adds the "taskdump" Cargo feature, important
dependencies, and fixes inference issues introduced by the
introduction of `serde_json` (see @rust-lang/rust#46257).
Introduces an API for dumping information about a runtime as JSON.
This initial implementation does not yet include any information
about tasks; only metrics.
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Feb 17, 2023
Copy link
Contributor

@hds hds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that some of the concepts here require further discussion before continuing. Probably worth continuing on the original issue and/or on Discord before adding too much more.

@@ -85,6 +85,10 @@ signal = [
"windows-sys/Win32_System_Console",
]
sync = []
taskdump = [
"serde",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even allowing that this would be an opt-in feature, adding serde and serde_json seems like a fairly large addition to tokio's dependencies.

/// Display-formatting a `Handle` produces a JSON dump of the runtime's internal state.
///
/// This feature is in very early development, and its API and output is subject to change.
impl fmt::Display for Handle {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already brought up on the feature proposal issue (#5457), but I'll add it here too.

I feel that performing excessive work inside the Display trait violates the consistency principal (principal of least surprise). My opinion (and so it is just that) is that most users would expect adding a Handle to some println!() call to be "harmless", and I don't think serializing the state of the runtime can be classed as such.

@sfackler
Copy link
Contributor

Is this a task dump API or a metrics API? IIRC tokio already has the ability to emit these metrics.

@jswrenn
Copy link
Contributor Author

jswrenn commented Feb 19, 2023

@sfackler My goal with this initial PR is to pin down the taskdump API and to touch as little of Tokio's internals as possible. Indeed, the reason this only includes metrics now is because Tokio already has an API for metrics. Once we settle on a satisfactory API for task dumps, I'll submit follow-up PRs that include incrementally increasing information about tasks.

@sfackler
Copy link
Contributor

But why would we want this API to report metrics in the first place? If you just want to add the method, it may as well just return an empty JSON object or something.

@hawkw hawkw self-requested a review February 19, 2023 18:55
@jswrenn
Copy link
Contributor Author

jswrenn commented Feb 20, 2023

My goal is to surface as much important information from Tokio's internals as is possible without introducing new runtime costs. Runtime metrics are readily (and cheaply) available already. They even might be helpful for debugging. I couldn't see a reason not to include them.

Additionally, the goal of this PR is to lay the groundwork for future PRs. There are three components to it: the public API, the serialization mechanism, and the testing approach. Returning {} would have made it difficult to develop and meaningfully validate the testing approach.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-metrics Module: tokio/runtime/metrics labels Apr 3, 2023
@Darksonn
Copy link
Contributor

Darksonn commented Apr 3, 2023

As discussed privately, the initial task dump PR will use a different implementation. I will close this. Go ahead and open a new PR when your changes are ready.

@Darksonn Darksonn closed this Apr 3, 2023
@Darksonn Darksonn added the M-taskdump --cfg tokio_taskdump label Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-metrics Module: tokio/runtime/metrics M-taskdump --cfg tokio_taskdump R-loom Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants