[SPARK-18364][YARN] Expose metrics for YarnShuffleService #17401

ash211 · 2017-03-23T20:43:43Z

Registers the shuffle server's metrics with the Hadoop Node Manager's
DefaultMetricsSystem.

What changes were proposed in this pull request?

Expose the shuffle service metrics not only on the ExternalShuffleService as done in SPARK-16405, but also in the YarnShuffleService. Because the YARN Node Manager's metrics system is the Hadoop-internal one and not the Dropwizard Metrics system that Spark uses, this requires a conversion from DW metrics to Hadoop metrics.

How was this patch tested?

Manual deploy of new yarn-shuffle jar into a Node Manager and verifying that the metrics appear in the Node Manager-standard location. This is JMX with an query endpoint at /jmx.

Resulting metrics look like this:

[user@host ~]$ curl -sk -XGET https://`hostname -f`:8042/jmx | jq . | grep 'shuffleservice' -B 1 -A 18
    {
      "name": "Hadoop:service=NodeManager,name=shuffleservice",
      "modelerType": "shuffleservice",
      "tag.Hostname": "<redacted>",
      "openBlockRequestLatencyMillis_count": 1,
      "openBlockRequestLatencyMillis_rate15": 0.0011080303990206543,
      "openBlockRequestLatencyMillis_rate5": 0.0033057092356765017,
      "openBlockRequestLatencyMillis_rate1": 0.015991117074135343,
      "openBlockRequestLatencyMillis_rateMean": 0.003843993699021382,
      "blockTransferRateBytes_count": 118,
      "blockTransferRateBytes_rate15": 0.1307475870844372,
      "blockTransferRateBytes_rate5": 0.39007368980982715,
      "blockTransferRateBytes_rate1": 1.8869518147479705,
      "blockTransferRateBytes_rateMean": 0.45359183094454836,
      "registeredExecutorsSize": 2,
      "registerExecutorRequestLatencyMillis_count": 2,
      "registerExecutorRequestLatencyMillis_rate15": 0.001697343764758814,
      "registerExecutorRequestLatencyMillis_rate5": 0.002970701813078509,
      "registerExecutorRequestLatencyMillis_rate1": 0.0005857750515146702,
      "registerExecutorRequestLatencyMillis_rateMean": 0.007687995987242345
    },
[user@host ~]$

Registers the shuffle server's metrics with the Hadoop Node Manager's DefaultMetricsSystem.

SparkQA · 2017-03-23T21:02:27Z

Test build #75115 has finished for PR 17401 at commit 4caf4d2.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class YarnShuffleServiceMetrics implements MetricsSource

jerryshao · 2017-03-24T04:44:31Z

Actually we follow two space indent for java code in Spark, would you please change the format?

ash211 · 2017-03-24T05:29:38Z

Thanks for taking a look @jerryshao ! I've reformatted to two-space indentation and run ./dev/lint-java to make sure this code passes the linter.

src/main/java/org/apache/spark/sql/streaming/GroupStateTimeout.java still has some errors but those are separate from this change

jerryshao · 2017-03-24T05:39:10Z

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java

+   * @param all       if true, return all metrics even if unchanged.
+   */
+  @Override
+  public void getMetrics(MetricsCollector collector, boolean all) {


Is it possible to add a unit test to verify the correctness of converting codahale metrics to Hadoop metrics?

should be able to, I'm working on creating one now. By correctness, I think you mostly mean that the values passed through are the same, even though the naming schemes are different?

Yes, you're right. From my understanding the correctness means Gauge still coverts to Gauge, Meter still to Meter, not sure can it be guaranteed?

jerryshao · 2017-03-24T05:41:33Z

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java

+        Method registerSourceMethod = metricsSystem.getClass().getDeclaredMethod("registerSource",
+                String.class, String.class, MetricsSource.class);
+        registerSourceMethod.setAccessible(true);
+        registerSourceMethod.invoke(metricsSystem, "shuffleservice", "Metrics on the Spark " +


nit. "shuffleService" camel case might be better.

will change shortly

SparkQA · 2017-03-24T05:48:54Z

Test build #75150 has finished for PR 17401 at commit 13aa4ff.

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

ash211 · 2017-03-24T07:15:33Z

Thanks again for the comments @jerryshao ! I've now added some tests to verify that the metrics get converted in the expected way to the collector, and camel-cased shuffleService

SparkQA · 2017-03-24T07:19:33Z

Test build #75157 has finished for PR 17401 at commit 9992c10.

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

SparkQA · 2017-03-24T19:30:59Z

Test build #75165 has finished for PR 17401 at commit 96a0882.

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

ash211 · 2017-03-24T21:20:40Z

Ready for further review.

jerryshao · 2017-03-27T08:56:55Z

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java

+  }
+
+  @VisibleForTesting
+  public static void collectMetric(MetricsRecordBuilder metricsRecordBuilder, String name, Metric metric) {


Is it necessary to use static here? Looks like here it is it is only for the test convenience.

I use static here to make it clear that the method does not need to be run in the context of an instance. This prevents it from accidentally accessing instance variables when I don't intend it to

jerryshao · 2017-03-27T08:58:18Z

...agers/yarn/src/test/scala/org/apache/spark/network/yarn/YarnShuffleServiceMetricsSuite.scala

+import org.mockito.Matchers._
+import org.mockito.Mockito.{mock, times, verify, when}
+import org.scalatest.Matchers
+import scala.collection.JavaConverters._


The import ordering is not correct, Scala package should be before the third party packages.

jerryshao · 2017-03-27T09:00:12Z

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java

+        MetricsSystemImpl metricsSystem = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+
+        Method registerSourceMethod = metricsSystem.getClass().getDeclaredMethod("registerSource",
+                String.class, String.class, MetricsSource.class);


Nit, here I think should be 2 space indent follow the above line.

jerryshao · 2017-03-27T09:15:07Z

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java

+                String.class, String.class, MetricsSource.class);
+        registerSourceMethod.setAccessible(true);
+        registerSourceMethod.invoke(metricsSystem, "shuffleService", "Metrics on the Spark " +
+                "Shuffle Service", serviceMetrics);


With import order specified at http://spark.apache.org/contributing.html

SparkQA · 2017-03-28T19:56:51Z

Test build #75322 has finished for PR 17401 at commit 7c7d6d4.

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

ash211 · 2017-03-28T23:35:27Z

@jerryshao ready for re-review

jerryshao

Sorry for the delay, just some style related comments. We should ping others to take a review.

jerryshao · 2017-04-05T13:26:12Z

...work-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockHandler.java

@@ -176,7 +176,8 @@ private void checkAuth(TransportClient client, String appId) {
  /**
   * A simple class to wrap all shuffle service wrapper metrics
   */
-  private class ShuffleMetrics implements MetricSet {
+  @VisibleForTesting


@VisibleForTesting causes classpath issues. Please note this in the java doc instead (SPARK-11615).

This is a scalastyle output, would be better to remove this annotation.

jerryshao · 2017-04-05T13:28:46Z

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java

@@ -184,7 +204,7 @@ protected void serviceInit(Configuration conf) throws Exception {
      boundPort = port;
      String authEnabledString = authEnabled ? "enabled" : "not enabled";
      logger.info("Started YARN shuffle service for Spark on port {}. " +
-        "Authentication is {}.  Registered executor file is {}", port, authEnabledString,
+          "Authentication is {}.  Registered executor file is {}", port, authEnabledString,


Nit: this 2 space indent looks like not necessary.

jerryshao · 2017-04-05T13:33:05Z

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java

+    }
+  }
+
+  @VisibleForTesting


jerryshao · 2017-04-05T13:41:19Z

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java

+      Gauge m = (Gauge) metric;
+      Object gaugeValue = m.getValue();
+      if (gaugeValue instanceof Integer) {
+        Integer intValue = (Integer) gaugeValue;


Does it mean that we could only handle integer Gauge, what if later on we add different metric in ExternalShuffleBlockHandler? Looks like we should also manually handle the case one by one here. At least we should an else branch for the fallback check.

HyukjinKwon · 2017-06-19T04:54:10Z

gentle ping @ash211. I just wonder if it is active now.

## What changes were proposed in this pull request? This PR is follow-up of closed apache#17401 which only ended due to of inactivity, but its still nice feature to have. Given review by jerryshao taken in consideration and edited: - VisibleForTesting deleted because of dependency conflicts - removed unnecessary reflection for `MetricsSystemImpl` - added more available types for gauge ## How was this patch tested? Manual deploy of new yarn-shuffle jar into a Node Manager and verifying that the metrics appear in the Node Manager-standard location. This is JMX with an query endpoint running on `hostname:port` Resulting metrics look like this: ``` curl -sk -XGET hostname:port | grep -v '#' | grep 'shuffleService' hadoop_nodemanager_openblockrequestlatencymillis_rate15{name="shuffleService",} 0.31428910657834713 hadoop_nodemanager_blocktransferratebytes_rate15{name="shuffleService",} 566144.9983653595 hadoop_nodemanager_blocktransferratebytes_ratemean{name="shuffleService",} 2464409.9678099006 hadoop_nodemanager_openblockrequestlatencymillis_rate1{name="shuffleService",} 1.2893844732240272 hadoop_nodemanager_registeredexecutorssize{name="shuffleService",} 2.0 hadoop_nodemanager_openblockrequestlatencymillis_ratemean{name="shuffleService",} 1.255574678369966 hadoop_nodemanager_openblockrequestlatencymillis_count{name="shuffleService",} 315.0 hadoop_nodemanager_openblockrequestlatencymillis_rate5{name="shuffleService",} 0.7661929192569739 hadoop_nodemanager_registerexecutorrequestlatencymillis_ratemean{name="shuffleService",} 0.0 hadoop_nodemanager_registerexecutorrequestlatencymillis_count{name="shuffleService",} 0.0 hadoop_nodemanager_registerexecutorrequestlatencymillis_rate1{name="shuffleService",} 0.0 hadoop_nodemanager_registerexecutorrequestlatencymillis_rate5{name="shuffleService",} 0.0 hadoop_nodemanager_blocktransferratebytes_count{name="shuffleService",} 6.18271213E8 hadoop_nodemanager_registerexecutorrequestlatencymillis_rate15{name="shuffleService",} 0.0 hadoop_nodemanager_blocktransferratebytes_rate5{name="shuffleService",} 1154114.4881816586 hadoop_nodemanager_blocktransferratebytes_rate1{name="shuffleService",} 574745.0749848988 ``` Closes apache#22485 from mareksimunek/SPARK-18364. Lead-authored-by: marek.simunek <[email protected]> Co-authored-by: Andrew Ash <[email protected]> Signed-off-by: Thomas Graves <[email protected]>

## What changes were proposed in this pull request? This PR is follow-up of closed apache#17401 which only ended due to of inactivity, but its still nice feature to have. Given review by jerryshao taken in consideration and edited: - VisibleForTesting deleted because of dependency conflicts - removed unnecessary reflection for `MetricsSystemImpl` - added more available types for gauge ## How was this patch tested? Manual deploy of new yarn-shuffle jar into a Node Manager and verifying that the metrics appear in the Node Manager-standard location. This is JMX with an query endpoint running on `hostname:port` Resulting metrics look like this: ``` curl -sk -XGET hostname:port | grep -v '#' | grep 'shuffleService' hadoop_nodemanager_openblockrequestlatencymillis_rate15{name="shuffleService",} 0.31428910657834713 hadoop_nodemanager_blocktransferratebytes_rate15{name="shuffleService",} 566144.9983653595 hadoop_nodemanager_blocktransferratebytes_ratemean{name="shuffleService",} 2464409.9678099006 hadoop_nodemanager_openblockrequestlatencymillis_rate1{name="shuffleService",} 1.2893844732240272 hadoop_nodemanager_registeredexecutorssize{name="shuffleService",} 2.0 hadoop_nodemanager_openblockrequestlatencymillis_ratemean{name="shuffleService",} 1.255574678369966 hadoop_nodemanager_openblockrequestlatencymillis_count{name="shuffleService",} 315.0 hadoop_nodemanager_openblockrequestlatencymillis_rate5{name="shuffleService",} 0.7661929192569739 hadoop_nodemanager_registerexecutorrequestlatencymillis_ratemean{name="shuffleService",} 0.0 hadoop_nodemanager_registerexecutorrequestlatencymillis_count{name="shuffleService",} 0.0 hadoop_nodemanager_registerexecutorrequestlatencymillis_rate1{name="shuffleService",} 0.0 hadoop_nodemanager_registerexecutorrequestlatencymillis_rate5{name="shuffleService",} 0.0 hadoop_nodemanager_blocktransferratebytes_count{name="shuffleService",} 6.18271213E8 hadoop_nodemanager_registerexecutorrequestlatencymillis_rate15{name="shuffleService",} 0.0 hadoop_nodemanager_blocktransferratebytes_rate5{name="shuffleService",} 1154114.4881816586 hadoop_nodemanager_blocktransferratebytes_rate1{name="shuffleService",} 574745.0749848988 ``` Closes apache#22485 from mareksimunek/SPARK-18364. Lead-authored-by: marek.simunek <[email protected]> Co-authored-by: Andrew Ash <[email protected]> Signed-off-by: Thomas Graves <[email protected]> (cherry picked from commit a802c69)

[SPARK-18364][YARN] Expose metrics for YarnShuffleService

4caf4d2

Registers the shuffle server's metrics with the Hadoop Node Manager's DefaultMetricsSystem.

2-space indentation on java files; pass ./dev/lint-java

13aa4ff

jerryshao reviewed Mar 24, 2017

View reviewed changes

ash211 added 2 commits March 24, 2017 00:13

Test metric collector gets right converted calls

83b1d73

camel-case shuffleService

9992c10

Pass scalastyle

96a0882

jerryshao reviewed Mar 27, 2017

View reviewed changes

Reformat and organize imports

7c7d6d4

With import order specified at http://spark.apache.org/contributing.html

jerryshao reviewed Apr 5, 2017

View reviewed changes

HyukjinKwon mentioned this pull request Jun 25, 2017

[INFRA] Close stale PRs #18417

Closed

HyukjinKwon mentioned this pull request Jul 31, 2017

[INFRA] Close stale PRs #18780

Closed

asfgit closed this in 3a45c7f Aug 5, 2017

mareksimunek mentioned this pull request Sep 20, 2018

[SPARK-18364][YARN] Expose metrics for YarnShuffleService #22485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-18364][YARN] Expose metrics for YarnShuffleService #17401

[SPARK-18364][YARN] Expose metrics for YarnShuffleService #17401

ash211 commented Mar 23, 2017 •

edited

Loading

SparkQA commented Mar 23, 2017

jerryshao commented Mar 24, 2017

ash211 commented Mar 24, 2017

jerryshao Mar 24, 2017

ash211 Mar 24, 2017

jerryshao Mar 24, 2017

jerryshao Mar 24, 2017

ash211 Mar 24, 2017

SparkQA commented Mar 24, 2017

ash211 commented Mar 24, 2017

SparkQA commented Mar 24, 2017

SparkQA commented Mar 24, 2017

ash211 commented Mar 24, 2017

jerryshao Mar 27, 2017

ash211 Mar 28, 2017

jerryshao Mar 27, 2017

jerryshao Mar 27, 2017 •

edited

Loading

jerryshao Mar 27, 2017

SparkQA commented Mar 28, 2017

ash211 commented Mar 28, 2017

jerryshao left a comment

jerryshao Apr 5, 2017

jerryshao Apr 5, 2017

jerryshao Apr 5, 2017

jerryshao Apr 5, 2017

HyukjinKwon commented Jun 19, 2017

[SPARK-18364][YARN] Expose metrics for YarnShuffleService #17401

[SPARK-18364][YARN] Expose metrics for YarnShuffleService #17401

Conversation

ash211 commented Mar 23, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Mar 23, 2017

jerryshao commented Mar 24, 2017

ash211 commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 24, 2017

ash211 commented Mar 24, 2017

SparkQA commented Mar 24, 2017

SparkQA commented Mar 24, 2017

ash211 commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 28, 2017

ash211 commented Mar 28, 2017

jerryshao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Jun 19, 2017

ash211 commented Mar 23, 2017 •

edited

Loading

jerryshao Mar 27, 2017 •

edited

Loading