feature: add resource detail view #188

zaharidichev · 2021-11-23T17:20:13Z

This PR introduces the resoruce detail view. It also brings a couple of additional changes:

we now rely on context of entered spans rather than explicit parent - child relationships
we expose internal resources (BatchSempahore)
we show the relationships between resources (Mutex -> BatchSemaphore)
we show the async ops that are live on a specific resource
we also show the tasks that are awaiting on these async ops, which allows the user to draw a relationship between tasks and resources
there are more examples added in the examples folder (semaphore and mutex)

Some screenshots:

Signed-off-by: Zahari Dichev [email protected]

Signed-off-by: Zahari Dichev <[email protected]>

hawkw · 2021-11-23T17:55:29Z

This looks amazing. I'm going to try to give this branch a detailed review today, but the screenshots look awesome. Amazing work!

Signed-off-by: Zahari Dichev <[email protected]>

zaharidichev · 2021-11-24T12:34:47Z

@hawkw Thanks a bunch for taking the time. Let me know after a quick glance whether you prefer me to break up the PR somehow or whether I can do anything to make it easier to review.

Signed-off-by: Zahari Dichev <[email protected]>

hawkw

overall, this seems pretty good --- and the UI screenshots you posted are awesome! i had a bunch of minor suggestions.

also, i wonder if it would be worth adding a way to hide/show all the internal resources---and maybe hide them by default? i don't know if we really want to expose tokio internals like the batch semaphore by default, unless the user asks for them, it seems like it could potentially just be a bunch of information that's confusing to a user who isn't familiar with tokio's internals. but, we could address that in a separate PR.

hawkw · 2021-11-29T20:37:25Z

console-api/proto/async_ops.proto

@@ -37,6 +37,10 @@ message AsyncOp {
    // The source of this async operation. Most commonly this should be the name
    // of the method where the instantiation of this op has happened.
    string source = 3;
+    // The ID of the parent async op.


nit: it might be nice if this comment explained a bit more what the "parent async op" of an async op is --- my guess is that this is for situations such as a Sleep inside of an Interval, or a Semaphore op inside a channel? but, it would be nice if the comment explained that.

also, if a given async op doesn't have a parent, this will not be set, right? it might be worth documenting that as well.

hawkw · 2021-11-29T20:37:37Z

console-api/proto/common.proto

+// State attributes of an entity. These are dependent on the type of the entity.
+// For example, a timer resource will have a duration while a semaphore resource may


nit:

Suggested change

// State attributes of an entity. These are dependent on the type of the entity.

// For example, a timer resource will have a duration while a semaphore resource may

// State attributes of an entity. These are dependent on the type of the entity.

//

// For example, a timer resource will have a duration, while a semaphore resource may

hawkw · 2021-11-29T20:38:17Z

console-api/proto/common.proto

+
+// State attributes of an entity. These are dependent on the type of the entity.
+// For example, a timer resource will have a duration while a semaphore resource may
+// have permits as an attribute. Likewise the async ops of a sempahore may have attributes


nit/typo:

Suggested change

// have permits as an attribute. Likewise the async ops of a sempahore may have attributes

// have permits as an attribute. Likewise, the async ops of a semaphore may have attributes

hawkw · 2021-11-29T20:40:54Z

console-subscriber/examples/barrier.rs

@@ -0,0 +1,40 @@
+use std::sync::Arc;


thanks for adding new examples, these are great!

take it or leave it, but it might also be nice to have some examples that combine multiple resource types. that way, we could simulate what a real application that uses a large number of resources might look like. we could maybe stick similar code to these examples into examples/app.rs (maybe with a CLI option) so that the example can also spawn a number of tasks that use barriers, mutexen, etc.

it would be totally fine to make that change in a follow-up branch, though.

hawkw · 2021-11-29T20:41:35Z

console-subscriber/examples/mutex.rs

+            loop {
+                if *count.lock().await >= 50 {
+                    break;
+                }
+            }


nit, take it or leave it: this seems like it could be a while loop:

Suggested change

loop {

if *count.lock().await >= 50 {

break;

}

}

while *count.lock().await < 50 {}

hawkw · 2021-11-30T23:08:40Z

console/src/view/resource.rs

+        let mut overview = Vec::with_capacity(6);
+        overview.push(Spans::from(vec![
+            bold("ID: "),
+            Span::raw(format!("{} ", resource.id())),


it would be nice not to format this on every frame...

hawkw · 2021-11-30T23:09:01Z

console/src/view/resource.rs

+
+        overview.push(Spans::from(vec![
+            bold("Kind: "),
+            Span::raw(format!("{} ", resource.kind())),


can we store the kind string as well?

In these situations do you mean storing the string as an InternetStr on the Resource/AsyncOp objects?

Depending on the string, we may or may not want to intern them. In the case of resource.kind(), I believe they already are InternedStrs:

console/console/src/state/resources.rs

Lines 33 to 37 in 042c1f7

#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd)]

pub(crate) enum Kind {

Timer,

Other(InternedStr),

}

I think we could probably add a method to resources::Kind that returns an &'static str, so that we don't need to format! it here. I can open a quick PR for that change?

~~...actually, i just double-checked, and resource.kind() already returns an &str. so i think this can just be~~

Suggested change

Span::raw(format!("{} ", resource.kind())),

Span::raw(resource.kind()),

Edit: oh, hmm, that doesn't actually work due to lifetime issues. i think we may need to make some additional changes to InternedStr to make this work nicely...

hawkw · 2021-11-30T23:09:12Z

console/src/view/resource.rs

+
+        overview.push(Spans::from(vec![
+            bold("Type: "),
+            Span::raw(format!("{} ", resource.concrete_type())),


...and the concrete type string?

hawkw · 2021-11-30T23:09:53Z

console/src/view/resources.rs

        "Kind",
        "Total",
        "Target",
        "Type",
+        "Viz",


i have a slight preference for spelling this "Vis", as it's short for "Visibility"...or maybe it should just be "pub"? but, i could be convinced otherwise...

hawkw · 2021-11-30T23:10:15Z

console/src/view/resources.rs

+                        Some(id) => {
+                            format!("{}", id)
+                        }
+                        None => "n/a".to_string(),


another string that shouldn't have to be format!ed every time we render a frame...

Signed-off-by: Zahari Dichev <[email protected]>

hawkw

overall, this looks pretty good to me and i think we should merge this and address the bigger potential improvements in follow-up branches. i commented on a few minor suggestions we may want to address first.

hawkw · 2021-12-06T18:17:48Z

console-api/proto/async_ops.proto

+    // The ID of the parent async op. This field is set when the this async op
+    // is created in the context of another one. An example of that is for example
+    // a `Mutex::lock` which internally calls `Semaphore::acquire`. This field can
+    // be empty.


some minor nits here:

Suggested change

// The ID of the parent async op. This field is set when the this async op

// is created in the context of another one. An example of that is for example

// a `Mutex::lock` which internally calls `Semaphore::acquire`. This field can

// be empty.

// The ID of the parent async op.

//

// This field is only set if this async op was created while inside of another

// async op. For example, `tokio::sync`'s `Mutex::lock` internally calls

// `Semaphore::acquire`.

//

// This field can be empty; if it is empty, this async op is not a child of another

// async op.

hawkw · 2021-12-06T18:18:12Z

console-api/proto/common.proto

+// State attributes of an entity. These are dependent on the type of the entity.
+//
+// For example, a timer resource will have a duration, while a semaphore resource may
+// have permits as an attribute. Likewise the async ops of a semaphore may have attributes


Suggested change

// have permits as an attribute. Likewise the async ops of a semaphore may have attributes

// have a permit count. Likewise, the async ops of a semaphore may have attributes

hawkw · 2021-12-06T18:19:55Z

console-subscriber/src/aggregator/mod.rs

+                match update_type {
+                    UpdateType::Resource => {
+                        if let Some(parent) = self
+                            .resources
+                            .get(&update_id)
+                            .and_then(|r| self.resources.get(r.parent_id.as_ref()?))
+                            .filter(|parent| parent.inherit_child_attrs)
+                        {
+                            to_update.push((parent.id, UpdateType::Resource));
+                        }
+                    }
+                    UpdateType::AsyncOp => {
+                        if let Some(parent) = self
+                            .async_ops
+                            .get(&update_id)
+                            .and_then(|r| self.async_ops.get(r.parent_id.as_ref()?))
+                            .filter(|parent| parent.inherit_child_attrs)
+                        {
+                            to_update.push((parent.id, UpdateType::AsyncOp));
+                        }
+                    }


i think in theory there might be a way to not repeat this code quite as much, but i don't know if it's worth the effort of figuring that out. we could maybe have a function that takes a hash map and performs the lookup part? but, not a blocker.

Alternatively, you could have a method that takes an UpdateType and gives you back the hashmap, and then leave the currently duplicated code inline as a single non-duplicated if statement. (This works in part because you can just pass along the UpdateType into the to_update.push invocation, rather than writing out the specific variant the way you are doing here.

yeah, that's more or less what i was imagining. i think the filter might have to be kept separate, as well, because the field access is being performed on a different type. we could introduce a trait for that, but, hmm...

hawkw · 2021-12-06T18:26:15Z

console/src/state/async_ops.rs

@@ -34,7 +34,7 @@ pub(crate) enum SortBy {
 #[derive(Debug)]
 pub(crate) struct AsyncOp {
    id: u64,
-    parent_id: Option<u64>,
+    parent_id: InternedStr,


i think the parent ID could probably just be a normal String rather than an InternedStr, unless we expect a single async op to have a lot of children. but, we'll clean up unused interned strings anyway so this is fine in either case.

@hawkw I actually wonder whether we can take a different approach with all the parent info. Namely, what if whenever we have a parent we simply have an Option<AsyncOpRef> or Option<ResourceRef> and then get all the needed strings, etc by upgrading the parent ref? That will make it much easier when we have a toggle that determines whether we should display any info about internal resources. WDYT?

We could definitely do something like that; it might be worth doing in a follow-up branch so that we can get this PR merged sooner?

hawkw · 2021-12-06T18:30:11Z

console/src/state/resources.rs

+            let parent_id = strings.string(match parent_id {
+                Some(id) => {
+                    format!("{}", id)
+                }
+                None => "n/a".to_string(),


nit, take it or leave it: this could maybe be simplified to

Suggested change

let parent_id = strings.string(match parent_id {

Some(id) => {

format!("{}", id)

}

None => "n/a".to_string(),

let parent_id = strings.string(parent_id

.map(u64::to_string)

.unwrap_or_else(|| "n/a".to_string());

);

hawkw · 2021-12-06T18:31:28Z

console/src/state/resources.rs

+                        let r = r.borrow();
+                        format!("{} ({}::{})", r.id(), r.target(), r.concrete_type())
+                    })
+                    .unwrap_or(format!("{}", id)),


nit: this should probably be

Suggested change

.unwrap_or(format!("{}", id)),

.unwrap_or_else(|| format!("{}", id)),

so that we don't format the string if we did find a parent (see the documentation for https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.unwrap_or)

also, it might be slightly more efficient not to go through the format! macro when the format arguments are just "{}"; we can use the fmt::Display implementation for u64 directly:

Suggested change

.unwrap_or(format!("{}", id)),

.unwrap_or_else(|| id.to_string()),

but, that's probably not a huge performance impact, regardless.

hawkw · 2021-12-06T18:33:25Z

console/src/state/async_ops.rs

+        let task_id = pb.task_id.map(|id| id.id);
+        let task_id_str = strings.string(
+            task_id
+                .map(|tid| format!("{}", tid))


nit: could be

Suggested change

.map(|tid| format!("{}", tid))

.map(u64::to_string)

which might be very slightly more efficient (but not a big deal)

hawkw · 2021-12-06T19:28:31Z

console/src/view/async_ops.rs

+                    let task_str = match task {
+                        Some(task_str) => task_str,
+                        None => async_op.task_id_str().to_owned(),
                    };


nit/tioli: could just be

Suggested change

let task_str = match task {

Some(task_str) => task_str,

None => async_op.task_id_str().to_owned(),

};

let task_str = task.unwrap_or_else(|| async_op.task_id_str().to_owned());

Signed-off-by: Zahari Dichev <[email protected]>

hawkw · 2021-12-06T21:59:29Z

@zaharidichev is this ready to merge or are you planning on making an additional changes in this branch?

zaharidichev · 2021-12-06T22:04:18Z

I can address more of the feedback in follow-ups. Its already getting quite big. I think the corresponding tokio branch needs to be sanity checked before we merge this though: tokio-rs/tokio#4302

…sole into zd/resource-details

Signed-off-by: Zahari Dichev <[email protected]>

carllerche · 2021-12-14T21:56:01Z

@hawkw @zaharidichev to confirm: this PR is ready to merge pending the Tokio PR?

carllerche · 2021-12-15T18:43:52Z

@zaharidichev Tokio 1.15 is out.

Signed-off-by: Zahari Dichev <[email protected]>

console-api/proto/common.proto

hawkw

added docs

console-api/proto/resources.proto

console-api/proto/common.proto

add resource detail view

df60c24

Signed-off-by: Zahari Dichev <[email protected]>

add barrier example

0e156a3

Signed-off-by: Zahari Dichev <[email protected]>

add RwLock example

827a5f2

Signed-off-by: Zahari Dichev <[email protected]>

hawkw reviewed Nov 30, 2021

View reviewed changes

hawkw and others added 2 commits December 2, 2021 09:23

Merge branch 'main' into zd/resource-details

d650970

interning feedback

08eedb0

Signed-off-by: Zahari Dichev <[email protected]>

hawkw approved these changes Dec 6, 2021

View reviewed changes

zaharidichev and others added 4 commits December 6, 2021 20:16

lint

4c1279c

Signed-off-by: Zahari Dichev <[email protected]>

more feedback

46bcadc

Signed-off-by: Zahari Dichev <[email protected]>

Merge branch 'main' into zd/resource-details

a2d723e

update tokio dep

d1d4548

Signed-off-by: Zahari Dichev <[email protected]>

zaharidichev mentioned this pull request Dec 6, 2021

tracing: instrument more resources tokio-rs/tokio#4302

Merged

zaharidichev added 2 commits December 11, 2021 15:22

Merge branch 'zd/resource-details' of https://github.com/tokio-rs/con…

cfbc1af

…sole into zd/resource-details

update tokio patch

d506ec5

Signed-off-by: Zahari Dichev <[email protected]>

pnkfelix mentioned this pull request Dec 14, 2021

work items before publishing to crates.io #193

Closed

6 tasks

zaharidichev and others added 2 commits December 15, 2021 19:41

update tokio

34c6982

Signed-off-by: Zahari Dichev <[email protected]>

Merge branch 'main' into zd/resource-details

9311ef1

hawkw reviewed Dec 15, 2021

View reviewed changes

console-api/proto/common.proto Outdated Show resolved Hide resolved

hawkw approved these changes Dec 15, 2021

View reviewed changes

console-api/proto/resources.proto Show resolved Hide resolved

hawkw added 2 commits December 15, 2021 12:12

docs suggestions from code review

af74840

Merge branch 'main' into zd/resource-details

d93d336

hawkw reviewed Dec 15, 2021

View reviewed changes

console-api/proto/common.proto Outdated Show resolved Hide resolved

Update console-api/proto/common.proto

4a342dd

hawkw merged commit 1aa9b59 into main Dec 15, 2021

hawkw deleted the zd/resource-details branch December 15, 2021 20:34

hawkw mentioned this pull request Dec 15, 2021

Create a view of Resources #63

Closed

hawkw added this to the 0.1 milestone Dec 15, 2021

		// State attributes of an entity. These are dependent on the type of the entity.
		// For example, a timer resource will have a duration while a semaphore resource may

	// have permits as an attribute. Likewise the async ops of a sempahore may have attributes
	// have permits as an attribute. Likewise, the async ops of a semaphore may have attributes

	#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd)]
	pub(crate) enum Kind {
	Timer,
	Other(InternedStr),
	}

	Span::raw(format!("{} ", resource.kind())),
	Span::raw(resource.kind()),

	// have permits as an attribute. Likewise the async ops of a semaphore may have attributes
	// have a permit count. Likewise, the async ops of a semaphore may have attributes

	.unwrap_or(format!("{}", id)),
	.unwrap_or_else(\|\| format!("{}", id)),

	.unwrap_or(format!("{}", id)),
	.unwrap_or_else(\|\| id.to_string()),

feature: add resource detail view #188

feature: add resource detail view #188

Conversation

zaharidichev commented Nov 23, 2021

hawkw commented Nov 23, 2021

zaharidichev commented Nov 24, 2021

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Dec 6, 2021

zaharidichev commented Dec 6, 2021

carllerche commented Dec 14, 2021

carllerche commented Dec 15, 2021

hawkw left a comment

Choose a reason for hiding this comment

hawkw Dec 3, 2021 •

edited

Loading