[FLINK-5095] Add explicit notifyOfAddedX methods to MetricReporter interface

• Type: Improvement
• Status: Closed
• Priority: Minor
• Resolution: Later
• Affects Version/s: 1.1.3
• Component/s: Runtime / Metrics
• Assignee: Unassigned
• Reporter: Chesnay Schepler
• Created: 18/Nov/16 13:06
• Updated: 25/Sep/20 10:28
• Resolved: 25/Sep/20 10:28

Description

I would like to start a discussion on the MetricReporter interface, specifically the methods that notify a reporter of added or removed metrics.

Currently, the methods are defined as follows:

void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group);
void notifyOfRemovedMetric(Metric metric, String metricName, MetricGroup group);

All metrics, regardless of their actual type, are passed to the reporter with these methods.

Since the different metric types have to be handled differently we thus force every reporter to do something like this:

if (metric instanceof Counter) {
        Counter c = (Counter) metric;
	// deal with counter
} else if (metric instanceof Gauge) {
	// deal with gauge
} else if (metric instanceof Histogram) {
	// deal with histogram
} else if (metric instanceof Meter) {
	// deal with meter
} else {
	// log something or throw an exception
}

This has a few issues

• the instanceof checks and castings are unnecessary overhead
• it requires the implementer to be aware of every metric type
• it encourages throwing an exception in the final else block

We could remedy all of these by reworking the interface to contain explicit add/remove methods for every metric type. This would however be a breaking change and blow up the interface to 12 methods from the current 4. We could also add a RichMetricReporter interface with these methods, which would require relatively little changes but add additional complexity.

I was wondering what other people think about this.

Issue Links

• relates to: FLINK-6053 Gauge<T> should only take subclasses of Number, rather than everything