[query] Workaround for non monotonic histogram buckets (#2388)

scripts/comparator/compare.go

@@ -53,7 +53,7 @@ func main() {
 
 		pQueryFile    = flag.String("input", "", "the query file")
 		pPromAddress  = flag.String("promAdress", "0.0.0.0:9090", "prom address")
-		pQueryAddress = flag.String("queryAddress", "0.0.0.0:7201", "query address")
+		pQueryAddress = flag.String("queryAddress", "0.0.0.0:7201/m3query", "M3 query address")
 
 		pComparatorAddress = flag.String("comparator", "", "comparator address")
 		pRegressionDir     = flag.String("regressionDir", "", "optional directory for regression tests")

scripts/comparator/regression_data/non_monotonic_buckets.json

@@ -0,0 +1,93 @@
+{
+    "name": "Prometheus compatible workaround for nonmonotonic histogram buckets",
+    "query": "histogram_quantile(0.99, nonmonotonic_bucket)",
+    "startMillis": 1584825526,
+    "endMillis": 1584828792,
+    "step": 2,
+    "data": [
+        {
+            "start": "2020-03-21T21:23:32Z",
+            "end": "2020-03-21T22:08:20Z",
+            "tags": [
+                ["__name__", "nonmonotonic_bucket"],
+                ["le", "0.1"]
+            ],
+            "datapoints": [
+                {
+                    "val": "1",
+                    "ts": "2020-03-21T21:23:34.269Z"
+                }
+            ]
+        },
+        {
+            "start": "2020-03-21T21:23:32Z",
+            "end": "2020-03-21T22:08:20Z",
+            "tags": [
+                ["__name__", "nonmonotonic_bucket"],
+                ["le", "1"]
+            ],
+            "datapoints": [
+                {
+                    "val": "9",
+                    "ts": "2020-03-21T21:23:34.269Z"
+                }
+            ]
+        },
+        {
+            "start": "2020-03-21T21:23:32Z",
+            "end": "2020-03-21T22:08:20Z",
+            "tags": [
+                ["__name__", "nonmonotonic_bucket"],
+                ["le", "10"]
+            ],
+            "datapoints": [
+                {
+                    "val": "8",
+                    "ts": "2020-03-21T21:23:34.269Z"
+                }
+            ]
+        },
+        {
+            "start": "2020-03-21T21:23:32Z",
+            "end": "2020-03-21T22:08:20Z",
+            "tags": [
+                ["__name__", "nonmonotonic_bucket"],
+                ["le", "100"]
+            ],
+            "datapoints": [
+                {
+                    "val": "8",
+                    "ts": "2020-03-21T21:23:34.269Z"
+                }
+            ]
+        },
+        {
+            "start": "2020-03-21T21:23:32Z",
+            "end": "2020-03-21T22:08:20Z",
+            "tags": [
+                ["__name__", "nonmonotonic_bucket"],
+                ["le", "1000"]
+            ],
+            "datapoints": [
+                {
+                    "val": "9",
+                    "ts": "2020-03-21T21:23:34.269Z"
+                }
+            ]
+        },
+        {
+            "start": "2020-03-21T21:23:32Z",
+            "end": "2020-03-21T22:08:20Z",
+            "tags": [
+                ["__name__", "nonmonotonic_bucket"],
+                ["le", "+Inf"]
+            ],
+            "datapoints": [
+                {
+                    "val": "9",
+                    "ts": "2020-03-21T21:23:34.269Z"
+                }
+            ]
+        }
+    ]
+    }

src/query/functions/linear/histogram_quantile.go

@@ -168,23 +168,23 @@ func gatherSeriesToBuckets(metas []block.SeriesMeta) validSeriesBuckets {
 
 		excludeTags := [][]byte{tags.Opts.MetricName(), tags.Opts.BucketName()}
 		tagsWithoutKeys := tags.TagsWithoutKeys(excludeTags)
-		id := tagsWithoutKeys.ID()
+		id := string(tagsWithoutKeys.ID())
 		newBucket := indexedBucket{
 			upperBound: bound,
 			idx:        i,
 		}
 
-		if buckets, found := bucketsForID[string(id)]; !found {
+		if buckets, found := bucketsForID[id]; !found {
 			// add a single indexed bucket for this ID with the current index only.
 			newBuckets := make([]indexedBucket, 0, initIndexBucketLength)
 			newBuckets = append(newBuckets, newBucket)
-			bucketsForID[string(id)] = indexedBuckets{
+			bucketsForID[id] = indexedBuckets{
 				buckets: newBuckets,
 				tags:    tagsWithoutKeys,
 			}
 		} else {
 			buckets.buckets = append(buckets.buckets, newBucket)
-			bucketsForID[string(id)] = buckets
+			bucketsForID[id] = buckets
 		}
 	}
 
@@ -316,6 +316,20 @@ func setupBuilder(
 	return builder, nil
 }
 
+// Enforce monotonicity for binary search to work.
+// See https://github.com/prometheus/prometheus/commit/896f951e6846ce252d9d19fd4707a4110ceda5ee
+func ensureMonotonic(bucketValues []bucketValue) {
+	max := math.Inf(-1)
+	for i := range bucketValues {
+		switch {
+		case bucketValues[i].value >= max:
+			max = bucketValues[i].value
+		case bucketValues[i].value < max:
+			bucketValues[i].value = max
+		}
+	}
+}
+
 func processValidQuantile(
 	queryCtx *models.QueryContext,
 	q float64,
@@ -352,6 +366,8 @@ func processValidQuantile(
 				}
 			}
 
+			ensureMonotonic(bucketValues)
+
 			aggregatedValues = append(aggregatedValues, bucketQuantile(q, bucketValues))
 		}
 

src/query/functions/linear/histogram_quantile_test.go

@@ -146,6 +146,72 @@ func TestSanitizeBuckets(t *testing.T) {
 	assert.Equal(t, expected, sanitizeBuckets(bucketed))
 }
 
+func TestEnsureMonotonic(t *testing.T) {
+	tests := []struct {
+		name string
+		data []bucketValue
+		want []bucketValue
+	}{
+		{
+			"empty",
+			[]bucketValue{},
+			[]bucketValue{},
+		},
+		{
+			"one",
+			[]bucketValue{{upperBound: 1, value: 5}},
+			[]bucketValue{{upperBound: 1, value: 5}},
+		},
+		{
+			"two monotonic",
+			[]bucketValue{{upperBound: 1, value: 5}, {upperBound: 2, value: 6}},
+			[]bucketValue{{upperBound: 1, value: 5}, {upperBound: 2, value: 6}},
+		},
+		{
+			"two nonmonotonic",
+			[]bucketValue{{upperBound: 1, value: 5}, {upperBound: 2, value: 4}},
+			[]bucketValue{{upperBound: 1, value: 5}, {upperBound: 2, value: 5}},
+		},
+		{
+			"three monotonic",
+			[]bucketValue{{upperBound: 1, value: 5}, {upperBound: 2, value: 6}, {upperBound: 3, value: 7}},
+			[]bucketValue{{upperBound: 1, value: 5}, {upperBound: 2, value: 6}, {upperBound: 3, value: 7}},
+		},
+		{
+			"three nonmonotonic",
+			[]bucketValue{{upperBound: 1, value: 5}, {upperBound: 2, value: 3}, {upperBound: 3, value: 4}},
+			[]bucketValue{{upperBound: 1, value: 5}, {upperBound: 2, value: 5}, {upperBound: 3, value: 5}},
+		},
+		{
+			"four nonmonotonic",
+			[]bucketValue{{upperBound: 1, value: 5}, {upperBound: 2, value: 3}, {upperBound: 3, value: 6}, {upperBound: 4, value: 3}},
+			[]bucketValue{{upperBound: 1, value: 5}, {upperBound: 2, value: 5}, {upperBound: 3, value: 6}, {upperBound: 4, value: 6}},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ensureMonotonic(tt.data)
+			assert.Equal(t, tt.want, tt.data)
+		})
+	}
+}
+
+func TestEnsureMonotonicPreserveNaN(t *testing.T) {
+	data := []bucketValue{
+		{upperBound: 1, value: 5},
+		{upperBound: 2, value: 3},
+		{upperBound: 3, value: math.NaN()},
+		{upperBound: 4, value: 0},
+	}
+	ensureMonotonic(data)
+	assert.Equal(t, data[0], bucketValue{upperBound: 1, value: 5})
+	assert.Equal(t, data[1], bucketValue{upperBound: 2, value: 5})
+	assert.Equal(t, data[2].upperBound, float64(3))
+	assert.True(t, math.IsNaN(data[2].value))
+	assert.Equal(t, data[3], bucketValue{upperBound: 4, value: 5})
+}
+
 func TestBucketQuantile(t *testing.T) {
 	// single bucket returns nan
 	actual := bucketQuantile(0.5, []bucketValue{{upperBound: 1, value: 1}})