percentileOfSeries: return first of filtered datapoints in case only …

…one series have valid data percentileOfSeries should return first of filtered datapoints and not first of all datapoints in case all other series have NaNs for the values. Fixes #516
go-graphite · Aug 19, 2020 · cf06aa7 · cf06aa7
1 parent 4f73530
commit cf06aa7
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 1 deletion.
diff --git a/CHANGES.md b/CHANGES.md
@@ -29,6 +29,7 @@ CHANGELOG
  - [Fix] prometheus backend: carbonapi should send 'end' instead of 'stop' in queries (thx to Alexandre Vincent)
  - [Fix] change metrics resulting tags to match graphite-web in some cases (thx to @Felixoid)
  - [Fix] Prometheus backend: trust timestamps from the backend (Fixes #504, Fixes #514)
+ - [Fix] percentileOfSeries: return first of filtered datapoints in case only one series have valid data (Fixes #516)
  - [Code] merge aliasByNode and aliasByTags (thx to @Felixoid)
 
 **0.14.0**

diff --git a/cmd/mockbackend/e2etesting.go b/cmd/mockbackend/e2etesting.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"math"
 	"net/http"
 	"net/url"
 	"strconv"
@@ -74,6 +75,10 @@ func (d *Datapoint) UnmarshalJSON(data []byte) error {
 		return fmt.Errorf("failed to parse Timestamp: %v", err)
 	}
 
+	if valueStr == "null" || valueStr == "\"null\"" {
+		d.Value = math.NaN()
+		return nil
+	}
 	d.Value, err = strconv.ParseFloat(valueStr, 64)
 	if err != nil {
 		return fmt.Errorf("failed to parse Value: %v", err)
@@ -101,6 +106,10 @@ func (d *Datapoint) UnmarshalYAML(unmarshal func(interface{}) error) error {
 		return fmt.Errorf("failed to parse Timestamp: %v", err)
 	}
 
+	if valueStr == "null" || valueStr == "\"null\"" {
+		d.Value = math.NaN()
+		return nil
+	}
 	d.Value, err = strconv.ParseFloat(valueStr, 64)
 	if err != nil {
 		return fmt.Errorf("failed to parse Value: %v", err)

diff --git a/cmd/mockbackend/testcases/i516/i516.yaml b/cmd/mockbackend/testcases/i516/i516.yaml
@@ -0,0 +1,36 @@
+version: "v1"
+test:
+    apps:
+        - name: "carbonapi"
+          binary: "./carbonapi"
+          args:
+              - "-config"
+              - "./cmd/mockbackend/carbonapi_singlebackend.yaml"
+    queries:
+            - endpoint: "http://127.0.0.1:8081"
+              delay: 1
+              type: "GET"
+              URL: "/render?format=json&target=percentileOfSeries(carbon.api.*.cache_size, 95, false)"
+              expectedResponse:
+                  httpCode: 200
+                  contentType: "application/json"
+                  expectedResults:
+                          - metrics:
+                                  - target: "percentileOfSeries(carbon.api.*.cache_size, 95, false)"
+                                    datapoints: [[0.0, 1],[0.0, 2],[0.0, 3],[100500.0, 4],[100500.0, 5],[100500.0, 6]]
+listeners:
+        - address: ":9070"
+          expressions:
+                     "carbon.api.*.cache_size":
+                         pathExpression: "carbon.api.*.cache_size"
+                         data:
+                             - metricName: "carbon.api.host001.cache_size"
+                               values: [.NaN, .NaN, .NaN, .NaN, .NaN, .NaN]
+                             - metricName: "carbon.api.host002.cache_size"
+                               values: [.NaN, .NaN, .NaN, .NaN, .NaN, .NaN]
+                             - metricName: "carbon.api.host003.cache_size"
+                               values: [0.0, 0.0, 0.0, 100500.0, 100500.0, 100500.0]
+                             - metricName: "carbon.api.host004.cache_size"
+                               values: [.NaN, .NaN, .NaN, .NaN, .NaN, .NaN]
+                             - metricName: "carbon.api.host005.cache_size"
+                               values: [.NaN, .NaN, .NaN, .NaN, .NaN, .NaN]
diff --git a/e2e_test.sh b/e2e_test.sh
@@ -2,6 +2,10 @@
 
 TESTS=$(ls cmd/mockbackend/testcases/)
 
+if [[ ! -z "${1}" ]]; then
+	TESTS="${1}"
+fi
+
 FAILED_TESTS=""
 for t in ${TESTS}; do
 	if [[ "${TRAVIS}" == "true" ]]; then

diff --git a/expr/consolidations/consolidations.go b/expr/consolidations/consolidations.go
@@ -79,7 +79,7 @@ func Percentile(data []float64, percent float64, interpolate bool) float64 {
 		return math.NaN()
 	}
 	if len(dataFiltered) == 1 {
-		return data[0]
+		return dataFiltered[0]
 	}
 
 	k := (float64(len(dataFiltered)-1) * percent) / 100

diff --git a/expr/functions/percentileOfSeries/function_test.go b/expr/functions/percentileOfSeries/function_test.go
@@ -57,6 +57,19 @@ func TestPercentileOfSeriesSeries(t *testing.T) {
 			},
 			[]*types.MetricData{types.MakeMetricData("percentileOfSeries(metric1.foo.*.*,50,interpolate=true)", []float64{6.5, 7.5, 8.5, 9.5, 11, math.NaN()}, 1, now32)},
 		},
+		{
+			`percentileOfSeries(metric1.foo.*.*,95,false)`,
+			map[parser.MetricRequest][]*types.MetricData{
+				{"metric1.foo.*.*", 0, 1}: {
+					types.MakeMetricData("metric1.foo.bar1.qux", []float64{math.NaN(), math.NaN(), math.NaN(), math.NaN(), math.NaN(), math.NaN()}, 1, now32),
+					types.MakeMetricData("metric1.foo.bar2.qux", []float64{math.NaN(), math.NaN(), math.NaN(), math.NaN(), math.NaN(), 0}, 1, now32),
+					types.MakeMetricData("metric1.foo.bar3.qux", []float64{0, 0, 0, 100500, 100501, 1005002}, 1, now32),
+					types.MakeMetricData("metric1.foo.bar4.qux", []float64{math.NaN(), math.NaN(), math.NaN(), math.NaN(), math.NaN(), 0}, 1, now32),
+					types.MakeMetricData("metric1.foo.bar5.qux", []float64{math.NaN(), math.NaN(), math.NaN(), math.NaN(), math.NaN(), 0}, 1, now32),
+				},
+			},
+			[]*types.MetricData{types.MakeMetricData("percentileOfSeries(metric1.foo.*.*,95,false)", []float64{0, 0, 0, 100500, 100501, 1005002}, 1, now32)},
+		},
 	}
 
 	for _, tt := range tests {