[SPARK-12221] add cpu time to metrics #10212

jisookim0513 · 2015-12-09T00:57:00Z

Currently task metrics don't support executor CPU time, so there's no way to calculate how much CPU time a stage/task took from History Server metrics. This PR enables reporting CPU time.

steveloughran · 2015-12-09T12:21:12Z

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

what happens when the history server is tries to deserialise a history from an earlier spark version, one which doesn't have a CPU time? As it looks to me through my scan through the code that this is going to fail.

HistoryServerSuite is the regression test here —it does have job histories without the relevant metric.

It would benefit from having another reference test run here for playback

Yeah it won't be able to deserialize a history from an earlier version. Would it be better to make this backward-compatible? (Sorry for the super late response)

steveloughran · 2015-12-09T12:21:36Z

Jenkins, test this please

steveloughran · 2015-12-09T12:21:53Z

(I may be trusted enough to start a run..let's see)

vanzin · 2016-08-04T17:52:27Z

@jisookim0513 are you still around? Do you mind updating the patch so we can trigger tests?

jisookim0513 · 2016-08-08T05:13:22Z

@vanzin sure will do

jisookim0513 · 2016-08-19T17:33:24Z

@vanzin I updated the patch

vanzin · 2016-08-19T17:41:17Z

ok to test

SparkQA · 2016-08-19T17:44:05Z

Test build #64085 has finished for PR 10212 at commit bd19098.

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

SparkQA · 2016-08-19T20:10:25Z

Test build #64096 has finished for PR 10212 at commit bf8d4f8.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-08-19T22:48:42Z

Test build #64099 has finished for PR 10212 at commit 5a9ea8e.

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

jisookim0513 · 2016-09-20T18:55:45Z

@vanzin this PR had passed all tests. Could you merge it if I fix the recently introduced conflicts?

vanzin · 2016-09-20T19:18:59Z

Sure. Just remember to ping someone, otherwise things get lost in the mountain of e-mails github generates.

SparkQA · 2016-09-20T21:42:49Z

Test build #65679 has finished for PR 10212 at commit d4d6f76.

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

SparkQA · 2016-09-21T00:59:06Z

Test build #65686 has finished for PR 10212 at commit 2ff5bdc.

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

jisookim0513 · 2016-09-21T04:55:21Z

@vanzin could you merge this? Thanks!

vanzin · 2016-09-22T16:46:47Z

@jisookim0513 unfortunately there are conflicts once more.

vanzin

A few minor things to clean up, otherwise looks ok.

vanzin · 2016-09-22T21:12:39Z

core/src/test/resources/HistoryServerExpectations/complete_stage_list_json_expectation.json

  "numCompleteTasks" : 8,
  "numFailedTasks" : 0,
  "executorRunTime" : 162,
+  "executorCpuTime" : 0,


Do you need to add these values to all these files? The code should be able to handle the old logs that don't have the value, and not adding these would be a good test case for that.

Hmm I thought HistoryServerSuite runs with included log files (that don't have CPU time). So this is an expected result since those logs don't have cpu time fields.

Oh, I thought these were input to the history server, not "golden files" that it checks against... if that's the case, ignore my comment.

Oh no, these are expected outputs. I think the inputs are stored under src/test/resources/spark-events.

vanzin · 2016-09-22T21:12:57Z

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

    metrics.setExecutorRunTime((json \ "Executor Run Time").extract[Long])
+    metrics.setExecutorCpuTime((json \ "Executor CPU Time") match {
+      case JNothing => 0
+      case x => x.extract[Long]}


nit: move '}' to next line (with the ')'). Also above.

Will fix this.

vanzin · 2016-09-22T21:13:50Z

core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala

      |  },
      |  "Task Metrics": {
      |    "Executor Deserialize Time": 300,
+      |    "Executor Deserialize CPU Time": 0,


Could you use values other than 0 so that you're sure the code is actually parsing the value, instead of falling into the default case?

AFAIK, JsonProtolSuite creates a JSON string from the event created by makeTaskMetrics():
'makeTaskMetrics(300L, 400L, 500L, 600L, 700, 800, hasHadoopInput = true, hasOutput = false))'.
I tried changing makeTaskMetrics() to accept deserialize CPU time and CPU time as arguments , but that ended up violating scalaStyle by having more than 10 parameters..

I still think it would be worth to fix this; just find some way around the style check.

Otherwise, you're not really testing whether the parsing code is actually parsing the field (what if there's a typo somewhere?).

Yeah I tested it on my testing cluster, but this makes sense. I will add non-zero CPU times by setting the CPU times same as given wall times.

SparkQA · 2016-09-22T22:46:06Z

Test build #65787 has finished for PR 10212 at commit d9c5f8f.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class SparkContext(config: SparkConf) extends Logging
- class ChiSqSelector @Since(\"2.1.0\") () extends Serializable

SparkQA · 2016-09-23T07:40:48Z

Test build #65815 has finished for PR 10212 at commit f0ef503.

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

vanzin · 2016-09-23T18:21:41Z

Failure looks unrelated... retest this please

jisookim0513 · 2016-09-23T18:22:24Z

@vanzin thanks, I was about to ask for a retest :)

vanzin · 2016-09-23T18:25:04Z

LGTM pending tests.

SparkQA · 2016-09-23T20:40:24Z

Test build #65834 has finished for PR 10212 at commit f0ef503.

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

vanzin · 2016-09-23T20:43:23Z

Merging to master.

jisookim0513 · 2016-09-23T21:11:58Z

@vanzin thanks a lot!

add cpu time to metrics

30752cb

jisookim0513 changed the title ~~add cpu time to metrics~~ [SPARK-12221] add cpu time to metrics Dec 9, 2015

steveloughran reviewed Dec 9, 2015
View reviewed changes

jisookim0513 force-pushed the add-cpu-time-metric branch from 1b77424 to 30752cb Compare March 3, 2016 23:04

make it backward-compatible

6033aa5

jisookim0513 added 2 commits August 18, 2016 15:18

rebase master

e02037b

fix tests

7b38442

remove unnecessary *

bd19098

fix scalastyle issue

bf8d4f8

fix DirectMissingMethodProblem

5a9ea8e

fix conflicts

d4d6f76

fix mistakes happened during resolving conflicts

2ff5bdc

fix merge-conflict

d9c5f8f

vanzin requested changes Sep 22, 2016

View reviewed changes

fix style and add non-zero cpu times in JsonProtocolSuite testing

f0ef503

asfgit closed this in 90a30f4 Sep 23, 2016

jisookim0513 mentioned this pull request Feb 28, 2017

[SPARK-16333][Core] Enable EventLoggingListener to log less #16714

Closed

[SPARK-12221] add cpu time to metrics #10212

[SPARK-12221] add cpu time to metrics #10212

Uh oh!

Conversation

jisookim0513 commented Dec 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveloughran commented Dec 9, 2015

Uh oh!

steveloughran commented Dec 9, 2015

Uh oh!

vanzin commented Aug 4, 2016

Uh oh!

jisookim0513 commented Aug 8, 2016

Uh oh!

jisookim0513 commented Aug 19, 2016

Uh oh!

vanzin commented Aug 19, 2016

Uh oh!

SparkQA commented Aug 19, 2016

Uh oh!

SparkQA commented Aug 19, 2016

Uh oh!

SparkQA commented Aug 19, 2016

Uh oh!

jisookim0513 commented Sep 20, 2016

Uh oh!

vanzin commented Sep 20, 2016

Uh oh!

SparkQA commented Sep 20, 2016

Uh oh!

SparkQA commented Sep 21, 2016

Uh oh!

jisookim0513 commented Sep 21, 2016

Uh oh!

vanzin commented Sep 22, 2016

Uh oh!

vanzin 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!

vanzin Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 22, 2016

Uh oh!

SparkQA commented Sep 23, 2016

Uh oh!

vanzin commented Sep 23, 2016

Uh oh!

jisookim0513 commented Sep 23, 2016

Uh oh!

vanzin commented Sep 23, 2016

Uh oh!

SparkQA commented Sep 23, 2016

Uh oh!

vanzin commented Sep 23, 2016

Uh oh!

jisookim0513 commented Sep 23, 2016

vanzin Sep 23, 2016 •

edited

Loading