Latency utils #50

shaqianqian · 2019-06-25T15:46:40Z

Description

use LatencyUtil to calculate the mean and percentile of datas

How Has This Been Tested?

Yes

update the version of jmeter

ubikloadpack · 2019-07-05T11:48:14Z

src/core/org/apache/jmeter/visualizers/SamplingStatCalculator.java

@@ -266,6 +267,7 @@ public String toString() {
        mySB.append("Max: " + this.getMax() + "  ");
        mySB.append("Error Rate: " + this.getErrorPercentage() + "  ");
        mySB.append("Sample Rate: " + this.getRate());
+        mySB.append("elapse: " + this.getElapsed());


Modify 'elapse:' to 'elapsed:'

ubikloadpack · 2019-07-05T11:51:16Z

src/jorphan/org/apache/jorphan/math/HistogramStatCalculatorLong.java

+    }
+
+    @Override
+    public void clear() {


Shouldn't you be clearing/resetting latencyStats ? and also call stop on object to make threads stop ?

ubikloadpack · 2019-07-05T11:55:22Z

src/core/org/apache/jmeter/report/processor/StatisticsSummaryData.java

@@ -25,17 +28,14 @@
 *
 */
 public class StatisticsSummaryData {
-
+    LatencyStats latencyStats = new LatencyStats(1,3600000000000L,2,1024,10000000000L,null);


Why is PauseDetector null ? We want this feature

Is the window 1024 big enough ? What is the logic behind ? Make it a configurable property

intervalEstimatorWindowLength is 10000000000L, What is the logic behind ?

ubikloadpack · 2019-07-05T11:56:15Z

src/jorphan/org/apache/jorphan/math/IStatCalculator.java

+
+    void addAll(IStatCalculator<T> calc);
+
+    T getMedian();


Document all interface methods

ubikloadpack · 2019-07-05T11:59:29Z

src/jorphan/org/apache/jorphan/math/HistogramStatCalculatorLong.java

+    @Override
+    public Map<Number, Number[]> getDistribution() {
+        Map<Number, Number[]> result = new HashMap<>();
+        histogram.percentiles(5).forEach(p -> {


vlsi · 2019-07-16T07:20:28Z

licenses/bin/HdrHistogram-2.1.8.txt

@@ -0,0 +1,27 @@
+BSD License
+
+Copyright (c) 2000-2015 www.hamcrest.org


Are you sure HdrHistogram is copyrighted by hamcrest.org?

vlsi · 2019-07-16T07:20:50Z

licenses/bin/LatencyUtils-2.0.3.txt

@@ -0,0 +1,27 @@
+BSD License
+
+Copyright (c) 2000-2015 www.hamcrest.org


Are you sure LatencyUtils is copyrighted by hamcrest?

vlsi · 2019-07-16T07:22:17Z

src/core/org/apache/jmeter/report/processor/StatisticsSummaryData.java

-
+    private LatencyStats latencyStats = LatencyStats.Builder.create().build();
+    Histogram intervalHistogram = latencyStats.getIntervalHistogram();
+    private Histogram histogram = new Histogram(intervalHistogram);


What's the precision of the resulting histogram?
What's the memory consumption?

This is work in progress. See question asked on dev list also

vlsi · 2019-07-16T07:27:24Z

src/jorphan/org/apache/jorphan/math/HistogramStatCalculatorLong.java

+        min = Math.min(val, min);
+        updateValueCount(val,1);
+    }
+    private void updateValueCount(Long actualValue, long sampleCount) {


This is sad. Is it really required?

It is a method for calculating distribution, so I try to record the value when it is added . Can you give another idea? Thanks

What's the purpose of valuesMap?
Histogram / LatencyStats should track the distribution.

In fact in the old code, the distribution value comes from this method :

public Map<Number, Number[]> getDistribution() { Map<Number, Number[]> items = new HashMap<>(); for (Entry<T, MutableLong> entry : valuesMap.entrySet()) { Number[] dis = new Number[2]; dis[0] = entry.getKey(); dis[1] = entry.getValue(); items.put(entry.getKey(), dis); } return items; }

I don't think the LatencyStats can track this distribution

vlsi · 2019-07-16T07:29:00Z

test/src/org/apache/jmeter/report/processor/StatisticsSummaryDataTest.java

+            histogram.recordValue(l*1000000L);
+        }
+        // test max/min
+        assertTrue(10 == statisticsSummaryData.getMax());


Please avoid writing tests like that.
It would be hard to analyze especially at Travis/Jenkins CI.

Please use something like assertEquals(Arrays.toString(values) + ".getMax()", 10, statisticsSummaryData.getMax())

General rule: assertTrue and assertFalse are almost always bad.

vlsi · 2019-07-16T07:30:11Z

test/src/org/apache/jorphan/math/TestHisStatCalculator.java

+import org.junit.Before;
+import org.junit.Test;
+
+public class TestHisStatCalculator {


Please avoid use his / her wording

his => Histogram
But better use TestHistogramStatCalculator

vlsi · 2019-07-16T07:30:29Z

test/src/org/apache/jorphan/math/TestHisStatCalculator.java

+            calc1.addValue(l);
+        }
+        calc.addAll(calc1);
+        assertTrue(23 == calc.getMax());


Please avoid assertTrue

vlsi · 2019-07-16T07:31:18Z

LICENSE

@@ -268,6 +268,8 @@ For details, please see the files under: licenses/bin
 * asm-7.0.jar (BSD)
 * dnsjava-2.1.8.jar (BSD)
 * hamcrest-core-1.3.jar (BSD)
+* LatencyUtils-2.0.3.jar (BSD)


BSD is an invalid (too broad) license reference. Please use SPDX license ID when possible: https://spdx.org/licenses/

https://github.com/LatencyUtils/LatencyUtils/blob/master/LICENSE
https://github.com/HdrHistogram/HdrHistogram/blob/master/LICENSE.txt
These two licenses are both in BSD, what can I do to use SPDX License ? Thanks

vlsi · 2019-07-16T07:33:17Z

build.properties

@@ -41,6 +41,7 @@
 # are contained in the JMeter binary release.

 maven2.repo                 = https://repo1.maven.org/maven2
+maven.repo                  = https://repo.maven.apache.org/maven2


AFAIK, LatencyUtils is available in Maven Central. Why do you add maven.repo?

vlsi

I think methods like addSentBytes, addBytes, getTotalBytes, getTotalSentBytes do not belong to a generic IStatCalculator interface, and the methods should be removed from the interface.

vlsi · 2019-07-16T07:36:03Z

src/jorphan/org/apache/jorphan/math/IStatCalculator.java

+     * 
+     * @param newValue number of newly sent bytes
+     */
+    void addSentBytes(long newValue);


This looks like a violation of "single responsibility principle".
How getStandardDeviation is related to addSentBytes?

I think methods like addSentBytes, addBytes, getTotalBytes, getTotalSentBytes do not belong to a generic IStatCalculator interface.

Is "getStandardDeviation" related to sentBytes? In my view the standardDeviation is for the response time of Http Request, and "sent bytes" is for the amount of data used for transmission

ubikloadpack · 2019-07-16T07:48:17Z

src/core/org/apache/jmeter/report/processor/StatisticsSummaryData.java

-        percentile3 = new PercentileAggregator(percentileIndex3);
-        mean = new MeanAggregator();
+    public final long getPercentile(double percent) {
+        return histogram.getValueAtPercentile(percent)/1000000;


This should not be needed as per https://stackoverflow.com/questions/56986503/time-unit-problem-about-latencyutils-recordlatency-method

The only answer in thie question doesn't give us enough information.
So don't we need to muliple and divide by 1,000,000 each time?

When the number of Thread is a big number like 1000 -10000, the maximum is bigger than percentile 99%. And IntcountHistogram can also give a correct result.

…e assertTrue to assertEquals

shaqianqian added 18 commits April 1, 2019 20:03

Merge pull request #1 from ubikloadpack/trunk

296049d

update the version of jmeter

import packqge

545f8a1

import packqge

4224ccc

tot

8ce628e

add HdrHistogram

d4f2ee3

init latencyUtils

ea64d31

use latencyUtil to replace Hd

cb6912e

replace StatisticsSummaryData by latencyUtil

ee9e805

add comment apache for new class

ba56cad

add comment apache for new class

210cc43

add comment apache for new class

a30ad92

remove System.out

cc229f0

add espace in the fin of file

32d7487

add espace in the fin of file

9664943

update the version of Histogram

1be0644

update the version of Histogram

9264161

remove final

63a1f2e

modify json

168e937

ubikloadpack requested changes Jul 5, 2019

View reviewed changes

shaqianqian added 7 commits July 8, 2019 17:50

modify latency

8b5302b

change method to private

3663437

modify 'gitDistribuion()'

ba10bc6

add junit test for statisummarydata

b8e4ef6

change method to private

f9f7dfb

modify the parameters of LatencyUtils

821c0de

modify Latencyutils

0e2f2a3

vlsi suggested changes Jul 16, 2019

View reviewed changes

ubikloadpack reviewed Jul 16, 2019

View reviewed changes

modify the licenses,remove uselesses methods of IStatCalculate, chang…

6de2f12

…e assertTrue to assertEquals

update license of LatencyUtils and HdrHistrogram

d1fbb52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Latency utils #50

Latency utils #50

shaqianqian commented Jun 25, 2019

ubikloadpack Jul 5, 2019

ubikloadpack Jul 5, 2019

ubikloadpack Jul 5, 2019

ubikloadpack Jul 5, 2019

ubikloadpack Jul 5, 2019

vlsi Jul 16, 2019

ubikloadpack Jul 16, 2019

vlsi Jul 16, 2019

ubikloadpack Jul 16, 2019

vlsi Jul 16, 2019

ubikloadpack Jul 16, 2019

vlsi Jul 16, 2019

shaqianqian Jul 16, 2019

vlsi Jul 16, 2019

shaqianqian Jul 16, 2019

vlsi Jul 16, 2019

vlsi Jul 16, 2019

ubikloadpack Jul 16, 2019

vlsi Jul 16, 2019

ubikloadpack Jul 16, 2019

vlsi Jul 16, 2019

ubikloadpack Jul 16, 2019

shaqianqian Jul 16, 2019 •

edited

Loading

vlsi Jul 16, 2019

ubikloadpack Jul 16, 2019

vlsi left a comment

vlsi Jul 16, 2019

ubikloadpack Jul 16, 2019

shaqianqian Jul 16, 2019

ubikloadpack Jul 16, 2019

shaqianqian Jul 16, 2019

shaqianqian Jul 16, 2019

		@@ -0,0 +1,27 @@
		BSD License

		Copyright (c) 2000-2015 www.hamcrest.org

Latency utils #50

Are you sure you want to change the base?

Latency utils #50

Conversation

shaqianqian commented Jun 25, 2019

Description

How Has This Been Tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaqianqian Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaqianqian Jul 16, 2019 •

edited

Loading