Skip to content

[SPARK-25228][CORE]Add executor CPU time metric. #22218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
LucaCanali wants to merge 7 commits into from
Closed

[SPARK-25228][CORE]Add executor CPU time metric. #22218

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c715096
Add Executor CPU Time metric
LucaCanali Aug 24, 2018
438bf90
Add executor CPU Time using ProcessCpuTime method in OperatingSystem …
LucaCanali Aug 31, 2018
807119b
Add executor CPU Time using ProcessCpuTime method in OperatingSystem …
LucaCanali Aug 31, 2018
d522fa2
Add executor CPU Time metric (jvmCpuTime) using ProcessCpuTime method…
LucaCanali Sep 1, 2018
b7fdec2
Add executor CPU Time metric (jvmCpuTime) using ProcessCpuTime method…
LucaCanali Sep 1, 2018
95d31f6
Add executor CPU Time metric (jvmCpuTime) using ProcessCpuTime method…
LucaCanali Sep 1, 2018
e72966e
removed check for null value.
LucaCanali Sep 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions core/src/main/scala/org/apache/spark/executor/ExecutorSource.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@

package org.apache.spark.executor

import java.lang.management.ManagementFactory
import java.util.concurrent.ThreadPoolExecutor
import javax.management.{MBeanServer, ObjectName}

import scala.collection.JavaConverters._

Expand Down Expand Up @@ -73,6 +75,29 @@ class ExecutorSource(threadPool: ThreadPoolExecutor, executorId: String) extends
registerFileSystemStat(scheme, "write_ops", _.getWriteOps(), 0)
}

/** Dropwizard metrics gauge measuring the executor's process CPU time.
Copy link
Member

@srowen srowen Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the comments should begin on the next line. But this is scaladoc syntax, and inside a code block, normally we just use // block comments.

* This code will try to get JVM Process CPU time or return -1 otherwise.
* The CPU time value is returned in nanoseconds.
* It will use proprietary extensions as com.sun.management.OperatingSystemMXBean or
* com.ibm.lang.management.OperatingSystemMXBean if available
*/
val mBean: MBeanServer = ManagementFactory.getPlatformMBeanServer
Copy link
Member

@srowen srowen Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that these become fields in the parent object. These should go inside the new Gauge... { I think?

val name = new ObjectName("java.lang", "type", "OperatingSystem")
metricRegistry.register(MetricRegistry.name("executorCPUTime" ), new Gauge[Long] {
Copy link
Member

@maropu maropu Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a little confused with the exsiting cpuTime. How about jvmCpuTime?

Copy link
Member

@kiszk kiszk Sep 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: name("executorCPUTime" ) -> name("executorCPUTime")

override def getValue: Long = {
try {
val attribute = mBean.getAttribute(name, "ProcessCpuTime")
if (attribute != null) {
Copy link
Member

@maropu maropu Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the doc for `getAttribute though, when does it return null?
https://docs.oracle.com/javase/8/docs/api/javax/management/MBeanServerConnection.html#getAttribute-javax.management.ObjectName-java.lang.String-

Copy link
Contributor Author

@LucaCanali LucaCanali Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed good point. I'll remove this additional check for null value.

Copy link
Member

@srowen srowen Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't mind the defensive checks, because who knows what to really expect from these implementations? but this is OK by me. In case of a bad impl this would still return -1.

attribute.asInstanceOf[Long]
} else {
-1L
Copy link
Member

@maropu maropu Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to return -1 instead of 0?

Copy link
Contributor Author

@LucaCanali LucaCanali Sep 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the idea from com.sun.management.OperatingSystemMXBean.getProcessCpuTime, according to the documentation: "Returns: the CPU time used by the process in nanoseconds, or -1 if this operation is not supported."
I guess it makes sense to return an invalid value as -1L for the CPU time if something goes wrong with gathering CPU Time values, so the error condition will appear evident to the end user of the metric. Returning 0 is also possible, of course.

Copy link
Member

@maropu maropu Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks.

}
} catch {
case _ : Exception => -1L
Copy link
Member

@maropu maropu Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case NonFatal(_) => -1?

}
}
})

// Expose executor task metrics using the Dropwizard metrics system.
// The list is taken from TaskMetrics.scala
val METRIC_CPU_TIME = metricRegistry.counter(MetricRegistry.name("cpuTime"))
Expand Down