[SPARK-4016] Allow user to show/hide UI metrics.

[kayousterhout]

This commit adds a set of checkboxes to the stage detail
page that the user can use to show additional task metrics,
including the GC time, result serialization time, result fetch
time, and scheduler delay. All of these metrics are now
hidden by default. This allows advanced users to look at more
detailed metrics, without distracting the average user.

This change also cleans up the stage detail page so that metrics
are shown in the same order in the summary table as in the task table,
and updates the metrics in both tables such that they contain the same
set of metrics.

The ability to remember a user's preferences for which metrics
should be shown has been filed as SPARK-4024.

Here's what the stage detail page looks like by default:

and once a user clicks "Show additional metrics" (note that all the metrics get checked by default):

cc @shivaram @andrewor14

[SparkQA]

QA tests have started for PR 2867 at commit e989560.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2867 at commit e989560.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21952/
Test PASSed.

[JoshRosen]

@kayousterhout This might integrate nicely with my #2852, which introduces some new abstractions to simplify the web UI's table rendering code. With my framework, I think you might be able to automatically generate the ids used to show / hide columns rather than having to have a class that holds a bunch of strings.

[andrewor14]

I haven't done a close review yet, but I think it looks nicer if the boxes are listed vertically and the whole "Show additional metrics" section is collapsible.

[kayousterhout]

Haha @andrewor14 that's exactly how I had it originally but @pwendell wanted it to look like this. I see arguments for both versions so happy with whatever the two of you agree on!

[andrewor14]

Hm, I think from the UX perspective this requires the cursor to move further if they want to toggle multiple options at once, which could be annoying. Also, if we align them horizontally we'll eventually run out of space as we add more options.

[ash211]

The first thing I can imagine doing if I start looking at these is to just check them all. Thoughts to making that operation easier, with say a check-all button?

[kayousterhout]

@ash211 that's a great idea, I'll add that and incorporate @andrewor14's suggestion about a drop-down menu. Thanks all!

[kayousterhout]

Ok I updated this, as well as the photos in the description at the top. As per @andrewor14's suggestion, there's now an expandable menu of options. Based on @ash211's suggestion, when you click the menu to expand it, all of the options get checked by default (and then the user can uncheck options she isn't interested in, if necessary). Let me know what your thoughts are!

[SparkQA]

QA tests have started for PR 2867 at commit 1e3504d.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2867 at commit 1e3504d.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22043/
Test PASSed.

[andrewor14]

Looks awesome from a visual point of view, though I haven't looked at the code in detail yet.

[andrewor14]

Do we have tooltips or a short description of what each of those metrics are before we check the boxes? Or do we have to hover over the column name itself to learn that?

[kayousterhout]

@andrewor14 I can add tooltips to each metric name, or just list it in line (e.g., Scheduler Delay: A metric that explains ....). Which do you think is better?

[aarondav]

+1 to some description. I still don't know what scheduler delay actually means.

[andrewor14]

Yeah I'm not sure. I kinda like the way it looks right now. It might look a little cluttered if it's inlined.

[JoshRosen]

I've decided to hold off on introducing my table-builder abstraction, since it's kind of leaky because there are corner-cases that it doesn't handle cleanly. Therefore, this shouldn't block on that. It would be great to try to get this merged so that it can unblock your other metrics patch.

Regarding tooptips: I agree with Andrew that inline descriptions of the metrics might look a little cluttered, so hover tooltips would be a nice addition. This is a net improvement over what we have now, though, so I suppose we could wait until later to add those tooltips.

[kayousterhout]

I can add the tooltips in an hour or so -- was waiting to modify this to
hear if there were more review comments, but it sounds like there are not!
Will post an updated version in a bit. Thanks for the update Josh!

On Fri, Oct 24, 2014 at 1:14 PM, Josh Rosen [email protected]
wrote:

I've decided to hold off on introducing my table-builder abstraction,
since it's kind of leaky because there are corner-cases that it doesn't
handle cleanly. Therefore, this shouldn't block on that. It would be great
to try to get this merged so that it can unblock your other metrics patch.

Regarding tooptips: I agree with Andrew that inline descriptions of the
metrics might look a little cluttered, so hover tooltips would be a nice
addition. This is a net improvement over what we have now, though, so I
suppose we could wait until later to add those tooltips.

—
Reply to this email directly or view it on GitHub
#2867 (comment).

[SparkQA]

Test build #22220 has started for PR 2867 at commit c5a8b99.

• This patch merges cleanly.

[SparkQA]

Test build #22220 has finished for PR 2867 at commit c5a8b99.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22220/
Test PASSed.

[kayousterhout]

@JoshRosen @andrewor14 does one of you have time to look at this today? I added the tool tips to each additional metric. Would be great to get this in so that I can update my other metrics pull request that depends on this one.

[andrewor14]

I'll take a look tonight if not early tomorrow.

[andrewor14]

space before {, though I don't think we run style checks on JS

[andrewor14]

Can you just inline this?

[kayousterhout]

I could but then it becomes a n^2 computation on the headers, because it has to scan through all of the headers each time it adds new header content. n is not huge so maybe a non-issue; would you still prefer inlining it?

[andrewor14]

Oh I see, don't worry about it then. I thought it's just a coding style thing.

[andrewor14]

Hey @kayousterhout this LGTM modulo a few very minor comments, though I haven't looked at the JS / CSS code in detail. Do we plan to make more of the existing columns "additional columns"? Is this just a starting point?

[kayousterhout]

Thanks Andrew! I'm happy to make more "additional" but all the rest seemed
like things people would want by default? Did you have things in mind?

On Tue, Oct 28, 2014 at 11:55 AM, andrewor14 [email protected]
wrote:

Hey @kayousterhout https://github.com/kayousterhout this LGTM other
than a few very minor comments, though I haven't looked at the JS / CSS
code in detail. Do we plan to make more of the existing columns "additional
columns"? Is this just a starting point?

—
Reply to this email directly or view it on GitHub
#2867 (comment).

[andrewor14]

Actually one other thing we might want is a De/select all checkbox that I think @ash211 suggested. It would be useful if the user just doesn't care about any of these metrics and want their UIs to look more compact.

I'm happy to make more "additional" but all the rest seemed
like things people would want by default? Did you have things in mind?

I was thinking accumulators; it seems to be empty most of the time.

[kayousterhout]

@andrewor14 I filed a JIRA for the accumulators thing, which I think we should actually handle by just hiding that column when no accumulators are defined (similar to what we do for shuffle read, shuffle write, etc.): https://issues.apache.org/jira/browse/SPARK-4141

Re:(de)select all, currently, everything is deselected by default. If you click the "show additional metrics" expandy header, it then shows you all the options and selects all of them -- which I thought might most closely match what most people will want (based on my use of this and @ash211's comment). Can we wait to add more additional select / deselect-all boxes until we see how people end up using this and what's actually helpful?

[andrewor14]

Yeah makes sense. Anything to add @JoshRosen?

[JoshRosen]

I downloaded this and tried it out. It looks good to me, except for a few minor usability things:

• I should be able to click on either the checkbox or the checkbox label to expand / hide a metric. I'm pretty sure that there's some standard HTML way to group labels and input elements together so that clicking the label focuses or toggles the field. The larger click target makes this easier to use / more accessible.
• I'd place the arrow next to "Show additional metrics" inside of the link / header so that it's also clickable.

These are really minor nitpicks, though, although the first one could make a big usability difference for users who have trouble clicking small things.

[JoshRosen]

@kayousterhout You mentioned upthread that you fixed some of Andrew's style comments, but I don't see those changes; did you forget to push your commit?

You shouldn't block on my latest UX comments; I can fix them myself in a subsequent UI cleanup PR.

[kayousterhout]

@JoshRosen thanks for pointing that out -- I did indeed fail to push! I rebased and pushed a new version that also incorporates your two usability comments. Unfortunately the simple label fix doesn't work because of the javascript associated with clicking, but I figured out a pretty simple way to do this with a modest amount of javascript (let me know if you see a better way!).

[SparkQA]

Test build #22567 has started for PR 2867 at commit 6015913.

• This patch merges cleanly.

[kayousterhout]

Also, @JoshRosen, thanks for trying this out!!

[SparkQA]

Test build #22567 has finished for PR 2867 at commit 6015913.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class WriteAheadLogBackedBlockRDDPartition(
  • class WriteAheadLogBackedBlockRDD[T: ClassTag](

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22567/
Test PASSed.

[JoshRosen]

Thanks for updating this. This looks good, so I'm going to merge it. Thanks!

[kayousterhout]

Thanks Josh, and thanks to you and Andrew both for the thorough reviews!!
This is much more usable now than my original version.

On Fri, Oct 31, 2014 at 10:30 AM, asfgit [email protected] wrote:

Closed #2867 #2867 via adb6415
adb6415
.

—
Reply to this email directly or view it on GitHub
#2867 (comment).

[andrewor14]

Hey by the way here's a usability nit that grew adversely on me over time. Every time I unchecked everything, it feels very natural to close the menu, but when I open the menu again to see what I unchecked, it automatically checks everything. I would think that the little triangle doesn't actually change my settings but only collapses/expands the boxes.

[kayousterhout]

That's a good point and something I've found too. What do you think is the most usable way to change this?

One idea is for the "Show additional metrics" / little arrow to only expand and collapse the options, and then have a separate "select all" box at the top to choose all of the metrics. The issue with that is that it still requires two clicks for someone to enable everything.

A second idea, which seems perhaps preferable to me, is to have it say "Show additional metrics (show all)", with the show all in grey. And then if you click the "(show all)" part, it expands the options and selects all of them. Is that too confusing? What do you think?