[SPARK-5847][CORE] Allow for configuring MetricsSystem's use of app ID to namespace all metrics #14270

markgrover · 2016-07-19T19:47:13Z

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.

markgrover · 2016-07-19T20:42:58Z

A few design choices I made along the way are:

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, 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.
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, for example).
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 [SPARK-5847] Allow for namespacing metrics by conf params other than spark.app.id #4632 made the same call and I agree with him.

SparkQA · 2016-07-19T21:55:41Z

Test build #62549 has finished for PR 14270 at commit 605e690.

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

markgrover · 2016-07-19T22:48:15Z

Tests passed! I'd really appreciate a review!

vanzin · 2016-07-20T00:06:34Z

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.

ericl · 2016-07-20T20:28:21Z

+1 on using the config refs provided by #14022

markgrover · 2016-07-21T16:50:05Z

ok, thanks, I will rely on changes in #14022 (SPARK-16272), now that it's been committed.

markgrover · 2016-07-23T02:04:42Z

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.

SparkQA · 2016-07-23T04:05:12Z

Test build #62744 has finished for PR 14270 at commit 3c8ea96.

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

vanzin · 2016-07-25T19:50:29Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

Instead of createOptional you can use createWithDefault("${spark.app.id}").

Thanks, but I think that changes the semantics in cases where spark.app.id is not defined.
Currently, the code in this PR 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.

vanzin · 2016-07-26T18:41:28Z

Minor nit otherwise LGTM. Tests could use shorter names. Also, now there's a conflict...

…D 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.

markgrover · 2016-07-26T23:39:31Z

Fixed the nits, resolved the merge conflict. Shortened some test names.

…r newly added tests

SparkQA · 2016-07-27T01:43:40Z

Test build #62903 has finished for PR 14270 at commit b9c9a7a.

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

SparkQA · 2016-07-27T01:57:16Z

Test build #62904 has finished for PR 14270 at commit 8923c58.

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

vanzin · 2016-07-27T17:12:56Z

LGTM, merging to master.

markgrover · 2016-07-27T17:23:07Z

Thanks a lot, @vanzin!

Ianwww · 2016-08-25T21:01:41Z

Can this configuration be set in spark-defaults.conf as spark.metrics.namespace=${spark.app.name}
@markgrover

markgrover · 2016-08-25T21:19:48Z

Sure.

…use of app ID to namespace all metrics This is a backport of apache#14270. Because the spark.internal.config system does not exists in branch 1.6, a simpler substitution scheme for ${} in the spark.metrics.namespace value, using only Spark configuration had to be added to preserve the behaviour discussed in the tickets and tested. This backport is contributed by Criteo SA under the Apache v2 licence. 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 any given value or to another spark configuration key reference 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. Added new unit tests, modified existing unit tests.

AnthonyTruchet · 2016-09-09T13:09:32Z

Hello @markgrover @vanzin ! As this is just a backport of your work, would you please consider reviewing it ?

…use of app ID to namespace all metrics This is a backport of apache#14270. Because the spark.internal.config system does not exists in branch 1.6, a simpler substitution scheme for ${} in the spark.metrics.namespace value, using only Spark configuration had to be added to preserve the behaviour discussed in the tickets and tested. This backport is contributed by Criteo SA under the Apache v2 licence. 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 any given value or to another spark configuration key reference 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. Added new unit tests, modified existing unit tests.

markgrover force-pushed the spark-5847 branch from 605e690 to 3c8ea96 Compare July 23, 2016 02:03

vanzin reviewed Jul 25, 2016
View reviewed changes

markgrover added 5 commits July 26, 2016 16:22

Minor fixes based on self-reviewing

95356ca

Basing the work to use expansion capabilities brought in by SPARK-16272

6b02fe1

Incorporating review feedback

de1d431

Shorterning test names

b9c9a7a

markgrover force-pushed the spark-5847 branch from 3c8ea96 to b9c9a7a Compare July 26, 2016 23:39

Making the last test use set(ConfigEntry) as well, just like the othe…

8923c58

…r newly added tests

asfgit closed this in 70f846a Jul 27, 2016

AnthonyTruchet mentioned this pull request Sep 9, 2016

Backport [SPARK-5847] Allow for configuring MetricsSystem's prefix #15023

Closed

ianlcsd mentioned this pull request Jul 1, 2017

[SPARK-5847][CORE][BRANCH-1.6] backport of #14270 alteryx/spark#188

Closed

[SPARK-5847][CORE] Allow for configuring MetricsSystem's use of app ID to namespace all metrics #14270

[SPARK-5847][CORE] Allow for configuring MetricsSystem's use of app ID to namespace all metrics #14270

Uh oh!

Conversation

markgrover commented Jul 19, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

markgrover commented Jul 19, 2016

Uh oh!

SparkQA commented Jul 19, 2016

Uh oh!

markgrover commented Jul 19, 2016

Uh oh!

vanzin commented Jul 20, 2016

Uh oh!

ericl commented Jul 20, 2016

Uh oh!

markgrover commented Jul 21, 2016

Uh oh!

markgrover commented Jul 23, 2016

Uh oh!

SparkQA commented Jul 23, 2016

Uh oh!

vanzin Jul 25, 2016

Choose a reason for hiding this comment

Uh oh!

markgrover Jul 26, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin commented Jul 26, 2016

Uh oh!

markgrover commented Jul 26, 2016

Uh oh!

SparkQA commented Jul 27, 2016

Uh oh!

SparkQA commented Jul 27, 2016

Uh oh!

vanzin commented Jul 27, 2016

Uh oh!

markgrover commented Jul 27, 2016

Uh oh!

Ianwww commented Aug 25, 2016

Uh oh!

markgrover commented Aug 25, 2016

Uh oh!

AnthonyTruchet commented Sep 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants