[jira] [Commented] (FLINK-5095) Add explicit notifyOfAddedX methods to MetricReporter interface
[
https://issues.apache.org/jira/browse/FLINK-5095?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16929178#comment-16929178
]
Till Rohrmann commented on FLINK-5095:
--
Do we still want to do this eventually [~Zentol]?
> Add explicit notifyOfAddedX methods to MetricReporter interface
> ---
>
> Key: FLINK-5095
> URL: https://issues.apache.org/jira/browse/FLINK-5095
> Project: Flink
> Issue Type: Improvement
> Components: Runtime / Metrics
>Affects Versions: 1.1.3
>Reporter: Chesnay Schepler
>Priority: Minor
>
> 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:
> {code}
> void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group);
> void notifyOfRemovedMetric(Metric metric, String metricName, MetricGroup
> group);
> {code}
> 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:
> {code}
> 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
> }
> {code}
> 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.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
[jira] [Commented] (FLINK-5095) Add explicit notifyOfAddedX methods to MetricReporter interface
[
https://issues.apache.org/jira/browse/FLINK-5095?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16008517#comment-16008517
]
Chesnay Schepler commented on FLINK-5095:
-
The following is what is sketched out a while ago when i first got the idea,
and is pretty much exactly the first draft i wrote down.
We'll probably need a new reporter interface, but let's talk about the metrics
first:
For number/string metrics we would add new generic interfaces (name TBD):
{code}
interface NumberMetric {
Number getValue();
}
{code}
{code}
interface StringMetric {
String getValue();
}
{code}
Forward-compatibility with existing metrics for new reporters is addressed by
adding wrappers. This is necessary so that we don't change the existing
interfaces.
For counters we return the count.
For Meters we return the rate.
For Gauges we return _.toString()
Backwards-compatibility for old reporters would be achieved by wrapping the new
metrics into a Gauge.
The Histogram could be generalized into the following interfaces:
These would result in rather verbose implementations, but we can stream-line
this a lot if we want to.
{code}
interface MultiNumberMetric {
C getContainer()
Collection getSubNumberMetrics()
}
interface SubNumberMetric {
String getName()
N getValue(C container)
}
{code}
With the access pattern being
{code}
C container = metric.getContainer();
for (SubNumberMetric subMetric :
metric.getSubNumberMetrics()) {
String name = subMetric.getName();
Number value = subMetric.getValue(container);
// export sub metric
}
{code}
Forward-compatibility is again achieved by wrapping the histogram in an adapter.
Backwards-compatibility would be achieved by registering each SubNumberMetric
separately as a Gauge.
Now for the reporter interface:
For registering/removing metrics we would reduce the interface down to 6
methods:
{code}
void notifyOfAddedMetric(NumberMetric ...)
void notifyOfRemovedMetric(NumberMetric ...)
void notifyOfAddedMetric(StringMetric ...)
void notifyOfRemovedMetric(StringMetric ...)
void notifyOfAddedMetric(MultiNubmerMetric ...)
void notifyOfRemovedMetric(MultiNumberMetric ...)
{code}
We could modularize this by moving these methods into opt-in interfaces that a
reporter would implement, especially the StringMetric variant. (I.e
StringReporter, NumberReporter etc.)
Initially i also thought about adding a CollectionMetric to support the latency
metric but it would suffer from the same problems we do right now for gauges.
We can keep the latency metric, but it will be never be reported to the outside
and strictly kept for the webUI. At the moment it's essentially unusable when
exported by a reporter anyway.
So that's what i cooked up.
> Add explicit notifyOfAddedX methods to MetricReporter interface
> ---
>
> Key: FLINK-5095
> URL: https://issues.apache.org/jira/browse/FLINK-5095
> Project: Flink
> Issue Type: Improvement
> Components: Metrics
>Affects Versions: 1.1.3
>Reporter: Chesnay Schepler
>Priority: Minor
>
> 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:
> {code}
> void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group);
> void notifyOfRemovedMetric(Metric metric, String metricName, MetricGroup
> group);
> {code}
> 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:
> {code}
> 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
> }
> {code}
> 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
[jira] [Commented] (FLINK-5095) Add explicit notifyOfAddedX methods to MetricReporter interface
[
https://issues.apache.org/jira/browse/FLINK-5095?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16008430#comment-16008430
]
Bowen Li commented on FLINK-5095:
-
[~Zentol] Can you list your proposed interfaces here? Want to make sure we are
in the same page for the discussion.
> Add explicit notifyOfAddedX methods to MetricReporter interface
> ---
>
> Key: FLINK-5095
> URL: https://issues.apache.org/jira/browse/FLINK-5095
> Project: Flink
> Issue Type: Improvement
> Components: Metrics
>Affects Versions: 1.1.3
>Reporter: Chesnay Schepler
>Priority: Minor
>
> 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:
> {code}
> void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group);
> void notifyOfRemovedMetric(Metric metric, String metricName, MetricGroup
> group);
> {code}
> 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:
> {code}
> 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
> }
> {code}
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (FLINK-5095) Add explicit notifyOfAddedX methods to MetricReporter interface
[
https://issues.apache.org/jira/browse/FLINK-5095?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15978103#comment-15978103
]
Chesnay Schepler commented on FLINK-5095:
-
It's less maintainable. By introducing a set of generic primitives we can add
more metric types without having to change all reporters. Take a timer for
example. If we introduced that now not a single existing reporter could report
it and would have to be updated. In contrast, if the timer would just be
another generic "NumberMetric" they would be instantly supported.
Furthermore, we would no longer have the generic Gauge type which is just a
pain to deal with; if you can't deal with non-numeric gauges you cannot just
ignore them in notify; after all a metric might return null and you can't check
the type of that. You can only do that (somewhat reliably) in report().
> Add explicit notifyOfAddedX methods to MetricReporter interface
> ---
>
> Key: FLINK-5095
> URL: https://issues.apache.org/jira/browse/FLINK-5095
> Project: Flink
> Issue Type: Improvement
> Components: Metrics
>Affects Versions: 1.1.3
>Reporter: Chesnay Schepler
>Priority: Minor
>
> 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:
> {code}
> void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group);
> void notifyOfRemovedMetric(Metric metric, String metricName, MetricGroup
> group);
> {code}
> 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:
> {code}
> 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
> }
> {code}
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (FLINK-5095) Add explicit notifyOfAddedX methods to MetricReporter interface
[
https://issues.apache.org/jira/browse/FLINK-5095?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15977893#comment-15977893
]
Bowen Li commented on FLINK-5095:
-
What's the main disadvantage of having counter, meters, and gauge separately
besides the increased number of API?
> Add explicit notifyOfAddedX methods to MetricReporter interface
> ---
>
> Key: FLINK-5095
> URL: https://issues.apache.org/jira/browse/FLINK-5095
> Project: Flink
> Issue Type: Improvement
> Components: Metrics
>Affects Versions: 1.1.3
>Reporter: Chesnay Schepler
>Priority: Minor
>
> 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:
> {code}
> void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group);
> void notifyOfRemovedMetric(Metric metric, String metricName, MetricGroup
> group);
> {code}
> 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:
> {code}
> 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
> }
> {code}
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (FLINK-5095) Add explicit notifyOfAddedX methods to MetricReporter interface
[
https://issues.apache.org/jira/browse/FLINK-5095?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15974410#comment-15974410
]
Chesnay Schepler commented on FLINK-5095:
-
My current thinking goes i the direction to abstract away the specific metric
type. In a way, counters, meters and Gauge can all be handled
identically, like a Gauge. Similarly, all remaining Gauges can
be handled like a Gauge. The Histogram can be generalized into a
Multi-Gauge, which is useful anyway.
This would reduce the total number of metric types that the reporter have to
deal with to 3. This number also wouldn't necessarily increase over time; a
Timer for example would also just be a Gauge.
> Add explicit notifyOfAddedX methods to MetricReporter interface
> ---
>
> Key: FLINK-5095
> URL: https://issues.apache.org/jira/browse/FLINK-5095
> Project: Flink
> Issue Type: Improvement
> Components: Metrics
>Affects Versions: 1.1.3
>Reporter: Chesnay Schepler
>Priority: Minor
>
> 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:
> {code}
> void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group);
> void notifyOfRemovedMetric(Metric metric, String metricName, MetricGroup
> group);
> {code}
> 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:
> {code}
> 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
> }
> {code}
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (FLINK-5095) Add explicit notifyOfAddedX methods to MetricReporter interface
[
https://issues.apache.org/jira/browse/FLINK-5095?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15974103#comment-15974103
]
Bowen Li commented on FLINK-5095:
-
I'll spend time in the following few days to see how other projects (hadoop,
kafka, etc) handles this situation, and kick off a discussion
> Add explicit notifyOfAddedX methods to MetricReporter interface
> ---
>
> Key: FLINK-5095
> URL: https://issues.apache.org/jira/browse/FLINK-5095
> Project: Flink
> Issue Type: Improvement
> Components: Metrics
>Affects Versions: 1.1.3
>Reporter: Chesnay Schepler
>Priority: Minor
>
> 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:
> {code}
> void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group);
> void notifyOfRemovedMetric(Metric metric, String metricName, MetricGroup
> group);
> {code}
> 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:
> {code}
> 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
> }
> {code}
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (FLINK-5095) Add explicit notifyOfAddedX methods to MetricReporter interface
[
https://issues.apache.org/jira/browse/FLINK-5095?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15926684#comment-15926684
]
Bowen Li commented on FLINK-5095:
-
Agree that MetricReporter interface is not extendable and scalable, and will
definitely causing problems while Flink's metrics and monitoring systems grow.
I'll take some time to think about this as well as
https://issues.apache.org/jira/browse/FLINK-6053
> Add explicit notifyOfAddedX methods to MetricReporter interface
> ---
>
> Key: FLINK-5095
> URL: https://issues.apache.org/jira/browse/FLINK-5095
> Project: Flink
> Issue Type: Improvement
> Components: Metrics
>Affects Versions: 1.1.3
>Reporter: Chesnay Schepler
>Priority: Minor
>
> 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:
> {code}
> void notifyOfAddedMetric(Metric metric, String metricName, MetricGroup group);
> void notifyOfRemovedMetric(Metric metric, String metricName, MetricGroup
> group);
> {code}
> 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:
> {code}
> 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
> }
> {code}
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
