[SPARK-31081][UI][SQL] Make the display of stageId/stageAttemptId/taskId of sql metrics configurable in UI #27849

Ngone51 · 2020-03-08T10:06:43Z

What changes were proposed in this pull request?

This PR adds a conf to make the display of stageId/StageAttemptId/taskId of SQL metrics configurable in UI. And also propose to not display them by default.

Why are the changes needed?

It makes metrics harder to read after #26843 and user may not interest in extra info(stageId/StageAttemptId/taskId ) when they do not need debug. So, I think we need to a way to make the display configurable.

Does this PR introduce any user-facing change?

Yes.

Before this PR:

After this PR without stageId/StageAttemptId/taskId:

How was this patch tested?

Updated SQLMetricsSuite and tested manually.

Ngone51 · 2020-03-08T12:33:41Z

ping @cloud-fan @gengliangwang @nartal1 Please take a look, thanks!

SparkQA · 2020-03-08T14:06:33Z

Test build #119534 has finished for PR 27849 at commit 6f61906.

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

dongjoon-hyun

Hi, @Ngone51 .
The PR title Make SQLMetrics more readable from UI sounds too broad. Could you revise more specifically?

Ngone51 · 2020-03-10T07:13:33Z

sql/core/src/main/scala/org/apache/spark/sql/execution/metric/SQLMetrics.scala

    acc
  }

  def createNanoTimingMetric(sc: SparkContext, name: String): SQLMetric = {
    // Same with createTimingMetric, just normalize the unit of time to millisecond.
    val acc = new SQLMetric(NS_TIMING_METRIC, -1)
-    acc.register(sc, name = Some(s"$name total (min, med, max (stageId (attemptId): taskId))"),
-      countFailedValues = false)
+      acc.register(sc, name = Some(s"name"), countFailedValues = false)


TODO: fix "name"

dongjoon-hyun · 2020-03-11T18:40:33Z

Thank you for narrowing down title, @Ngone51 .

dongjoon-hyun · 2020-03-11T18:43:10Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala

+    buildStaticConf("spark.sql.ui.displayTaskInfoForMaxMetric")
+      .doc("If turn on, Spark will display stageId-stageAttemptId-taskId of the max metrics to " +
+        "tell where the max value comes from. It's useful to help debug job quicker.")
+      .version("3.0.0")


@Ngone51 .
Since SPARK-31081 is filed as an Improvement, this should be 3.1.0.
If you want to consider this as a bug or regression at 3.0.0, you had better change JIRA first.

cc @rxin since he is the release manager of 3.0.0 (also cc @gatorsmile ).

This is a UI issue to me. The SQL web UI is really hard to read now with the stage id stuff. Ideally, we should revisit #26843 and think of a better way to improve readability. But no one has proposed an idea yet.

This PR disables the stage id stuff, which seems like a good compromise for 3.0: we keep the UI unchanged, but for people who really need the stage id info, they can still enable it.

+1 for making this 3.0. The UI changes in #26843 can be confusing to users after 3.0 release.

dongjoon-hyun · 2020-03-11T18:44:09Z

sql/core/src/main/scala/org/apache/spark/sql/execution/metric/SQLMetrics.scala

@@ -116,8 +117,8 @@ object SQLMetrics {
    // data size total (min, med, max):
    // 100GB (100MB, 1GB, 10GB)
    val acc = new SQLMetric(SIZE_METRIC, -1)
-    acc.register(sc, name = Some(s"$name total (min, med, max (stageId (attemptId): taskId))"),
-      countFailedValues = false)
+      acc.register(sc, name = Some(s"$name total (min, med, ${attachTaskId("max", sc.conf)})"),


The new indentation looks incorrect at this line?

I also noted that, thanks.

dongjoon-hyun · 2020-03-11T18:45:13Z

sql/core/src/main/scala/org/apache/spark/sql/execution/metric/SQLMetrics.scala

@@ -233,4 +228,13 @@ object SQLMetrics {
        SparkListenerDriverAccumUpdates(executionId.toLong, metrics.map(m => m.id -> m.value)))
    }
  }
+
+  private def attachTaskId(name: String, conf: SparkConf): String = {


We are attaching stageId and attemptId, too.

Yeah, I know. I'm thinking about a better name, while adding stageId/attemptId makes me feel too long for the method name.

attachTaskInfo? stage id is part of the task info which tells which stage the task comes from.

SparkQA · 2020-03-11T19:03:37Z

Test build #119674 has finished for PR 27849 at commit a41d9c4.

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

cloud-fan · 2020-03-12T06:36:21Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala

@@ -135,6 +135,14 @@ object StaticSQLConf {
      .intConf
      .createWithDefault(1000)

+  val DISPLAY_TASK_ID_FOR_MAX_METRIC =
+    buildStaticConf("spark.sql.ui.displayTaskInfoForMaxMetric")


shall we make it an internal conf?

gengliangwang · 2020-03-12T08:19:39Z

cc @tgravescs as well

tgravescs · 2020-03-12T21:43:02Z

Sorry, I disagree. I understand that it adds more clutter and am definitely open to improvements, but I think it does bring valuable information that once people realize is there will use it. This entire ui is for debugging purposes so I don't get your argument with that. If a user isn't actively looking to see what went on with their job why would they go to this UI at all.
If you have a better way to map these SQL boxes to the stage it ran in, please let me know?
This also quickly tells you which task was the slowest to aid in debugging further.

Can we leave on by default or don't display and have a checkbox like we have on other tables? (disclaimer - I haven't looked how hard that would be in code to add the checkbox)

tgravescs · 2020-03-12T21:45:22Z

BTW thanks @gengliangwang for adding me, it would have been nice if I was added right away since I added this feature that you are now trying to take back out.
I really hope in the future with all the things being reverted that I see we copy the original authors and reviewers.

Ngone51 · 2020-03-13T01:43:50Z

This entire ui is for debugging purposes so I don't get your argument with that. If a user isn't actively looking to see what went on with their job why would they go to this UI at all.

I agree that UI is mostly for debugging purposes. But please also note that this is living UI(rather than SHS UI). And living UI may shutdown at any time before user figure out the root cause (except those long running applications). So, personally, I aways need to restart and insert Thread.sleep when I need to debug on living UI in case it shutdown before I figure out the root cause. So, I think user could always turn on spark.sql.ui.displayTaskInfoForMaxMetric when they realize they need to restart and debug on living UI.

And if they really want to see those ids at any time, they can just turn it on always.

Or if we can have better way to display ids, then we can turn it on by default. And here's my previous propose to display ids in case you would like it:

I really hope in the future with all the things being reverted that I see we copy the original authors and reviewers.

Sorry, this is my bad. I'll take care in the future.

cloud-fan · 2020-03-13T03:30:05Z

The UI is mostly for debugging but the stage id stuff is not always needed. At least this has been distracting me multiple times when I try to debug something with the SQL UI.

The new proposal from @Ngone51 looks good, it's much less distracting to have a short (1.0:14) at the end. @tgravescs If you don't like it, I think we at least should have this config so that people can disable it.

gengliangwang · 2020-03-13T03:49:11Z

@tgravescs as @gatorsmile and I mentioned in #26843 (comment), the new change is not readable. I prefer to hide the slowest task as well.

Can we leave on by default or don't display and have a checkbox like we have on other tables? (disclaimer - I haven't looked how hard that would be in code to add the checkbox)

I like the idea of "don't display and have a checkbox like we have on other tables". @Ngone51 will you consider that?

gengliangwang · 2020-03-13T04:08:33Z

I checked with @Ngone51 offline. He is fine with the checkbox idea. But he is not very familiar with the front-end work. I am also busy with other priorities recently.

@sarutak @HeartSaVioR would you like to take this task (create a checkbox for hild/show the max stage/stage attamp/task id in the graph) over?

HeartSaVioR · 2020-03-13T04:12:10Z

Thanks for pinging me and I'm sorry I'm not familiar with front-end work as well - that's why I only had to skim the front-end side while reviewing structured streaming UI PR.

tgravescs · 2020-03-13T14:42:37Z

the problem with a config is that if you turn it off, especially by default most users won't see it to know its even an option.
I'd much prefer the checkbox type approach (note again I haven't looked to see how complicated is with these boxes).
I'm also fine with the other format (0:0:14) as long as the legend if obvious on what it is. could have a hover there or something as well.

Perhaps I should ask everyone this - when you were looking at the UI, why were you looking at it? What values in there were you using for instance? A lot of people have asked me ok, I did this groupby or join and I see it in that box, but how do I know what stage that ran in..... this at least helps with that question. So how do you do that now? my previous answer was you had to click through a lot of jobs/stages to figure it out, which is very time consuming.

sarutak · 2020-03-16T01:58:50Z

@gengliangwang I'll try to add the checkbox.

…of sql metrics toggleable ### What changes were proposed in this pull request? This is another solution for `SPARK-31081` and #27849 . I added a checkbox which can toggle display of stageId/taskid in the SQL's DAG page. Mainly, I implemented the toggleable texts in boxes with HTML label feature provided by `dagre-d3`. The additional metrics are enclosed by `<span>` and control the appearance of the text. But the exception is additional metrics in clusters. We can use HTML label for cluster but layout will be broken so I choosed normal text label for clusters. Due to that, this solution contains a little bit tricky code in`spark-sql-viz.js` to manipulate the metric texts and generate DOMs. ### Why are the changes needed? It makes metrics harder to read after #26843 and user may not interest in extra info(stageId/StageAttemptId/taskId ) when they do not need debug. #27849 control the appearance by a new configuration property but providing a checkbox is more flexible. ### Does this PR introduce any user-facing change? Yes. [Additional metrics shown] ![with-checked](https://user-images.githubusercontent.com/4736016/77244214-0f6cd780-6c56-11ea-9275-a30758dd5339.png) [Additional metrics hidden] ![without-chedked](https://user-images.githubusercontent.com/4736016/77244219-14ca2200-6c56-11ea-9874-33a466085fce.png) ### How was this patch tested? Tested manually with a simple DataFrame operation. * The appearance of additional metrics in the boxes are controlled by the newly added checkbox. * No error found with JS-debugger. * Checked/not-checked state is preserved after reloading. Closes #27927 from sarutak/SPARK-31081. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Gengliang Wang <gengliang.wang@databricks.com>

…of sql metrics toggleable ### What changes were proposed in this pull request? This is another solution for `SPARK-31081` and #27849 . I added a checkbox which can toggle display of stageId/taskid in the SQL's DAG page. Mainly, I implemented the toggleable texts in boxes with HTML label feature provided by `dagre-d3`. The additional metrics are enclosed by `<span>` and control the appearance of the text. But the exception is additional metrics in clusters. We can use HTML label for cluster but layout will be broken so I choosed normal text label for clusters. Due to that, this solution contains a little bit tricky code in`spark-sql-viz.js` to manipulate the metric texts and generate DOMs. ### Why are the changes needed? It makes metrics harder to read after #26843 and user may not interest in extra info(stageId/StageAttemptId/taskId ) when they do not need debug. #27849 control the appearance by a new configuration property but providing a checkbox is more flexible. ### Does this PR introduce any user-facing change? Yes. [Additional metrics shown] ![with-checked](https://user-images.githubusercontent.com/4736016/77244214-0f6cd780-6c56-11ea-9275-a30758dd5339.png) [Additional metrics hidden] ![without-chedked](https://user-images.githubusercontent.com/4736016/77244219-14ca2200-6c56-11ea-9874-33a466085fce.png) ### How was this patch tested? Tested manually with a simple DataFrame operation. * The appearance of additional metrics in the boxes are controlled by the newly added checkbox. * No error found with JS-debugger. * Checked/not-checked state is preserved after reloading. Closes #27927 from sarutak/SPARK-31081. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Gengliang Wang <gengliang.wang@databricks.com> (cherry picked from commit 999c9ed) Signed-off-by: Gengliang Wang <gengliang.wang@databricks.com>

…of sql metrics toggleable ### What changes were proposed in this pull request? This is another solution for `SPARK-31081` and apache#27849 . I added a checkbox which can toggle display of stageId/taskid in the SQL's DAG page. Mainly, I implemented the toggleable texts in boxes with HTML label feature provided by `dagre-d3`. The additional metrics are enclosed by `<span>` and control the appearance of the text. But the exception is additional metrics in clusters. We can use HTML label for cluster but layout will be broken so I choosed normal text label for clusters. Due to that, this solution contains a little bit tricky code in`spark-sql-viz.js` to manipulate the metric texts and generate DOMs. ### Why are the changes needed? It makes metrics harder to read after apache#26843 and user may not interest in extra info(stageId/StageAttemptId/taskId ) when they do not need debug. apache#27849 control the appearance by a new configuration property but providing a checkbox is more flexible. ### Does this PR introduce any user-facing change? Yes. [Additional metrics shown] ![with-checked](https://user-images.githubusercontent.com/4736016/77244214-0f6cd780-6c56-11ea-9275-a30758dd5339.png) [Additional metrics hidden] ![without-chedked](https://user-images.githubusercontent.com/4736016/77244219-14ca2200-6c56-11ea-9874-33a466085fce.png) ### How was this patch tested? Tested manually with a simple DataFrame operation. * The appearance of additional metrics in the boxes are controlled by the newly added checkbox. * No error found with JS-debugger. * Checked/not-checked state is preserved after reloading. Closes apache#27927 from sarutak/SPARK-31081. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Gengliang Wang <gengliang.wang@databricks.com>

github-actions · 2020-06-25T00:23:33Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

HyukjinKwon · 2020-06-25T01:57:40Z

Closing in favour of #27927

Ngone51 added 3 commits March 8, 2020 16:43

format

df16356

fix

7389d05

fix test

6f61906

dongjoon-hyun requested changes Mar 8, 2020

View reviewed changes

Ngone51 changed the title ~~[SPARK-31081][UI][SQL] Make SQLMetrics more readable from UI~~ [SPARK-31081][UI][SQL] Make metrics in SparkPlanGraphNode more readable from Spark UI Mar 9, 2020

Ngone51 commented Mar 10, 2020

View reviewed changes

config only

a41d9c4

Ngone51 changed the title ~~[SPARK-31081][UI][SQL] Make metrics in SparkPlanGraphNode more readable from Spark UI~~ [SPARK-31081][UI][SQL] Make the display of stageId/stageAttemptId/taskId of sql metrics configurable in UI Mar 11, 2020

dongjoon-hyun reviewed Mar 11, 2020

View reviewed changes

dongjoon-hyun added WEB UI SQL labels Mar 11, 2020

cloud-fan reviewed Mar 12, 2020

View reviewed changes

sarutak mentioned this pull request Mar 16, 2020

[SPARK-31081][UI][SQL] Make display of stageId/stageAttemptId/taskId of sql metrics toggleable #27927

Closed

github-actions bot added the Stale label Jun 25, 2020

HyukjinKwon closed this Jun 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-31081][UI][SQL] Make the display of stageId/stageAttemptId/taskId of sql metrics configurable in UI #27849

[SPARK-31081][UI][SQL] Make the display of stageId/stageAttemptId/taskId of sql metrics configurable in UI #27849

Ngone51 commented Mar 8, 2020 •

edited

Ngone51 commented Mar 8, 2020

SparkQA commented Mar 8, 2020

dongjoon-hyun left a comment

Ngone51 Mar 10, 2020

dongjoon-hyun commented Mar 11, 2020

dongjoon-hyun Mar 11, 2020

cloud-fan Mar 12, 2020

gengliangwang Mar 12, 2020

dongjoon-hyun Mar 11, 2020

Ngone51 Mar 12, 2020

dongjoon-hyun Mar 11, 2020

Ngone51 Mar 12, 2020

cloud-fan Mar 12, 2020

SparkQA commented Mar 11, 2020

cloud-fan Mar 12, 2020

gengliangwang commented Mar 12, 2020

tgravescs commented Mar 12, 2020

tgravescs commented Mar 12, 2020

Ngone51 commented Mar 13, 2020

cloud-fan commented Mar 13, 2020

gengliangwang commented Mar 13, 2020

gengliangwang commented Mar 13, 2020

HeartSaVioR commented Mar 13, 2020 •

edited

tgravescs commented Mar 13, 2020

sarutak commented Mar 16, 2020

github-actions bot commented Jun 25, 2020

HyukjinKwon commented Jun 25, 2020

[SPARK-31081][UI][SQL] Make the display of stageId/stageAttemptId/taskId of sql metrics configurable in UI #27849

[SPARK-31081][UI][SQL] Make the display of stageId/stageAttemptId/taskId of sql metrics configurable in UI #27849

Conversation

Ngone51 commented Mar 8, 2020 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Ngone51 commented Mar 8, 2020

SparkQA commented Mar 8, 2020

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Mar 11, 2020

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

SparkQA commented Mar 11, 2020

Choose a reason for hiding this comment

gengliangwang commented Mar 12, 2020

tgravescs commented Mar 12, 2020

tgravescs commented Mar 12, 2020

Ngone51 commented Mar 13, 2020

cloud-fan commented Mar 13, 2020

gengliangwang commented Mar 13, 2020

gengliangwang commented Mar 13, 2020

HeartSaVioR commented Mar 13, 2020 • edited

tgravescs commented Mar 13, 2020

sarutak commented Mar 16, 2020

github-actions bot commented Jun 25, 2020

HyukjinKwon commented Jun 25, 2020

Ngone51 commented Mar 8, 2020 •

edited

HeartSaVioR commented Mar 13, 2020 •

edited