You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by GitBox <gi...@apache.org> on 2020/04/21 14:13:13 UTC
[GitHub] [storm] agresch commented on a change in pull request #3255: [STORM-3627] Add flag to use metrics shortname for topology
agresch commented on a change in pull request #3255:
URL: https://github.com/apache/storm/pull/3255#discussion_r412219276
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -36,10 +37,21 @@
private final MetricRegistry registry = new MetricRegistry();
private final List<StormReporter> reporters = new ArrayList<>();
private String hostName = null;
+ private boolean useShortName = false;
+
+ public StormMetricRegistry(boolean useShortName) {
Review comment:
I would pass the topology conf instead in case we want to add other future options to configure this class.
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -36,10 +37,21 @@
private final MetricRegistry registry = new MetricRegistry();
private final List<StormReporter> reporters = new ArrayList<>();
private String hostName = null;
+ private boolean useShortName = false;
+
+ public StormMetricRegistry(boolean useShortName) {
+ this.useShortName = useShortName;
+ }
+
+ public StormMetricRegistry() {
+ this.useShortName = false;
+ LOG.warn("Constructing StormMetricsRegistry: " + ClusterUtils.stringifyError(new Exception("Stack trace")));
+ }
public <T> SimpleGauge<T> gauge(
T initialValue, String name, String topologyId, String componentId, Integer taskId, Integer port) {
String metricName = metricName(name, topologyId, componentId, taskId, port);
+ LOG.warn("Adding gauge: " + metricName);
Review comment:
Why warn here? Debug code?
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -36,10 +37,21 @@
private final MetricRegistry registry = new MetricRegistry();
private final List<StormReporter> reporters = new ArrayList<>();
private String hostName = null;
+ private boolean useShortName = false;
+
+ public StormMetricRegistry(boolean useShortName) {
+ this.useShortName = useShortName;
+ }
+
+ public StormMetricRegistry() {
+ this.useShortName = false;
+ LOG.warn("Constructing StormMetricsRegistry: " + ClusterUtils.stringifyError(new Exception("Stack trace")));
Review comment:
Why is this a warning?
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -111,6 +126,9 @@ public void stop() {
}
private String metricName(String name, String stormId, String componentId, String streamId, Integer taskId, Integer workerPort) {
+ if (this.useShortName) {
Review comment:
useShortWorkerName instead possibly?
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -111,6 +126,9 @@ public void stop() {
}
private String metricName(String name, String stormId, String componentId, String streamId, Integer taskId, Integer workerPort) {
+ if (this.useShortName) {
+ return "storm.worker." + name;
Review comment:
Can we replace all of these with a constant?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org