You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tillrohrmann <gi...@git.apache.org> on 2016/07/08 14:54:19 UTC
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
GitHub user tillrohrmann opened a pull request:
https://github.com/apache/flink/pull/2220
[FLINK-4184] [metrics] Replace invalid characters in ScheduledDropwizardReporter
The GraphiteReporter and GangliaReporter report metric names which can contain invalid
characters. These characters include quotes and dots. In order to properly report metrics
to these systems, the afore-mentioned characters have to be replaced in metric names.
The PR also removes quotes from the garbage collector metric name.
The PR sets the default value for TTL in the GangliaReporter to 1, because -1 causes the
reporter to fail.
R @zentol.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tillrohrmann/flink fixDropwizardReporters
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2220.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 #2220
----
commit d63911499aef10f54d7c4c0774f4f22a520bcc66
Author: Till Rohrmann <tr...@apache.org>
Date: 2016-07-08T14:41:39Z
[FLINK-4184] [metrics] Replace invalid characters in ScheduledDropwizardReporter
The GraphiteReporter and GangliaReporter report metric names which can contain invalid
characters. These characters include quotes and dots. In order to properly report metrics
to these systems, the afore-mentioned characters have to be replaced in metric names.
The PR also removes quotes from the garbage collector metric name.
The PR sets the default value for TTL in the GangliaReporter to 1, because -1 causes the
reporter to fail.
----
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2220#discussion_r70777251
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/groups/AbstractMetricGroup.java ---
@@ -100,10 +101,29 @@ public AbstractMetricGroup(MetricRegistry registry, String[] scope) {
* @return fully qualified metric name
*/
public String getMetricIdentifier(String metricName) {
+ return getMetricIdentifier(metricName, null);
+ }
+
+ /**
+ * Returns the fully qualified metric name, for example
+ * {@code "host-7.taskmanager-2.window_word_count.my-mapper.metricName"}
+ *
+ * @param metricName metric name
+ * @param filter character filter which is applied to the fully qualified metric name
--- End diff --
Will adapt the comment
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2220
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters in Sche...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2220
only one comment left, 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 issue #2220: [FLINK-4184] [metrics] Replace invalid characters in Sche...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2220
do we have a test that verifies that reporters properly pass their filter when notified of new metrics?
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters in Sche...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2220
Thanks for the review @zentol. I've addressed your comments and added test cases which verify that the corresponding `MetricReporters` filter out invalid characters in the fully qualified metric 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 issue #2220: [FLINK-4184] [metrics] Replace invalid characters in Sche...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2220
Thanks for the review @zentol. Will be merging this PR then.
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2220#discussion_r70777393
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/groups/AbstractMetricGroup.java ---
@@ -100,10 +101,29 @@ public AbstractMetricGroup(MetricRegistry registry, String[] scope) {
* @return fully qualified metric name
*/
public String getMetricIdentifier(String metricName) {
+ return getMetricIdentifier(metricName, null);
+ }
+
+ /**
+ * Returns the fully qualified metric name, for example
+ * {@code "host-7.taskmanager-2.window_word_count.my-mapper.metricName"}
+ *
+ * @param metricName metric name
+ * @param filter character filter which is applied to the fully qualified metric name
+ * @return fully qualified metric name
+ */
+ public String getMetricIdentifier(String metricName, CharacterFilter filter) {
if (scopeString == null) {
- scopeString = ScopeFormat.concat(registry.getDelimiter(), scopeComponents);
+ if (filter != null) {
+ scopeString = ScopeFormat.concat(registry.getDelimiter(), scopeComponents);
--- End diff --
True, this is wrong. Fill fix it.
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters in Sche...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2220
True, it conflicts with your proposed changes for the definable metric group delimiter. I will rebase and adapt this PR wrt #2219.
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters in Sche...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2220
I rebased on the latest master and introduced a `CharacterFilter` interface. The `CharacterFilter` allows to filter out invalid characters while generating the fully qualified metric name.
In order to do this, the `AbstractMetricGroup#generateMetricName` takes a `CharacterFilter` as argument. The `AbstractReporter` and the `ScheduledDropwizardReporter` implement this interface to filter out reporter specific characters.
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters in Sche...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2220
I would appreciate it if you would give me time to answer to your response before going ahead with a merge.
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2220#discussion_r70845241
--- Diff: flink-metrics/flink-metrics-dropwizard/src/main/java/org/apache/flink/dropwizard/ScheduledDropwizardReporter.java ---
@@ -74,6 +75,15 @@ protected ScheduledDropwizardReporter() {
}
// ------------------------------------------------------------------------
+ // Getters
+ // ------------------------------------------------------------------------
+
+ // used for testing purposes
+ Map<Counter, String> getCounters() {
--- End diff --
could we move this into the TestingScheduledDropwizardReporter?
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2220#discussion_r70979472
--- Diff: flink-metrics/flink-metrics-dropwizard/src/main/java/org/apache/flink/dropwizard/ScheduledDropwizardReporter.java ---
@@ -74,6 +75,15 @@ protected ScheduledDropwizardReporter() {
}
// ------------------------------------------------------------------------
+ // Getters
+ // ------------------------------------------------------------------------
+
+ // used for testing purposes
+ Map<Counter, String> getCounters() {
--- End diff --
You could also access the protected registry and get the counters from there.
We may not even need the gauges/counters/histograms fields in the ScheduledDropwizardReporter.
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2220#discussion_r70770115
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/groups/AbstractMetricGroup.java ---
@@ -100,10 +101,29 @@ public AbstractMetricGroup(MetricRegistry registry, String[] scope) {
* @return fully qualified metric name
*/
public String getMetricIdentifier(String metricName) {
+ return getMetricIdentifier(metricName, null);
+ }
+
+ /**
+ * Returns the fully qualified metric name, for example
+ * {@code "host-7.taskmanager-2.window_word_count.my-mapper.metricName"}
+ *
+ * @param metricName metric name
+ * @param filter character filter which is applied to the fully qualified metric name
--- End diff --
this is misleading; it is not applied to the fully qualified name (as delimiter's are not filtered)
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2220#discussion_r70768940
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/groups/AbstractMetricGroup.java ---
@@ -100,10 +101,29 @@ public AbstractMetricGroup(MetricRegistry registry, String[] scope) {
* @return fully qualified metric name
*/
public String getMetricIdentifier(String metricName) {
+ return getMetricIdentifier(metricName, null);
+ }
+
+ /**
+ * Returns the fully qualified metric name, for example
+ * {@code "host-7.taskmanager-2.window_word_count.my-mapper.metricName"}
+ *
+ * @param metricName metric name
+ * @param filter character filter which is applied to the fully qualified metric name
+ * @return fully qualified metric name
+ */
+ public String getMetricIdentifier(String metricName, CharacterFilter filter) {
if (scopeString == null) {
- scopeString = ScopeFormat.concat(registry.getDelimiter(), scopeComponents);
+ if (filter != null) {
+ scopeString = ScopeFormat.concat(registry.getDelimiter(), scopeComponents);
--- End diff --
if no filter is given we will now never assign to scopeString, breaking all names. We should assume that no filtering is required.
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2220#discussion_r70095846
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/AbstractReporter.java ---
@@ -84,4 +84,24 @@ public void notifyOfRemovedMetric(Metric metric, String metricName, AbstractMetr
protected String replaceInvalidChars(String metricName) {
return metricName;
}
+
+ /**
+ * Method which constructs the fully qualified metric name from the metric group and the metric
+ * name.
+ *
+ * @param metricName Name of the metric
+ * @param group Associated metric group
+ * @return Fully qualified metric name
+ */
+ private String constructMetricName(String metricName, AbstractMetricGroup group) {
+ StringBuilder builder = new StringBuilder();
+
+ for (String componentName : group.getScopeComponents()) {
+ builder.append(replaceInvalidChars(componentName)).append(".");
+ }
+
+ builder.append(replaceInvalidChars(metricName));
--- End diff --
the call to replaceInvalidChars is not required; metric names can only contain alphanumeric characters. I don't think any reporter will not support these.
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2220#discussion_r70977551
--- Diff: flink-metrics/flink-metrics-dropwizard/src/main/java/org/apache/flink/dropwizard/ScheduledDropwizardReporter.java ---
@@ -74,6 +75,15 @@ protected ScheduledDropwizardReporter() {
}
// ------------------------------------------------------------------------
+ // Getters
+ // ------------------------------------------------------------------------
+
+ // used for testing purposes
+ Map<Counter, String> getCounters() {
--- End diff --
We can, if we mark the counters, gauges and histograms fields as protected. But then we would expose the implementation details to all sub-classes instead of having a getter which is package private. I think the latter option is a bit nicer, because it hides the implementation details.
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters in Sche...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2220
This PR will heavily conflict with #2219, in their current state we can't merge one without blocking the other.
---
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 #2220: [FLINK-4184] [metrics] Replace invalid characters ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2220#discussion_r70097230
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/AbstractReporter.java ---
@@ -84,4 +84,24 @@ public void notifyOfRemovedMetric(Metric metric, String metricName, AbstractMetr
protected String replaceInvalidChars(String metricName) {
return metricName;
}
+
+ /**
+ * Method which constructs the fully qualified metric name from the metric group and the metric
+ * name.
+ *
+ * @param metricName Name of the metric
+ * @param group Associated metric group
+ * @return Fully qualified metric name
+ */
+ private String constructMetricName(String metricName, AbstractMetricGroup group) {
+ StringBuilder builder = new StringBuilder();
+
+ for (String componentName : group.getScopeComponents()) {
+ builder.append(replaceInvalidChars(componentName)).append(".");
--- End diff --
this is a bit inefficient. The output of this loop is identical for all metrics on that group, yet is computed for every single metric. Instead you could modify the getScopeString() method to accept a charFilter argument that is passed into ScopeFormat.concat().
---
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.
---