[SPARK-31081][UI][SQL] Make display of stageId/stageAttemptId/taskId of sql metrics toggleable by sarutak · Pull Request #27927 · apache/spark

sarutak · 2020-03-16T16:59:08Z

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  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 inspark-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]

[Additional metrics hidden]

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.

SparkQA · 2020-03-16T18:36:25Z

Test build #119879 has finished for PR 27927 at commit ecbcd54.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-03-16T19:36:32Z

Test build #119890 has finished for PR 27927 at commit e54ac44.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-03-17T04:04:49Z

Test build #119897 has finished for PR 27927 at commit 6c9814a.

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

gengliangwang · 2020-03-18T07:54:54Z

sql/core/src/main/resources/org/apache/spark/sql/execution/ui/static/spark-sql-viz.js

+  });
+  var isChecked = window.localStorage.getItem("stageId-and-taskId-checked") == "true";
+  $("#stageId-and-taskId-checkbox").prop("checked", isChecked);
+  $(".stageId-and-taskId-metrics").hide();


this line seems unneeded.

Thanks! I'll remove it.

gengliangwang · 2020-03-18T08:20:48Z

sql/core/src/main/resources/org/apache/spark/sql/execution/ui/static/spark-sql-viz.js

 }

+// labelSeparator should be a non-graphical character in order not to affect the width of boxes.
+var labelSeparator = "\x01";


Could you explain more about the labelSeparator as "\x01" here? It seem that the label of cluster will contain the  as well.

It seem that the label of cluster will contain the as well

Yes, but we can't add  here because the label type of cluster is not html.
I tried to make the label type html but I found that the DAG had broken layout.

The width of boxes are determined based on the length of text by dagre-d3.
So if the separator character has width (graphical), the width of boxes are slightly widened against the original texts.

gengliangwang · 2020-03-18T08:22:19Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ui/ExecutionPage.scala

        </div>
+        <div>
+          <input type="checkbox" id="stageId-and-taskId-checkbox"></input>
+          <span>Show Stage/Task IDs</span>


how about

Show Stage/Task IDs with the max metrics

Maybe we should just have this be the entire format:
Show the Stage (Stage Attempt): Task ID that corresponds to the max metric

gengliangwang · 2020-03-18T08:23:41Z

@sarutak Thanks for improving this!!

cc @tgravescs @Ngone51 as well
BTW, I try the PR on my local setup. The checkbox size is normal, not like the one the PR description..

tgravescs · 2020-03-18T14:10:12Z

@sarutak thanks for picking this up.

Ngone51

I don't know this part well but only a minor nit. Overall, it looks good and cool from the screenshot!

Ngone51 · 2020-03-18T15:13:05Z

sql/core/src/main/resources/org/apache/spark/sql/execution/ui/static/spark-sql-viz.js

+  if (isChecked) {
+      additionalMetrics.show();
+  } else {
+      additionalMetrics.hide();
+  }


nit: indent

Thanks! I'll fix it.

sarutak · 2020-03-22T06:26:19Z

BTW, I try the PR on my local setup. The checkbox size is normal, not like the one the PR description..

Maybe, it depends on OS. I use Pop!_OS which is based on Ubuntu.

…e checkbox.

sarutak · 2020-03-22T07:02:28Z

I've reflected comments and modified the captures in the description.

SparkQA · 2020-03-22T11:16:54Z

Test build #120155 has finished for PR 27927 at commit 22384e7.

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

SparkQA · 2020-03-22T11:39:02Z

Test build #120157 has finished for PR 27927 at commit 377fb49.

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

gengliangwang

LGTM

gengliangwang · 2020-03-24T20:36:26Z

Thanks, merging to master/3.0

…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 `` 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>

cloud-fan · 2020-03-26T13:56:20Z

sql/core/src/main/resources/org/apache/spark/sql/execution/ui/static/spark-sql-viz.js


+// labelSeparator should be a non-graphical character in order not to affect the width of boxes.
+var labelSeparator = "\x01";
+var stageAndTaskMetricsPattern = "^(.*)(\\(stage.*attempt.*task[^)]*\\))(.*)$";


Not related to this PR, but a minor comment: now we display task info like stage 1 (attempt 2): task 3. A more standard (and simpler) way to refer to a stage is stage 1.2: task 3.

@sarutak can you help fix it?

That makes sense. I'll fix it.

…StageId ### What changes were proposed in this pull request? In ExecutionPage, metrics format for stageId, attemptId and taskId are displayed like `(stageId (attemptId): taskId)` for now. I changed this format like `(stageId.attemptId taskId)`. ### Why are the changes needed? As cloud-fan suggested [here](#27927 (comment)), `stageId.attemptId` is more standard in Spark. ### Does this PR introduce any user-facing change? Yes. Before applying this change, we can see the UI like as follows. ![with-checked](https://user-images.githubusercontent.com/4736016/77682421-42a6c200-6fda-11ea-92e4-e9f4554adb71.png) And after this change applied, we can like as follows. ![fix-merics-format-with-checked](https://user-images.githubusercontent.com/4736016/77682493-61a55400-6fda-11ea-801f-91a67da698fd.png) ### How was this patch tested? Modified `SQLMetricsSuite` and manual test. Closes #28039 from sarutak/improve-metrics-format. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…StageId ### What changes were proposed in this pull request? In ExecutionPage, metrics format for stageId, attemptId and taskId are displayed like `(stageId (attemptId): taskId)` for now. I changed this format like `(stageId.attemptId taskId)`. ### Why are the changes needed? As cloud-fan suggested [here](#27927 (comment)), `stageId.attemptId` is more standard in Spark. ### Does this PR introduce any user-facing change? Yes. Before applying this change, we can see the UI like as follows. ![with-checked](https://user-images.githubusercontent.com/4736016/77682421-42a6c200-6fda-11ea-92e4-e9f4554adb71.png) And after this change applied, we can like as follows. ![fix-merics-format-with-checked](https://user-images.githubusercontent.com/4736016/77682493-61a55400-6fda-11ea-801f-91a67da698fd.png) ### How was this patch tested? Modified `SQLMetricsSuite` and manual test. Closes #28039 from sarutak/improve-metrics-format. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit bc37fdc) Signed-off-by: Wenchen Fan <wenchen@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 `` 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>

…StageId ### What changes were proposed in this pull request? In ExecutionPage, metrics format for stageId, attemptId and taskId are displayed like `(stageId (attemptId): taskId)` for now. I changed this format like `(stageId.attemptId taskId)`. ### Why are the changes needed? As cloud-fan suggested [here](apache#27927 (comment)), `stageId.attemptId` is more standard in Spark. ### Does this PR introduce any user-facing change? Yes. Before applying this change, we can see the UI like as follows. ![with-checked](https://user-images.githubusercontent.com/4736016/77682421-42a6c200-6fda-11ea-92e4-e9f4554adb71.png) And after this change applied, we can like as follows. ![fix-merics-format-with-checked](https://user-images.githubusercontent.com/4736016/77682493-61a55400-6fda-11ea-801f-91a67da698fd.png) ### How was this patch tested? Modified `SQLMetricsSuite` and manual test. Closes apache#28039 from sarutak/improve-metrics-format. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

sarutak added 2 commits March 17, 2020 01:33

add checkbox for additional metrics in ExecutionPage.

7a9499e

fix style

ecbcd54

Adjust the width of boxes

e54ac44

refactoring.

6c9814a

gengliangwang reviewed Mar 18, 2020

View reviewed changes

Ngone51 reviewed Mar 18, 2020

View reviewed changes

removed unneeded line, fixed indentation and revised the label for th…

22384e7

…e checkbox.

minor style fix

377fb49

gengliangwang approved these changes Mar 24, 2020

View reviewed changes

gengliangwang closed this in 999c9ed Mar 24, 2020

cloud-fan reviewed Mar 26, 2020

View reviewed changes

sarutak mentioned this pull request Mar 26, 2020

[SPARK-31275][WEBUI] Improve the metrics format in ExecutionPage for StageId. #28039

Closed

HyukjinKwon mentioned this pull request Jun 25, 2020

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

Closed

sarutak deleted the SPARK-31081 branch June 4, 2021 20:44

Comments

Conversation

sarutak commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

SparkQA commented Mar 16, 2020

Uh oh!

SparkQA commented Mar 16, 2020

Uh oh!

SparkQA commented Mar 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengliangwang commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgravescs commented Mar 18, 2020

Uh oh!

Ngone51 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarutak commented Mar 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarutak commented Mar 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Mar 22, 2020

Uh oh!

SparkQA commented Mar 22, 2020

Uh oh!

gengliangwang left a comment

Choose a reason for hiding this comment

Uh oh!

gengliangwang commented Mar 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sarutak commented Mar 16, 2020 •

edited

Loading

gengliangwang commented Mar 18, 2020 •

edited

Loading

sarutak commented Mar 22, 2020 •

edited

Loading

sarutak commented Mar 22, 2020 •

edited

Loading