Skip to content

Commit

Permalink
lazyquerier: deep copy hints (#10228)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>

* Don't copy nil params

Signed-off-by: Dimitar Dimitrov <[email protected]>

* Fix filename typo

Signed-off-by: Dimitar Dimitrov <[email protected]>

* Clean up code a bit

Signed-off-by: Dimitar Dimitrov <[email protected]>

---------

Signed-off-by: Dimitar Dimitrov <[email protected]>
  • Loading branch information
dimitarvdimitrov authored Dec 19, 2024
1 parent a53f79d commit aeff3be
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 1 deletion.
14 changes: 13 additions & 1 deletion pkg/storage/lazyquery/lazyquery.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package lazyquery

import (
"context"
"slices"

"github.com/prometheus/prometheus/model/labels"
"github.com/prometheus/prometheus/storage"
Expand Down Expand Up @@ -49,15 +50,26 @@ 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{
future: future,
}
}

func copyParams(params *storage.SelectHints) *storage.SelectHints {
if params == nil {
return nil
}
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...)
Expand Down
49 changes: 49 additions & 0 deletions pkg/storage/lazyquery/lazyquery_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// SPDX-License-Identifier: AGPL-3.0-only

package lazyquery

import (
"reflect"
"testing"

"github.com/prometheus/prometheus/storage"
"github.com/stretchr/testify/assert"
)

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
assert.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
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
case reflect.Slice, reflect.Map, reflect.Ptr:
if !originalField.IsNil() {
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.
}
}
}

0 comments on commit aeff3be

Please sign in to comment.