From 2a2679eefb8c633ef3039fe5354ba695b193cc3e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E5=90=B4=E6=99=9F=20Wu=20Sheng?= <wu.sheng@foxmail.com>
Date: Mon, 2 Sep 2019 10:14:33 +0800
Subject: [PATCH] Fix Pxx calculate bug. (#3391)

---
 .../core/analysis/metrics/PxxMetrics.java     |  8 +++-
 .../core/analysis/metrics/PxxMetricsTest.java | 44 ++++++++++++++++++-
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/PxxMetrics.java b/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/PxxMetrics.java
index 94141de63..a9026801c 100644
--- a/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/PxxMetrics.java
+++ b/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/PxxMetrics.java
@@ -18,6 +18,7 @@
 
 package org.apache.skywalking.oap.server.core.analysis.metrics;
 
+import java.util.Comparator;
 import lombok.*;
 import org.apache.skywalking.oap.server.core.analysis.metrics.annotation.*;
 import org.apache.skywalking.oap.server.core.query.sql.Function;
@@ -81,7 +82,12 @@ public abstract class PxxMetrics extends GroupMetrics implements IntValueHolder
             int roof = Math.round(total * percentileRank * 1.0f / 100);
 
             int count = 0;
-            for (IntKeyLongValue element : detailGroup.values()) {
+            IntKeyLongValue[] sortedData = detailGroup.values().stream().sorted(new Comparator<IntKeyLongValue>() {
+                @Override public int compare(IntKeyLongValue o1, IntKeyLongValue o2) {
+                    return o1.getKey() - o2.getKey();
+                }
+            }).toArray(IntKeyLongValue[]::new);
+            for (IntKeyLongValue element : sortedData) {
                 count += element.getValue();
                 if (count >= roof) {
                     value = element.getKey() * precision;
diff --git a/oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/analysis/metrics/PxxMetricsTest.java b/oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/analysis/metrics/PxxMetricsTest.java
index 8fb44bb04..2324660e6 100644
--- a/oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/analysis/metrics/PxxMetricsTest.java
+++ b/oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/analysis/metrics/PxxMetricsTest.java
@@ -19,7 +19,8 @@
 package org.apache.skywalking.oap.server.core.analysis.metrics;
 
 import org.apache.skywalking.oap.server.core.remote.grpc.proto.RemoteData;
-import org.junit.*;
+import org.junit.Assert;
+import org.junit.Test;
 
 /**
  * @author wusheng
@@ -126,4 +127,45 @@ public class PxxMetricsTest {
             return 0;
         }
     }
+
+    @Test
+    public void testAccurate() {
+        IntKeyLongValueHashMap map = new IntKeyLongValueHashMap();
+        map.toObject("0,109|128,3|130,1|131,1|132,2|5,16|6,23|10,1|12,1|13,25|14,10|15,2|17,1|146,2|18,1|19,16|20,9|21,4|22,1|23,2|152,1|25,4|26,4|27,3|28,1|31,1|32,2|34,1|44,1|318,1|319,7|320,2|321,1|323,1|324,1|325,2|326,1|327,3|328,1|330,2|205,27|206,14|208,1|337,1|219,15|220,2|221,2|222,1|224,1|352,1|225,1|226,3|227,1|229,1|232,2|105,16|233,1|106,13|108,1|113,20|114,4|115,3|116,2|118,6|119,12|120,4|121,4|122,6|250,1|124,4|125,1|126,4|127,2");
+
+        PxxMetricsMocker metrics50Mocker = new PxxMetricsMocker(50);
+        metrics50Mocker.setDetailGroup(map);
+        metrics50Mocker.setPrecision(10);
+        metrics50Mocker.calculate();
+        int p50 = metrics50Mocker.getValue();
+
+        PxxMetricsMocker metrics75Mocker = new PxxMetricsMocker(75);
+        metrics75Mocker.setDetailGroup(map);
+        metrics75Mocker.setPrecision(10);
+        metrics75Mocker.calculate();
+        int p75 = metrics75Mocker.getValue();
+
+        PxxMetricsMocker metrics90Mocker = new PxxMetricsMocker(90);
+        metrics90Mocker.setDetailGroup(map);
+        metrics90Mocker.setPrecision(10);
+        metrics90Mocker.calculate();
+        int p90 = metrics90Mocker.getValue();
+
+        PxxMetricsMocker metrics95Mocker = new PxxMetricsMocker(95);
+        metrics95Mocker.setDetailGroup(map);
+        metrics95Mocker.setPrecision(10);
+        metrics95Mocker.calculate();
+        int p95 = metrics95Mocker.getValue();
+
+        PxxMetricsMocker metrics99Mocker = new PxxMetricsMocker(99);
+        metrics99Mocker.setDetailGroup(map);
+        metrics99Mocker.setPrecision(10);
+        metrics99Mocker.calculate();
+        int p99 = metrics99Mocker.getValue();
+
+        Assert.assertTrue(p50 < p75);
+        Assert.assertTrue(p75 < p90);
+        Assert.assertTrue(p90 < p95);
+        Assert.assertTrue(p95 < p99);
+    }
 }