[FLINK-6221] Add PrometheusReporter #3833

mbode · 2017-05-06T10:52:02Z

No description provided.

zentol · 2017-05-06T17:01:46Z

I've glanced over this for now but it looks pretty good.

If possible though i would like to not use the DropwizardExports class, for 2 reasons:

We now introduce this odd pattern of extending ScheduledDropwizardReporter without actually being scheduled nor creating an actual Dropwizard reporter.
I've looked into the source code and it has the typical Dropwizard reporter problem where absolutely nothing gets cached and effectively constant objects are re-created again and again. We can make this way more efficient.

mbode · 2017-05-06T18:25:53Z

Thanks for looking at it so quickly! I somewhat had the same instinct as far as your first point is concerned and thought about pulling out a DropwizardReporter without Scheduling but decided against it to not have to touch too many places. I like your suggestion of converting metrics directly without using Dropwizard as an intermediate step and am going to try that.

Edit: Now that I think about it, this might also be advantageous as one could use Prometheus' multi-dimensional data model more flexibly that way.

hadronzoo · 2017-05-11T21:21:25Z

@mbode thanks for working on this!

One thing that I've found useful when exporting Flink's statsd metrics to Prometheus is to make several of the metric fields tags: like job_name, task_name, operator_name, etc. This statsd-exporter mapping has tags that have worked well for me. I'm not tagging host names or IP addresses because Prometheus's Kubernetes support does that already, but that could be useful for people running standalone clusters.

mbode · 2017-05-13T22:02:28Z

@hadronzoo I tried to keep it as general as possible by exporting all variables available to the metric group as labels. I am not sure if this might lead to label overuse at some point, did you ever run into difficulties in that context? Unfortunately I do not have a lot of experience running Prometheus in a production environment yet.

zentol · 2017-05-13T22:26:44Z

As it stands a metric can have at most 10 labels, and there aren't any current plans to extend this set; according to the docs that's still ok. If this does indeed become a problem we can add a switch to rely on the scope formats instead, giving the user control as to how many labels there are.

zentol

I like this a lot more than the previous version.

I haven't found anything major so far; but it would be good to shade the dependencies and get rid of guava as we don't actually use it much.

zentol · 2017-05-13T22:36:13Z

...metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusReporter.java

+
+@PublicEvolving
+public class PrometheusReporter implements MetricReporter {
+	private static final Logger log = LoggerFactory.getLogger(PrometheusReporter.class);


by convention this should be upper-case.

zentol · 2017-05-13T22:37:27Z

flink-metrics/flink-metrics-prometheus/pom.xml

+		</dependency>
+
+		<dependency>
+			<groupId>com.google.guava</groupId>


We try to avoid guava unless absolutely necessary, and virtually all modules that do depend on it shade it away.

zentol · 2017-05-13T22:38:15Z

...metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusReporter.java

+public class PrometheusReporter implements MetricReporter {
+	private static final Logger log = LoggerFactory.getLogger(PrometheusReporter.class);
+
+	static final         String ARG_PORT     = "port";


The indentation here in inconsistent with the rest of the code base.

zentol · 2017-05-13T22:38:36Z

...metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusReporter.java

+	private static final char SCOPE_SEPARATOR = '_';
+
+	private PrometheusEndpoint prometheusEndpoint;
+	private Map<String, Collector> collectorsByMetricName = new HashMap<>();


Could be final.

zentol · 2017-05-13T22:41:08Z

...metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusReporter.java

+		try {
+			prometheusEndpoint.start(NanoHTTPD.SOCKET_READ_TIMEOUT, true);
+		} catch (IOException e) {
+			log.error("Could not start PrometheusEndpoint on port " + port, e);


It would be good to throw an exception here despite the current interface. Otherwise the reporter will still be notified of added/removed metrics, even though there's no benefit to it.

zentol · 2017-05-13T22:43:03Z

...metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusReporter.java

+		} else if (metric instanceof Meter) {
+			collector = createCounter((Meter) metric, validMetricName, metricIdentifier, dimensionKeys, dimensionValues);
+		} else {
+			log.error("Cannot add unknown metric type: {}. This indicates that the metric type is not supported by this reporter.",


By convention classes that are part of the metric system never log something as errors but only warnings, the reason being that errors are something that interfere with the job being executed.

Ok, I changed it. The MetricRegistry has a few error logs in it as well, logging things that probably don't interfere with job execution either (for example in the constructor, where it tries to open reporters). Do you want me to change them to warn as well?

That should be done in a separate issue.

zentol · 2017-05-13T22:49:25Z

...metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusReporter.java

+			.setChild(new io.prometheus.client.Counter.Child() {
+				@Override
+				public double get() {
+					return (double) meter.getCount();


We should also export the rate of the meter as a prometheus gauge.

Personally i think we can drop this method completely; I'm not too big of a fan of exporting the count of a meter; if that would be of interest there should always be a separate counter for it. (and as far as Flink is concerned, there is)

zentol · 2017-05-13T22:51:09Z

...ics-prometheus/src/test/java/org/apache/flink/metrics/prometheus/PrometheusReporterTest.java

+		return HOST_NAME + SCOPE_SEPARATOR + "taskmanager" + SCOPE_SEPARATOR + TASK_MANAGER + SCOPE_SEPARATOR + metricName;
+	}
+
+	private static MetricRegistry prepareMetricRegistry() {


This method is a bit overkill for a one-liner that is called once.

zentol · 2017-05-13T22:51:18Z

...ics-prometheus/src/test/java/org/apache/flink/metrics/prometheus/PrometheusReporterTest.java

+	@Rule
+	public ExpectedException thrown = ExpectedException.none();
+
+	private MetricRegistry registry = prepareMetricRegistry();


could be final.

zentol · 2017-05-13T22:53:24Z

...metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusReporter.java

+			this.histogram = histogram;
+			this.metricName = metricName;
+			this.metricIdentifier = metricIdentifier;
+			labelNamesWithQuantile = ImmutableList.<String>builder().addAll(labelNames).add("quantile").build();


should be prefixed with this as well for consistency.

…ements. - Fix code style - Only log warn's in metric system, no errors - Avoid using guava - Throw exception on failed startup - Export rate for meters instead of count - Add application prefix to exported metrics

mbode · 2017-05-14T09:24:33Z

Okay, guava is not used anymore. I am not sure about the shading part. Would you expect either prometheus clients or nanohttpd to be used in Flink user code? Or are there other advantages to shading? If so, could you point me to a module I could copy the "Flink way of shading" from?

zentol · 2017-05-14T10:03:30Z

...metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusReporter.java

+	}
+
+	private Collector createGauge(final Meter meter, final String name, final String identifier, final List<String> labelNames, final List<String> labelValues) {
+		return newGauge(name + "_rate", identifier, labelNames, labelValues, new io.prometheus.client.Gauge.Child() {


Meter metrics should contain "rate" in their name already, so we don't have to append "_rate" here.

zentol · 2017-05-14T10:14:22Z

Yu can find an example on how to shade here: https://github.com/apache/flink/blob/master/flink-metrics/flink-metrics-datadog/pom.xml.

Shading dependencies in reporters/connectors has become a safety-precaution form our side.

It is not that unlikely that user-code contains the same dependencies. For one user-code also includes other reporters, so by the very existence of this reporter there is a precedent :) Besides that something that pops up from time to time on the mailing lists is users talking to REST endpoints in their functions/source/sinks, which may also rely on http-related dependencies.

- Shade dependencies - Do not append "_rate" to meters - Add information about exported labels to docs

mbode · 2017-05-18T05:26:02Z

@zentol Would you mind checking that I got the shading right?

zentol · 2017-05-19T12:08:33Z

I'll take a look later today.

zentol · 2017-05-19T13:09:01Z

Shading looks correct; dependencies are included and the correct jar is included in flink-dist.

rmetzger · 2017-06-01T12:03:45Z

flink-metrics/flink-metrics-prometheus/pom.xml

+	<parent>
+		<groupId>org.apache.flink</groupId>
+		<artifactId>flink-metrics</artifactId>
+		<version>1.3-SNAPSHOT</version>


I guess that's outdated by now. Sorry :(

…ough ScheduledDropwizardReporter.

…ements. - Fix code style - Only log warn's in metric system, no errors - Avoid using guava - Throw exception on failed startup - Export rate for meters instead of count - Add application prefix to exported metrics

- Shade dependencies - Do not append "_rate" to meters - Add information about exported labels to docs

mbode · 2017-06-02T05:18:07Z

Oh, I broke the stricter checkstyle. To me, it feels a bit weird to have to put javadoc on tests, is that intended?

zentol · 2017-06-02T09:21:41Z

I think it is intended. The majority of tests can be (and are) commented with "Tests for the {@link }.", which is totally OK because it tells us that this is the general test battery for that class and not something else. These are also easy to add so there's not a real overhead in doing so.

But this doesn't apply to all of them; and you only really notice the missing javadoc when you stumble on a test and you spend 10 minutes trying to figure out what it is doing because the method names aren't helping at all.

The widely-accepted notion that the name of a class and test method are supposed to be enough documentation for test is quite questionable IMO; and by enforcing a javadoc on the class we give a little nudge to make the developer hopefully think about whether something needs to be documented or not.

zentol · 2017-06-02T12:08:10Z

Another thing to consider is that this rule also covers the myriad of testing utilities that so far did not have to be documented.

mbode · 2017-06-09T11:30:10Z

I see, makes sense. Finally got around to fixing the dependency and getting a Green Travis.

zentol · 2017-06-26T15:44:23Z

Finally found time to try this out, and it works great, merging.

This closes apache#3833.

zentol reviewed May 13, 2017

View reviewed changes

zentol reviewed May 14, 2017

View reviewed changes

mbode added a commit to mbode/flink that referenced this pull request May 14, 2017

[Flink-6221] Implement further review comments (apache#3833).

44b8f00

- Shade dependencies - Do not append "_rate" to meters - Add information about exported labels to docs

mbode force-pushed the PrometheusReporter branch from 44b8f00 to f35e7af Compare May 14, 2017 12:37

mbode added a commit to mbode/flink that referenced this pull request May 14, 2017

[Flink-6221] Implement further review comments (apache#3833).

f35e7af

- Shade dependencies - Do not append "_rate" to meters - Add information about exported labels to docs

rmetzger reviewed Jun 1, 2017

View reviewed changes

mbode added 6 commits June 1, 2017 18:12

[FLINK-6221] Add PrometheusReporter

f86dcaa

[Flink-6221] Use Prometheus Java client directly instead of going thr…

97fa830

…ough ScheduledDropwizardReporter.

[Flink-6221] Make scope explicit in exported metric names.

ef61fb2

[Flink-6221] Implement further review comments (apache#3833).

deae9e1

- Shade dependencies - Do not append "_rate" to meters - Add information about exported labels to docs

[FLINK-6221] Bump version 1.3-SNAPSHOT -> 1.4-SNAPSHOT (apache#3833)

7adece0

mbode force-pushed the PrometheusReporter branch from f35e7af to 7adece0 Compare June 1, 2017 16:17

[FLINK-6221] Fix strict checkstyle violations

6be27ec

[FLINK-6221] Parametrize Scala version in flink-metrics-prometheus pom

7deb24e

mbode force-pushed the PrometheusReporter branch from 576ee20 to 7deb24e Compare June 8, 2017 19:36

zentol pushed a commit to zentol/flink that referenced this pull request Jun 26, 2017

[FLINK-6221] Add PrometheusReporter

046d64a

This closes apache#3833.

zentol pushed a commit to zentol/flink that referenced this pull request Jun 27, 2017

[FLINK-6221] Add PrometheusReporter

a34e103

This closes apache#3833.

zentol pushed a commit to zentol/flink that referenced this pull request Jun 27, 2017

[FLINK-6221] Add PrometheusReporter

effa321

This closes apache#3833.

zentol pushed a commit to zentol/flink that referenced this pull request Jun 28, 2017

[FLINK-6221] Add PrometheusReporter

4731898

This closes apache#3833.

zentol pushed a commit to zentol/flink that referenced this pull request Jun 28, 2017

[FLINK-6221] Add PrometheusReporter

50c2704

This closes apache#3833.

zentol pushed a commit to zentol/flink that referenced this pull request Jun 28, 2017

[FLINK-6221] Add PrometheusReporter

9ce6915

This closes apache#3833.

zentol pushed a commit to zentol/flink that referenced this pull request Jun 29, 2017

[FLINK-6221] Add PrometheusReporter

c9bba90

This closes apache#3833.

zentol pushed a commit to zentol/flink that referenced this pull request Jun 29, 2017

[FLINK-6221] Add PrometheusReporter

1ec771c

This closes apache#3833.

zentol pushed a commit to zentol/flink that referenced this pull request Jun 30, 2017

[FLINK-6221] Add PrometheusReporter

9a00a7f

This closes apache#3833.

asfgit closed this in bb9505d Jul 1, 2017

mbode deleted the PrometheusReporter branch August 24, 2017 13:54

skidder pushed a commit to muxinc/flink that referenced this pull request Nov 28, 2017

[FLINK-6221] Add PrometheusReporter

6f4ac19

This closes apache#3833.

rmetzger added the component=Runtime/Metrics label Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-6221] Add PrometheusReporter #3833

[FLINK-6221] Add PrometheusReporter #3833

mbode commented May 6, 2017

zentol commented May 6, 2017

mbode commented May 6, 2017 •

edited

Loading

hadronzoo commented May 11, 2017

mbode commented May 13, 2017

zentol commented May 13, 2017

zentol left a comment

zentol May 13, 2017

zentol May 13, 2017

zentol May 13, 2017

zentol May 13, 2017

zentol May 13, 2017

zentol May 13, 2017

mbode May 14, 2017

zentol May 14, 2017 •

edited

Loading

zentol May 13, 2017

zentol May 13, 2017

zentol May 13, 2017

zentol May 13, 2017

mbode commented May 14, 2017

zentol May 14, 2017

zentol commented May 14, 2017

mbode commented May 18, 2017

zentol commented May 19, 2017

zentol commented May 19, 2017

rmetzger Jun 1, 2017

mbode Jun 1, 2017

mbode commented Jun 2, 2017

zentol commented Jun 2, 2017

zentol commented Jun 2, 2017

mbode commented Jun 9, 2017

zentol commented Jun 26, 2017

[FLINK-6221] Add PrometheusReporter #3833

[FLINK-6221] Add PrometheusReporter #3833

Conversation

mbode commented May 6, 2017

zentol commented May 6, 2017

mbode commented May 6, 2017 • edited Loading

hadronzoo commented May 11, 2017

mbode commented May 13, 2017

zentol commented May 13, 2017

zentol 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zentol May 14, 2017 • edited Loading

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

mbode commented May 14, 2017

Choose a reason for hiding this comment

zentol commented May 14, 2017

mbode commented May 18, 2017

zentol commented May 19, 2017

zentol commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbode commented Jun 2, 2017

zentol commented Jun 2, 2017

zentol commented Jun 2, 2017

mbode commented Jun 9, 2017

zentol commented Jun 26, 2017

mbode commented May 6, 2017 •

edited

Loading

zentol May 14, 2017 •

edited

Loading