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/11/10 22:24:30 UTC
[GitHub] [storm] agresch opened a new pull request #3350: STORM-3714 add rate tracking to TaskMetrics
agresch opened a new pull request #3350:
URL: https://github.com/apache/storm/pull/3350
## What is the purpose of the change
Add RateCounter class to have a metric with a count and one minute rate without the performance impact of a Meter. The code is based on Flink's MeterView, which updates the rate in a background thread. Switch TaskMetrics to use this to report the rate.
## How was the change tested
Ran word count topology and validated metric was reported and looked valid. Ran storm-client unit tests.
----------------------------------------------------------------
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
[GitHub] [storm] agresch commented on pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
agresch commented on pull request #3350:
URL: https://github.com/apache/storm/pull/3350#issuecomment-754845380
Crap, need to clean up...
----------------------------------------------------------------
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
[GitHub] [storm] agresch commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r549426741
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report the average rate of events per second over 1 minute. This class
+ * was added as a compromise to using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
Review comment:
What would be your preferred name?
----------------------------------------------------------------
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
[GitHub] [storm] agresch merged pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
agresch merged pull request #3350:
URL: https://github.com/apache/storm/pull/3350
----------------------------------------------------------------
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r542852889
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
+ * using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
+ taskId, workerPort);
+ this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
+ metricRegistry.getRateCounterUpdateIntervalSeconds());
+ this.values = new long[this.timeSpanInSeconds / metricRegistry.getRateCounterUpdateIntervalSeconds() + 1];
+
+ }
+
+ /**
+ * Reports the 1 minute rate for the metric.
+ * @return the rate
+ */
+ @Override
+ public Double getValue() {
+ return currentRate;
+ }
+
+ public void inc(long n) {
+ counter.inc(n);
+ }
+
+ /**
+ * Updates the rate in a background thread by a StormMetricRegistry at a fixed frequency.
+ */
+ void update() {
+ time = (time + 1) % values.length;
+ values[time] = counter.getCount();
+ currentRate = ((double) (values[time] - values[(time + 1) % values.length]) / timeSpanInSeconds);
Review comment:
My understanding is `value` is a ring, and the length of this ring is the elapsed time. `(valueAtTime - valueAtTimePlus1)` gives you the difference during this time frame. So the rate is `(valueAtTime - valueAtTimePlus1) / elapsed time`
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
+ * using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
+ taskId, workerPort);
+ this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
+ metricRegistry.getRateCounterUpdateIntervalSeconds());
+ this.values = new long[this.timeSpanInSeconds / metricRegistry.getRateCounterUpdateIntervalSeconds() + 1];
+
+ }
+
+ /**
+ * Reports the 1 minute rate for the metric.
+ * @return the rate
+ */
+ @Override
+ public Double getValue() {
+ return currentRate;
+ }
+
+ public void inc(long n) {
+ counter.inc(n);
+ }
+
+ /**
+ * Updates the rate in a background thread by a StormMetricRegistry at a fixed frequency.
+ */
+ void update() {
+ time = (time + 1) % values.length;
+ values[time] = counter.getCount();
+ currentRate = ((double) (values[time] - values[(time + 1) % values.length]) / timeSpanInSeconds);
Review comment:
My understanding is `values` is a ring, and the length of this ring is the elapsed time. `(valueAtTime - valueAtTimePlus1)` gives you the difference during this time frame. So the rate is `(valueAtTime - valueAtTimePlus1) / elapsed time`
----------------------------------------------------------------
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
[GitHub] [storm] agresch commented on pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
agresch commented on pull request #3350:
URL: https://github.com/apache/storm/pull/3350#issuecomment-754844976
removed whitespace and squashed commits
----------------------------------------------------------------
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r543032290
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
+ * using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
+ taskId, workerPort);
+ this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
+ metricRegistry.getRateCounterUpdateIntervalSeconds());
+ this.values = new long[this.timeSpanInSeconds / metricRegistry.getRateCounterUpdateIntervalSeconds() + 1];
+
+ }
+
+ /**
+ * Reports the 1 minute rate for the metric.
+ * @return the rate
+ */
+ @Override
+ public Double getValue() {
+ return currentRate;
+ }
+
+ public void inc(long n) {
+ counter.inc(n);
+ }
+
+ /**
+ * Updates the rate in a background thread by a StormMetricRegistry at a fixed frequency.
+ */
+ void update() {
+ time = (time + 1) % values.length;
+ values[time] = counter.getCount();
+ currentRate = ((double) (values[time] - values[(time + 1) % values.length]) / timeSpanInSeconds);
Review comment:
Yes the call of `update` happens in a separate thread
----------------------------------------------------------------
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r552170739
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report the average rate of events per second over 1 minute. This class
Review comment:
I was thinking that since `RATE_COUNTER_UPDATE_INTERVAL_SECONDS` is passed from `StormMetricRegistry`, if it changes to some value like `21s`, then
```
this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
metricRegistry.getRateCounterUpdateIntervalSeconds());
```
we have `this.timeSpanInSeconds = 42s` so the metric is the average rate of events per 42s, instead of one minute.
But I think it is okay for now since `RATE_COUNTER_UPDATE_INTERVAL_SECONDS` is not configurable and not likely to be changed to a value like 21s.
Future developers need to be aware of this implementation details if they want to change `RATE_COUNTER_UPDATE_INTERVAL_SECONDS` or make it configurable.
----------------------------------------------------------------
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
[GitHub] [storm] bipinprasad commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r542780777
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
+ * using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
+ taskId, workerPort);
+ this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
+ metricRegistry.getRateCounterUpdateIntervalSeconds());
+ this.values = new long[this.timeSpanInSeconds / metricRegistry.getRateCounterUpdateIntervalSeconds() + 1];
+
+ }
+
+ /**
+ * Reports the 1 minute rate for the metric.
+ * @return the rate
+ */
+ @Override
+ public Double getValue() {
+ return currentRate;
+ }
+
+ public void inc(long n) {
+ counter.inc(n);
+ }
+
+ /**
+ * Updates the rate in a background thread by a StormMetricRegistry at a fixed frequency.
+ */
+ void update() {
+ time = (time + 1) % values.length;
+ values[time] = counter.getCount();
+ currentRate = ((double) (values[time] - values[(time + 1) % values.length]) / timeSpanInSeconds);
Review comment:
Should this (mod) subtraction be from (valueAtTime - valueAtTimeMinus1), instead of (valueAtTime - valueAtTimePlus1)
----------------------------------------------------------------
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
[GitHub] [storm] agresch commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r549426670
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report the average rate of events per second over 1 minute. This class
Review comment:
In our case, timeSpanInSeconds is set to 60. I could allow passing in a value to make it more flexible if desired.
----------------------------------------------------------------
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
[GitHub] [storm] bipinprasad commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r542902434
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
+ * using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
+ taskId, workerPort);
+ this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
+ metricRegistry.getRateCounterUpdateIntervalSeconds());
+ this.values = new long[this.timeSpanInSeconds / metricRegistry.getRateCounterUpdateIntervalSeconds() + 1];
+
+ }
+
+ /**
+ * Reports the 1 minute rate for the metric.
+ * @return the rate
+ */
+ @Override
+ public Double getValue() {
+ return currentRate;
+ }
+
+ public void inc(long n) {
+ counter.inc(n);
+ }
+
+ /**
+ * Updates the rate in a background thread by a StormMetricRegistry at a fixed frequency.
+ */
+ void update() {
+ time = (time + 1) % values.length;
+ values[time] = counter.getCount();
+ currentRate = ((double) (values[time] - values[(time + 1) % values.length]) / timeSpanInSeconds);
Review comment:
Unless, the whole ring represents exact duration of timeSpanInSeconds - but them that would require some guarantee in the call frequency of update().
----------------------------------------------------------------
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
[GitHub] [storm] agresch commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r546891337
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
+ * using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
+ taskId, workerPort);
+ this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
+ metricRegistry.getRateCounterUpdateIntervalSeconds());
+ this.values = new long[this.timeSpanInSeconds / metricRegistry.getRateCounterUpdateIntervalSeconds() + 1];
+
+ }
+
+ /**
+ * Reports the 1 minute rate for the metric.
+ * @return the rate
+ */
+ @Override
+ public Double getValue() {
+ return currentRate;
+ }
+
+ public void inc(long n) {
+ counter.inc(n);
+ }
+
+ /**
+ * Updates the rate in a background thread by a StormMetricRegistry at a fixed frequency.
+ */
+ void update() {
+ time = (time + 1) % values.length;
+ values[time] = counter.getCount();
+ currentRate = ((double) (values[time] - values[(time + 1) % values.length]) / timeSpanInSeconds);
Review comment:
Ethan is correct. A separate thread updates at a fixed frequency.
----------------------------------------------------------------
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r542849974
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
Review comment:
The calculation in the `update()` method looks to me is a per second rate, instead of 1 minute rate. Am I misunderstanding?
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
+ * using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
+ taskId, workerPort);
+ this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
+ metricRegistry.getRateCounterUpdateIntervalSeconds());
Review comment:
I am having difficulties understanding this. Can you please elaborate? What does `timeSpanInSeconds ` and this equation mean? Thanks
----------------------------------------------------------------
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
[GitHub] [storm] agresch commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r546889275
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
+ * using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
+ taskId, workerPort);
+ this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
+ metricRegistry.getRateCounterUpdateIntervalSeconds());
Review comment:
timeSpanInSeconds is 60 for this. The mod seems to be attempting to handle the case where the update interval is odd divisor of the desired timespan.
----------------------------------------------------------------
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r548268637
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
+ * using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
+ taskId, workerPort);
+ this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
+ metricRegistry.getRateCounterUpdateIntervalSeconds());
Review comment:
Got it. Thanks for explaining
----------------------------------------------------------------
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
[GitHub] [storm] agresch commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r546889476
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
Review comment:
You are correct, I will update. It should be rate per second over a one minute period.
----------------------------------------------------------------
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
[GitHub] [storm] bipinprasad commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r542891954
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report a 1 minute rate. This class was added as a compromise to
+ * using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
+ taskId, workerPort);
+ this.timeSpanInSeconds = Math.max(60 - (60 % metricRegistry.getRateCounterUpdateIntervalSeconds()),
+ metricRegistry.getRateCounterUpdateIntervalSeconds());
+ this.values = new long[this.timeSpanInSeconds / metricRegistry.getRateCounterUpdateIntervalSeconds() + 1];
+
+ }
+
+ /**
+ * Reports the 1 minute rate for the metric.
+ * @return the rate
+ */
+ @Override
+ public Double getValue() {
+ return currentRate;
+ }
+
+ public void inc(long n) {
+ counter.inc(n);
+ }
+
+ /**
+ * Updates the rate in a background thread by a StormMetricRegistry at a fixed frequency.
+ */
+ void update() {
+ time = (time + 1) % values.length;
+ values[time] = counter.getCount();
+ currentRate = ((double) (values[time] - values[(time + 1) % values.length]) / timeSpanInSeconds);
Review comment:
It is a ring - therefore the use of time%length for index. That is fine.
But at each update() call, the index "time" is first incremented. Then the counter is placed in idx "time". Therefore the previos value is in index-1.
The increase in counter value should still be currentValue - previousValue. Otherwise the difference will be a negative number.
----------------------------------------------------------------
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3350: STORM-3714 add rate tracking to TaskMetrics
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3350:
URL: https://github.com/apache/storm/pull/3350#discussion_r548267320
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -369,6 +398,7 @@ private String dotToUnderScore(String str) {
return str.replace('.', '_');
}
+
Review comment:
nit: this newline can be removed
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report the average rate of events per second over 1 minute. This class
Review comment:
It is not always event per second over 1 minute. It depends on what the value of `timeSpanInSeconds` is
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/RateCounter.java
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version
+ * 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
+ * and limitations under the License.
+ */
+
+package org.apache.storm.metrics2;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+
+/**
+ * A Counter metric that also implements a Gauge to report the average rate of events per second over 1 minute. This class
+ * was added as a compromise to using a Meter, which has a much larger performance impact.
+ */
+public class RateCounter implements Gauge<Double> {
+ private Counter counter;
+ private double currentRate = 0;
+ private int time = 0;
+ private final long[] values;
+ private final int timeSpanInSeconds;
+
+ RateCounter(StormMetricRegistry metricRegistry, String metricName, String topologyId,
+ String componentId, int taskId, int workerPort, String streamId) {
+ counter = metricRegistry.counter(metricName, topologyId, componentId,
+ taskId, workerPort, streamId);
+ metricRegistry.gauge(metricName + ".m1_rate", this, topologyId, componentId, streamId,
Review comment:
`m1_rate` needs to change
----------------------------------------------------------------
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