From b9fdb9672a1b51462cfab69f89489077eaf2c14b Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Thu, 12 Dec 2024 19:38:40 +0200 Subject: [PATCH 1/4] lazyquerier: deep copy hints The promQL engine can modify the grouping that it passes to us after calling. First the LazyQuerier is called in [`populateSeries`](https://github.com/grafana/mimir/blob/76ab843e3bb3d6517fcd1ee47e919bd9d107ef0d/vendor/github.com/prometheus/prometheus/promql/engine.go#L953-L976), and then the grouping is changed (via sorting) when evaluating the [aggregate expression](https://github.com/grafana/mimir/blob/76ab843e3bb3d6517fcd1ee47e919bd9d107ef0d/vendor/github.com/prometheus/prometheus/promql/engine.go#L1571-L1573) Signed-off-by: Dimitar Dimitrov --- pkg/storage/lazyquery/lazyqeury_test.go | 48 +++++++++++++++++++++++++ pkg/storage/lazyquery/lazyquery.go | 11 +++++- 2 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 pkg/storage/lazyquery/lazyqeury_test.go diff --git a/pkg/storage/lazyquery/lazyqeury_test.go b/pkg/storage/lazyquery/lazyqeury_test.go new file mode 100644 index 00000000000..8ff12d39e60 --- /dev/null +++ b/pkg/storage/lazyquery/lazyqeury_test.go @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package lazyquery + +import ( + "reflect" + "testing" + + "github.com/prometheus/prometheus/storage" + "github.com/stretchr/testify/require" +) + +func TestCopyParamsDeepCopy(t *testing.T) { + original := &storage.SelectHints{ + Start: 1000, + End: 2000, + Step: 10, + Range: 3600, + Func: "rate", + Grouping: []string{"label1", "label2"}, + } + + copied := copyParams(original) + + // First verify the structs themselves are different + require.NotSame(t, original, copied) + + // Then check each field is a different pointer + originalVal := reflect.ValueOf(original).Elem() + copiedVal := reflect.ValueOf(copied).Elem() + typ := originalVal.Type() + for i := 0; i < typ.NumField(); i++ { + originalField := originalVal.Field(i) + copiedField := copiedVal.Field(i) + + // Check if values are equal + require.Equal(t, originalField.Interface(), copiedField.Interface(), "Field %s has different values", typ.Field(i).Name) + + // For reference types, ensure they point to different memory + if originalField.Kind() == reflect.Slice || + originalField.Kind() == reflect.Map || + originalField.Kind() == reflect.Ptr { + if !originalField.IsNil() { + require.NotEqual(t, originalField.UnsafePointer(), copiedField.UnsafePointer(), "Field %s shares memory between original and copy", typ.Field(i).Name) + } + } + } +} diff --git a/pkg/storage/lazyquery/lazyquery.go b/pkg/storage/lazyquery/lazyquery.go index 80d157926cb..1e7918d8c33 100644 --- a/pkg/storage/lazyquery/lazyquery.go +++ b/pkg/storage/lazyquery/lazyquery.go @@ -7,6 +7,7 @@ package lazyquery import ( "context" + "slices" "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/storage" @@ -49,8 +50,9 @@ func (l LazyQuerier) Select(ctx context.Context, selectSorted bool, params *stor // make sure there is space in the buffer, to unblock the goroutine and let it die even if nobody is // waiting for the result yet (or anymore). future := make(chan storage.SeriesSet, 1) + copiedParams := copyParams(params) go func() { - future <- l.next.Select(ctx, selectSorted, params, matchers...) + future <- l.next.Select(ctx, selectSorted, copiedParams, matchers...) }() return &lazySeriesSet{ @@ -58,6 +60,13 @@ func (l LazyQuerier) Select(ctx context.Context, selectSorted bool, params *stor } } +func copyParams(params *storage.SelectHints) *storage.SelectHints { + copiedParams := *params + copiedParams.Grouping = slices.Clone(params.Grouping) + + return &copiedParams +} + // LabelValues implements Storage.Querier func (l LazyQuerier) LabelValues(ctx context.Context, name string, hints *storage.LabelHints, matchers ...*labels.Matcher) ([]string, annotations.Annotations, error) { return l.next.LabelValues(ctx, name, hints, matchers...) From fccb51de334c72a937eda75bc44aee8a3be03d5f Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Mon, 16 Dec 2024 17:27:13 +0200 Subject: [PATCH 2/4] Don't copy nil params Signed-off-by: Dimitar Dimitrov --- pkg/storage/lazyquery/lazyquery.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/storage/lazyquery/lazyquery.go b/pkg/storage/lazyquery/lazyquery.go index 1e7918d8c33..33e901cfaa8 100644 --- a/pkg/storage/lazyquery/lazyquery.go +++ b/pkg/storage/lazyquery/lazyquery.go @@ -61,6 +61,9 @@ func (l LazyQuerier) Select(ctx context.Context, selectSorted bool, params *stor } func copyParams(params *storage.SelectHints) *storage.SelectHints { + if params == nil { + return nil + } copiedParams := *params copiedParams.Grouping = slices.Clone(params.Grouping) From 65b8be2a09616e233c94089795ba35e7b621e523 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Wed, 18 Dec 2024 11:17:24 +0200 Subject: [PATCH 3/4] Fix filename typo Signed-off-by: Dimitar Dimitrov --- pkg/storage/lazyquery/{lazyqeury_test.go => lazyquery_test.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename pkg/storage/lazyquery/{lazyqeury_test.go => lazyquery_test.go} (100%) diff --git a/pkg/storage/lazyquery/lazyqeury_test.go b/pkg/storage/lazyquery/lazyquery_test.go similarity index 100% rename from pkg/storage/lazyquery/lazyqeury_test.go rename to pkg/storage/lazyquery/lazyquery_test.go From 95a33d942033b8dd703c345d667cb397e28e7a8a Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Thu, 19 Dec 2024 15:44:39 +0200 Subject: [PATCH 4/4] Clean up code a bit Signed-off-by: Dimitar Dimitrov --- pkg/storage/lazyquery/lazyquery_test.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/pkg/storage/lazyquery/lazyquery_test.go b/pkg/storage/lazyquery/lazyquery_test.go index 8ff12d39e60..b0f3c00d4f8 100644 --- a/pkg/storage/lazyquery/lazyquery_test.go +++ b/pkg/storage/lazyquery/lazyquery_test.go @@ -7,7 +7,7 @@ import ( "testing" "github.com/prometheus/prometheus/storage" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" ) func TestCopyParamsDeepCopy(t *testing.T) { @@ -23,7 +23,7 @@ func TestCopyParamsDeepCopy(t *testing.T) { copied := copyParams(original) // First verify the structs themselves are different - require.NotSame(t, original, copied) + assert.NotSame(t, original, copied) // Then check each field is a different pointer originalVal := reflect.ValueOf(original).Elem() @@ -34,15 +34,16 @@ func TestCopyParamsDeepCopy(t *testing.T) { copiedField := copiedVal.Field(i) // Check if values are equal - require.Equal(t, originalField.Interface(), copiedField.Interface(), "Field %s has different values", typ.Field(i).Name) + assert.Equal(t, originalField.Interface(), copiedField.Interface(), "Field %s has different values", typ.Field(i).Name) + switch originalField.Kind() { // For reference types, ensure they point to different memory - if originalField.Kind() == reflect.Slice || - originalField.Kind() == reflect.Map || - originalField.Kind() == reflect.Ptr { + case reflect.Slice, reflect.Map, reflect.Ptr: if !originalField.IsNil() { - require.NotEqual(t, originalField.UnsafePointer(), copiedField.UnsafePointer(), "Field %s shares memory between original and copy", typ.Field(i).Name) + assert.NotEqual(t, originalField.UnsafePointer(), copiedField.UnsafePointer(), "Field %s shares memory between original and copy", typ.Field(i).Name) } + default: + // Any other types are copied by value, so the assert.Equal above is enough. } } }