[SPARK-5847] Allow for namespacing metrics by conf params other than spark.app.id

[ryan-williams]

No description provided.

[SparkQA]

Test build #27578 has finished for PR 4632 at commit ffdc06a.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class AnalysisException protected[sql] (
  • implicit class AnalysisErrorAt(t: TreeNode[_])
  • case class Origin(
  • case class IsNull(attribute: String) extends Filter
  • case class IsNotNull(attribute: String) extends Filter
  • case class And(left: Filter, right: Filter) extends Filter
  • case class Or(left: Filter, right: Filter) extends Filter
  • case class Not(child: Filter) extends Filter

[ryan-williams]

scala style check should pass now

[SparkQA]

Test build #27581 has finished for PR 4632 at commit 00df060.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class AnalysisException protected[sql] (
  • implicit class AnalysisErrorAt(t: TreeNode[_])
  • case class Origin(
  • case class IsNull(attribute: String) extends Filter
  • case class IsNotNull(attribute: String) extends Filter
  • case class And(left: Filter, right: Filter) extends Filter
  • case class Or(left: Filter, right: Filter) extends Filter
  • case class Not(child: Filter) extends Filter

[ryan-williams]

This one is ready for review, I think

[SparkQA]

Test build #28076 has finished for PR 4632 at commit 64a7eee.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • logError("User class threw exception: " + cause.getMessage, cause)

[ryan-williams]

bump, @pwendell any ideas who would be a good reviewer for this?

This is a pretty important piece of my grafana-spark-dashboards setup that it would be good to allow others to benefit from.

[pwendell]

Hey @JoshRosen can probably take a look at this. One thing though, IIRC the reason why we have a unique ID here is because some sets of users requested the exact opposite behavior as what you want:

https://issues.apache.org/jira/browse/SPARK-3377

If we do decide to allow this, I think it's best to have a specific set of naming schemes (don't give them a latch to chose an arbitrary conf), in order to limit the surface area of possible configurations. I'm also not so sure about the right way to do this in terms of the current MetricsConfing infrastructure. It seems a bit weird to hard code handling of this particular configuration in the MetricsConfig class. Only had time to take a quick look though and I'm not familiar with that code at all.

[ryan-williams]

Thanks @pwendell. I had stumbled across that SPARK-3377 work as well.

I think there are solid arguments for each of these use-cases being supported:

• app.id-prefixing can be pathologically hard on Graphite's disk I/O / for short-running jobs.
• app.name-prefixing is no good if you have jobs running simultaneously.

Here are three options for supporting both (all defaulting to app.id but providing an escape hatch):

1. Only admit id and name values here, and use the value from the appropriate config key. The main downside is that we would essentially introduce two new, made-up "magic strings" to do this; "id" and "name"? "app.id" and "app.name"? At that point, we're basically at…
2. Allow usage of any existing conf value as the metrics prefix, which is what this PR currently does.
3. Default to app.id but allow the user to specify a string that is used as the metrics' prefix (as opposed to a string that keys into SparkConfig), e.g. --conf spark.metrics.prefix=my-app-name;
   • this could be a --conf param or happen in the MetricsConfig's file.

I feel like doing this via the MetricsConfig's spark.metrics.conf file makes more sense than adding another --conf param, but I could be persuaded otherwise.

It seems a bit weird to hard code handling of this particular configuration in the MetricsConfig class.

This bit I disagree with; plenty of config params are {read by, relevant to} just one class.

[dajac]

Hey @pwendell and @JoshRosen, what is the status on this?

I would like to monitor all my recurrent jobs and having fixed set of metrics which doesn't change between two executions would make it way easier.

[andrewor14]

Perhaps @vanzin or @srowen can look at this?

[srowen]

I have little knowledge of the metrics system. I can appreciate the overhead of yet another config flag, and can also see how you might want to monitor apps by instance (ID) or class (name) so to speak.

The change here seems like a fairly uninvasive version, as you can only name an existing property to draw the value from, and it defaults to current behavior. I can see the argument for limiting it further to "id" or "name" only.

I expect the downside are new bugs: my metrics failed because my app name is <WOOT>. Is there an extra issue of escaping anywhere here? IDs may not have had such a problem.

It seems plausible to me, even if I feel like I don't know enough to review.

[SparkQA]

Test build #42437 has finished for PR 4632 at commit 578162b.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #42444 has finished for PR 4632 at commit 7d0204e.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[ryan-williams]

I'm not sure how to figure out what test failed from the Jenkins output here https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42444/consoleFull

all I see is

[info] Run completed in 1 hour, 58 minutes, 26 seconds.
[info] Total number of tests run: 667
[info] Suites: completed 136, aborted 0
[info] Tests: succeeded 667, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[info] Passed: Total 774, Failed 0, Errors 0, Passed 774
[error] (core/test:test) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 7320 s, completed Sep 14, 2015 4:38:56 PM
[error] running /home/jenkins/workspace/SparkPullRequestBuilder@2/build/sbt -Pyarn -Phadoop-2.3 -Dhadoop.version=2.3.0 -Phive -Pkinesis-asl -Phive-thriftserver test ; received return code 1

at the end and what looks like many passing tests

[vanzin]

https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42444/testReport/

[paulbes]

Being able to configure a fixed set of metrics names is something we are also interested in, what is the progress on this work?

[vanzin]

since you're touching this... for (

[vanzin]

retest this please

[vanzin]

Does docs/monitoring.md need an update to include the new config?

[SparkQA]

Test build #45193 has finished for PR 4632 at commit 7d0204e.

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

[iteratec-hh]

I'm also very interested in this config-param! I'm looking forward to get this in next release :)
+1

[rxin]

I'm going to close this pull request. If this is still relevant and you are interested in pushing it forward, please open a new pull request. Thanks!