-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-27189][CORE] Add Executor metrics and memory usage instrumentation to the metrics system #24132
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.
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
[SPARK-27189][CORE] Add Executor metrics and memory usage instrumentation to the metrics system #24132
Changes from 2 commits
005f95e
3f2e8ff
01c5925
1a0a3e9
4ab39cd
bb5f2b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,7 @@ import org.apache.spark.annotation.DeveloperApi | |
| import org.apache.spark.broadcast.Broadcast | ||
| import org.apache.spark.deploy.{LocalSparkCluster, SparkHadoopUtil} | ||
| import org.apache.spark.deploy.StandaloneResourceUtils._ | ||
| import org.apache.spark.executor.ExecutorMetrics | ||
| import org.apache.spark.executor.{ExecutorMetrics, ExecutorMetricsSource} | ||
| import org.apache.spark.input.{FixedLengthBinaryInputFormat, PortableDataStream, StreamInputFormat, WholeTextFileInputFormat} | ||
| import org.apache.spark.internal.Logging | ||
| import org.apache.spark.internal.config._ | ||
|
|
@@ -551,9 +551,16 @@ class SparkContext(config: SparkConf) extends Logging { | |
| _dagScheduler = new DAGScheduler(this) | ||
| _heartbeatReceiver.ask[Boolean](TaskSchedulerIsSet) | ||
|
|
||
| val executorMetricsSource = | ||
| if (_conf.get(METRICS_EXECUTORMETRICS_SOURCE_ENABLED)) { | ||
| new ExecutorMetricsSource | ||
| } else { | ||
| null | ||
| } | ||
|
|
||
| // create and start the heartbeater for collecting memory metrics | ||
| _heartbeater = new Heartbeater( | ||
| () => SparkContext.this.reportHeartBeat(), | ||
| () => SparkContext.this.reportHeartBeat(executorMetricsSource), | ||
| "driver-heartbeater", | ||
| conf.get(EXECUTOR_HEARTBEAT_INTERVAL)) | ||
| _heartbeater.start() | ||
|
|
@@ -622,6 +629,10 @@ class SparkContext(config: SparkConf) extends Logging { | |
| _env.metricsSystem.registerSource(_dagScheduler.metricsSource) | ||
| _env.metricsSystem.registerSource(new BlockManagerSource(_env.blockManager)) | ||
| _env.metricsSystem.registerSource(new JVMCPUSource()) | ||
| if (executorMetricsSource != null) { | ||
| executorMetricsSource.register | ||
| env.metricsSystem.registerSource(executorMetricsSource) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead how about changing these two lines to |
||
| } | ||
| _executorAllocationManager.foreach { e => | ||
| _env.metricsSystem.registerSource(e.executorAllocationManagerSource) | ||
| } | ||
|
|
@@ -2473,8 +2484,10 @@ class SparkContext(config: SparkConf) extends Logging { | |
| } | ||
|
|
||
| /** Reports heartbeat metrics for the driver. */ | ||
| private def reportHeartBeat(): Unit = { | ||
| private def reportHeartBeat(executorMetricsSource: ExecutorMetricsSource): Unit = { | ||
| val currentMetrics = ExecutorMetrics.getCurrentMetrics(env.memoryManager) | ||
| executorMetricsSource.updateMetricsSnapshot(currentMetrics) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. won't this be an NPE if its not enabled? another reason to use an Option here. |
||
|
|
||
| val driverUpdates = new HashMap[(Int, Int), ExecutorMetrics] | ||
| // In the driver, we do not track per-stage metrics, so use a dummy stage for the key | ||
| driverUpdates.put(EventLoggingListener.DRIVER_STAGE_KEY, new ExecutorMetrics(currentMetrics)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,10 +113,21 @@ private[spark] class Executor( | |
| // create. The map key is a task id. | ||
| private val taskReaperForTask: HashMap[Long, TaskReaper] = HashMap[Long, TaskReaper]() | ||
|
|
||
| val executorMetricsSource = | ||
| if (conf.get(METRICS_EXECUTORMETRICS_SOURCE_ENABLED)) { | ||
| new ExecutorMetricsSource | ||
| } else { | ||
| null | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here on using an Option |
||
| } | ||
|
|
||
| if (!isLocal) { | ||
| env.blockManager.initialize(conf.getAppId) | ||
| env.metricsSystem.registerSource(executorSource) | ||
| env.metricsSystem.registerSource(new JVMCPUSource()) | ||
| if (conf.get(METRICS_EXECUTORMETRICS_SOURCE_ENABLED)) { | ||
| executorMetricsSource.register | ||
| env.metricsSystem.registerSource(executorMetricsSource) | ||
| } | ||
| env.metricsSystem.registerSource(env.blockManager.shuffleMetricsSource) | ||
| } | ||
|
|
||
|
|
@@ -210,7 +221,8 @@ private[spark] class Executor( | |
| // Poller for the memory metrics. Visible for testing. | ||
| private[executor] val metricsPoller = new ExecutorMetricsPoller( | ||
| env.memoryManager, | ||
| METRICS_POLLING_INTERVAL_MS) | ||
| METRICS_POLLING_INTERVAL_MS, | ||
| executorMetricsSource) | ||
|
|
||
| // Executor for the heartbeat task. | ||
| private val heartbeater = new Heartbeater( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.executor | ||
|
|
||
| import com.codahale.metrics.{Gauge, MetricRegistry} | ||
|
|
||
| import org.apache.spark.metrics.ExecutorMetricType | ||
| import org.apache.spark.metrics.source.Source | ||
|
|
||
| private[spark] class ExecutorMetricsSource extends Source { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd include some comments here explaining why this is setup this way (eg. we're exposing metrics that are a little expensive to check, so a couple of optimizations; (1) the procfs metrics are gathered all in one-go; (2) the values are not checked too often; (3) we re-use the cached values for spark internal stage-level metrics. |
||
|
|
||
| override val metricRegistry = new MetricRegistry() | ||
| override val sourceName = "ExecutorMetrics" | ||
| @volatile var metricsSnapshot: Array[Long] = Array.fill(ExecutorMetricType.numMetrics)(0L) | ||
|
|
||
| // called by ExecutorMetricsPoller | ||
| def updateMetricsSnapshot(metricsUpdates: Array[Long]): Unit = { | ||
| metricsSnapshot = metricsUpdates | ||
| } | ||
|
|
||
| class ExecutorMetricGauge(idx: Int) extends Gauge[Long] { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made this class private when I merged it |
||
| def getValue: Long = metricsSnapshot(idx) | ||
| } | ||
|
|
||
| def register: Unit = { | ||
| // This looks like a bunch of independent gauges as far the metric system | ||
| // is concerned, but actually they're all using one shared snapshot. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this was my comment originally, but I meant it more to compare against other approaches, I think its confusing for someone new to this trying to understand what is going on here. I'd change to something like This takes the values of the metrics we have already computed and stored in our snapshot, and exposes them as individual gauges for the metric system. This means the value never gets updated when polled from the metric system, only when we decide the ExecutorMetricPoller updates the snapshot. |
||
| val gauges: IndexedSeq[ExecutorMetricGauge] = (0 until ExecutorMetricType.numMetrics).map { | ||
| idx => new ExecutorMetricGauge(idx) | ||
| }.toIndexedSeq | ||
|
|
||
| ExecutorMetricType.metricToOffset.foreach { | ||
| case (name, idx) => | ||
| metricRegistry.register(MetricRegistry.name(name), gauges(idx)) | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use an
Optionhere, like we do for other optional members of SparkContext