[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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #2220: [FLINK-4184] [metrics] Replace invalid characters ...
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 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
