[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2771 ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r207672905
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
--- End diff --
Yes, I agree that for the non-MetricSet metrics, we can just use the
getOrAdd wrappers. If we don't need MetricSet with a non-static registry, we
should be good if we merge the changes in
https://github.com/apache/storm/pull/2783.
I agree that we should upgrade, but versions past 4.x have removed the
metrics-ganglia module. I'm not sure if it's been spun off somewhere, or if
it's just been deleted, but I didn't want to start removing stuff related to
Ganglia in https://github.com/apache/storm/pull/2783 as well. If we want to
upgrade past 3.1 I think we should do it in a separate PR.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r207665264
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -50,19 +61,19 @@ public static void startMetricsReporters(Map topoConf) {
}
}
-private static T register(final String name, T
metric) {
-T ret;
+@Override
+//This is more similar to super#getOrAdd than super#register
+public T register(final String name, T metric)
throws IllegalArgumentException {
--- End diff --
It is fine. But I would like to see more javadoc for this method.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r207663344
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -50,19 +61,19 @@ public static void startMetricsReporters(Map topoConf) {
}
}
-private static T register(final String name, T
metric) {
-T ret;
+@Override
+//This is more similar to super#getOrAdd than super#register
+public T register(final String name, T metric)
throws IllegalArgumentException {
--- End diff --
I am okay with it this way for now.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r207640687
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
--- End diff --
Our customization upon MetricsRegistry is actually very similar to the
wrapping methods of `#getOrAdd`, such as `gauge`, `timer`, `meter`, and
`histogram`. They do not have a way to eliminate double registration of
MetricSet though, although we could avoid this altogether with a non-static
registry.
In addition, I think we should probably upgrading to a newer version of
Dropwizard, since current version (3.1.0) is about to be EOL. Their 4.x has a
lot of improvement and provides more features on top of Java 8.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r207638261
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
--- End diff --
I'll take a look at this
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r207636496
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -50,19 +61,19 @@ public static void startMetricsReporters(Map topoConf) {
}
}
-private static T register(final String name, T
metric) {
-T ret;
+@Override
+//This is more similar to super#getOrAdd than super#register
+public T register(final String name, T metric)
throws IllegalArgumentException {
--- End diff --
I added a test for registerMetricSet and unregisterMetricSet in #2754 and
#2764 to show that this method has solved the issue of double registration. See
https://github.com/apache/storm/pull/2754/commits/597c6bb2d41a7aa0d25c6aab9201b451f6b1eaf1
I don't know if we should move the change to `registerMetricSet` and
`unregisterMetricSet` back to this PR, but the lack of them seems to be
confusing people of the purpose of this change. @Ethanlm @srdo @revans2
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r207601720
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
--- End diff --
I think we can make the registry non-static without it being a huge hassle
https://github.com/apache/storm/pull/2783. I'll rebase it once
https://github.com/apache/storm/pull/2752 goes in, and hopefully we can use it
to resolve https://github.com/apache/storm/pull/2714.
Regarding the subclassing, I'm wondering if we should instead go ask the
Dropwizard guys whether the inability to use `getOrAdd` for MetricSets is
intended, and if not whether they'd be open to adding a method to
MetricRegistry to allow it. If we're hitting this issue, other people probably
are too, and I'd much rather fix it in the library than try to hack around it
here.
What do you think @zd-project?
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r207599113
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
--- End diff --
The reason I changed the `static register()` to instance `register()`
method is stated here. Also I should point out that because
StormMetricsRegistry is a static singleton, it will persist throughout each
UnitTest. So if a UnitTest class has multiple tests in it, all metrics will
actually be registered multiple times. We either have to change all unit tests
to have set up and tear down, or we have to completely revamp the
StormMetricsRegistry class to make it non-static. As I don't have that much
time to do either of them, I came up with this walkaround. @srdo @revans2
@Ethanlm
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r207594931
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -50,19 +61,19 @@ public static void startMetricsReporters(Map topoConf) {
}
}
-private static T register(final String name, T
metric) {
-T ret;
+@Override
+//This is more similar to super#getOrAdd than super#register
+public T register(final String name, T metric)
throws IllegalArgumentException {
--- End diff --
if the metric is MetricSet, it will be having the same issue with the old
code.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r205520696
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
--- End diff --
Yes, that's why I opened #2714 to discuss about improvement.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r205046141
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -88,19 +110,24 @@ public static void startMetricsReporters(Map topoConf) {
}
}
-private static T register(final String name, T
metric) {
-T ret;
-try {
-ret = DEFAULT_REGISTRY.register(name, metric);
-} catch (IllegalArgumentException e) {
-// swallow IllegalArgumentException when the metric exists
already
-ret = (T) DEFAULT_REGISTRY.getMetrics().get(name);
-if (ret == null) {
-throw e;
-} else {
-LOG.warn("Metric {} has already been registered", name);
-}
+public static String name(String prefix, String name) {
+assert name != null;
+return StringUtils.isEmpty(prefix) ? name : prefix + ':' + name;
+}
+
+public static String name(Class klass, String names) {
+return name(klass.getName(), names);
+}
+
+@Override
+//This is more similar to super#getOrAdd than super#register
--- End diff --
Yes, I think otherwise you'd still have to put the try-catch block around
the `super.register` call.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r205045839
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -65,21 +67,41 @@ public static Meter registerMeter(final String name) {
* @param type of value the gauge measures
*/
public static void registerProvidedGauge(final String name, final
Gauge gauge) {
-register(name, gauge);
+DEFAULT_REGISTRY.register(name, gauge);
+}
+
+public static Histogram registerHistogram(String name) {
--- End diff --
Yes, I thought it was a little weird that their API also did that. It's
fine, thanks for explaining.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r205045449
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
--- End diff --
Ok. I really dislike the contortions we have to go through in order to make
this work as a static singleton, but thanks for explaining.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r205044870
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,79 +12,122 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Timer;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-public static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
+private static final StormMetricsRegistry DEFAULT_REGISTRY = new
StormMetricsRegistry();
private static final Logger LOG =
LoggerFactory.getLogger(StormMetricsRegistry.class);
-public static Meter registerMeter(String name) {
-Meter meter = new Meter();
-return register(name, meter);
-}
+private StormMetricsRegistry() {/*Singleton pattern*/}
-// TODO: should replace Callable to Gauge when nimbus.clj is
translated to java
-public static Gauge registerGauge(final String name, final
Callable fn) {
-Gauge gauge = new Gauge() {
-@Override
-public Integer getValue() {
-try {
-return (Integer) fn.call();
-} catch (Exception e) {
-LOG.error("Error getting gauge value for {}", name, e);
-}
-return 0;
+/**
+ * Register a gauge with provided callback. This swallows all
exceptions
+ * thrown from the callback, consider using {@link
#registerProvidedGauge(String, Gauge)}
+ * if no exceptions will be thrown by the callable.
+ *
+ * @param name name of the gauge
+ * @param fn callback that measures
+ * @param type of measurement the callback returns, also the type
of gauge
+ * @return registered gauge
+ */
+public static Gauge registerGauge(final String name, final
Callable fn) {
+return DEFAULT_REGISTRY.register(name, () -> {
+try {
+return fn.call();
+} catch (Exception e) {
+LOG.error("Error getting gauge value for {}", name, e);
}
-};
-return register(name, gauge);
+return null;
+});
+}
+
+/**
+ * Register a provided gauge. Use this method if a custom gauge is
needed or
+ * exceptions have already been handled.
+ *
+ * @param name name of the gauge
+ * @param gauge gauge
+ * @param type of value the gauge measures
+ */
+public static void registerProvidedGauge(final String name, final
Gauge gauge) {
+DEFAULT_REGISTRY.register(name, gauge);
}
-public static void registerProvidedGauge(final String name, Gauge
gauge) {
-register(name, gauge);
+public static Histogram registerHistogram(String name) {
+return registerHistogram(name, new
ExponentiallyDecayingReservoir());
}
public static Histogram registerHistogram(String name, Reservoir
reservoir) {
-Histogram histogram = new Histogram(reservoir);
-return register(name, histogram);
+return DEFAULT_REGISTRY.register(name, new Histogram(reservoir));
}
+public static Meter registerMeter(String name) {
+return DEFAULT_REGISTRY.register(name, new Meter());
+}
+
+//Change the name to avoid name conflict in future Metrics release
--- End diff --
You are right, my bad.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204920511
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -88,19 +110,24 @@ public static void startMetricsReporters(Map topoConf) {
}
}
-private static T register(final String name, T
metric) {
-T ret;
-try {
-ret = DEFAULT_REGISTRY.register(name, metric);
-} catch (IllegalArgumentException e) {
-// swallow IllegalArgumentException when the metric exists
already
--- End diff --
DEFAULT_REGISTRY.getMetrics().get(name) won't properly capture registration
of a MetricSet because under the hood MetricRegistry registers each metric in
MetricSet with name as prefix, instead of the whole MetricSet with name.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204917201
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/supervisor/Supervisor.java
---
@@ -312,7 +315,7 @@ public void launchDaemon() {
launch();
Utils.addShutdownHookWithForceKillIn1Sec(this::close);
-registerWorkerNumGauge("supervisor:num-slots-used-gauge",
conf);
+registerGauge(name(SUPERVISOR, "num-slots-used-gauge"), () ->
SupervisorUtils.supervisorWorkerIds(conf).size());
--- End diff --
Okay will refactor this.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204916859
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,79 +12,122 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Timer;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-public static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
+private static final StormMetricsRegistry DEFAULT_REGISTRY = new
StormMetricsRegistry();
private static final Logger LOG =
LoggerFactory.getLogger(StormMetricsRegistry.class);
-public static Meter registerMeter(String name) {
-Meter meter = new Meter();
-return register(name, meter);
-}
+private StormMetricsRegistry() {/*Singleton pattern*/}
-// TODO: should replace Callable to Gauge when nimbus.clj is
translated to java
-public static Gauge registerGauge(final String name, final
Callable fn) {
-Gauge gauge = new Gauge() {
-@Override
-public Integer getValue() {
-try {
-return (Integer) fn.call();
-} catch (Exception e) {
-LOG.error("Error getting gauge value for {}", name, e);
-}
-return 0;
+/**
+ * Register a gauge with provided callback. This swallows all
exceptions
+ * thrown from the callback, consider using {@link
#registerProvidedGauge(String, Gauge)}
+ * if no exceptions will be thrown by the callable.
+ *
+ * @param name name of the gauge
+ * @param fn callback that measures
+ * @param type of measurement the callback returns, also the type
of gauge
+ * @return registered gauge
+ */
+public static Gauge registerGauge(final String name, final
Callable fn) {
+return DEFAULT_REGISTRY.register(name, () -> {
+try {
+return fn.call();
+} catch (Exception e) {
+LOG.error("Error getting gauge value for {}", name, e);
}
-};
-return register(name, gauge);
+return null;
+});
+}
+
+/**
+ * Register a provided gauge. Use this method if a custom gauge is
needed or
+ * exceptions have already been handled.
+ *
+ * @param name name of the gauge
+ * @param gauge gauge
+ * @param type of value the gauge measures
+ */
+public static void registerProvidedGauge(final String name, final
Gauge gauge) {
+DEFAULT_REGISTRY.register(name, gauge);
}
-public static void registerProvidedGauge(final String name, Gauge
gauge) {
-register(name, gauge);
+public static Histogram registerHistogram(String name) {
+return registerHistogram(name, new
ExponentiallyDecayingReservoir());
}
public static Histogram registerHistogram(String name, Reservoir
reservoir) {
-Histogram histogram = new Histogram(reservoir);
-return register(name, histogram);
+return DEFAULT_REGISTRY.register(name, new Histogram(reservoir));
}
+public static Meter registerMeter(String name) {
+return DEFAULT_REGISTRY.register(name, new Meter());
+}
+
+//Change the name to avoid name conflict in future Metrics release
--- End diff --
I believe in java static keyword doesn't affect function signature.
Therefore you'd get error if you have a static method in child class with same
signature as an instance method in parent class, saying "static method cannot
override instance method".
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204915515
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -65,21 +67,41 @@ public static Meter registerMeter(final String name) {
* @param type of value the gauge measures
*/
public static void registerProvidedGauge(final String name, final
Gauge gauge) {
-register(name, gauge);
+DEFAULT_REGISTRY.register(name, gauge);
+}
+
+public static Histogram registerHistogram(String name) {
--- End diff --
I observed that most of the histogram we used has
ExponentiallyDecayingReservoir as default, so I was thinking we have a default
method. Notice that ExponentiallyDecayingReservoir is also the the default when
invoking `MetricRegistry#histogram(String name)`.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204894886
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
--- End diff --
Though, after the change, client still cannot call `register` directly,
because the singleton is private.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204891653
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -88,19 +110,24 @@ public static void startMetricsReporters(Map topoConf) {
}
}
-private static T register(final String name, T
metric) {
-T ret;
-try {
-ret = DEFAULT_REGISTRY.register(name, metric);
-} catch (IllegalArgumentException e) {
-// swallow IllegalArgumentException when the metric exists
already
-ret = (T) DEFAULT_REGISTRY.getMetrics().get(name);
-if (ret == null) {
-throw e;
-} else {
-LOG.warn("Metric {} has already been registered", name);
-}
+public static String name(String prefix, String name) {
+assert name != null;
+return StringUtils.isEmpty(prefix) ? name : prefix + ':' + name;
+}
+
+public static String name(Class klass, String names) {
+return name(klass.getName(), names);
+}
+
+@Override
+//This is more similar to super#getOrAdd than super#register
--- End diff --
Would you recommend switch back to try-catch block then?
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204890243
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
--- End diff --
I switched to inheritance because I need to override parent's `register()`
method and swallows the exceptions, instead of provide a static `register()`
method which delegate the registration. This is because dropwizard handles
registering MetricSet differently under the hood and double registration can't
be caught by current `static register()` method.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204862238
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -65,21 +67,41 @@ public static Meter registerMeter(final String name) {
* @param type of value the gauge measures
*/
public static void registerProvidedGauge(final String name, final
Gauge gauge) {
-register(name, gauge);
+DEFAULT_REGISTRY.register(name, gauge);
+}
+
+public static Histogram registerHistogram(String name) {
+return registerHistogram(name, new
ExponentiallyDecayingReservoir());
}
public static Histogram registerHistogram(String name, Reservoir
reservoir) {
-return register(name, new Histogram(reservoir));
+return DEFAULT_REGISTRY.register(name, new Histogram(reservoir));
+}
+
+public static Meter registerMeter(String name) {
+return DEFAULT_REGISTRY.register(name, new Meter());
+}
+
+//Change the name to avoid name conflict in future Metrics release
+public static void registerMetricSet(MetricSet metrics) {
+DEFAULT_REGISTRY.registerAll(metrics);
}
-public static void registerAll(final String prefix, MetricSet metrics)
{
-register(prefix, metrics);
+public static void unregisterMetricSet(com.codahale.metrics.MetricSet
metrics) {
+for (String s : metrics.getMetrics().keySet()) {
+DEFAULT_REGISTRY.remove(s);
+}
}
-public static void unregister(final String name) {
-DEFAULT_REGISTRY.remove(name);
+public static Timer registerTimer(String name) {
+return DEFAULT_REGISTRY.register(name, new Timer());
}
+/**
+ * Start metrics reporter with this metric registry.
--- End diff --
Nit: It's not clear what `this` is here. Consider rewording to e.g. `Start
metrics reporters for the registry singleton`.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204860271
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -65,21 +67,41 @@ public static Meter registerMeter(final String name) {
* @param type of value the gauge measures
*/
public static void registerProvidedGauge(final String name, final
Gauge gauge) {
-register(name, gauge);
+DEFAULT_REGISTRY.register(name, gauge);
+}
+
+public static Histogram registerHistogram(String name) {
--- End diff --
Nit: Might be helpful to add a note to the javadoc here that it uses this
particular reservoir, or maybe rename the method to describe it, e.g.
`registerFiveMinuteHistogram` or something like that.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204876125
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/supervisor/Supervisor.java
---
@@ -312,7 +315,7 @@ public void launchDaemon() {
launch();
Utils.addShutdownHookWithForceKillIn1Sec(this::close);
-registerWorkerNumGauge("supervisor:num-slots-used-gauge",
conf);
+registerGauge(name(SUPERVISOR, "num-slots-used-gauge"), () ->
SupervisorUtils.supervisorWorkerIds(conf).size());
--- End diff --
This is personal preference, but I think static method imports can tend to
make the code less readable. It's fine when the static import is something
well-known like `assertThat` in a test, but having something like
`registerGauge` show up like this makes it look like the method exists on this
class.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204867270
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -88,19 +110,24 @@ public static void startMetricsReporters(Map topoConf) {
}
}
-private static T register(final String name, T
metric) {
-T ret;
-try {
-ret = DEFAULT_REGISTRY.register(name, metric);
-} catch (IllegalArgumentException e) {
-// swallow IllegalArgumentException when the metric exists
already
-ret = (T) DEFAULT_REGISTRY.getMetrics().get(name);
-if (ret == null) {
-throw e;
-} else {
-LOG.warn("Metric {} has already been registered", name);
-}
+public static String name(String prefix, String name) {
+assert name != null;
+return StringUtils.isEmpty(prefix) ? name : prefix + ':' + name;
+}
+
+public static String name(Class klass, String names) {
+return name(klass.getName(), names);
+}
+
+@Override
+//This is more similar to super#getOrAdd than super#register
--- End diff --
I think there's an issue with concurrency here now, thread 1 can do the
existence check for `metric1`, thread 2 registers `metric1`, and thread 1 then
calls `super.register("metric1")` and gets an uncaught IllegalArgumentException.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204861682
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -65,21 +67,41 @@ public static Meter registerMeter(final String name) {
* @param type of value the gauge measures
*/
public static void registerProvidedGauge(final String name, final
Gauge gauge) {
-register(name, gauge);
+DEFAULT_REGISTRY.register(name, gauge);
+}
+
+public static Histogram registerHistogram(String name) {
+return registerHistogram(name, new
ExponentiallyDecayingReservoir());
}
public static Histogram registerHistogram(String name, Reservoir
reservoir) {
-return register(name, new Histogram(reservoir));
+return DEFAULT_REGISTRY.register(name, new Histogram(reservoir));
+}
+
+public static Meter registerMeter(String name) {
+return DEFAULT_REGISTRY.register(name, new Meter());
+}
+
+//Change the name to avoid name conflict in future Metrics release
+public static void registerMetricSet(MetricSet metrics) {
+DEFAULT_REGISTRY.registerAll(metrics);
}
-public static void registerAll(final String prefix, MetricSet metrics)
{
-register(prefix, metrics);
+public static void unregisterMetricSet(com.codahale.metrics.MetricSet
metrics) {
--- End diff --
Fully qualified class name seems unnecessary.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204875066
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,79 +12,122 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Timer;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-public static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
+private static final StormMetricsRegistry DEFAULT_REGISTRY = new
StormMetricsRegistry();
private static final Logger LOG =
LoggerFactory.getLogger(StormMetricsRegistry.class);
-public static Meter registerMeter(String name) {
-Meter meter = new Meter();
-return register(name, meter);
-}
+private StormMetricsRegistry() {/*Singleton pattern*/}
-// TODO: should replace Callable to Gauge when nimbus.clj is
translated to java
-public static Gauge registerGauge(final String name, final
Callable fn) {
-Gauge gauge = new Gauge() {
-@Override
-public Integer getValue() {
-try {
-return (Integer) fn.call();
-} catch (Exception e) {
-LOG.error("Error getting gauge value for {}", name, e);
-}
-return 0;
+/**
+ * Register a gauge with provided callback. This swallows all
exceptions
+ * thrown from the callback, consider using {@link
#registerProvidedGauge(String, Gauge)}
+ * if no exceptions will be thrown by the callable.
+ *
+ * @param name name of the gauge
+ * @param fn callback that measures
+ * @param type of measurement the callback returns, also the type
of gauge
+ * @return registered gauge
+ */
+public static Gauge registerGauge(final String name, final
Callable fn) {
+return DEFAULT_REGISTRY.register(name, () -> {
+try {
+return fn.call();
+} catch (Exception e) {
+LOG.error("Error getting gauge value for {}", name, e);
}
-};
-return register(name, gauge);
+return null;
+});
+}
+
+/**
+ * Register a provided gauge. Use this method if a custom gauge is
needed or
+ * exceptions have already been handled.
+ *
+ * @param name name of the gauge
+ * @param gauge gauge
+ * @param type of value the gauge measures
+ */
+public static void registerProvidedGauge(final String name, final
Gauge gauge) {
+DEFAULT_REGISTRY.register(name, gauge);
}
-public static void registerProvidedGauge(final String name, Gauge
gauge) {
-register(name, gauge);
+public static Histogram registerHistogram(String name) {
+return registerHistogram(name, new
ExponentiallyDecayingReservoir());
}
public static Histogram registerHistogram(String name, Reservoir
reservoir) {
-Histogram histogram = new Histogram(reservoir);
-return register(name, histogram);
+return DEFAULT_REGISTRY.register(name, new Histogram(reservoir));
}
+public static Meter registerMeter(String name) {
+return DEFAULT_REGISTRY.register(name, new Meter());
+}
+
+//Change the name to avoid name conflict in future Metrics release
--- End diff --
There couldn't be a name conflict, because this method is static, and the
one on the super class isn't.
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204862534
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
+private static final StormMetricsRegistry DEFAULT_REGISTRY = new
StormMetricsRegistry();
--- End diff --
Nit: I don't think we allow any non-default registry in this class, so
consider renaming to just `REGISTRY`
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2771#discussion_r204858543
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -12,28 +12,30 @@
package org.apache.storm.metric;
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.Meter;
import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.MetricSet;
import com.codahale.metrics.Reservoir;
+
import java.util.Map;
import java.util.concurrent.Callable;
+
+import com.codahale.metrics.Timer;
+import org.apache.commons.lang.StringUtils;
import org.apache.storm.daemon.metrics.MetricsUtils;
import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@SuppressWarnings("unchecked")
-public class StormMetricsRegistry {
-private static final MetricRegistry DEFAULT_REGISTRY = new
MetricRegistry();
+public class StormMetricsRegistry extends MetricRegistry {
--- End diff --
Why is it better to extend MetricRegistry rather than wrap an instance?
---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
GitHub user zd-project opened a pull request: https://github.com/apache/storm/pull/2771 STORM-3157: General improvement to StormMetricsRegistry This contains and address the issues in #2763. This should help standardize the process to register metrics and MetricSet in the future. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zd-project/storm STORM-3157 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2771.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 #2771 commit 802014c88a67ae5ef044c1f87889d976063eee71 Author: Zhengdai Hu Date: 2018-07-12T16:31:00Z STORM-3150: Improve Gauge registration methods and refactored code commit 2cd815011cb0bb37c124d2360c3bcc757d926038 Author: Zhengdai Hu Date: 2018-07-19T15:46:29Z STORM-3150: Refactoring commit ff45063c7ce1516c2d887d64affea636e2f031a1 Author: Zhengdai Hu Date: 2018-07-20T18:41:51Z STORM-3157: Improve StormMetricsRegistry in general ---
