[FLINK-4184] [metrics] Replace invalid characters in ScheduledDropwizardReporter#2220
[FLINK-4184] [metrics] Replace invalid characters in ScheduledDropwizardReporter#2220tillrohrmann wants to merge 3 commits into
Conversation
| builder.append(replaceInvalidChars(componentName)).append("."); | ||
| } | ||
|
|
||
| builder.append(replaceInvalidChars(metricName)); |
There was a problem hiding this comment.
the call to replaceInvalidChars is not required; metric names can only contain alphanumeric characters. I don't think any reporter will not support these.
|
This PR will heavily conflict with #2219, in their current state we can't merge one without blocking the other. |
|
True, it conflicts with your proposed changes for the definable metric group delimiter. I will rebase and adapt this PR wrt #2219. |
…ardReporter 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.
…c name out The character filter is applied to all components of the fully qualified metric name. The ScheduledDropwizardReporter and AbstractReporter implement this interface to generate compatible metric names.
tillrohrmann
force-pushed
the
fixDropwizardReporters
branch
from
d639114 to
8387528
Compare
|
I rebased on the latest master and introduced a In order to do this, the |
| if (scopeString == null) { | ||
| scopeString = ScopeFormat.concat(registry.getDelimiter(), scopeComponents); | ||
| if (filter != null) { | ||
| scopeString = ScopeFormat.concat(registry.getDelimiter(), scopeComponents); |
There was a problem hiding this comment.
if no filter is given we will now never assign to scopeString, breaking all names. We should assume that no filtering is required.
There was a problem hiding this comment.
True, this is wrong. Fill fix it.
|
do we have a test that verifies that reporters properly pass their filter when notified of new metrics? |
| * {@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 |
There was a problem hiding this comment.
this is misleading; it is not applied to the fully qualified name (as delimiter's are not filtered)
There was a problem hiding this comment.
Will adapt the comment
…eck that reporters filter out invalid characters
|
Thanks for the review @zentol. I've addressed your comments and added test cases which verify that the corresponding |
| // Getters | ||
| // ------------------------------------------------------------------------ | ||
|
|
||
| // used for testing purposes |
There was a problem hiding this comment.
could we move this into the TestingScheduledDropwizardReporter?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
only one comment left, otherwise +1 |
|
Thanks for the review @zentol. Will be merging this PR then. |
|
I would appreciate it if you would give me time to answer to your response before going ahead with a merge. |
3 participants
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.