[GitHub] [spark] tgravescs commented on a change in pull request #28528: [SPARK-31711][CORE] Register the executor source with the metrics system when running in local mode.

[GitBox <gi...@apache.org>]

tgravescs commented on a change in pull request #28528:
URL: https://github.com/apache/spark/pull/28528#discussion_r487946798



##########
File path: core/src/main/scala/org/apache/spark/SparkContext.scala
##########
@@ -623,6 +623,9 @@ class SparkContext(config: SparkConf) extends Logging {
 
     // Post init
     _taskScheduler.postStartHook()
+    if (isLocal) {
+      _env.metricsSystem.registerSource(Executor.executorSource)

Review comment:
       So I haven't went and looked but if I don't configure the metrics system files does this cause any overhead?

##########
File path: core/src/main/scala/org/apache/spark/executor/Executor.scala
##########
@@ -135,6 +135,8 @@ private[spark] class Executor(
     env.metricsSystem.registerSource(new JVMCPUSource())
     executorMetricsSource.foreach(_.register(env.metricsSystem))
     env.metricsSystem.registerSource(env.blockManager.shuffleMetricsSource)
+  } else {
+    Executor.executorSource = executorSource

Review comment:
       we now have 2 executorSource variables, one in the object and one in the class. That seems a bit weird/confusing. I see the reason why but perhaps we can rename the object one to have LocalModeOnly in the name or something as to not accidentally be used in non-local mode.

##########
File path: core/src/main/scala/org/apache/spark/executor/Executor.scala
##########
@@ -134,8 +134,11 @@ private[spark] class Executor(
     env.metricsSystem.registerSource(new JVMCPUSource())
     executorMetricsSource.foreach(_.register(env.metricsSystem))
     env.metricsSystem.registerSource(env.blockManager.shuffleMetricsSource)
+  } else {
+    Executor.executorSource = executorSource

Review comment:
       I can think of other ways to do this more cleanly but it would be quite a bit more code change.  I do think we should explicitly make a comment about the appId so that someone who comes and looks at this later realizes that

##########
File path: docs/monitoring.md
##########
@@ -1153,6 +1153,11 @@ This is the component with the largest amount of instrumented metrics
 - namespace=JVMCPU
   - jvmCpuTime
 
+- namespace=executor

Review comment:
       this looks the same as the ExecutorMetrics but I think they are actually different in that this doesn't give you the JVM metrics - correct?  Perhaps we need to update the one below.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org