[SPARK-28475][CORE] Add regex MetricFilter to GraphiteSink #25232

nkarpov · 2019-07-22T17:41:07Z

What changes were proposed in this pull request?

Today all registered metric sources are reported to GraphiteSink with no filtering mechanism, although the codahale project does support it.

GraphiteReporter (ScheduledReporter) from the codahale project requires you implement and supply the MetricFilter interface (there is only a single implementation by default in the codahale project, MetricFilter.ALL).

Propose to add an additional regex config to match and filter metrics to the GraphiteSink

How was this patch tested?

Included a GraphiteSinkSuite that tests:

Absence of regex filter (existing default behavior maintained)
Presence of regex=<regexexpr> correctly filters metric keys

dongjoon-hyun · 2019-07-22T22:25:31Z

ok to test

dongjoon-hyun · 2019-07-22T22:26:39Z

Thank you for your first contribution, @nkarpov . Could you elaborate more about the verification steps in your PR?

SparkQA · 2019-07-22T22:33:12Z

Test build #108023 has finished for PR 25232 at commit eaf37c8.

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

dongjoon-hyun · 2019-07-22T22:54:17Z

core/src/main/scala/org/apache/spark/metrics/sink/GraphiteSink.scala

-import com.codahale.metrics.MetricRegistry
+import com.codahale.metrics.{Metric, MetricFilter, MetricRegistry}
 import com.codahale.metrics.graphite.{Graphite, GraphiteReporter, GraphiteUDP}
-


This empty line is one of the required Apache Spark coding styles.

[error] /home/jenkins/workspace/SparkPullRequestBuilder/core/src/main/scala/org/apache/spark/metrics/sink/GraphiteSink.scala:25:0: There should at least one a single empty line separating groups 3rdParty and spark.

You can run the code style checker by the following.

$ dev/scalastyle

nkarpov · 2019-07-22T23:49:31Z

Hi @dongjoon-hyun!

Setup: start a graphite server & add host, port etc. in metrics.properties

For each scenario below step through w/ debugger to confirm MetricFilter is registered and corresponding metrics are posted to graphite server:

Did not provide driver.sink.graphite.regex in metrics.properties -> successfully defaults to MetricFilter.ALL & all (driver) metrics reported
Did provide driver.sink.graphite.regex=streaming in metrics.properties, enable streaming metrics configuration (spark.conf.set("spark.sql.streaming.metricsEnabled", true)) & start a MemoryStream to console -> successfully registers anonymous MetricFilter in this PR and reports the 6 metrics that are expected to match regex expression streaming

I couldn't find an existing testing suite for the GraphiteSink so I verified manually as above. The change seemed minor enough for that to be OK but let me know if a more robust suite should be added.

Also, if we're happy with the naming convention here, I will add documentation in the following files as part of the PR

https://github.com/apache/spark/blob/master/docs/monitoring.md &
https://github.com/apache/spark/blob/master/conf/metrics.properties.template

HyukjinKwon · 2019-07-23T01:23:53Z

@nkarpov, please update this comment in the PR description under How was this patch tested?.

Also, since there is no test, it would be better to describe, with commands, step by step from scratch. With explicit steps, we and our descendants later can detect regressions later other fixes come. Otherwise, every time we should parse your steps and come up with the proper commands.

Ideally it should be copy-and-pastable to reduce human efforts since there's no test case that detects regression automatically.

SparkQA · 2019-07-23T02:24:01Z

Test build #108026 has finished for PR 25232 at commit d0543a0.

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

nkarpov · 2019-07-23T06:41:49Z

Thanks @HyukjinKwon - in that case it's worth to just add the tests. Added in latest commit.

SparkQA · 2019-07-23T07:05:02Z

Test build #108036 has finished for PR 25232 at commit 4314fa7.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class GraphiteSinkSuite extends SparkFunSuite

HyukjinKwon · 2019-07-23T07:07:41Z

Thanks for adding a test.

HyukjinKwon · 2019-07-23T07:07:49Z

retest this please

SparkQA · 2019-07-23T10:13:07Z

Test build #108039 has finished for PR 25232 at commit 4314fa7.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class GraphiteSinkSuite extends SparkFunSuite

HeartSaVioR · 2019-07-24T03:52:52Z

core/src/test/scala/org/apache/spark/metrics/sink/GraphiteSinkSuite.scala

+    val props = new Properties
+    props.put("host", "127.0.0.1")
+    props.put("port", "54321")
+    props.put("regex", "streaming")


It would be ideal to have regex pattern on test case, as we support regex.

HeartSaVioR · 2019-07-24T04:04:04Z

Thanks for the contribution. I'm seeing the benefit on the change as I introduced similar thing (not for Spark though), but it also ended up with advanced (complicated) supports: accepting multiple patterns, with mode either "whitelist" or "blacklist" (not both, of course).

Hopefully I'm not seeing too many Dropwizard metrics in Spark so this might be good enough to start.

nkarpov · 2019-07-24T15:59:52Z

Thanks @HeartSaVioR - I've added a better regex example in the test. And you make a good point re: whitelist/blacklist - I think it'll be clear with an example in metrics.properties template? Should we rename the config?

Otherwise, @HeartSaVioR @HyukjinKwon @dongjoon-hyun, it's my first commit so please help me understand what it takes to be merged :) My only outstanding item ATM is to include an example in metrics.properties template file if we are good on naming convention.

SparkQA · 2019-07-24T18:44:22Z

Test build #108111 has finished for PR 25232 at commit 72754cd.

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

HeartSaVioR · 2019-07-25T00:50:03Z

I'm also one of contributors trying to help reviewing. :) @HyukjinKwon and @dongjoon-hyun are able to review and approve the patch. So let's wait their next round of reviews.

HyukjinKwon · 2019-07-25T01:51:59Z

Can we make the tests passed? Also, you can cc people who appears in Git blame. cc @jerryshao

nkarpov · 2019-07-25T17:02:37Z

I've appended some documentation. I don't think the previous failures were related

SparkQA · 2019-07-25T20:37:25Z

Test build #108180 has finished for PR 25232 at commit 768777a.

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

HeartSaVioR

LGTM, but as I said earlier you need to get approvals from committers.

jerryshao · 2019-08-02T02:44:24Z

LGTM. Trigger the test again.

jerryshao · 2019-08-02T02:44:33Z

Jenkins, retest this please.

SparkQA · 2019-08-02T05:01:46Z

Test build #108540 has finished for PR 25232 at commit 768777a.

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

jerryshao

LGTM.

jerryshao · 2019-08-02T09:38:12Z

Thanks for the contribution, merging to master branch.

dongjoon-hyun · 2019-08-02T16:18:57Z

Thank you so much, @jerryshao ! :D

Add regex filter to GraphiteSink

eaf37c8

dongjoon-hyun added the SPARK CORE label Jul 22, 2019

dongjoon-hyun reviewed Jul 22, 2019

View reviewed changes

readding removed empty line separating spark & 3rd party libs

d0543a0

adding tests

4314fa7

HeartSaVioR reviewed Jul 24, 2019

View reviewed changes

added better regex test

72754cd

add new config to doc & template

768777a

HeartSaVioR approved these changes Aug 2, 2019

View reviewed changes

jerryshao approved these changes Aug 2, 2019

View reviewed changes

jerryshao closed this in 6d32dee Aug 2, 2019

[SPARK-28475][CORE] Add regex MetricFilter to GraphiteSink #25232

[SPARK-28475][CORE] Add regex MetricFilter to GraphiteSink #25232

Uh oh!

Conversation

nkarpov commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

dongjoon-hyun commented Jul 22, 2019

Uh oh!

dongjoon-hyun commented Jul 22, 2019

Uh oh!

SparkQA commented Jul 22, 2019

Uh oh!

dongjoon-hyun Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

nkarpov commented Jul 22, 2019

Uh oh!

HyukjinKwon commented Jul 23, 2019

Uh oh!

SparkQA commented Jul 23, 2019

Uh oh!

nkarpov commented Jul 23, 2019

Uh oh!

SparkQA commented Jul 23, 2019

Uh oh!

HyukjinKwon commented Jul 23, 2019

Uh oh!

HyukjinKwon commented Jul 23, 2019

Uh oh!

SparkQA commented Jul 23, 2019

Uh oh!

HeartSaVioR Jul 24, 2019

Choose a reason for hiding this comment

Uh oh!

nkarpov Jul 24, 2019

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Jul 24, 2019

Uh oh!

nkarpov commented Jul 24, 2019

Uh oh!

SparkQA commented Jul 24, 2019

Uh oh!

HeartSaVioR commented Jul 25, 2019

Uh oh!

HyukjinKwon commented Jul 25, 2019

Uh oh!

nkarpov commented Jul 25, 2019

Uh oh!

SparkQA commented Jul 25, 2019

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

jerryshao commented Aug 2, 2019

Uh oh!

jerryshao commented Aug 2, 2019

Uh oh!

SparkQA commented Aug 2, 2019

Uh oh!

jerryshao left a comment

Choose a reason for hiding this comment

Uh oh!

jerryshao commented Aug 2, 2019

Uh oh!

dongjoon-hyun commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nkarpov commented Jul 22, 2019 •

edited

Loading