[SPARK-34898][CORE] We should log SparkListenerExecutorMetricsUpdateEvent of `driver` appropriately when `spark.eventLog.logStageExecutorMetrics` is true #31992

AngersZhuuuu · 2021-03-29T09:09:28Z

What changes were proposed in this pull request?

In current EventLoggingListener, we won't write SparkListenerExecutorMetricsUpdate message to event log file at all

override def onExecutorMetricsUpdate(event: SparkListenerExecutorMetricsUpdate): Unit = {
  if (shouldLogStageExecutorMetrics) {
    event.executorUpdates.foreach { case (stageKey1, newPeaks) =>
      liveStageExecutorMetrics.foreach { case (stageKey2, metricsPerExecutor) =>
        // If the update came from the driver, stageKey1 will be the dummy key (-1, -1),
        // so record those peaks for all active stages.
        // Otherwise, record the peaks for the matching stage.
        if (stageKey1 == DRIVER_STAGE_KEY || stageKey1 == stageKey2) {
          val metrics = metricsPerExecutor.getOrElseUpdate(
            event.execId, new ExecutorMetrics())
          metrics.compareAndUpdatePeakValues(newPeaks)
        }
      }
    }
  }
}

In history server's restful API about executor, we can get Executor's metrics but can't get all driver's metrics. Executor's executor metrics can be updated with TaskEnd event etc...

So in this pr, I add support to log SparkListenerExecutorMetricsUpdateEvent of driver when spark.eventLog.logStageExecutorMetrics is true.

Why are the changes needed?

Make user can got driver's peakMemoryMetrics in SHS.

Does this PR introduce any user-facing change?

user can got driver's executor metrics in SHS's restful API.

How was this patch tested?

Mannul test

… EventLog of `driver` appropriately

SparkQA · 2021-03-29T09:38:48Z

Test build #136645 has finished for PR 31992 at commit 58ace57.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2021-03-29T11:36:02Z

cc @Ngone51 FYI

SparkQA · 2021-03-29T12:56:06Z

Test build #136646 has finished for PR 31992 at commit 936eefd.

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

SparkQA · 2021-03-29T13:35:45Z

Test build #136653 has started for PR 31992 at commit 7054b58.

SparkQA · 2021-03-29T13:47:46Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41227/

SparkQA · 2021-03-29T13:57:50Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41227/

SparkQA · 2021-03-29T13:59:03Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41228/

SparkQA · 2021-03-29T14:52:50Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41228/

SparkQA · 2021-03-29T17:55:14Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41235/

SparkQA · 2021-03-29T18:04:05Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41235/

mridulm

Who is consuming this event in spark ?
Btw, if all you want is monitor jvm stats (outside of spark ui), use codahale integration instead ?

core/src/test/scala/org/apache/spark/scheduler/EventLoggingListenerSuite.scala

mridulm · 2021-03-29T18:50:05Z

core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala

@@ -249,6 +249,9 @@ private[spark] class EventLoggingListener(
  }

  override def onExecutorMetricsUpdate(event: SparkListenerExecutorMetricsUpdate): Unit = {
+    if (event.execId == SparkContext.DRIVER_IDENTIFIER) {
+      logEvent(event)
+    }


Do this only when shouldLogStageExecutorMetrics is enabled ?

Do this only when shouldLogStageExecutorMetrics is enabled ?

I don't thinks it should be controlled by shouldLogStageExecutorMetrics . since driver's metrics is not related to stage executor.

Currently, we have a single event for both driver and executor metrics update - differentiated by exec id.
I dont have strong opinions on this, but if we have a flag (shouldLogStageExecutorMetrics) controlling whether metrics are to be updated, we should consistently apply it IMO.

Currently, we have a single event for both driver and executor metrics update - differentiated by exec id.
I dont have strong opinions on this, but if we have a flag (shouldLogStageExecutorMetrics) controlling whether metrics are to be updated, we should consistently apply it IMO.

@mridulm Follow this comment, how about current.

AngersZhuuuu · 2021-03-30T02:19:57Z

Who is consuming this event in spark ?
Btw, if all you want is monitor jvm stats (outside of spark ui), use codahale integration instead ?

AppStatusListener will consumer this

 override def onExecutorMetricsUpdate(event: SparkListenerExecutorMetricsUpdate): Unit = {

I know spark metrics frame work can have a monitor on this. But when we want to build an application analysis system like Dr.elephant. We want to get app status from restful api, right? Driver's memory usage is an important index too.

SparkQA · 2021-03-30T03:31:19Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41262/

SparkQA · 2021-03-30T03:39:35Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41262/

SparkQA · 2021-03-30T05:22:18Z

Test build #136680 has finished for PR 31992 at commit 02ad9de.

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

AngersZhuuuu · 2021-04-01T06:07:40Z

ping @mridulm Any more update?

mridulm · 2021-04-05T18:03:46Z

As I understood based on description and proposed changes, the right level of integration for this would be with metrics subsystem.. While we can do it via other means, it is not optimal to do so.
Having said that, I dont have strong opinions on this ... will let some more involved with this comment better.

AngersZhuuuu · 2021-04-07T03:16:37Z

As I understood based on description and proposed changes, the right level of integration for this would be with metrics subsystem.. While we can do it via other means, it is not optimal to do so.
Having said that, I dont have strong opinions on this ... will let some more involved with this comment better.

Yea, also ping @srowen @Ngone51 @gengliangwang

srowen · 2021-04-07T03:30:36Z

I don't know enough to have an opinion on this. I think the key questions are - what is the most consistent thing to do, and, are there any performance problems with adding this information to events?

SparkQA · 2021-04-19T06:11:04Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42129/

SparkQA · 2021-04-19T06:11:06Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42129/

SparkQA · 2021-04-19T07:43:12Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42136/

SparkQA · 2021-04-19T07:43:13Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42136/

SparkQA · 2021-04-19T08:09:38Z

Test build #137559 has finished for PR 31992 at commit 02ad9de.

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

SparkQA · 2021-04-19T10:19:52Z

Test build #137583 has finished for PR 31992 at commit 02ad9de.

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

HyukjinKwon · 2021-06-16T04:55:01Z

@AngersZhuuuu BTW did you disable GA in your fork repo? It should be enabled so PR leverage the GA resources in your forked repo.

HyukjinKwon · 2021-06-16T04:55:25Z

cc @HeartSaVioR too FYI

…nto SPARK-34898

AngersZhuuuu · 2021-06-16T05:09:54Z

@AngersZhuuuu, mind making the PR description disambiguous? what's "driver executor peakMemoryMetrics"?

How about current

AngersZhuuuu · 2021-06-16T05:10:47Z

@AngersZhuuuu BTW did you disable GA in your fork repo? It should be enabled so PR leverage the GA resources in your forked repo.

No, but this pr is a little long.

SparkQA · 2021-06-16T05:17:49Z

Test build #139843 has finished for PR 31992 at commit 87a079e.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-06-16T05:21:12Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44365/

SparkQA · 2021-06-16T06:02:11Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44371/

SparkQA · 2021-06-16T06:02:54Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44370/

AngersZhuuuu · 2021-06-16T06:06:43Z

retest this please

SparkQA · 2021-06-16T06:36:15Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44371/

SparkQA · 2021-06-16T06:44:47Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44370/

SparkQA · 2021-06-16T06:55:15Z

Test build #139837 has finished for PR 31992 at commit c9a4a67.

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

SparkQA · 2021-06-16T07:58:22Z

Test build #139842 has finished for PR 31992 at commit 6c81e2d.

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

SparkQA · 2021-06-16T08:00:02Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44379/

SparkQA · 2021-06-16T08:10:39Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44378/

SparkQA · 2021-06-16T08:23:07Z

Test build #139850 has finished for PR 31992 at commit 87a079e.

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

SparkQA · 2021-06-16T08:33:29Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44379/

AngersZhuuuu · 2021-06-16T08:33:33Z

retest this please

SparkQA · 2021-06-16T08:43:41Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44378/

SparkQA · 2021-06-16T11:39:27Z

Test build #139866 has finished for PR 31992 at commit 87a079e.

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

SparkQA · 2021-06-16T11:55:59Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44396/

SparkQA · 2021-06-16T12:28:09Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44396/

AngersZhuuuu · 2021-06-17T07:41:44Z

Any more suggestion?

HyukjinKwon · 2021-06-17T09:16:10Z

I'll leave it to @mridulm and @Ngone51

mridulm · 2021-06-17T17:09:54Z

Merging to master, thanks for working on this and pushing this through @AngersZhuuuu !

Thanks for the reviews @HyukjinKwon, @srowen !

[SPARK-34898][CORE] We should send SparkListenerExecutorMetricsUpdate…

58ace57

… EventLog of `driver` appropriately

Update EventLoggingListener.scala

936eefd

github-actions bot added the CORE label Mar 29, 2021

Update EventLoggingListenerSuite.scala

7054b58

mridulm reviewed Mar 29, 2021

View reviewed changes

Update EventLoggingListenerSuite.scala

02ad9de

AngersZhuuuu added 2 commits June 16, 2021 13:04

Merge branch 'master' into SPARK-34898

51dcf2b

Merge branch 'SPARK-34898' of https://github.com/AngersZhuuuu/spark i…

87a079e

…nto SPARK-34898

asfgit closed this in 79362c4 Jun 17, 2021

mridulm mentioned this pull request Dec 6, 2021

[SPARK-37493][CORE] show gc time and duration time of driver in executors page #34749

Closed

[SPARK-34898][CORE] We should log SparkListenerExecutorMetricsUpdateEvent of driver appropriately when spark.eventLog.logStageExecutorMetrics is true #31992

[SPARK-34898][CORE] We should log SparkListenerExecutorMetricsUpdateEvent of driver appropriately when spark.eventLog.logStageExecutorMetrics is true #31992

Conversation

AngersZhuuuu commented Mar 29, 2021 • 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?

SparkQA commented Mar 29, 2021

HyukjinKwon commented Mar 29, 2021

SparkQA commented Mar 29, 2021

SparkQA commented Mar 29, 2021

SparkQA commented Mar 29, 2021

SparkQA commented Mar 29, 2021

SparkQA commented Mar 29, 2021

SparkQA commented Mar 29, 2021

SparkQA commented Mar 29, 2021

SparkQA commented Mar 29, 2021

mridulm left a comment

Choose a reason for hiding this comment

mridulm Mar 29, 2021

Choose a reason for hiding this comment

AngersZhuuuu Mar 30, 2021

Choose a reason for hiding this comment

mridulm Apr 5, 2021

Choose a reason for hiding this comment

AngersZhuuuu Jun 16, 2021

Choose a reason for hiding this comment

AngersZhuuuu commented Mar 30, 2021

SparkQA commented Mar 30, 2021

SparkQA commented Mar 30, 2021

SparkQA commented Mar 30, 2021

AngersZhuuuu commented Apr 1, 2021

mridulm commented Apr 5, 2021

AngersZhuuuu commented Apr 7, 2021 • edited

srowen commented Apr 7, 2021

SparkQA commented Apr 19, 2021

SparkQA commented Apr 19, 2021

SparkQA commented Apr 19, 2021

SparkQA commented Apr 19, 2021

SparkQA commented Apr 19, 2021

SparkQA commented Apr 19, 2021

HyukjinKwon commented Jun 16, 2021

HyukjinKwon commented Jun 16, 2021

AngersZhuuuu commented Jun 16, 2021

AngersZhuuuu commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

AngersZhuuuu commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

AngersZhuuuu commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

SparkQA commented Jun 16, 2021

AngersZhuuuu commented Jun 17, 2021

HyukjinKwon commented Jun 17, 2021

mridulm commented Jun 17, 2021

[SPARK-34898][CORE] We should log SparkListenerExecutorMetricsUpdateEvent of `driver` appropriately when `spark.eventLog.logStageExecutorMetrics` is true #31992

[SPARK-34898][CORE] We should log SparkListenerExecutorMetricsUpdateEvent of `driver` appropriately when `spark.eventLog.logStageExecutorMetrics` is true #31992

AngersZhuuuu commented Mar 29, 2021 •

edited

AngersZhuuuu commented Apr 7, 2021 •

edited