Update to latest mimir-prometheus (#9508)

* Update to latest mimir-prometheus * Add changelog entries * Address PR feedback: improve changelog entries Co-authored-by: George Krajcsovits <[email protected]> * Fix breaking change * Bring in grafana/mimir-prometheus#705 too * Adjust behaviour to match prometheus/prometheus#14821 * Re-enable tests now that we have prometheus/prometheus#14910 * Add more changelog entries --------- Co-authored-by: George Krajcsovits <[email protected]>
grafana · Oct 4, 2024 · 9a03bb5 · 9a03bb5
1 parent 42dd5e2
commit 9a03bb5
Show file tree

Hide file tree

Showing 28 changed files with 323 additions and 181 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -30,6 +30,10 @@
 * [FEATURE] Ingester: Experimental support for ingesting out-of-order native histograms. This is disabled by default and can be enabled by setting `-ingester.ooo-native-histograms-ingestion-enabled` to `true`. #7175
 * [ENHANCEMENT] Ruler: Support `exclude_alerts` parameter in `<prometheus-http-prefix>/api/v1/rules` endpoint. #9300
 * [ENHANCEMENT] Distributor: add a metric to track tenants who are sending newlines in their label values called `cortex_distributor_label_values_with_newlines_total`. #9400
+* [ENHANCEMENT] Ingester: improve performance of reading the WAL. #9508
+* [BUGFIX] Fix issue where functions such as `rate()` over native histograms could return incorrect values if a float stale marker was present in the selected range. #9508
+* [BUGFIX] Fix issue where negation of native histograms (eg. `-some_native_histogram_series`) did nothing. #9508
+* [BUGFIX] Fix issue where `metric might not be a counter, name does not end in _total/_sum/_count/_bucket` annotation would be emitted even if `rate` or `increase` did not have enough samples to compute a result. #9508
 
 ### Mixin
 

diff --git a/go.mod b/go.mod
@@ -282,7 +282,7 @@ require (
 )
 
 // Using a fork of Prometheus with Mimir-specific changes.
-replace github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20240925112120-6046bf43c9b2
+replace github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20241003072550-24c9a629c6d7
 
 // client_golang v1.20.3 has some data races in histogram exemplars.
 // Stick to v1.19.1 until they are fixed.

diff --git a/go.sum b/go.sum
@@ -1262,8 +1262,8 @@ github.com/grafana/gomemcache v0.0.0-20240229205252-cd6a66d6fb56 h1:X8IKQ0wu40wp
 github.com/grafana/gomemcache v0.0.0-20240229205252-cd6a66d6fb56/go.mod h1:PGk3RjYHpxMM8HFPhKKo+vve3DdlPUELZLSDEFehPuU=
 github.com/grafana/memberlist v0.3.1-0.20220714140823-09ffed8adbbe h1:yIXAAbLswn7VNWBIvM71O2QsgfgW9fRXZNR0DXe6pDU=
 github.com/grafana/memberlist v0.3.1-0.20220714140823-09ffed8adbbe/go.mod h1:MS2lj3INKhZjWNqd3N0m3J+Jxf3DAOnAH9VT3Sh9MUE=
-github.com/grafana/mimir-prometheus v0.0.0-20240925112120-6046bf43c9b2 h1:440vf/oyTq3xubH0iMHveDfEM8p+t/lzLMHiJNQRfY0=
-github.com/grafana/mimir-prometheus v0.0.0-20240925112120-6046bf43c9b2/go.mod h1:oyDm7JaLUh+QGuGkC7iXC8IyTUq5rlh1ba2CRm9DlVg=
+github.com/grafana/mimir-prometheus v0.0.0-20241003072550-24c9a629c6d7 h1:BXhk8YD7F0FUokP3LD2Oa+AtF1W38qLfqgrfpomVNj0=
+github.com/grafana/mimir-prometheus v0.0.0-20241003072550-24c9a629c6d7/go.mod h1:oyDm7JaLUh+QGuGkC7iXC8IyTUq5rlh1ba2CRm9DlVg=
 github.com/grafana/opentracing-contrib-go-stdlib v0.0.0-20230509071955-f410e79da956 h1:em1oddjXL8c1tL0iFdtVtPloq2hRPen2MJQKoAWpxu0=
 github.com/grafana/opentracing-contrib-go-stdlib v0.0.0-20230509071955-f410e79da956/go.mod h1:qtI1ogk+2JhVPIXVc6q+NHziSmy2W5GbdQZFUHADCBU=
 github.com/grafana/prometheus-alertmanager v0.25.1-0.20240924175849-b8b7c2c74eb6 h1:nT8QXdJo6wHMBcF0xEoXxEWkoUZOyzV/jyi/u9l7YEk=

diff --git a/integration/compactor_test.go b/integration/compactor_test.go
@@ -232,6 +232,17 @@ func (s sample) Type() chunkenc.ValueType {
 	}
 }
 
+func (s sample) Copy() chunks.Sample {
+	c := sample{t: s.t, v: s.v}
+	if s.h != nil {
+		c.h = s.h.Copy()
+	}
+	if s.fh != nil {
+		c.fh = s.fh.Copy()
+	}
+	return c
+}
+
 func must[T any](v T, err error) T {
 	if err != nil {
 		panic(err)

diff --git a/pkg/compactor/compactor_test.go b/pkg/compactor/compactor_test.go
@@ -2380,6 +2380,17 @@ func (s sample) Type() chunkenc.ValueType {
 	}
 }
 
+func (s sample) Copy() chunks.Sample {
+	c := sample{t: s.t, v: s.v}
+	if s.h != nil {
+		c.h = s.h.Copy()
+	}
+	if s.fh != nil {
+		c.fh = s.fh.Copy()
+	}
+	return c
+}
+
 type bucketWithMockedAttributes struct {
 	objstore.Bucket
 

diff --git a/pkg/storegateway/gateway_test.go b/pkg/storegateway/gateway_test.go
@@ -36,6 +36,7 @@ import (
 	"github.com/prometheus/prometheus/model/labels"
 	"github.com/prometheus/prometheus/tsdb"
 	"github.com/prometheus/prometheus/tsdb/chunkenc"
+	"github.com/prometheus/prometheus/tsdb/chunks"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -1670,6 +1671,17 @@ func (s sample) Type() chunkenc.ValueType {
 	}
 }
 
+func (s sample) Copy() chunks.Sample {
+	c := sample{t: s.t, v: s.v}
+	if s.h != nil {
+		c.h = s.h.Copy()
+	}
+	if s.fh != nil {
+		c.fh = s.fh.Copy()
+	}
+	return c
+}
+
 func defaultLimitsConfig() validation.Limits {
 	limits := validation.Limits{}
 	flagext.DefaultValues(&limits)

diff --git a/pkg/streamingpromql/benchmarks/comparison_test.go b/pkg/streamingpromql/benchmarks/comparison_test.go
@@ -67,7 +67,7 @@ func BenchmarkQuery(b *testing.B) {
 				prometheusResult, prometheusClose := c.Run(ctx, b, start, end, interval, prometheusEngine, q)
 				mimirResult, mimirClose := c.Run(ctx, b, start, end, interval, mimirEngine, q)
 
-				testutils.RequireEqualResults(b, c.Expr, prometheusResult, mimirResult, true)
+				testutils.RequireEqualResults(b, c.Expr, prometheusResult, mimirResult)
 
 				prometheusClose()
 				mimirClose()
@@ -108,7 +108,7 @@ func TestBothEnginesReturnSameResultsForBenchmarkQueries(t *testing.T) {
 			prometheusResult, prometheusClose := c.Run(ctx, t, start, end, interval, prometheusEngine, q)
 			mimirResult, mimirClose := c.Run(ctx, t, start, end, interval, mimirEngine, q)
 
-			testutils.RequireEqualResults(t, c.Expr, prometheusResult, mimirResult, true)
+			testutils.RequireEqualResults(t, c.Expr, prometheusResult, mimirResult)
 
 			prometheusClose()
 			mimirClose()

diff --git a/pkg/streamingpromql/engine_test.go b/pkg/streamingpromql/engine_test.go
@@ -1707,8 +1707,6 @@ func TestAnnotations(t *testing.T) {
 			expr:                       fmt.Sprintf("%s(series[45s])", function),
 			expectedWarningAnnotations: []string{},
 			expectedInfoAnnotations:    []string{},
-			// This can be removed once https://github.com/prometheus/prometheus/pull/14910 is vendored.
-			skipComparisonWithPrometheusReason: "Prometheus only considers the type of the last point in the vector selector rather than the output value",
 		}
 		testCases[fmt.Sprintf("%s() over one point in range", function)] = testCase{
 			data: `
@@ -1717,8 +1715,6 @@ func TestAnnotations(t *testing.T) {
 			expr:                       fmt.Sprintf("%s(series[1m])", function),
 			expectedWarningAnnotations: []string{},
 			expectedInfoAnnotations:    []string{},
-			// This can be removed once https://github.com/prometheus/prometheus/pull/14910 is vendored.
-			skipComparisonWithPrometheusReason: "Prometheus only considers the type of the last point in the vector selector rather than the output value",
 		}
 	}
 
@@ -1866,9 +1862,7 @@ func runMixedMetricsTests(t *testing.T, expressions []string, pointsPerSeries in
 				defer q.Close()
 				mimirResults := q.Exec(context.Background())
 
-				// We currently omit checking the annotations due to a difference between the engines.
-				// This can be re-enabled once https://github.com/prometheus/prometheus/pull/14910 is vendored.
-				testutils.RequireEqualResults(t, expr, expectedResults, mimirResults, false)
+				testutils.RequireEqualResults(t, expr, expectedResults, mimirResults)
 			})
 		}
 	}

diff --git a/pkg/streamingpromql/functions/math.go b/pkg/streamingpromql/functions/math.go
@@ -57,8 +57,14 @@ var UnaryNegation InstantVectorSeriesFunction = func(seriesData types.InstantVec
 		seriesData.Floats[i].F = -seriesData.Floats[i].F
 	}
 
-	// Prometheus' engine currently leaves histograms as-is for unary negation, so we do the same.
-	// See https://github.com/prometheus/prometheus/pull/14821 for more discussion of this.
+	for i := range seriesData.Histograms {
+		if i > 0 && seriesData.Histograms[i].H == seriesData.Histograms[i-1].H {
+			// Previous point shares the same histogram instance, which we've already negated, so don't negate it again.
+			continue
+		}
+
+		seriesData.Histograms[i].H.Mul(-1) // Mul modifies the histogram in-place, so we don't need to do anything with the result here.
+	}
 
 	return seriesData, nil
 }
diff --git a/pkg/streamingpromql/testdata/ours/functions.test b/pkg/streamingpromql/testdata/ours/functions.test
@@ -218,11 +218,13 @@ load 6m
   metric{series="float"} 1 2 3 4 -5 0 NaN -NaN
   metric{series="histogram"} {{schema:3 sum:4 count:23 buckets:[1 2 4] n_buckets:[3 5 8]}} {{schema:3 sum:14 count:27 buckets:[1 2 6] n_buckets:[3 5 10]}}
 
-# Unary negation of a native histogram currently does nothing in Prometheus' engine, which seems incorrect, but we mirror its behaviour for consistency.
-# See https://github.com/prometheus/prometheus/pull/14821 for more discussion.
 eval range from 0 to 42m step 6m -metric
   {series="float"} -1 -2 -3 -4 5 0 -NaN NaN
-  {series="histogram"} {{schema:3 sum:4 count:23 buckets:[1 2 4] n_buckets:[3 5 8]}} {{schema:3 sum:14 count:27 buckets:[1 2 6] n_buckets:[3 5 10]}}
+  {series="histogram"} {{schema:3 sum:-4 count:-23 buckets:[-1 -2 -4] n_buckets:[-3 -5 -8]}} {{schema:3 sum:-14 count:-27 buckets:[-1 -2 -6] n_buckets:[-3 -5 -10]}}
+
+# Ensure unary negation of histograms behaves correctly when lookback is involved.
+eval range from 0 to 1m step 1m -metric{series="histogram"}
+  {series="histogram"} {{schema:3 sum:-4 count:-23 buckets:[-1 -2 -4] n_buckets:[-3 -5 -8]}} {{schema:3 sum:-4 count:-23 buckets:[-1 -2 -4] n_buckets:[-3 -5 -8]}}
 
 clear
 

diff --git a/pkg/streamingpromql/testdata/upstream/native_histograms.test b/pkg/streamingpromql/testdata/upstream/native_histograms.test
@@ -883,27 +883,39 @@ load 10m
 
 # Apply multiplication and division operator to histogram.
 load 10m
-    histogram_mul_div {{schema:0 count:21 sum:33 z_bucket:3 z_bucket_w:0.001 buckets:[3 3 3] n_buckets:[3 3 3]}}x1
+    histogram_mul_div {{schema:0 count:30 sum:33 z_bucket:3 z_bucket_w:0.001 buckets:[3 3 3] n_buckets:[6 6 6]}}x1
     float_series_3 3+0x1
     float_series_0 0+0x1
 
 eval instant at 10m histogram_mul_div*3
-    {} {{schema:0 count:63 sum:99 z_bucket:9 z_bucket_w:0.001 buckets:[9 9 9] n_buckets:[9 9 9]}}
+    {} {{schema:0 count:90 sum:99 z_bucket:9 z_bucket_w:0.001 buckets:[9 9 9] n_buckets:[18 18 18]}}
+
+eval instant at 10m histogram_mul_div*-1
+    {} {{schema:0 count:-30 sum:-33 z_bucket:-3 z_bucket_w:0.001 buckets:[-3 -3 -3] n_buckets:[-6 -6 -6]}}
+
+eval instant at 10m -histogram_mul_div
+    {} {{schema:0 count:-30 sum:-33 z_bucket:-3 z_bucket_w:0.001 buckets:[-3 -3 -3] n_buckets:[-6 -6 -6]}}
+
+eval instant at 10m histogram_mul_div*-3
+    {} {{schema:0 count:-90 sum:-99 z_bucket:-9 z_bucket_w:0.001 buckets:[-9 -9 -9] n_buckets:[-18 -18 -18]}}
 
 eval instant at 10m 3*histogram_mul_div
-    {} {{schema:0 count:63 sum:99 z_bucket:9 z_bucket_w:0.001 buckets:[9 9 9] n_buckets:[9 9 9]}}
+    {} {{schema:0 count:90 sum:99 z_bucket:9 z_bucket_w:0.001 buckets:[9 9 9] n_buckets:[18 18 18]}}
 
 eval instant at 10m histogram_mul_div*float_series_3
-    {} {{schema:0 count:63 sum:99 z_bucket:9 z_bucket_w:0.001 buckets:[9 9 9] n_buckets:[9 9 9]}}
+    {} {{schema:0 count:90 sum:99 z_bucket:9 z_bucket_w:0.001 buckets:[9 9 9] n_buckets:[18 18 18]}}
 
 eval instant at 10m float_series_3*histogram_mul_div
-    {} {{schema:0 count:63 sum:99 z_bucket:9 z_bucket_w:0.001 buckets:[9 9 9] n_buckets:[9 9 9]}}
+    {} {{schema:0 count:90 sum:99 z_bucket:9 z_bucket_w:0.001 buckets:[9 9 9] n_buckets:[18 18 18]}}
 
 eval instant at 10m histogram_mul_div/3
-    {} {{schema:0 count:7 sum:11 z_bucket:1 z_bucket_w:0.001 buckets:[1 1 1] n_buckets:[1 1 1]}}
+    {} {{schema:0 count:10 sum:11 z_bucket:1 z_bucket_w:0.001 buckets:[1 1 1] n_buckets:[2 2 2]}}
+
+eval instant at 10m histogram_mul_div/-3
+    {} {{schema:0 count:-10 sum:-11 z_bucket:-1 z_bucket_w:0.001 buckets:[-1 -1 -1] n_buckets:[-2 -2 -2]}}
 
 eval instant at 10m histogram_mul_div/float_series_3
-    {} {{schema:0 count:7 sum:11 z_bucket:1 z_bucket_w:0.001 buckets:[1 1 1] n_buckets:[1 1 1]}}
+    {} {{schema:0 count:10 sum:11 z_bucket:1 z_bucket_w:0.001 buckets:[1 1 1] n_buckets:[2 2 2]}}
 
 eval instant at 10m histogram_mul_div*0
     {} {{schema:0 count:0 sum:0 z_bucket:0 z_bucket_w:0.001 buckets:[0 0 0] n_buckets:[0 0 0]}}

diff --git a/pkg/streamingpromql/testutils/utils.go b/pkg/streamingpromql/testutils/utils.go
@@ -15,16 +15,14 @@ import (
 
 // Why do we do this rather than require.Equal(t, expected, actual)?
 // It's possible that floating point values are slightly different due to imprecision, but require.Equal doesn't allow us to set an allowable difference.
-func RequireEqualResults(t testing.TB, expr string, expected, actual *promql.Result, checkAnnotations bool) {
+func RequireEqualResults(t testing.TB, expr string, expected, actual *promql.Result) {
 	require.Equal(t, expected.Err, actual.Err)
 	require.Equal(t, expected.Value.Type(), actual.Value.Type())
 
-	if checkAnnotations {
-		expectedWarnings, expectedInfos := expected.Warnings.AsStrings(expr, 0, 0)
-		actualWarnings, actualInfos := actual.Warnings.AsStrings(expr, 0, 0)
-		require.ElementsMatch(t, expectedWarnings, actualWarnings)
-		require.ElementsMatch(t, expectedInfos, actualInfos)
-	}
+	expectedWarnings, expectedInfos := expected.Warnings.AsStrings(expr, 0, 0)
+	actualWarnings, actualInfos := actual.Warnings.AsStrings(expr, 0, 0)
+	require.ElementsMatch(t, expectedWarnings, actualWarnings)
+	require.ElementsMatch(t, expectedInfos, actualInfos)
 
 	switch expected.Value.Type() {
 	case parser.ValueTypeVector:

diff --git a/tools/tsdb-chunks/main_test.go b/tools/tsdb-chunks/main_test.go
@@ -86,6 +86,17 @@ func (s sample) Type() chunkenc.ValueType {
 	}
 }
 
+func (s sample) Copy() chunks.Sample {
+	c := sample{t: s.t, v: s.v}
+	if s.h != nil {
+		c.h = s.h.Copy()
+	}
+	if s.fh != nil {
+		c.fh = s.fh.Copy()
+	}
+	return c
+}
+
 func must[T any](v T, err error) T {
 	if err != nil {
 		panic(err)

diff --git a/tools/tsdb-index-health/main_test.go b/tools/tsdb-index-health/main_test.go
@@ -86,6 +86,17 @@ func (s sample) Type() chunkenc.ValueType {
 	}
 }
 
+func (s sample) Copy() chunks.Sample {
+	c := sample{t: s.t, v: s.v}
+	if s.h != nil {
+		c.h = s.h.Copy()
+	}
+	if s.fh != nil {
+		c.fh = s.fh.Copy()
+	}
+	return c
+}
+
 func must[T any](v T, err error) T {
 	if err != nil {
 		panic(err)

diff --git a/tools/tsdb-print-chunk/main_test.go b/tools/tsdb-print-chunk/main_test.go
@@ -86,6 +86,17 @@ func (s sample) Type() chunkenc.ValueType {
 	}
 }
 
+func (s sample) Copy() chunks.Sample {
+	c := sample{t: s.t, v: s.v}
+	if s.h != nil {
+		c.h = s.h.Copy()
+	}
+	if s.fh != nil {
+		c.fh = s.fh.Copy()
+	}
+	return c
+}
+
 func must[T any](v T, err error) T {
 	if err != nil {
 		panic(err)

diff --git a/vendor/github.com/prometheus/prometheus/model/histogram/float_histogram.go b/vendor/github.com/prometheus/prometheus/model/histogram/float_histogram.go
diff --git a/vendor/github.com/prometheus/prometheus/promql/engine.go b/vendor/github.com/prometheus/prometheus/promql/engine.go
diff --git a/vendor/github.com/prometheus/prometheus/promql/parser/generated_parser.y b/vendor/github.com/prometheus/prometheus/promql/parser/generated_parser.y