Skip to content

Commit fdc7cd2

Browse files
querier: clamp start time for Series, LabelValues, LabelNames (#9129)
* querier: clamp start time for Series, LabelValues, LabelNames Start and End times in Series, LabelNames, LabelValues calls are optional. When they're unset Prometheus defaults them to MinTime (effectively MinInt) and MaxTime (MaxInt). Currently MaxQueryIntoFuture clamps the start time relative to now. After that the start time is clamped relative to the end because of the max_label_query_length limit. However with PR XXX we're preparing to remove that option, so we'll no longer be clamping max time relative to now. Without clamping the end time we end up clamping start to MaxTime, so the whole query is for some (e.g.) 30 days in the year 292277025. Which usually returns no data. What does this PR do In this PR we're only clamping start time. It's clamped to end time if that was provided or to now, otherwise. We don't clamp end time because there's likely no data to query and we don't have to think about ignoring samples with a future timestamp that have been ingested. Not clamping end time is potentially a problem if there's large clock skew between clients sending data and the query-frontend. I think this is very unlikely to cause problems. Signed-off-by: Dimitar Dimitrov <[email protected]> * Add CHANGELOG.md entry Signed-off-by: Dimitar Dimitrov <[email protected]> * Update CHANGELOG.md Co-authored-by: Charles Korn <[email protected]> * Rename clamping function Signed-off-by: Dimitar Dimitrov <[email protected]> --------- Signed-off-by: Dimitar Dimitrov <[email protected]> Co-authored-by: Charles Korn <[email protected]>
1 parent 3699327 commit fdc7cd2

File tree

5 files changed

+162
-46
lines changed

5 files changed

+162
-46
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@
100100
* [BUGFIX] Ruler: map invalid org-id errors to 400 status code. #8935
101101
* [BUGFIX] Querier: Fix invalid query results when multiple chunks are being merged. #8992
102102
* [BUGFIX] Query-frontend: return annotations generated during evaluation of sharded queries. #9138
103+
* [BUGFIX] Querier: Support optional start and end times on `/prometheus/api/v1/labels`, `/prometheus/api/v1/label/<label>/values`, and `/prometheus/api/v1/series` when `max_query_into_future: 0`. #9129
103104

104105
### Mixin
105106

pkg/querier/blocks_store_queryable.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,7 @@ func (q *blocksStoreQuerier) LabelNames(ctx context.Context, _ *storage.LabelHin
355355
// Clamp minT; we cannot push this down into queryWithConsistencyCheck as not all its callers need to clamp minT
356356
maxQueryLength := q.limits.MaxLabelsQueryLength(tenantID)
357357
if maxQueryLength != 0 {
358-
minT = clampMinTime(spanLog, minT, maxT, -maxQueryLength, "max label query length")
358+
minT = clampToMaxLabelQueryLength(spanLog, minT, maxT, time.Now().UnixMilli(), maxQueryLength.Milliseconds())
359359
}
360360

361361
var (
@@ -400,7 +400,7 @@ func (q *blocksStoreQuerier) LabelValues(ctx context.Context, name string, _ *st
400400
// Clamp minT; we cannot push this down into queryWithConsistencyCheck as not all its callers need to clamp minT
401401
maxQueryLength := q.limits.MaxLabelsQueryLength(tenantID)
402402
if maxQueryLength != 0 {
403-
minT = clampMinTime(spanLog, minT, maxT, -maxQueryLength, "max label query length")
403+
minT = clampToMaxLabelQueryLength(spanLog, minT, maxT, time.Now().UnixMilli(), maxQueryLength.Milliseconds())
404404
}
405405

406406
var (

pkg/querier/blocks_store_queryable_test.go

Lines changed: 51 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import (
3737
"github.com/prometheus/prometheus/promql"
3838
"github.com/prometheus/prometheus/storage"
3939
"github.com/prometheus/prometheus/tsdb/chunkenc"
40+
v1 "github.com/prometheus/prometheus/web/api/v1"
4041
"github.com/stretchr/testify/assert"
4142
"github.com/stretchr/testify/mock"
4243
"github.com/stretchr/testify/require"
@@ -2640,6 +2641,35 @@ func TestBlocksStoreQuerier_MaxLabelsQueryRange(t *testing.T) {
26402641
expectedMinT: util.TimeToMillis(now.Add(-sevenDays)),
26412642
expectedMaxT: util.TimeToMillis(now),
26422643
},
2644+
2645+
"should manipulate query on large time range over the limit": {
2646+
maxLabelsQueryLength: thirtyDays,
2647+
queryMinT: util.TimeToMillis(now.Add(-thirtyDays).Add(-100 * time.Hour)),
2648+
queryMaxT: util.TimeToMillis(now),
2649+
expectedMinT: util.TimeToMillis(now.Add(-thirtyDays)),
2650+
expectedMaxT: util.TimeToMillis(now),
2651+
},
2652+
"should manipulate the start of a query without start time": {
2653+
maxLabelsQueryLength: thirtyDays,
2654+
queryMinT: util.TimeToMillis(v1.MinTime),
2655+
queryMaxT: util.TimeToMillis(now),
2656+
expectedMinT: util.TimeToMillis(now.Add(-thirtyDays)),
2657+
expectedMaxT: util.TimeToMillis(now),
2658+
},
2659+
"should not manipulate query without end time, we allow querying arbitrarily into the future": {
2660+
maxLabelsQueryLength: thirtyDays,
2661+
queryMinT: util.TimeToMillis(now.Add(-time.Hour)),
2662+
queryMaxT: util.TimeToMillis(v1.MaxTime),
2663+
expectedMinT: util.TimeToMillis(now.Add(-time.Hour)),
2664+
expectedMaxT: util.TimeToMillis(v1.MaxTime),
2665+
},
2666+
"should manipulate the start of a query without start or end time, we allow querying arbitrarily into the future, but not the past": {
2667+
maxLabelsQueryLength: thirtyDays,
2668+
queryMinT: util.TimeToMillis(v1.MinTime),
2669+
queryMaxT: util.TimeToMillis(v1.MaxTime),
2670+
expectedMinT: util.TimeToMillis(now.Add(-thirtyDays)),
2671+
expectedMaxT: util.TimeToMillis(v1.MaxTime),
2672+
},
26432673
}
26442674

26452675
for testName, testData := range tests {
@@ -2661,17 +2691,28 @@ func TestBlocksStoreQuerier_MaxLabelsQueryRange(t *testing.T) {
26612691
},
26622692
}
26632693

2664-
_, _, err := q.LabelNames(ctx, &storage.LabelHints{})
2665-
require.NoError(t, err)
2666-
require.Len(t, finder.Calls, 1)
2667-
assert.Equal(t, testData.expectedMinT, finder.Calls[0].Arguments.Get(2))
2668-
assert.Equal(t, testData.expectedMaxT, finder.Calls[0].Arguments.Get(3))
2694+
assertCalledWithMinMaxTime := func() {
2695+
const delta = float64(5000)
2696+
require.Len(t, finder.Calls, 1)
2697+
gotStartMillis := finder.Calls[0].Arguments.Get(2).(int64)
2698+
assert.InDeltaf(t, testData.expectedMinT, gotStartMillis, delta, "expected start %s, got %s", util.TimeFromMillis(testData.expectedMinT).UTC(), util.TimeFromMillis(gotStartMillis).UTC())
2699+
gotEndMillis := finder.Calls[0].Arguments.Get(3).(int64)
2700+
assert.InDeltaf(t, testData.expectedMaxT, gotEndMillis, delta, "expected end %s, got %s", util.TimeFromMillis(testData.expectedMinT).UTC(), util.TimeFromMillis(gotEndMillis).UTC())
2701+
finder.Calls = finder.Calls[1:]
2702+
}
26692703

2670-
_, _, err = q.LabelValues(ctx, "foo", &storage.LabelHints{})
2671-
require.Len(t, finder.Calls, 2)
2672-
require.NoError(t, err)
2673-
assert.Equal(t, testData.expectedMinT, finder.Calls[1].Arguments.Get(2))
2674-
assert.Equal(t, testData.expectedMaxT, finder.Calls[1].Arguments.Get(3))
2704+
// Assert on the time range of the actual executed query (5s delta).
2705+
t.Run("LabelNames", func(t *testing.T) {
2706+
_, _, err := q.LabelNames(ctx, &storage.LabelHints{})
2707+
require.NoError(t, err)
2708+
assertCalledWithMinMaxTime()
2709+
})
2710+
2711+
t.Run("LabelValues", func(t *testing.T) {
2712+
_, _, err := q.LabelValues(ctx, "foo", &storage.LabelHints{})
2713+
require.NoError(t, err)
2714+
assertCalledWithMinMaxTime()
2715+
})
26752716
})
26762717
}
26772718
}

pkg/querier/querier.go

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/prometheus/prometheus/promql/parser"
2424
"github.com/prometheus/prometheus/storage"
2525
"github.com/prometheus/prometheus/util/annotations"
26+
v1 "github.com/prometheus/prometheus/web/api/v1"
2627
"golang.org/x/sync/errgroup"
2728

2829
"github.com/grafana/mimir/pkg/querier/engine"
@@ -338,8 +339,7 @@ func (mq multiQuerier) Select(ctx context.Context, _ bool, sp *storage.SelectHin
338339
return storage.ErrSeriesSet(err)
339340
}
340341
if sp.Func == "series" { // Clamp max time range for series-only queries, before we check max length.
341-
maxQueryLength := mq.limits.MaxLabelsQueryLength(userID)
342-
startMs = clampMinTime(spanLog, startMs, endMs, -maxQueryLength, "max label query length")
342+
startMs = clampToMaxLabelQueryLength(spanLog, startMs, endMs, now.UnixMilli(), mq.limits.MaxLabelsQueryLength(userID).Milliseconds())
343343
}
344344

345345
// The time range may have been manipulated during the validation,
@@ -382,6 +382,47 @@ func (mq multiQuerier) Select(ctx context.Context, _ bool, sp *storage.SelectHin
382382
return mq.mergeSeriesSets(result)
383383
}
384384

385+
func clampToMaxLabelQueryLength(spanLog *spanlogger.SpanLogger, startMs, endMs, nowMs, maxLabelQueryLengthMs int64) int64 {
386+
if maxLabelQueryLengthMs == 0 {
387+
// It's unlimited.
388+
return startMs
389+
}
390+
unsetStartTime := startMs == v1.MinTime.UnixMilli()
391+
unsetEndTime := endMs == v1.MaxTime.UnixMilli()
392+
393+
switch {
394+
case unsetStartTime && unsetEndTime:
395+
// The user asked for "everything", but that's too expensive.
396+
// We clamp the start, since the past likely has more data.
397+
// Allow querying into the future because that will likely have much less data.
398+
// Leaving end unchanged also allows to query the future for samples with timestamps in the future.
399+
earliestAllowedStart := nowMs - maxLabelQueryLengthMs
400+
logClampEvent(spanLog, startMs, earliestAllowedStart, "min", "max label query length")
401+
startMs = earliestAllowedStart
402+
case unsetStartTime:
403+
// We can't provide all data since the beginning of time.
404+
// But end was provided, so we use the end as the anchor.
405+
earliestAllowedStart := endMs - maxLabelQueryLengthMs
406+
logClampEvent(spanLog, startMs, earliestAllowedStart, "min", "max label query length")
407+
startMs = earliestAllowedStart
408+
case unsetEndTime:
409+
// Start was provided, but not end.
410+
// We clamp the start relative to now so that we don't query a lot of data.
411+
if earliestAllowedStart := nowMs - maxLabelQueryLengthMs; earliestAllowedStart > startMs {
412+
logClampEvent(spanLog, startMs, earliestAllowedStart, "min", "max label query length")
413+
startMs = earliestAllowedStart
414+
}
415+
default:
416+
// Both start and end were provided. We clamp the start.
417+
// There's no strong reason to do this vs clamping end.
418+
if earliestAllowedStart := endMs - maxLabelQueryLengthMs; earliestAllowedStart > startMs {
419+
logClampEvent(spanLog, startMs, earliestAllowedStart, "min", "max label query length")
420+
startMs = earliestAllowedStart
421+
}
422+
}
423+
return startMs
424+
}
425+
385426
// LabelValues implements storage.Querier.
386427
func (mq multiQuerier) LabelValues(ctx context.Context, name string, hints *storage.LabelHints, matchers ...*labels.Matcher) ([]string, annotations.Annotations, error) {
387428
ctx, queriers, err := mq.getQueriers(ctx)

pkg/querier/querier_test.go

Lines changed: 65 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"github.com/prometheus/prometheus/tsdb/chunkenc"
2525
"github.com/prometheus/prometheus/util/annotations"
2626
promtestutil "github.com/prometheus/prometheus/util/testutil"
27+
v1 "github.com/prometheus/prometheus/web/api/v1"
2728
"github.com/stretchr/testify/assert"
2829
"github.com/stretchr/testify/mock"
2930
"github.com/stretchr/testify/require"
@@ -873,9 +874,10 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
873874
},
874875
}
875876

877+
logger := log.NewNopLogger()
876878
// Create the PromQL engine to execute the queries.
877879
engine := promql.NewEngine(promql.EngineOpts{
878-
Logger: log.NewNopLogger(),
880+
Logger: logger,
879881
ActiveQueryTracker: nil,
880882
MaxSamples: 1e6,
881883
LookbackDelta: engineLookbackDelta,
@@ -900,7 +902,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
900902
distributor.On("Query", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(model.Matrix{}, nil)
901903
distributor.On("QueryStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(client.CombinedQueryStreamResponse{}, nil)
902904

903-
queryable, _, _, err := New(cfg, overrides, distributor, nil, nil, log.NewNopLogger(), nil)
905+
queryable, _, _, err := New(cfg, overrides, distributor, nil, nil, logger, nil)
904906
require.NoError(t, err)
905907

906908
query, err := engine.NewRangeQuery(ctx, queryable, nil, testData.query, testData.queryStartTime, testData.queryEndTime, time.Minute)
@@ -928,7 +930,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
928930
distributor := &mockDistributor{}
929931
distributor.On("MetricsForLabelMatchers", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]labels.Labels{}, nil)
930932

931-
queryable, _, _, err := New(cfg, overrides, distributor, nil, nil, log.NewNopLogger(), nil)
933+
queryable, _, _, err := New(cfg, overrides, distributor, nil, nil, logger, nil)
932934
require.NoError(t, err)
933935

934936
q, err := queryable.Querier(util.TimeToMillis(testData.queryStartTime), util.TimeToMillis(testData.queryEndTime))
@@ -965,7 +967,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
965967
distributor := &mockDistributor{}
966968
distributor.On("LabelNames", mock.Anything, mock.Anything, mock.Anything, matchers).Return([]string{}, nil)
967969

968-
queryable, _, _, err := New(cfg, overrides, distributor, nil, nil, log.NewNopLogger(), nil)
970+
queryable, _, _, err := New(cfg, overrides, distributor, nil, nil, logger, nil)
969971
require.NoError(t, err)
970972

971973
q, err := queryable.Querier(util.TimeToMillis(testData.queryStartTime), util.TimeToMillis(testData.queryEndTime))
@@ -993,7 +995,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
993995
distributor := &mockDistributor{}
994996
distributor.On("LabelValuesForLabelName", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]string{}, nil)
995997

996-
queryable, _, _, err := New(cfg, overrides, distributor, nil, nil, log.NewNopLogger(), nil)
998+
queryable, _, _, err := New(cfg, overrides, distributor, nil, nil, logger, nil)
997999
require.NoError(t, err)
9981000

9991001
q, err := queryable.Querier(util.TimeToMillis(testData.queryStartTime), util.TimeToMillis(testData.queryEndTime))
@@ -1021,7 +1023,7 @@ func TestQuerier_ValidateQueryTimeRange_MaxQueryLookback(t *testing.T) {
10211023
// Check that time range of /series is restricted by maxLabelsQueryLength.
10221024
// LabelName and LabelValues are checked in TestBlocksStoreQuerier_MaxLabelsQueryRange(),
10231025
// because the implementation of those makes it really hard to do in Querier.
1024-
func TestQuerier_MaxLabelsQueryRange(t *testing.T) {
1026+
func TestQuerier_ValidateQueryTimeRange_MaxLabelsQueryRange(t *testing.T) {
10251027
const (
10261028
thirtyDays = 30 * 24 * time.Hour
10271029
)
@@ -1042,6 +1044,34 @@ func TestQuerier_MaxLabelsQueryRange(t *testing.T) {
10421044
expectedMetadataStartTime: now.Add(-thirtyDays),
10431045
expectedMetadataEndTime: now,
10441046
},
1047+
"should not manipulate query short time range within the limit": {
1048+
maxLabelsQueryLength: model.Duration(thirtyDays),
1049+
queryStartTime: now.Add(-time.Hour),
1050+
queryEndTime: now,
1051+
expectedMetadataStartTime: now.Add(-time.Hour),
1052+
expectedMetadataEndTime: now,
1053+
},
1054+
"should manipulate the start of a query without start time": {
1055+
maxLabelsQueryLength: model.Duration(thirtyDays),
1056+
queryStartTime: v1.MinTime,
1057+
queryEndTime: now,
1058+
expectedMetadataStartTime: now.Add(-thirtyDays),
1059+
expectedMetadataEndTime: now,
1060+
},
1061+
"should not manipulate query without end time, we allow querying arbitrarily into the future": {
1062+
maxLabelsQueryLength: model.Duration(thirtyDays),
1063+
queryStartTime: now.Add(-time.Hour),
1064+
queryEndTime: v1.MaxTime,
1065+
expectedMetadataStartTime: now.Add(-time.Hour),
1066+
expectedMetadataEndTime: v1.MaxTime,
1067+
},
1068+
"should manipulate the start of a query without start or end time, we allow querying arbitrarily into the future, but not the past": {
1069+
maxLabelsQueryLength: model.Duration(thirtyDays),
1070+
queryStartTime: v1.MinTime,
1071+
queryEndTime: v1.MaxTime,
1072+
expectedMetadataStartTime: now.Add(-thirtyDays),
1073+
expectedMetadataEndTime: v1.MaxTime,
1074+
},
10451075
}
10461076

10471077
for testName, testData := range tests {
@@ -1050,46 +1080,49 @@ func TestQuerier_MaxLabelsQueryRange(t *testing.T) {
10501080

10511081
var cfg Config
10521082
flagext.DefaultValues(&cfg)
1083+
cfg.MaxQueryIntoFuture = 0
10531084

10541085
limits := defaultLimitsConfig()
10551086
limits.MaxQueryLookback = model.Duration(thirtyDays * 2)
10561087
limits.MaxLabelsQueryLength = testData.maxLabelsQueryLength
1088+
limits.MaxPartialQueryLength = testData.maxLabelsQueryLength
1089+
limits.MaxTotalQueryLength = testData.maxLabelsQueryLength
10571090
limits.QueryIngestersWithin = 0 // Always query ingesters in this test.
10581091
overrides, err := validation.NewOverrides(limits, nil)
10591092
require.NoError(t, err)
10601093

10611094
// block storage will not be hit; provide nil querier
10621095
var storeQueryable storage.Queryable
10631096

1064-
t.Run("series", func(t *testing.T) {
1065-
distributor := &mockDistributor{}
1066-
distributor.On("MetricsForLabelMatchers", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]labels.Labels{}, nil)
1067-
1068-
queryable, _, _, err := New(cfg, overrides, distributor, storeQueryable, nil, log.NewNopLogger(), nil)
1069-
require.NoError(t, err)
1070-
1071-
q, err := queryable.Querier(util.TimeToMillis(testData.queryStartTime), util.TimeToMillis(testData.queryEndTime))
1072-
require.NoError(t, err)
1073-
1074-
hints := &storage.SelectHints{
1075-
Start: util.TimeToMillis(testData.queryStartTime),
1076-
End: util.TimeToMillis(testData.queryEndTime),
1077-
Func: "series",
1078-
}
1079-
matcher := labels.MustNewMatcher(labels.MatchEqual, labels.MetricName, "test")
1097+
distributor := &mockDistributor{}
1098+
distributor.On("MetricsForLabelMatchers", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]labels.Labels{}, nil)
10801099

1081-
set := q.Select(ctx, false, hints, matcher)
1082-
require.False(t, set.Next()) // Expected to be empty.
1083-
require.NoError(t, set.Err())
1100+
queryable, _, _, err := New(cfg, overrides, distributor, storeQueryable, nil, log.NewNopLogger(), nil)
1101+
require.NoError(t, err)
10841102

1085-
// Assert on the time range of the actual executed query (5s delta).
1086-
delta := float64(5000)
1087-
require.Len(t, distributor.Calls, 1)
1088-
assert.Equal(t, "MetricsForLabelMatchers", distributor.Calls[0].Method)
1089-
assert.InDelta(t, util.TimeToMillis(testData.expectedMetadataStartTime), int64(distributor.Calls[0].Arguments.Get(1).(model.Time)), delta)
1090-
assert.InDelta(t, util.TimeToMillis(testData.expectedMetadataEndTime), int64(distributor.Calls[0].Arguments.Get(2).(model.Time)), delta)
1091-
})
1103+
q, err := queryable.Querier(util.TimeToMillis(testData.queryStartTime), util.TimeToMillis(testData.queryEndTime))
1104+
require.NoError(t, err)
10921105

1106+
hints := &storage.SelectHints{
1107+
Start: util.TimeToMillis(testData.queryStartTime),
1108+
End: util.TimeToMillis(testData.queryEndTime),
1109+
Func: "series",
1110+
}
1111+
matcher := labels.MustNewMatcher(labels.MatchEqual, labels.MetricName, "test")
1112+
1113+
set := q.Select(ctx, false, hints, matcher)
1114+
require.False(t, set.Next()) // Expected to be empty.
1115+
require.NoError(t, set.Err())
1116+
1117+
// Assert on the time range of the actual executed query (5s delta).
1118+
delta := float64(5000)
1119+
require.Len(t, distributor.Calls, 1)
1120+
assert.Equal(t, "MetricsForLabelMatchers", distributor.Calls[0].Method)
1121+
assert.Equal(t, "MetricsForLabelMatchers", distributor.Calls[0].Method)
1122+
gotStartMillis := int64(distributor.Calls[0].Arguments.Get(1).(model.Time))
1123+
assert.InDeltaf(t, util.TimeToMillis(testData.expectedMetadataStartTime), gotStartMillis, delta, "expected start %s, got %s", testData.expectedMetadataStartTime.UTC(), util.TimeFromMillis(gotStartMillis).UTC())
1124+
gotEndMillis := int64(distributor.Calls[0].Arguments.Get(2).(model.Time))
1125+
assert.InDeltaf(t, util.TimeToMillis(testData.expectedMetadataEndTime), gotEndMillis, delta, "expected end %s, got %s", testData.expectedMetadataEndTime.UTC(), util.TimeFromMillis(gotEndMillis).UTC())
10931126
})
10941127
}
10951128
}

0 commit comments

Comments
 (0)