Skip to content

Commit e65da18

Browse files
aallawalaalvinlin123
aallawala
and
alvinlin123
authored
[querier] honor querier mint,maxt if no SelectHints are passed to Select (#4413)
* [querier] honor querier mint,maxt if no SelectHints are passed to Select Signed-off-by: Abdurrahman J. Allawala <aallawala1@gmail.com> * export mockDistributor for later use in the ruler tests Signed-off-by: Abdurrahman J. Allawala <aallawala1@gmail.com> * add test for restoring FOR state in the ruler via distributors Signed-off-by: Abdurrahman J. Allawala <aallawala1@gmail.com> * test for always active alert Signed-off-by: Abdurrahman J. Allawala <aallawala1@gmail.com> * move querier/testutils to querier pkg Signed-off-by: Abdurrahman J. Allawala <aallawala1@gmail.com> * add empty chunk store to ruler test Signed-off-by: Abdurrahman J. Allawala <aallawala1@gmail.com> * add changelog entry Signed-off-by: Abdurrahman J. Allawala <aallawala1@gmail.com> * lint Signed-off-by: Abdurrahman J. Allawala <aallawala1@gmail.com> * honor mint,maxt if sp is null for other querier implementations Signed-off-by: Abdurrahman J. Allawala <aallawala1@gmail.com> * missed addition in rebase Signed-off-by: Abdurrahman J. Allawala <aallawala1@gmail.com> Co-authored-by: Alvin Lin <alvinlin@amazon.com>
1 parent 83d15b5 commit e65da18

11 files changed

+309
-113
lines changed

‎CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,10 @@
5353
* [BUGFIX] Memberlist: forward only changes, not entire original message. #4419
5454
* [BUGFIX] Memberlist: don't accept old tombstones as incoming change, and don't forward such messages to other gossip members. #4420
5555
* [BUGFIX] Querier: fixed panic when querying exemplars and using `-distributor.shard-by-all-labels=false`. #4473
56+
* [BUGFIX] Querier: honor querier minT,maxT if `nil` SelectHints are passed to Select(). #4413
5657
* [BUGFIX] Compactor: fixed panic while collecting Prometheus metrics. #4483
5758

59+
5860
## 1.10.0 / 2021-08-03
5961

6062
* [CHANGE] Prevent path traversal attack from users able to control the HTTP header `X-Scope-OrgID`. #4375 (CVE-2021-36157)

‎pkg/querier/chunk_store_queryable.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,14 +44,19 @@ func (q *chunkStoreQuerier) Select(_ bool, sp *storage.SelectHints, matchers ...
4444
return storage.ErrSeriesSet(err)
4545
}
4646

47+
minT, maxT := q.mint, q.maxt
48+
if sp != nil {
49+
minT, maxT = sp.Start, sp.End
50+
}
51+
4752
// We will hit this for /series lookup when -querier.query-store-for-labels-enabled is set.
4853
// If we don't skip here, it'll make /series lookups extremely slow as all the chunks will be loaded.
4954
// That flag is only to be set with blocks storage engine, and this is a protective measure.
50-
if sp == nil || sp.Func == "series" {
55+
if sp != nil && sp.Func == "series" {
5156
return storage.EmptySeriesSet()
5257
}
5358

54-
chunks, err := q.store.Get(q.ctx, userID, model.Time(sp.Start), model.Time(sp.End), matchers...)
59+
chunks, err := q.store.Get(q.ctx, userID, model.Time(minT), model.Time(maxT), matchers...)
5560
if err != nil {
5661
return storage.ErrSeriesSet(err)
5762
}

‎pkg/querier/distributor_queryable.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -83,23 +83,26 @@ func (q *distributorQuerier) Select(_ bool, sp *storage.SelectHints, matchers ..
8383
log, ctx := spanlogger.New(q.ctx, "distributorQuerier.Select")
8484
defer log.Span.Finish()
8585

86-
// Kludge: Prometheus passes nil SelectParams if it is doing a 'series' operation,
87-
// which needs only metadata. For this specific case we shouldn't apply the queryIngestersWithin
86+
minT, maxT := q.mint, q.maxt
87+
if sp != nil {
88+
minT, maxT = sp.Start, sp.End
89+
}
90+
91+
// If the querier receives a 'series' query, it means only metadata is needed.
92+
// For this specific case we shouldn't apply the queryIngestersWithin
8893
// time range manipulation, otherwise we'll end up returning no series at all for
8994
// older time ranges (while in Cortex we do ignore the start/end and always return
9095
// series in ingesters).
9196
// Also, in the recent versions of Prometheus, we pass in the hint but with Func set to "series".
9297
// See: https://github.com/prometheus/prometheus/pull/8050
93-
if sp == nil || sp.Func == "series" {
98+
if sp != nil && sp.Func == "series" {
9499
ms, err := q.distributor.MetricsForLabelMatchers(ctx, model.Time(q.mint), model.Time(q.maxt), matchers...)
95100
if err != nil {
96101
return storage.ErrSeriesSet(err)
97102
}
98103
return series.MetricsToSeriesSet(ms)
99104
}
100105

101-
minT, maxT := sp.Start, sp.End
102-
103106
// If queryIngestersWithin is enabled, we do manipulate the query mint to query samples up until
104107
// now - queryIngestersWithin, because older time ranges are covered by the storage. This
105108
// optimization is particularly important for the blocks storage where the blocks retention in the

‎pkg/querier/distributor_queryable_test.go

Lines changed: 9 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88

99
"github.com/prometheus/common/model"
1010
"github.com/prometheus/prometheus/pkg/labels"
11-
"github.com/prometheus/prometheus/scrape"
1211
"github.com/prometheus/prometheus/storage"
1312
"github.com/stretchr/testify/assert"
1413
"github.com/stretchr/testify/mock"
@@ -29,7 +28,7 @@ const (
2928
)
3029

3130
func TestDistributorQuerier(t *testing.T) {
32-
d := &mockDistributor{}
31+
d := &MockDistributor{}
3332
d.On("Query", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(
3433
model.Matrix{
3534
// Matrixes are unsorted, so this tests that the labels get sorted.
@@ -117,7 +116,7 @@ func TestDistributorQuerier_SelectShouldHonorQueryIngestersWithin(t *testing.T)
117116
for _, streamingEnabled := range []bool{false, true} {
118117
for testName, testData := range tests {
119118
t.Run(fmt.Sprintf("%s (streaming enabled: %t)", testName, streamingEnabled), func(t *testing.T) {
120-
distributor := &mockDistributor{}
119+
distributor := &MockDistributor{}
121120
distributor.On("Query", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(model.Matrix{}, nil)
122121
distributor.On("QueryStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&client.QueryStreamResponse{}, nil)
123122
distributor.On("MetricsForLabelMatchers", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]metric.Metric{}, nil)
@@ -127,10 +126,10 @@ func TestDistributorQuerier_SelectShouldHonorQueryIngestersWithin(t *testing.T)
127126
querier, err := queryable.Querier(ctx, testData.queryMinT, testData.queryMaxT)
128127
require.NoError(t, err)
129128

130-
// Select hints are not passed by Prometheus when querying /series.
129+
// Select hints are passed by Prometheus when querying /series.
131130
var hints *storage.SelectHints
132-
if !testData.querySeries {
133-
hints = &storage.SelectHints{Start: testData.queryMinT, End: testData.queryMaxT}
131+
if testData.querySeries {
132+
hints = &storage.SelectHints{Func: "series"}
134133
}
135134

136135
seriesSet := querier.Select(true, hints)
@@ -149,7 +148,7 @@ func TestDistributorQuerier_SelectShouldHonorQueryIngestersWithin(t *testing.T)
149148
}
150149

151150
func TestDistributorQueryableFilter(t *testing.T) {
152-
d := &mockDistributor{}
151+
d := &MockDistributor{}
153152
dq := newDistributorQueryable(d, false, nil, 1*time.Hour)
154153

155154
now := time.Now()
@@ -175,7 +174,7 @@ func TestIngesterStreaming(t *testing.T) {
175174
})
176175
require.NoError(t, err)
177176

178-
d := &mockDistributor{}
177+
d := &MockDistributor{}
179178
d.On("QueryStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(
180179
&client.QueryStreamResponse{
181180
Chunkseries: []client.TimeSeriesChunk{
@@ -244,7 +243,7 @@ func TestIngesterStreamingMixedResults(t *testing.T) {
244243
{Value: 5.5, TimestampMs: 5500},
245244
}
246245

247-
d := &mockDistributor{}
246+
d := &MockDistributor{}
248247
d.On("QueryStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(
249248
&client.QueryStreamResponse{
250249
Chunkseries: []client.TimeSeriesChunk{
@@ -316,7 +315,7 @@ func TestDistributorQuerier_LabelNames(t *testing.T) {
316315
{Metric: model.Metric{"job": "baz"}},
317316
{Metric: model.Metric{"job": "baz", "foo": "boom"}},
318317
}
319-
d := &mockDistributor{}
318+
d := &MockDistributor{}
320319
d.On("MetricsForLabelMatchers", mock.Anything, model.Time(mint), model.Time(maxt), someMatchers).
321320
Return(metrics, nil)
322321

@@ -350,37 +349,3 @@ func convertToChunks(t *testing.T, samples []cortexpb.Sample) []client.Chunk {
350349

351350
return clientChunks
352351
}
353-
354-
type mockDistributor struct {
355-
mock.Mock
356-
}
357-
358-
func (m *mockDistributor) Query(ctx context.Context, from, to model.Time, matchers ...*labels.Matcher) (model.Matrix, error) {
359-
args := m.Called(ctx, from, to, matchers)
360-
return args.Get(0).(model.Matrix), args.Error(1)
361-
}
362-
func (m *mockDistributor) QueryExemplars(ctx context.Context, from, to model.Time, matchers ...[]*labels.Matcher) (*client.ExemplarQueryResponse, error) {
363-
args := m.Called(ctx, from, to, matchers)
364-
return args.Get(0).(*client.ExemplarQueryResponse), args.Error(1)
365-
}
366-
func (m *mockDistributor) QueryStream(ctx context.Context, from, to model.Time, matchers ...*labels.Matcher) (*client.QueryStreamResponse, error) {
367-
args := m.Called(ctx, from, to, matchers)
368-
return args.Get(0).(*client.QueryStreamResponse), args.Error(1)
369-
}
370-
func (m *mockDistributor) LabelValuesForLabelName(ctx context.Context, from, to model.Time, lbl model.LabelName, matchers ...*labels.Matcher) ([]string, error) {
371-
args := m.Called(ctx, from, to, lbl, matchers)
372-
return args.Get(0).([]string), args.Error(1)
373-
}
374-
func (m *mockDistributor) LabelNames(ctx context.Context, from, to model.Time) ([]string, error) {
375-
args := m.Called(ctx, from, to)
376-
return args.Get(0).([]string), args.Error(1)
377-
}
378-
func (m *mockDistributor) MetricsForLabelMatchers(ctx context.Context, from, to model.Time, matchers ...*labels.Matcher) ([]metric.Metric, error) {
379-
args := m.Called(ctx, from, to, matchers)
380-
return args.Get(0).([]metric.Metric), args.Error(1)
381-
}
382-
383-
func (m *mockDistributor) MetricsMetadata(ctx context.Context) ([]scrape.MetricMetadata, error) {
384-
args := m.Called(ctx)
385-
return args.Get(0).([]scrape.MetricMetadata), args.Error(1)
386-
}

‎pkg/querier/metadata_handler_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import (
1313
)
1414

1515
func TestMetadataHandler_Success(t *testing.T) {
16-
d := &mockDistributor{}
16+
d := &MockDistributor{}
1717
d.On("MetricsMetadata", mock.Anything).Return(
1818
[]scrape.MetricMetadata{
1919
{Metric: "alertmanager_dispatcher_aggregation_groups", Help: "Number of active aggregation groups", Type: "gauge", Unit: ""},
@@ -51,7 +51,7 @@ func TestMetadataHandler_Success(t *testing.T) {
5151
}
5252

5353
func TestMetadataHandler_Error(t *testing.T) {
54-
d := &mockDistributor{}
54+
d := &MockDistributor{}
5555
d.On("MetricsMetadata", mock.Anything).Return([]scrape.MetricMetadata{}, fmt.Errorf("no user id"))
5656

5757
handler := MetadataHandler(d)

‎pkg/querier/querier.go

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -300,13 +300,15 @@ func (q querier) Select(_ bool, sp *storage.SelectHints, matchers ...*labels.Mat
300300
level.Debug(log).Log("start", util.TimeFromMillis(sp.Start).UTC().String(), "end", util.TimeFromMillis(sp.End).UTC().String(), "step", sp.Step, "matchers", matchers)
301301
}
302302

303-
// Kludge: Prometheus passes nil SelectHints if it is doing a 'series' operation,
304-
// which needs only metadata. Here we expect that metadataQuerier querier will handle that.
305-
// In Cortex it is not feasible to query entire history (with no mint/maxt), so we only ask ingesters and skip
306-
// querying the long-term storage.
307-
// Also, in the recent versions of Prometheus, we pass in the hint but with Func set to "series".
308-
// See: https://github.com/prometheus/prometheus/pull/8050
309-
if (sp == nil || sp.Func == "series") && !q.queryStoreForLabels {
303+
if sp == nil {
304+
// if SelectHints is null, rely on minT, maxT of querier to scope in range for Select stmt
305+
sp = &storage.SelectHints{Start: q.mint, End: q.maxt}
306+
} else if sp.Func == "series" && !q.queryStoreForLabels {
307+
// Else if the querier receives a 'series' query, it means only metadata is needed.
308+
// Here we expect that metadataQuerier querier will handle that.
309+
// Also, in the recent versions of Prometheus, we pass in the hint but with Func set to "series".
310+
// See: https://github.com/prometheus/prometheus/pull/8050
311+
310312
// In this case, the query time range has already been validated when the querier has been
311313
// created.
312314
return q.metadataQuerier.Select(true, sp, matchers...)

‎pkg/querier/querier_test.go

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ func TestQuerier(t *testing.T) {
157157
chunkStore, through := makeMockChunkStore(t, chunks, encoding.e)
158158
distributor := mockDistibutorFor(t, chunkStore, through)
159159

160-
overrides, err := validation.NewOverrides(defaultLimitsConfig(), nil)
160+
overrides, err := validation.NewOverrides(DefaultLimitsConfig(), nil)
161161
require.NoError(t, err)
162162

163163
queryables := []QueryableWithFilter{UseAlwaysQueryable(NewChunkStoreQueryable(cfg, chunkStore)), UseAlwaysQueryable(db)}
@@ -280,7 +280,7 @@ func TestNoHistoricalQueryToIngester(t *testing.T) {
280280
chunkStore, _ := makeMockChunkStore(t, 24, encodings[0].e)
281281
distributor := &errDistributor{}
282282

283-
overrides, err := validation.NewOverrides(defaultLimitsConfig(), nil)
283+
overrides, err := validation.NewOverrides(DefaultLimitsConfig(), nil)
284284
require.NoError(t, err)
285285

286286
queryable, _, _ := New(cfg, overrides, distributor, []QueryableWithFilter{UseAlwaysQueryable(NewChunkStoreQueryable(cfg, chunkStore))}, purger.NewTombstonesLoader(nil, nil), nil, log.NewNopLogger())
@@ -364,11 +364,11 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryIntoFuture(t *testing.T) {
364364
t.Run(fmt.Sprintf("%s (ingester streaming enabled = %t)", name, cfg.IngesterStreaming), func(t *testing.T) {
365365
// We don't need to query any data for this test, so an empty store is fine.
366366
chunkStore := &emptyChunkStore{}
367-
distributor := &mockDistributor{}
367+
distributor := &MockDistributor{}
368368
distributor.On("Query", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(model.Matrix{}, nil)
369369
distributor.On("QueryStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&client.QueryStreamResponse{}, nil)
370370

371-
overrides, err := validation.NewOverrides(defaultLimitsConfig(), nil)
371+
overrides, err := validation.NewOverrides(DefaultLimitsConfig(), nil)
372372
require.NoError(t, err)
373373

374374
queryables := []QueryableWithFilter{UseAlwaysQueryable(NewChunkStoreQueryable(cfg, chunkStore))}
@@ -438,7 +438,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLength(t *testing.T) {
438438
var cfg Config
439439
flagext.DefaultValues(&cfg)
440440

441-
limits := defaultLimitsConfig()
441+
limits := DefaultLimitsConfig()
442442
limits.MaxQueryLength = model.Duration(maxQueryLength)
443443
overrides, err := validation.NewOverrides(limits, nil)
444444
require.NoError(t, err)
@@ -560,7 +560,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
560560
flagext.DefaultValues(&cfg)
561561
cfg.IngesterStreaming = ingesterStreaming
562562

563-
limits := defaultLimitsConfig()
563+
limits := DefaultLimitsConfig()
564564
limits.MaxQueryLookback = testData.maxQueryLookback
565565
overrides, err := validation.NewOverrides(limits, nil)
566566
require.NoError(t, err)
@@ -570,7 +570,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
570570
queryables := []QueryableWithFilter{UseAlwaysQueryable(NewChunkStoreQueryable(cfg, chunkStore))}
571571

572572
t.Run("query range", func(t *testing.T) {
573-
distributor := &mockDistributor{}
573+
distributor := &MockDistributor{}
574574
distributor.On("Query", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(model.Matrix{}, nil)
575575
distributor.On("QueryStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&client.QueryStreamResponse{}, nil)
576576

@@ -599,7 +599,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
599599
})
600600

601601
t.Run("series", func(t *testing.T) {
602-
distributor := &mockDistributor{}
602+
distributor := &MockDistributor{}
603603
distributor.On("MetricsForLabelMatchers", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]metric.Metric{}, nil)
604604

605605
queryable, _, _ := New(cfg, overrides, distributor, queryables, purger.NewTombstonesLoader(nil, nil), nil, log.NewNopLogger())
@@ -631,7 +631,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
631631
})
632632

633633
t.Run("label names", func(t *testing.T) {
634-
distributor := &mockDistributor{}
634+
distributor := &MockDistributor{}
635635
distributor.On("LabelNames", mock.Anything, mock.Anything, mock.Anything).Return([]string{}, nil)
636636

637637
queryable, _, _ := New(cfg, overrides, distributor, queryables, purger.NewTombstonesLoader(nil, nil), nil, log.NewNopLogger())
@@ -658,7 +658,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
658658
matchers := []*labels.Matcher{
659659
labels.MustNewMatcher(labels.MatchNotEqual, "route", "get_user"),
660660
}
661-
distributor := &mockDistributor{}
661+
distributor := &MockDistributor{}
662662
distributor.On("MetricsForLabelMatchers", mock.Anything, mock.Anything, mock.Anything, matchers).Return([]metric.Metric{}, nil)
663663

664664
queryable, _, _ := New(cfg, overrides, distributor, queryables, purger.NewTombstonesLoader(nil, nil), nil, log.NewNopLogger())
@@ -684,7 +684,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
684684
})
685685

686686
t.Run("label values", func(t *testing.T) {
687-
distributor := &mockDistributor{}
687+
distributor := &MockDistributor{}
688688
distributor.On("LabelValuesForLabelName", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]string{}, nil)
689689

690690
queryable, _, _ := New(cfg, overrides, distributor, queryables, purger.NewTombstonesLoader(nil, nil), nil, log.NewNopLogger())
@@ -713,7 +713,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
713713

714714
// mockDistibutorFor duplicates the chunks in the mockChunkStore into the mockDistributor
715715
// so we can test everything is dedupe correctly.
716-
func mockDistibutorFor(t *testing.T, cs mockChunkStore, through model.Time) *mockDistributor {
716+
func mockDistibutorFor(t *testing.T, cs mockChunkStore, through model.Time) *MockDistributor {
717717
chunks, err := chunkcompat.ToChunks(cs.chunks)
718718
require.NoError(t, err)
719719

@@ -724,7 +724,7 @@ func mockDistibutorFor(t *testing.T, cs mockChunkStore, through model.Time) *moc
724724
matrix, err := chunk.ChunksToMatrix(context.Background(), cs.chunks, 0, through)
725725
require.NoError(t, err)
726726

727-
result := &mockDistributor{}
727+
result := &MockDistributor{}
728728
result.On("Query", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(matrix, nil)
729729
result.On("QueryStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&client.QueryStreamResponse{Chunkseries: []client.TimeSeriesChunk{tsc}}, nil)
730730
return result
@@ -902,7 +902,7 @@ func TestShortTermQueryToLTS(t *testing.T) {
902902
chunkStore := &emptyChunkStore{}
903903
distributor := &errDistributor{}
904904

905-
overrides, err := validation.NewOverrides(defaultLimitsConfig(), nil)
905+
overrides, err := validation.NewOverrides(DefaultLimitsConfig(), nil)
906906
require.NoError(t, err)
907907

908908
queryable, _, _ := New(cfg, overrides, distributor, []QueryableWithFilter{UseAlwaysQueryable(NewChunkStoreQueryable(cfg, chunkStore))}, purger.NewTombstonesLoader(nil, nil), nil, log.NewNopLogger())
@@ -1020,9 +1020,3 @@ func (m *mockQueryableWithFilter) UseQueryable(_ time.Time, _, _ int64) bool {
10201020
m.useQueryableCalled = true
10211021
return true
10221022
}
1023-
1024-
func defaultLimitsConfig() validation.Limits {
1025-
limits := validation.Limits{}
1026-
flagext.DefaultValues(&limits)
1027-
return limits
1028-
}

0 commit comments

Comments
 (0)