You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by aljoscha <gi...@git.apache.org> on 2016/07/22 13:28:09 UTC
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
GitHub user aljoscha opened a pull request:
https://github.com/apache/flink/pull/2285
[FLINK-4246] Allow Specifying Multiple Metrics Reporters
This also updates documentation and tests.
Reporters can now be specified like this:
metrics.reporters: foo,bar
metrics.reporter.foo.class: JMXReporter.class
metrics.reporter.foo.port: 10
metrics.reporter.bar.class: GangliaReporter.class
metrics.reporter.bar.port: 11
metrics.reporter.bar.interval: 10 SECONDS
metrics.reporter.bar.something: 42
R: @StephanEwen and @zentol for review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aljoscha/flink metrics-multiple-reporters
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2285.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 #2285
----
commit 2d3fda9b20b72b2539e7f52889997b227096fc79
Author: Aljoscha Krettek <al...@gmail.com>
Date: 2016-07-22T13:25:01Z
[FLINK-4246] Allow Specifying Multiple Metrics Reporters
This also updates documentation and tests.
Reporters can now be specified like this:
metrics.reporters: foo,bar
metrics.reporter.foo.class: JMXReporter.class
metrics.reporter.foo.port: 10
metrics.reporter.bar.class: GangliaReporter.class
metrics.reporter.bar.port: 11
metrics.reporter.bar.something: 42
----
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71881022
--- Diff: docs/apis/metrics.md ---
@@ -227,14 +227,25 @@ or by assigning unique names to jobs and operators.
## Reporter
-Metrics can be exposed to an external system by configuring a reporter in `conf/flink-conf.yaml`.
-
-- `metrics.reporter.class`: The class of the reporter to use.
- - Example: org.apache.flink.metrics.reporter.JMXReporter
-- `metrics.reporter.arguments`: A list of named parameters that are passed to the reporter.
- - Example: --host localhost --port 9010
-- `metrics.reporter.interval`: The interval between reports.
- - Example: 10 SECONDS
+Metrics can be exposed to an external system by configuring a one or several reporters in `conf/flink-conf.yaml`.
+
+- `metrics.reporters`: The list of named reporters, i.e. "foo,bar".
+- `metrics.reporter.<name>.<config>`: Generic setting `<config>` for the reporter named `<name>`.
+- `metrics.reporter.<name>.class`: The reporter class to use for the reporter named `<name>`.
+- `metrics.reporter.<name>.interval`: The reporter interval to use for the reporter named `<name>`.
+
+All reporters must at least have the `class` config, some allow specifying a reporting `interval`. Bellow,
+we will list more settings specific to each reporter.
+
+Example reporter configuration for using the built-in JMX reporter:
+
+```
+metrics.reporters: my_jmx_reporter
--- End diff --
i think we should show the configuration for multiple reporters
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71882489
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/ConfigConstants.java ---
@@ -651,14 +651,38 @@
// ---------------------------- Metrics -----------------------------------
+ // Per reporter:
+
+ /**
+ * The list of named reporters. Names are defined here and per-reporter configs
+ * are given with the reporter config prefix and the reporter name.
+ *
+ * Example:
+ * <pre>{@code
+ * metrics.reporters = foo, bar
+ *
+ * metrics.reporter.foo.class = FooReporter.class
+ * metrics.reporter.foo.interval = 10
+ *
+ * metrics.reporter.bar.class = JMXReporter.class
+ * metrics.reporter.bar.port = 1337
+ * }</pre>
+ */
+ public static final String METRICS_REPORTERS_LIST = "metrics.reporters";
+
+ /**
+ * The prefix for per-reporter configs. Has to be combined with a reporter name and
+ * the configs mentioned below.
+ */
+ public static final String METRICS_REPORTER_PREFIX = "metrics.reporter.";
+
/** The class of the reporter to use. */
- public static final String METRICS_REPORTER_CLASS = "metrics.reporter.class";
-
- /** A list of named parameters that are passed to the reporter. */
- public static final String METRICS_REPORTER_ARGUMENTS = "metrics.reporter.arguments";
+ public static final String METRICS_REPORTER_CLASS = "class";
--- End diff --
I think we should label both class and interval as suffix keys.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71883545
--- Diff: flink-metrics/flink-metrics-statsd/src/test/java/org/apache/flink/metrics/statsd/StatsDReporterTest.java ---
@@ -135,11 +138,14 @@ public void testStatsDHistogramReporting() throws Exception {
receiverThread.start();
int port = receiver.getPort();
+ System.out.println("PORT: " + port);
--- End diff --
leftover `println()` ?
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r72089377
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/MetricRegistry.java ---
@@ -74,81 +76,97 @@ public MetricRegistry(Configuration config) {
this.delimiter = delim;
// second, instantiate any custom configured reporters
+ this.reporters = new ArrayList<>();
+
+ final String definedReporters = config.getString(ConfigConstants.METRICS_REPORTERS_LIST, null);
- final String className = config.getString(ConfigConstants.METRICS_REPORTER_CLASS, null);
- if (className == null) {
+ if (definedReporters == null) {
+ // no reporters defined
// by default, don't report anything
LOG.info("No metrics reporter configured, no metrics will be exposed/reported.");
- this.reporter = null;
this.executor = null;
- }
- else {
- MetricReporter reporter;
- ScheduledExecutorService executor = null;
- try {
- String configuredPeriod = config.getString(ConfigConstants.METRICS_REPORTER_INTERVAL, null);
- TimeUnit timeunit = TimeUnit.SECONDS;
- long period = 10;
-
- if (configuredPeriod != null) {
- try {
- String[] interval = configuredPeriod.split(" ");
- period = Long.parseLong(interval[0]);
- timeunit = TimeUnit.valueOf(interval[1]);
- }
- catch (Exception e) {
- LOG.error("Cannot parse report interval from config: " + configuredPeriod +
- " - please use values like '10 SECONDS' or '500 MILLISECONDS'. " +
- "Using default reporting interval.");
- }
+ } else {
+ // we have some reporters so
+ String[] namedReporters = definedReporters.split(",");
+ for (String namedReporter : namedReporters) {
+
+ final String className =
+ config.getString(ConfigConstants.METRICS_REPORTER_PREFIX + namedReporter + "." + ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, null);
+ if (className == null) {
+ LOG.error("No reporter class set for reporter " + namedReporter + ". Metrics might not be exposed/reported.");
+ continue;
}
- MetricConfig reporterConfig = createReporterConfig(config);
+ try {
+ String configuredPeriod =
+ config.getString(ConfigConstants.METRICS_REPORTER_PREFIX + namedReporter + "." + ConfigConstants.METRICS_REPORTER_INTERVAL_SUFFIX, null);
+ TimeUnit timeunit = TimeUnit.SECONDS;
+ long period = 10;
+
+ if (configuredPeriod != null) {
+ try {
+ String[] interval = configuredPeriod.split(" ");
+ period = Long.parseLong(interval[0]);
+ timeunit = TimeUnit.valueOf(interval[1]);
+ }
+ catch (Exception e) {
+ LOG.error("Cannot parse report interval from config: " + configuredPeriod +
+ " - please use values like '10 SECONDS' or '500 MILLISECONDS'. " +
+ "Using default reporting interval.");
+ }
+ }
- Class<?> reporterClass = Class.forName(className);
- reporter = (MetricReporter) reporterClass.newInstance();
- reporter.open(reporterConfig);
+ Class<?> reporterClass = Class.forName(className);
+ MetricReporter reporterInstance = (MetricReporter) reporterClass.newInstance();
- if (reporter instanceof Scheduled) {
- executor = Executors.newSingleThreadScheduledExecutor();
- LOG.info("Periodically reporting metrics in intervals of {} {}", period, timeunit.name());
+ MetricConfig reporterConfig = new MetricConfig();
+ config.addAll(ConfigConstants.METRICS_REPORTER_PREFIX + namedReporter + ".", reporterConfig);
+ reporterInstance.open(reporterConfig);
- executor.scheduleWithFixedDelay(new ReporterTask((Scheduled) reporter), period, period, timeunit);
+ if (reporterInstance instanceof Scheduled) {
+ if (this.executor == null) {
+ executor = Executors.newSingleThreadScheduledExecutor();
+ }
+ LOG.info("Periodically reporting metrics in intervals of {} {}", period, timeunit.name());
--- End diff --
this message should include the reporter name.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898876
--- Diff: flink-metrics/flink-metrics-dropwizard/src/test/java/org/apache/flink/dropwizard/ScheduledDropwizardReporterTest.java ---
@@ -67,7 +67,9 @@ public void testAddingMetrics() throws NoSuchFieldException, IllegalAccessExcept
String taskManagerId = "tas:kMana::ger";
String counterName = "testCounter";
- configuration.setString(ConfigConstants.METRICS_REPORTER_CLASS, "org.apache.flink.dropwizard.ScheduledDropwizardReporterTest$TestingScheduledDropwizardReporter");
+ configuration.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test");
+ configuration.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test.class", "org.apache.flink.dropwizard.ScheduledDropwizardReporterTest$TestingScheduledDropwizardReporter");
--- End diff --
using `.class` instead of `METRICS_REPORTER_CLASS_SUFFIX`
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/2285
Ahh, you're right, I'll fix that.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898811
--- Diff: flink-core/src/test/java/org/apache/flink/metrics/reporter/JMXReporterTest.java ---
@@ -79,19 +81,24 @@ public void testGenerateName() {
@Test
public void testPortConflictHandling() throws Exception {
Configuration cfg = new Configuration();
- cfg.setString(ConfigConstants.METRICS_REPORTER_CLASS, TestReporter.class.getName());
+ cfg.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test1,test2");
+
+ cfg.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test1.class", JMXReporter.class.getName());
--- End diff --
using `.class` instead of `METRICS_REPORTER_CLASS_SUFFIX`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898715
--- Diff: flink-metrics/flink-metrics-statsd/src/test/java/org/apache/flink/metrics/statsd/StatsDReporterTest.java ---
@@ -138,12 +138,11 @@ public void testStatsDHistogramReporting() throws Exception {
receiverThread.start();
int port = receiver.getPort();
- System.out.println("PORT: " + port);
Configuration config = new Configuration();
config.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test");
config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test.class", StatsDReporter.class.getName());
--- End diff --
using `.class` instead of `METRICS_REPORTER_CLASS_SUFFIX`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71883806
--- Diff: flink-core/src/test/java/org/apache/flink/metrics/reporter/JMXReporterTest.java ---
@@ -129,17 +134,25 @@ public Integer getValue() {
public void testJMXAvailability() throws Exception {
Configuration cfg = new Configuration();
cfg.setString(ConfigConstants.METRICS_REPORTER_CLASS, TestReporter.class.getName());
+
+ cfg.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test1,test2");
+
+ cfg.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test1.class", JMXReporter.class.getName());
+ cfg.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test1.port", "9020-9035");
--- End diff --
why are you using a different port range?
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71879969
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/MetricRegistry.java ---
@@ -75,77 +78,92 @@ public MetricRegistry(Configuration config) {
this.delimiter = delim;
// second, instantiate any custom configured reporters
-
- final String className = config.getString(ConfigConstants.METRICS_REPORTER_CLASS, null);
- if (className == null) {
+ this.reporters = new ArrayList<>();
+
+ final String definedReporters = config.getString(ConfigConstants.METRICS_REPORTERS_LIST, null);
+
+ if (definedReporters == null) {
+ // no reporters defined
// by default, don't report anything
LOG.info("No metrics reporter configured, no metrics will be exposed/reported.");
- this.reporter = null;
this.executor = null;
- }
- else {
- MetricReporter reporter;
- ScheduledExecutorService executor = null;
- try {
- String configuredPeriod = config.getString(ConfigConstants.METRICS_REPORTER_INTERVAL, null);
- TimeUnit timeunit = TimeUnit.SECONDS;
- long period = 10;
-
- if (configuredPeriod != null) {
- try {
- String[] interval = configuredPeriod.split(" ");
- period = Long.parseLong(interval[0]);
- timeunit = TimeUnit.valueOf(interval[1]);
+ } else {
+ // we have some reporters so
+ String[] namedReporters = definedReporters.split(",");
+ for (String namedReporter : namedReporters) {
+ DelegatingConfiguration reporterConfig =
+ new DelegatingConfiguration(config, ConfigConstants.METRICS_REPORTER_PREFIX + namedReporter + ".");
+
+ final String className = reporterConfig.getString(ConfigConstants.METRICS_REPORTER_CLASS, null);
+ if (className == null) {
+ throw new IllegalStateException("No reporter class set for reporter " + namedReporter);
+ }
+
+ try {
+ String configuredPeriod = reporterConfig.getString(ConfigConstants.METRICS_REPORTER_INTERVAL, null);
+ TimeUnit timeunit = TimeUnit.SECONDS;
+ long period = 10;
+
+ if (configuredPeriod != null) {
+ try {
+ String[] interval = configuredPeriod.split(" ");
+ period = Long.parseLong(interval[0]);
+ timeunit = TimeUnit.valueOf(interval[1]);
+ }
+ catch (Exception e) {
+ LOG.error("Cannot parse report interval from config: " + configuredPeriod +
+ " - please use values like '10 SECONDS' or '500 MILLISECONDS'. " +
+ "Using default reporting interval.");
+ }
}
- catch (Exception e) {
- LOG.error("Cannot parse report interval from config: " + configuredPeriod +
- " - please use values like '10 SECONDS' or '500 MILLISECONDS'. " +
- "Using default reporting interval.");
+
+ Class<?> reporterClass = Class.forName(className);
+ MetricReporter reporterInstance = (MetricReporter) reporterClass.newInstance();
+ reporterInstance.open(reporterConfig);
+
+ if (reporterInstance instanceof Scheduled) {
+ ensureExecutorRunning();
--- End diff --
do we really need this method? it is only used once.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898897
--- Diff: flink-metrics/flink-metrics-statsd/src/test/java/org/apache/flink/metrics/statsd/StatsDReporterTest.java ---
@@ -137,9 +140,11 @@ public void testStatsDHistogramReporting() throws Exception {
int port = receiver.getPort();
Configuration config = new Configuration();
- config.setString(ConfigConstants.METRICS_REPORTER_CLASS, StatsDReporter.class.getName());
- config.setString(ConfigConstants.METRICS_REPORTER_INTERVAL, "1 SECONDS");
- config.setString(ConfigConstants.METRICS_REPORTER_ARGUMENTS, "--host localhost --port " + port);
+ config.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test");
+ config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test.class", StatsDReporter.class.getName());
--- End diff --
using `.class` instead of `METRICS_REPORTER_CLASS_SUFFIX`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71880869
--- Diff: docs/apis/metrics.md ---
@@ -227,14 +227,25 @@ or by assigning unique names to jobs and operators.
## Reporter
-Metrics can be exposed to an external system by configuring a reporter in `conf/flink-conf.yaml`.
-
-- `metrics.reporter.class`: The class of the reporter to use.
- - Example: org.apache.flink.metrics.reporter.JMXReporter
-- `metrics.reporter.arguments`: A list of named parameters that are passed to the reporter.
- - Example: --host localhost --port 9010
-- `metrics.reporter.interval`: The interval between reports.
- - Example: 10 SECONDS
+Metrics can be exposed to an external system by configuring a one or several reporters in `conf/flink-conf.yaml`.
--- End diff --
typo: configuring a one -> configuring one
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/2285
Jip, but that's not done very often, only when instantiating the `MetricRegistry`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898844
--- Diff: flink-core/src/test/java/org/apache/flink/metrics/reporter/JMXReporterTest.java ---
@@ -128,18 +133,26 @@ public Integer getValue() {
@Test
public void testJMXAvailability() throws Exception {
Configuration cfg = new Configuration();
- cfg.setString(ConfigConstants.METRICS_REPORTER_CLASS, TestReporter.class.getName());
+ cfg.setString(ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, TestReporter.class.getName());
+
+ cfg.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test1,test2");
+
+ cfg.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test1.class", JMXReporter.class.getName());
+ cfg.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test1.port", "9040-9055");
+
+ cfg.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test2.class", JMXReporter.class.getName());
--- End diff --
using `.class` instead of `METRICS_REPORTER_CLASS_SUFFIX`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71882304
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/ConfigConstants.java ---
@@ -651,14 +651,38 @@
// ---------------------------- Metrics -----------------------------------
+ // Per reporter:
+
+ /**
+ * The list of named reporters. Names are defined here and per-reporter configs
+ * are given with the reporter config prefix and the reporter name.
+ *
+ * Example:
+ * <pre>{@code
+ * metrics.reporters = foo, bar
+ *
+ * metrics.reporter.foo.class = FooReporter.class
--- End diff --
shouldn't the classes be fully qualified?
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71879710
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/MetricRegistry.java ---
@@ -75,77 +78,92 @@ public MetricRegistry(Configuration config) {
this.delimiter = delim;
// second, instantiate any custom configured reporters
-
- final String className = config.getString(ConfigConstants.METRICS_REPORTER_CLASS, null);
- if (className == null) {
+ this.reporters = new ArrayList<>();
+
+ final String definedReporters = config.getString(ConfigConstants.METRICS_REPORTERS_LIST, null);
+
+ if (definedReporters == null) {
+ // no reporters defined
// by default, don't report anything
LOG.info("No metrics reporter configured, no metrics will be exposed/reported.");
- this.reporter = null;
this.executor = null;
- }
- else {
- MetricReporter reporter;
- ScheduledExecutorService executor = null;
- try {
- String configuredPeriod = config.getString(ConfigConstants.METRICS_REPORTER_INTERVAL, null);
- TimeUnit timeunit = TimeUnit.SECONDS;
- long period = 10;
-
- if (configuredPeriod != null) {
- try {
- String[] interval = configuredPeriod.split(" ");
- period = Long.parseLong(interval[0]);
- timeunit = TimeUnit.valueOf(interval[1]);
+ } else {
+ // we have some reporters so
+ String[] namedReporters = definedReporters.split(",");
+ for (String namedReporter : namedReporters) {
+ DelegatingConfiguration reporterConfig =
+ new DelegatingConfiguration(config, ConfigConstants.METRICS_REPORTER_PREFIX + namedReporter + ".");
+
+ final String className = reporterConfig.getString(ConfigConstants.METRICS_REPORTER_CLASS, null);
+ if (className == null) {
+ throw new IllegalStateException("No reporter class set for reporter " + namedReporter);
--- End diff --
The MetricRegistry constructor should not throw any exceptions.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r72091326
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/MetricRegistry.java ---
@@ -74,81 +76,97 @@ public MetricRegistry(Configuration config) {
this.delimiter = delim;
// second, instantiate any custom configured reporters
+ this.reporters = new ArrayList<>();
+
+ final String definedReporters = config.getString(ConfigConstants.METRICS_REPORTERS_LIST, null);
- final String className = config.getString(ConfigConstants.METRICS_REPORTER_CLASS, null);
- if (className == null) {
+ if (definedReporters == null) {
+ // no reporters defined
// by default, don't report anything
LOG.info("No metrics reporter configured, no metrics will be exposed/reported.");
- this.reporter = null;
this.executor = null;
- }
- else {
- MetricReporter reporter;
- ScheduledExecutorService executor = null;
- try {
- String configuredPeriod = config.getString(ConfigConstants.METRICS_REPORTER_INTERVAL, null);
- TimeUnit timeunit = TimeUnit.SECONDS;
- long period = 10;
-
- if (configuredPeriod != null) {
- try {
- String[] interval = configuredPeriod.split(" ");
- period = Long.parseLong(interval[0]);
- timeunit = TimeUnit.valueOf(interval[1]);
- }
- catch (Exception e) {
- LOG.error("Cannot parse report interval from config: " + configuredPeriod +
- " - please use values like '10 SECONDS' or '500 MILLISECONDS'. " +
- "Using default reporting interval.");
- }
+ } else {
+ // we have some reporters so
+ String[] namedReporters = definedReporters.split(",");
+ for (String namedReporter : namedReporters) {
+
+ final String className =
+ config.getString(ConfigConstants.METRICS_REPORTER_PREFIX + namedReporter + "." + ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, null);
+ if (className == null) {
+ LOG.error("No reporter class set for reporter " + namedReporter + ". Metrics might not be exposed/reported.");
+ continue;
}
- MetricConfig reporterConfig = createReporterConfig(config);
+ try {
+ String configuredPeriod =
+ config.getString(ConfigConstants.METRICS_REPORTER_PREFIX + namedReporter + "." + ConfigConstants.METRICS_REPORTER_INTERVAL_SUFFIX, null);
+ TimeUnit timeunit = TimeUnit.SECONDS;
+ long period = 10;
+
+ if (configuredPeriod != null) {
+ try {
+ String[] interval = configuredPeriod.split(" ");
+ period = Long.parseLong(interval[0]);
+ timeunit = TimeUnit.valueOf(interval[1]);
+ }
+ catch (Exception e) {
+ LOG.error("Cannot parse report interval from config: " + configuredPeriod +
+ " - please use values like '10 SECONDS' or '500 MILLISECONDS'. " +
+ "Using default reporting interval.");
+ }
+ }
- Class<?> reporterClass = Class.forName(className);
- reporter = (MetricReporter) reporterClass.newInstance();
- reporter.open(reporterConfig);
+ Class<?> reporterClass = Class.forName(className);
+ MetricReporter reporterInstance = (MetricReporter) reporterClass.newInstance();
- if (reporter instanceof Scheduled) {
- executor = Executors.newSingleThreadScheduledExecutor();
- LOG.info("Periodically reporting metrics in intervals of {} {}", period, timeunit.name());
+ MetricConfig reporterConfig = new MetricConfig();
+ config.addAll(ConfigConstants.METRICS_REPORTER_PREFIX + namedReporter + ".", reporterConfig);
+ reporterInstance.open(reporterConfig);
- executor.scheduleWithFixedDelay(new ReporterTask((Scheduled) reporter), period, period, timeunit);
+ if (reporterInstance instanceof Scheduled) {
+ if (this.executor == null) {
+ executor = Executors.newSingleThreadScheduledExecutor();
+ }
+ LOG.info("Periodically reporting metrics in intervals of {} {}", period, timeunit.name());
--- End diff --
Fixing, I'll include the configured reporter name as well as the class.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71884351
--- Diff: docs/apis/metrics.md ---
@@ -227,14 +227,25 @@ or by assigning unique names to jobs and operators.
## Reporter
-Metrics can be exposed to an external system by configuring a reporter in `conf/flink-conf.yaml`.
-
-- `metrics.reporter.class`: The class of the reporter to use.
- - Example: org.apache.flink.metrics.reporter.JMXReporter
-- `metrics.reporter.arguments`: A list of named parameters that are passed to the reporter.
- - Example: --host localhost --port 9010
-- `metrics.reporter.interval`: The interval between reports.
- - Example: 10 SECONDS
+Metrics can be exposed to an external system by configuring a one or several reporters in `conf/flink-conf.yaml`.
+
+- `metrics.reporters`: The list of named reporters, i.e. "foo,bar".
+- `metrics.reporter.<name>.<config>`: Generic setting `<config>` for the reporter named `<name>`.
+- `metrics.reporter.<name>.class`: The reporter class to use for the reporter named `<name>`.
+- `metrics.reporter.<name>.interval`: The reporter interval to use for the reporter named `<name>`.
+
+All reporters must at least have the `class` config, some allow specifying a reporting `interval`. Bellow,
--- End diff --
i would `class` **property** instead of config.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71880194
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/MetricRegistry.java ---
@@ -198,8 +220,13 @@ public void register(Metric metric, String metricName, AbstractMetricGroup group
*/
public void unregister(Metric metric, String metricName, AbstractMetricGroup group) {
try {
- if (reporter != null) {
- reporter.notifyOfRemovedMetric(metric, metricName, group);
+ if (reporters != null) {
+ for (MetricReporter reporter : reporters) {
+
--- End diff --
let's remove this empty line so the code is formatting-wise identical to `register()`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71896724
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/MetricRegistry.java ---
@@ -75,77 +78,92 @@ public MetricRegistry(Configuration config) {
this.delimiter = delim;
// second, instantiate any custom configured reporters
-
- final String className = config.getString(ConfigConstants.METRICS_REPORTER_CLASS, null);
- if (className == null) {
+ this.reporters = new ArrayList<>();
+
+ final String definedReporters = config.getString(ConfigConstants.METRICS_REPORTERS_LIST, null);
+
+ if (definedReporters == null) {
+ // no reporters defined
// by default, don't report anything
LOG.info("No metrics reporter configured, no metrics will be exposed/reported.");
- this.reporter = null;
this.executor = null;
- }
- else {
- MetricReporter reporter;
- ScheduledExecutorService executor = null;
- try {
- String configuredPeriod = config.getString(ConfigConstants.METRICS_REPORTER_INTERVAL, null);
- TimeUnit timeunit = TimeUnit.SECONDS;
- long period = 10;
-
- if (configuredPeriod != null) {
- try {
- String[] interval = configuredPeriod.split(" ");
- period = Long.parseLong(interval[0]);
- timeunit = TimeUnit.valueOf(interval[1]);
+ } else {
+ // we have some reporters so
+ String[] namedReporters = definedReporters.split(",");
+ for (String namedReporter : namedReporters) {
--- End diff --
In that case the array should be empty and we should not enter the loop, right?
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898798
--- Diff: flink-core/src/test/java/org/apache/flink/metrics/MetricRegistryTest.java ---
@@ -88,8 +91,10 @@ public void open(Configuration config) {
public void testReporterScheduling() throws InterruptedException {
Configuration config = new Configuration();
- config.setString(ConfigConstants.METRICS_REPORTER_CLASS, TestReporter3.class.getName());
- config.setString(ConfigConstants.METRICS_REPORTER_INTERVAL, "50 MILLISECONDS");
+ config.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test");
+ config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test.class", TestReporter3.class.getName());
--- End diff --
using `.class` instead of `METRICS_REPORTER_CLASS_SUFFIX`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71882781
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/MetricRegistry.java ---
@@ -75,77 +78,92 @@ public MetricRegistry(Configuration config) {
this.delimiter = delim;
// second, instantiate any custom configured reporters
-
- final String className = config.getString(ConfigConstants.METRICS_REPORTER_CLASS, null);
- if (className == null) {
+ this.reporters = new ArrayList<>();
+
+ final String definedReporters = config.getString(ConfigConstants.METRICS_REPORTERS_LIST, null);
+
+ if (definedReporters == null) {
+ // no reporters defined
// by default, don't report anything
LOG.info("No metrics reporter configured, no metrics will be exposed/reported.");
- this.reporter = null;
this.executor = null;
- }
- else {
- MetricReporter reporter;
- ScheduledExecutorService executor = null;
- try {
- String configuredPeriod = config.getString(ConfigConstants.METRICS_REPORTER_INTERVAL, null);
- TimeUnit timeunit = TimeUnit.SECONDS;
- long period = 10;
-
- if (configuredPeriod != null) {
- try {
- String[] interval = configuredPeriod.split(" ");
- period = Long.parseLong(interval[0]);
- timeunit = TimeUnit.valueOf(interval[1]);
+ } else {
+ // we have some reporters so
+ String[] namedReporters = definedReporters.split(",");
+ for (String namedReporter : namedReporters) {
--- End diff --
Can it happen that namedReporters is an empty String?
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2285
yes, thank you.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71880255
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/MetricRegistry.java ---
@@ -263,6 +276,7 @@ private ReporterTask(Scheduled reporter) {
@Override
public void run() {
+ System.out.println("TRYING REPORT");
--- End diff --
leftover `println()`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71883403
--- Diff: flink-runtime/src/test/java/org/apache/flink/runtime/jobmanager/JobManagerMetricTest.java ---
@@ -51,9 +51,11 @@ public void testJobManagerMetricAccess() throws Exception {
Deadline deadline = new FiniteDuration(2, TimeUnit.MINUTES).fromNow();
Configuration flinkConfiguration = new Configuration();
- flinkConfiguration.setString(ConfigConstants.METRICS_REPORTER_CLASS, "org.apache.flink.metrics.reporter.JMXReporter");
+ flinkConfiguration.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test");
+ flinkConfiguration.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test.class", "org.apache.flink.metrics.reporter.JMXReporter");
--- End diff --
shouldn't we uses the ConfigConstants.METRICS_REPORTER_CLASS here instead of `.class` ? I know it blows up the line, but it still is a defined constant. (the same applies to ExecutionGraphMetricsTest)
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898858
--- Diff: flink-core/src/test/java/org/apache/flink/metrics/reporter/JMXReporterTest.java ---
@@ -197,7 +204,8 @@ public void testHistogramReporting() throws Exception {
try {
Configuration config = new Configuration();
- config.setString(ConfigConstants.METRICS_REPORTER_CLASS, "org.apache.flink.metrics.reporter.JMXReporter");
+ config.setString(ConfigConstants.METRICS_REPORTERS_LIST, "jmx_test");
+ config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "jmx_test.class", "org.apache.flink.metrics.reporter.JMXReporter");
--- End diff --
using `.class` instead of `METRICS_REPORTER_CLASS_SUFFIX`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha closed the pull request at:
https://github.com/apache/flink/pull/2285
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71881920
--- Diff: docs/apis/metrics.md ---
@@ -227,14 +227,25 @@ or by assigning unique names to jobs and operators.
## Reporter
-Metrics can be exposed to an external system by configuring a reporter in `conf/flink-conf.yaml`.
-
-- `metrics.reporter.class`: The class of the reporter to use.
- - Example: org.apache.flink.metrics.reporter.JMXReporter
-- `metrics.reporter.arguments`: A list of named parameters that are passed to the reporter.
- - Example: --host localhost --port 9010
-- `metrics.reporter.interval`: The interval between reports.
- - Example: 10 SECONDS
+Metrics can be exposed to an external system by configuring a one or several reporters in `conf/flink-conf.yaml`.
+
+- `metrics.reporters`: The list of named reporters, i.e. "foo,bar".
+- `metrics.reporter.<name>.<config>`: Generic setting `<config>` for the reporter named `<name>`.
+- `metrics.reporter.<name>.class`: The reporter class to use for the reporter named `<name>`.
+- `metrics.reporter.<name>.interval`: The reporter interval to use for the reporter named `<name>`.
+
+All reporters must at least have the `class` config, some allow specifying a reporting `interval`. Bellow,
+we will list more settings specific to each reporter.
+
+Example reporter configuration for using the built-in JMX reporter:
+
+```
+metrics.reporters: my_jmx_reporter
+
+metrics.reporter.my_jmx_reporter.class: org.apache.flink.metrics.reporter.JMXReporter
+metrics.reporter.my_jmx_reporter.port: 9020-940
--- End diff --
i think you're missing a zero; 940 -> 9040
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71881023
--- Diff: docs/apis/metrics.md ---
@@ -227,14 +227,25 @@ or by assigning unique names to jobs and operators.
## Reporter
-Metrics can be exposed to an external system by configuring a reporter in `conf/flink-conf.yaml`.
-
-- `metrics.reporter.class`: The class of the reporter to use.
- - Example: org.apache.flink.metrics.reporter.JMXReporter
-- `metrics.reporter.arguments`: A list of named parameters that are passed to the reporter.
- - Example: --host localhost --port 9010
-- `metrics.reporter.interval`: The interval between reports.
- - Example: 10 SECONDS
+Metrics can be exposed to an external system by configuring a one or several reporters in `conf/flink-conf.yaml`.
+
+- `metrics.reporters`: The list of named reporters, i.e. "foo,bar".
--- End diff --
since we have an example configuration just below we can omit the example here.
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2285
That would imply iterating over the entire configuration and checking for the prefix, correct?
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898033
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/MetricRegistry.java ---
@@ -75,77 +78,92 @@ public MetricRegistry(Configuration config) {
this.delimiter = delim;
// second, instantiate any custom configured reporters
-
- final String className = config.getString(ConfigConstants.METRICS_REPORTER_CLASS, null);
- if (className == null) {
+ this.reporters = new ArrayList<>();
+
+ final String definedReporters = config.getString(ConfigConstants.METRICS_REPORTERS_LIST, null);
+
+ if (definedReporters == null) {
+ // no reporters defined
// by default, don't report anything
LOG.info("No metrics reporter configured, no metrics will be exposed/reported.");
- this.reporter = null;
this.executor = null;
- }
- else {
- MetricReporter reporter;
- ScheduledExecutorService executor = null;
- try {
- String configuredPeriod = config.getString(ConfigConstants.METRICS_REPORTER_INTERVAL, null);
- TimeUnit timeunit = TimeUnit.SECONDS;
- long period = 10;
-
- if (configuredPeriod != null) {
- try {
- String[] interval = configuredPeriod.split(" ");
- period = Long.parseLong(interval[0]);
- timeunit = TimeUnit.valueOf(interval[1]);
+ } else {
+ // we have some reporters so
+ String[] namedReporters = definedReporters.split(",");
+ for (String namedReporter : namedReporters) {
--- End diff --
yes. but this will go completely unnoticed to a user since nothing is being logged.
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/2285
Ok, now we first have to merge the other PR. \U0001f603
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71879184
--- Diff: docs/apis/metrics.md ---
@@ -227,14 +227,25 @@ or by assigning unique names to jobs and operators.
## Reporter
-Metrics can be exposed to an external system by configuring a reporter in `conf/flink-conf.yaml`.
-
-- `metrics.reporter.class`: The class of the reporter to use.
- - Example: org.apache.flink.metrics.reporter.JMXReporter
-- `metrics.reporter.arguments`: A list of named parameters that are passed to the reporter.
- - Example: --host localhost --port 9010
-- `metrics.reporter.interval`: The interval between reports.
- - Example: 10 SECONDS
+Metrics can be exposed to an external system by configuring a one or several reporters in `conf/flink-conf.yaml`.
+
+- `metrics.reporters`: The list of named reporters, i.e. "foo,bar".
+- `metrics.reporter.<name>.<config>`: Generic setting `<config>` for the reporter named `<name>`.
+- `metrics.reporter.<name>.class`: The reporter class to use for the reporter named `<name>`.
+- `metrics.reporter.<name>.interval`: The reporter interval to use for the reporter named `<name>`.
+
+All reporters must at least have the `class` config, some allow specifying a reporting `interval`. Bellow,
--- End diff --
type: Bellow -> Below
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2285
The usage of the `DelagatingConfiguration` poses a few problems in regards to #2226. After that PR reporters will no longer work on a `Configuration` but instead a `MetricConfig` object that extends `Properties`. For this to work certain key have to be extracted from the `Configuration` and passes into the `MetricConfig`. Naturally, for this to work you must know the names of all keys required. This doesn't work for arbitrary user-defined settings though. We have to revert back to a `metrics.reporter.<name>.arguments` property.
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/2285
Thanks for the thorough review @zentol! I'm addressing the comments in a new commit.
About the move to `MetricConfig`, I think the Configuration could be extended for that case to move all parameters with a certain prefix.
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2285
A simple test to verify that multiple configured reporters are properly instantiated/notified would be good, otherwise +1.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898375
--- Diff: docs/apis/metrics.md ---
@@ -229,21 +229,25 @@ or by assigning unique names to jobs and operators.
Metrics can be exposed to an external system by configuring one or several reporters in `conf/flink-conf.yaml`.
-- `metrics.reporters`: The list of named reporters, i.e. "foo,bar".
+- `metrics.reporters`: The list of named reporters.
- `metrics.reporter.<name>.<config>`: Generic setting `<config>` for the reporter named `<name>`.
- `metrics.reporter.<name>.class`: The reporter class to use for the reporter named `<name>`.
- `metrics.reporter.<name>.interval`: The reporter interval to use for the reporter named `<name>`.
-All reporters must at least have the `class` config, some allow specifying a reporting `interval`. Below,
+All reporters must at least have the `class` property, some allow specifying a reporting `interval`. Below,
we will list more settings specific to each reporter.
-Example reporter configuration for using the built-in JMX reporter:
+Example reporter configuration that specifies multiple reporters:
```
-metrics.reporters: my_jmx_reporter
+metrics.reporters: my_jmx_reporter,my_other_reporter
metrics.reporter.my_jmx_reporter.class: org.apache.flink.metrics.reporter.JMXReporter
-metrics.reporter.my_jmx_reporter.port: 9020-940
+metrics.reporter.my_jmx_reporter.port: 9020-9040
+
+metrics.reporter.my_jmx_reporter.class: org.apache.flink.metrics.graphite.GraphiteReporter
--- End diff --
should be named my_other_reporter
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898835
--- Diff: flink-core/src/test/java/org/apache/flink/metrics/reporter/JMXReporterTest.java ---
@@ -128,18 +133,26 @@ public Integer getValue() {
@Test
public void testJMXAvailability() throws Exception {
Configuration cfg = new Configuration();
- cfg.setString(ConfigConstants.METRICS_REPORTER_CLASS, TestReporter.class.getName());
+ cfg.setString(ConfigConstants.METRICS_REPORTER_CLASS_SUFFIX, TestReporter.class.getName());
+
+ cfg.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test1,test2");
+
+ cfg.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test1.class", JMXReporter.class.getName());
--- End diff --
using `.class` instead of `METRICS_REPORTER_CLASS_SUFFIX`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71883212
--- Diff: flink-runtime/src/test/java/org/apache/flink/runtime/util/DelegatingConfigurationTest.java ---
@@ -28,7 +28,7 @@
import java.util.Comparator;
--- End diff --
this test should be removed, as it was moved into flink-core.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71882146
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/ConfigConstants.java ---
@@ -651,14 +651,38 @@
// ---------------------------- Metrics -----------------------------------
+ // Per reporter:
--- End diff --
i think this comment and the "For all reporters" don't stand out enough to be included. My first impression was that these were left-overs.
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/2285
I added a new test, is that what you had in mind?
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/2285
I addressed the comments.
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898790
--- Diff: flink-core/src/test/java/org/apache/flink/metrics/MetricRegistryTest.java ---
@@ -42,7 +42,8 @@
public void testReporterInstantiation() {
Configuration config = new Configuration();
- config.setString(ConfigConstants.METRICS_REPORTER_CLASS, TestReporter1.class.getName());
+ config.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test");
+ config.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test.class", TestReporter1.class.getName());
--- End diff --
using `.class` instead of `METRICS_REPORTER_CLASS_SUFFIX`
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898826
--- Diff: flink-core/src/test/java/org/apache/flink/metrics/reporter/JMXReporterTest.java ---
@@ -79,19 +81,24 @@ public void testGenerateName() {
@Test
public void testPortConflictHandling() throws Exception {
Configuration cfg = new Configuration();
- cfg.setString(ConfigConstants.METRICS_REPORTER_CLASS, TestReporter.class.getName());
+ cfg.setString(ConfigConstants.METRICS_REPORTERS_LIST, "test1,test2");
+
+ cfg.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test1.class", JMXReporter.class.getName());
+ cfg.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test1.port", "9020-9035");
+
+ cfg.setString(ConfigConstants.METRICS_REPORTER_PREFIX + "test2.class", JMXReporter.class.getName());
--- End diff --
using `.class` instead of `METRICS_REPORTER_CLASS_SUFFIX`
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/2285
There are tests in `JMXReporterTest` that instantiate multiple reporters but I'll also add a simple on to `MetricRegistryTest`.
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/2285
I rebased this on top of #2226
---
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.
---
[GitHub] flink pull request #2285: [FLINK-4246] Allow Specifying Multiple Metrics Rep...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2285#discussion_r71898559
--- Diff: flink-examples/flink-examples-streaming/src/main/java/org/apache/flink/streaming/examples/windowing/WindowWordCount.java ---
@@ -17,12 +17,13 @@
package org.apache.flink.streaming.examples.windowing;
+import org.apache.flink.api.common.functions.FilterFunction;
+import org.apache.flink.api.common.functions.ReduceFunction;
import org.apache.flink.api.java.tuple.Tuple2;
-import org.apache.flink.api.java.utils.ParameterTool;
-import org.apache.flink.examples.java.wordcount.util.WordCountData;
--- End diff --
well these changes don't belong here :)
---
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.
---
[GitHub] flink issue #2285: [FLINK-4246] Allow Specifying Multiple Metrics Reporters
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2285
the test doesn't verify that new metrics are properly forwarded to all reporters :)
---
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.
---