You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by markgrover <gi...@git.apache.org> on 2016/07/19 19:47:15 UTC
[GitHub] spark pull request #14270: [SPARK-5847][CORE] Allow for configuring MetricsS...
GitHub user markgrover opened a pull request:
https://github.com/apache/spark/pull/14270
[SPARK-5847][CORE] Allow for configuring MetricsSystem's use of app ID to namespace all metrics
## What changes were proposed in this pull request?
Adding a new property to SparkConf called spark.metrics.namespace that allows users to
set a custom namespace for executor and driver metrics in the metrics systems.
By default, the root namespace used for driver or executor metrics is
the value of `spark.app.id`. However, often times, users want to be able to track the metrics
across apps for driver and executor metrics, which is hard to do with application ID
(i.e. `spark.app.id`) since it changes with every invocation of the app. For such use cases,
users can set the `spark.metrics.namespace` property to another spark configuration key like
`spark.app.name` which is then used to populate the root namespace of the metrics system
(with the app name in our example). `spark.metrics.namespace` property can be set to any
arbitrary spark property key, whose value would be used to set the root namespace of the
metrics system. Non driver and executor metrics are never prefixed with `spark.app.id`, nor
does the `spark.metrics.namespace` property have any such affect on such metrics.
## How was this patch tested?
Added new unit tests, modified existing unit tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/markgrover/spark spark-5847
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/14270.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #14270
----
commit 7d023c820cae3b970e11b2278f6c2b3f3751cc3b
Author: Mark Grover <ma...@apache.org>
Date: 2016-07-19T05:38:31Z
[SPARK-5847][CORE] Allow for configuring MetricsSystem's use of app ID to namespace all metrics
Adding a new property to SparkConf called spark.metrics.namespace that allows users to
set a custom namespace for executor and driver metrics in the metrics systems.
By default, the root namespace used for driver or executor metrics is
the value of `spark.app.id`. However, often times, users want to be able to track the metrics
across apps for driver and executor metrics, which is hard to do with application ID
(i.e. `spark.app.id`) since it changes with every invocation of the app. For such use cases,
users can set the `spark.metrics.namespace` property to another spark configuration key like
`spark.app.name` which is then used to populate the root namespace of the metrics system
(with the app name in our example). `spark.metrics.namespace` property can be set to any
arbitrary spark property key, whose value would be used to set the root namespace of the
metrics system. Non driver and executor metrics are never prefixed with `spark.app.id`, nor
does the `spark.metrics.namespace` property have any such affect on such metrics.
commit 605e690eefe905fa52979dde16f04bd208c8b219
Author: Mark Grover <ma...@apache.org>
Date: 2016-07-19T19:44:55Z
Minor fixes based on self-reviewing
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by markgrover <gi...@git.apache.org>.
Github user markgrover commented on the issue:
https://github.com/apache/spark/pull/14270
Thanks a lot, @vanzin!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14270
LGTM, merging to master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by markgrover <gi...@git.apache.org>.
Github user markgrover commented on the issue:
https://github.com/apache/spark/pull/14270
ok, thanks, I will rely on changes in #14022 (SPARK-16272), now that it's been committed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14270: [SPARK-5847][CORE] Allow for configuring MetricsS...
Posted by markgrover <gi...@git.apache.org>.
Github user markgrover commented on a diff in the pull request:
https://github.com/apache/spark/pull/14270#discussion_r72357828
--- Diff: core/src/test/scala/org/apache/spark/metrics/MetricsSystemSuite.scala ---
@@ -183,4 +184,89 @@ class MetricsSystemSuite extends SparkFunSuite with BeforeAndAfter with PrivateM
assert(metricName != s"$appId.$executorId.${source.sourceName}")
assert(metricName === source.sourceName)
}
+
+ test("MetricsSystem with Executor instance, with custom namespace") {
+ val source = new Source {
+ override val sourceName = "dummySource"
+ override val metricRegistry = new MetricRegistry()
+ }
+
+ val appId = "testId"
+ val appName = "testName"
+ val executorId = "1"
+ conf.set("spark.app.id", appId)
+ conf.set("spark.app.name", appName)
+ conf.set("spark.executor.id", executorId)
+ conf.set(METRICS_NAMESPACE, "${spark.app.name}")
+
+ val instanceName = "executor"
+ val driverMetricsSystem = MetricsSystem.createMetricsSystem(instanceName, conf, securityMgr)
+
+ val metricName = driverMetricsSystem.buildRegistryName(source)
+ assert(metricName === s"$appName.$executorId.${source.sourceName}")
+ }
+
+ test("MetricsSystem with Executor instance and custom namespace which is not set") {
+ val source = new Source {
+ override val sourceName = "dummySource"
+ override val metricRegistry = new MetricRegistry()
+ }
+
+ val executorId = "1"
+ val namespaceToResolve = "${spark.doesnotexist}"
+ conf.set("spark.executor.id", executorId)
+ conf.set(METRICS_NAMESPACE, namespaceToResolve)
+
+ val instanceName = "executor"
+ val driverMetricsSystem = MetricsSystem.createMetricsSystem(instanceName, conf, securityMgr)
+
+ val metricName = driverMetricsSystem.buildRegistryName(source)
+ // If the user set the spark.metrics.namespace property to an expansion of another property
+ // (say ${spark.doesnotexist}, the unresolved name (i.e. litterally ${spark.doesnot})
--- End diff --
Appreciate your thoroughness!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14270
Minor nit otherwise LGTM. Tests could use shorter names. Also, now there's a conflict...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14270
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14270
**[Test build #62903 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62903/consoleFull)** for PR 14270 at commit [`b9c9a7a`](https://github.com/apache/spark/commit/b9c9a7aa2b831247ae04d655f537223a02bc8440).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14270: [SPARK-5847][CORE] Allow for configuring MetricsS...
Posted by markgrover <gi...@git.apache.org>.
Github user markgrover commented on a diff in the pull request:
https://github.com/apache/spark/pull/14270#discussion_r72357797
--- Diff: core/src/main/scala/org/apache/spark/metrics/MetricsSystem.scala ---
@@ -125,19 +126,26 @@ private[spark] class MetricsSystem private (
* application, executor/driver and metric source.
*/
private[spark] def buildRegistryName(source: Source): String = {
- val appId = conf.getOption("spark.app.id")
+ val metricsNamespace = conf.get(METRICS_NAMESPACE).map(Some(_))
--- End diff --
Good point, thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14270
**[Test build #62744 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62744/consoleFull)** for PR 14270 at commit [`3c8ea96`](https://github.com/apache/spark/commit/3c8ea966c5d3f356ad9fd4bead3fd2ced236c6bd).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by Ianwww <gi...@git.apache.org>.
Github user Ianwww commented on the issue:
https://github.com/apache/spark/pull/14270
Can this configuration be set in spark-defaults.conf as spark.metrics.namespace=${spark.app.name}
@markgrover
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by ericl <gi...@git.apache.org>.
Github user ericl commented on the issue:
https://github.com/apache/spark/pull/14270
+1 on using the config refs provided by #14022
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14270
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by markgrover <gi...@git.apache.org>.
Github user markgrover commented on the issue:
https://github.com/apache/spark/pull/14270
Sure.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14270
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by AnthonyTruchet <gi...@git.apache.org>.
Github user AnthonyTruchet commented on the issue:
https://github.com/apache/spark/pull/14270
Hello @markgrover @vanzin ! As this is just a backport of your work, would you please consider reviewing it ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14270
**[Test build #62744 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62744/consoleFull)** for PR 14270 at commit [`3c8ea96`](https://github.com/apache/spark/commit/3c8ea966c5d3f356ad9fd4bead3fd2ced236c6bd).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14270
**[Test build #62904 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62904/consoleFull)** for PR 14270 at commit [`8923c58`](https://github.com/apache/spark/commit/8923c58d324b8083ffb423d165f4707ec4395db2).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14270
**[Test build #62903 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62903/consoleFull)** for PR 14270 at commit [`b9c9a7a`](https://github.com/apache/spark/commit/b9c9a7aa2b831247ae04d655f537223a02bc8440).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14270: [SPARK-5847][CORE] Allow for configuring MetricsS...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/14270
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by markgrover <gi...@git.apache.org>.
Github user markgrover commented on the issue:
https://github.com/apache/spark/pull/14270
A few design choices I made along the way are:
1. Use a `SparkConf` property to control the namespace instead of a `MetricsConfig` property. The reason is that metrics config properties mean something particular and follow a particular pattern (see [here](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/metrics/MetricsSystem.scala#L54), for example). And, regardless of what we name this property, it doesn't follow that paradigm - the first part of the property name before the first dot doesn't represent an instance (like master, worker, executor, etc.) like it does for legit metrics properties. Also, the properties defined in `MetricsConfig` are obtained using the `getInstance()` call, where the instance is passed and again, such a retrieval doesn't really apply to this configuration property since it doesn't belong to an instance. I understand it's one more property in `SparkConf` but I really feel its belongs there, right by `spark.metrics.conf`.
2. Based on the previous point, this property shouldn't fall under the `spark.metrics.conf.` prefix. This is because if it does, it's mistakenly understood as a legit metrics property (see code [here](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala#L54), for example).
3. The proposed `spark.metrics.namespace` property allows one to specify an arbitrary property to be used as namespace. I did consider whitespacing it so users can use `spark.app.name` or `spark.app.id` only but then we'd have to deal with magic strings(app.name|app.id), and I didn't really feel inclined to do that. And, @ryan-williams who took a stab at the same issue #4632 made the same call and I agree with him.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14270
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by markgrover <gi...@git.apache.org>.
Github user markgrover commented on the issue:
https://github.com/apache/spark/pull/14270
Tests passed! I'd really appreciate a review!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14270
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62549/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14270: [SPARK-5847][CORE] Allow for configuring MetricsS...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14270#discussion_r72131840
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -103,4 +103,9 @@ package object config {
.stringConf
.checkValues(Set("hive", "in-memory"))
.createWithDefault("in-memory")
+
+ // This property sets the root namespace for metrics reporting
+ private[spark] val METRICS_NAMESPACE = ConfigBuilder("spark.metrics.namespace")
+ .stringConf
+ .createOptional
--- End diff --
Instead of `createOptional` you can use `createWithDefault("${spark.app.id}")`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14270
**[Test build #62549 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62549/consoleFull)** for PR 14270 at commit [`605e690`](https://github.com/apache/spark/commit/605e690eefe905fa52979dde16f04bd208c8b219).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14270
**[Test build #62904 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62904/consoleFull)** for PR 14270 at commit [`8923c58`](https://github.com/apache/spark/commit/8923c58d324b8083ffb423d165f4707ec4395db2).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14270
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62744/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14270
**[Test build #62549 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62549/consoleFull)** for PR 14270 at commit [`605e690`](https://github.com/apache/spark/commit/605e690eefe905fa52979dde16f04bd208c8b219).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14270
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62904/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14270
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62903/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by markgrover <gi...@git.apache.org>.
Github user markgrover commented on the issue:
https://github.com/apache/spark/pull/14270
Ok, I have pushed changes to use the expansion capabilities brought in by SPARK-16272. Overall, I think it was a very good call to use that, so thanks for the suggestions! Would appreciate a review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14270: [SPARK-5847][CORE] Allow for configuring MetricsS...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14270#discussion_r72310984
--- Diff: core/src/test/scala/org/apache/spark/metrics/MetricsSystemSuite.scala ---
@@ -183,4 +184,89 @@ class MetricsSystemSuite extends SparkFunSuite with BeforeAndAfter with PrivateM
assert(metricName != s"$appId.$executorId.${source.sourceName}")
assert(metricName === source.sourceName)
}
+
+ test("MetricsSystem with Executor instance, with custom namespace") {
+ val source = new Source {
+ override val sourceName = "dummySource"
+ override val metricRegistry = new MetricRegistry()
+ }
+
+ val appId = "testId"
+ val appName = "testName"
+ val executorId = "1"
+ conf.set("spark.app.id", appId)
+ conf.set("spark.app.name", appName)
+ conf.set("spark.executor.id", executorId)
+ conf.set(METRICS_NAMESPACE, "${spark.app.name}")
+
+ val instanceName = "executor"
+ val driverMetricsSystem = MetricsSystem.createMetricsSystem(instanceName, conf, securityMgr)
+
+ val metricName = driverMetricsSystem.buildRegistryName(source)
+ assert(metricName === s"$appName.$executorId.${source.sourceName}")
+ }
+
+ test("MetricsSystem with Executor instance and custom namespace which is not set") {
+ val source = new Source {
+ override val sourceName = "dummySource"
+ override val metricRegistry = new MetricRegistry()
+ }
+
+ val executorId = "1"
+ val namespaceToResolve = "${spark.doesnotexist}"
+ conf.set("spark.executor.id", executorId)
+ conf.set(METRICS_NAMESPACE, namespaceToResolve)
+
+ val instanceName = "executor"
+ val driverMetricsSystem = MetricsSystem.createMetricsSystem(instanceName, conf, securityMgr)
+
+ val metricName = driverMetricsSystem.buildRegistryName(source)
+ // If the user set the spark.metrics.namespace property to an expansion of another property
+ // (say ${spark.doesnotexist}, the unresolved name (i.e. litterally ${spark.doesnot})
--- End diff --
nit: "literally", then "doesnotexist"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14270: [SPARK-5847][CORE] Allow for configuring MetricsS...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14270#discussion_r72310731
--- Diff: core/src/main/scala/org/apache/spark/metrics/MetricsSystem.scala ---
@@ -125,19 +126,26 @@ private[spark] class MetricsSystem private (
* application, executor/driver and metric source.
*/
private[spark] def buildRegistryName(source: Source): String = {
- val appId = conf.getOption("spark.app.id")
+ val metricsNamespace = conf.get(METRICS_NAMESPACE).map(Some(_))
--- End diff --
Since you'd rather not change the constant, this is better written as:
conf.get(METRICS_NAMESPACE).orElse(conf.getOption("spark.app.id"))
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14270: [SPARK-5847][CORE] Allow for configuring MetricsS...
Posted by markgrover <gi...@git.apache.org>.
Github user markgrover commented on a diff in the pull request:
https://github.com/apache/spark/pull/14270#discussion_r72307684
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -103,4 +103,9 @@ package object config {
.stringConf
.checkValues(Set("hive", "in-memory"))
.createWithDefault("in-memory")
+
+ // This property sets the root namespace for metrics reporting
+ private[spark] val METRICS_NAMESPACE = ConfigBuilder("spark.metrics.namespace")
+ .stringConf
+ .createOptional
--- End diff --
Thanks, but I think that changes the semantics in cases where `spark.app.id` is not defined.
Currently, the code [in this PR](https://github.com/apache/spark/pull/14270/files#diff-7ea2624e832b166ca27cd4baca8691d9R129) creates an Option with value `None` if `spark.app.id` is not defined. With the proposed change, it will create an Option with value literal `${spark.app.id}` if `spark.app.id` not defined. And, that changes the existing behavior. Even though in practice, I believe, `spark.app.id` is always defined, there are some tests in MetrisSystemSuite.scala that test the scenario when `spark.app.id` is not defined. And, I am hesitant to change that behavior.
I suppose I could use `createWithDefault()` in package.scala for `METRICS_NAMESPACE` but then I'd have to check if there are any unresolved `spark.app.id` references in the value, but that'd be too big of a pain for not much benefit. So, I'd prefer to keep this the way it is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14270
It seems that the namespace must be the name of a configuration key, and cannot be an arbitrary string?
That sounds a little limiting. You could change the code to use the value of the namespace config as the default, in case there's no config with the name the user provided, to allow some more flexibility. Or maybe if you instead rely on the changes in #14022, then the user would have even more flexibility.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14270: [SPARK-5847][CORE] Allow for configuring MetricsSystem's...
Posted by markgrover <gi...@git.apache.org>.
Github user markgrover commented on the issue:
https://github.com/apache/spark/pull/14270
Fixed the nits, resolved the merge conflict. Shortened some test names.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org