diff --git a/CHANGELOG.md b/CHANGELOG.md index 541cc6856e3..330b9e0020f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,7 @@ * [BUGFIX] Querier: Fix unbounded resource leak in the bucket-scan blocks finder (used when the bucket index is disabled). Per-tenant metadata fetchers, their Prometheus registries, and on-disk meta caches are now evicted once a tenant is no longer active, instead of being retained for the lifetime of the process. #7573 * [BUGFIX] Alertmanager: Fix data race between ApplyConfig's dispatcher/inhibitor startup and Stop during config reload and shutdown, and reject lazy tenant creation after shutdown begins. #7618 * [BUGFIX] Distributor: Release the push worker pool goroutines on shutdown by stopping the async executor during the stopping phase when `-distributor.num-push-workers` is set. #7602 +* [BUGFIX] Query Frontend: Track data selection min and max time of query requests in query stats regardless of whether query priority and query rejection are enabled. #7724 * [BUGFIX] Querier: Fix flake in integration tests TestQuerierWithStoreGatewayDataBytesLimits and TestQuerierWithBlocksStorageLimits by waiting for the querier to see the store-gateway ACTIVE in the ring before querying. #7614 * [BUGFIX] Ruler: Register xfunctions (xincrease, xrate, xdelta) in the global parser before loading rule files. #7621 * [BUGFIX] Security: Reject empty entries in `-distributor.sign-write-requests-keys` caused by stray or trailing commas (e.g. `newkey,`). Previously these were silently accepted and produced an empty signing key, which downgraded HMAC stream-push authentication to a forgeable signature. Misconfigured flags now fail at process startup; audit your configs before upgrading. #7587 diff --git a/pkg/querier/tripperware/query_attribute_matcher.go b/pkg/querier/tripperware/query_attribute_matcher.go index 7e0990162bb..54dcaee85cf 100644 --- a/pkg/querier/tripperware/query_attribute_matcher.go +++ b/pkg/querier/tripperware/query_attribute_matcher.go @@ -18,9 +18,6 @@ import ( const QueryRejectErrorMessage = "This query does not perform well and has been rejected by the service operator." func rejectQueryOrSetPriority(r *http.Request, now time.Time, lookbackDelta time.Duration, limits Limits, userStr string, rejectedQueriesPerTenant *prometheus.CounterVec) error { - if limits == nil || (!limits.QueryPriority(userStr).Enabled && !limits.QueryRejection(userStr).Enabled) { - return nil - } op := getOperation(r) reqStats := stats.FromContext(r.Context()) @@ -32,6 +29,15 @@ func rejectQueryOrSetPriority(r *http.Request, now time.Time, lookbackDelta time } minTime, maxTime := util.FindMinMaxTime(r, expr, lookbackDelta, now) + // Track data selection time range for query stats purpose. It should + // be tracked regardless of query priority and query rejection being enabled. + reqStats.SetDataSelectMaxTime(maxTime) + reqStats.SetDataSelectMinTime(minTime) + + if limits == nil || (!limits.QueryPriority(userStr).Enabled && !limits.QueryRejection(userStr).Enabled) { + return nil + } + if queryReject := limits.QueryRejection(userStr); queryReject.Enabled && query != "" { for _, attribute := range queryReject.QueryAttributes { if matchAttributeForExpressionQuery(attribute, op, r, query, now, minTime, maxTime) { @@ -41,9 +47,6 @@ func rejectQueryOrSetPriority(r *http.Request, now time.Time, lookbackDelta time } } - reqStats.SetDataSelectMaxTime(maxTime) - reqStats.SetDataSelectMinTime(minTime) - if queryPriority := limits.QueryPriority(userStr); queryPriority.Enabled && len(queryPriority.Priorities) != 0 && query != "" { for _, priority := range queryPriority.Priorities { for _, attribute := range priority.QueryAttributes { @@ -56,6 +59,10 @@ func rejectQueryOrSetPriority(r *http.Request, now time.Time, lookbackDelta time reqStats.SetPriority(queryPriority.DefaultPriority) } } else { + if limits == nil || (!limits.QueryPriority(userStr).Enabled && !limits.QueryRejection(userStr).Enabled) { + return nil + } + if queryReject := limits.QueryRejection(userStr); queryReject.Enabled && (op == "series" || op == "labels" || op == "label_values") { for _, attribute := range queryReject.QueryAttributes { if matchAttributeForMetadataQuery(attribute, op, r, now) { diff --git a/pkg/querier/tripperware/query_attribute_matcher_test.go b/pkg/querier/tripperware/query_attribute_matcher_test.go index 31d6b455999..60d8d82f5a2 100644 --- a/pkg/querier/tripperware/query_attribute_matcher_test.go +++ b/pkg/querier/tripperware/query_attribute_matcher_test.go @@ -88,6 +88,61 @@ func Test_rejectQueryOrSetPriorityShouldReturnDefaultPriorityIfNotEnabledOrInval } } +func Test_rejectQueryOrSetPriorityShouldTrackDataSelectTimeRangeIndependentOfQueryPriorityAndRejection(t *testing.T) { + disabledLimits := mockLimits{ + queryPriority: validation.QueryPriority{Enabled: false}, + queryRejection: validation.QueryRejection{Enabled: false}, + } + lookbackDelta := 5 * time.Minute + + type testCase struct { + limits Limits + path string + expectedMinTime int64 + expectedMaxTime int64 + } + + tests := map[string]testCase{ + "should track data select time range for instant query when query priority and rejection are disabled": { + limits: disabledLimits, + path: "/api/v1/query?time=1536716898&query=sum%28container_memory_rss%29+by+%28namespace%29", + // min time is (time - lookbackDelta) exclusive. + expectedMinTime: 1536716898000 - lookbackDelta.Milliseconds() + 1, + expectedMaxTime: 1536716898000, + }, + "should track data select time range for range query when query priority and rejection are disabled": { + limits: disabledLimits, + path: "/api/v1/query_range?start=1536716898&end=1536720498&step=15&query=sum%28container_memory_rss%29+by+%28namespace%29", + expectedMinTime: 1536716898000 - lookbackDelta.Milliseconds() + 1, + expectedMaxTime: 1536720498000, + }, + "should track data select time range when limits is nil": { + limits: nil, + path: "/api/v1/query?time=1536716898&query=up", + expectedMinTime: 1536716898000 - lookbackDelta.Milliseconds() + 1, + expectedMaxTime: 1536716898000, + }, + "should not track data select time range for metadata query": { + limits: disabledLimits, + path: "/api/v1/labels?match[]=up", + expectedMinTime: 0, + expectedMaxTime: 0, + }, + } + + for testName, testData := range tests { + t.Run(testName, func(t *testing.T) { + req, err := http.NewRequest("GET", testData.path, http.NoBody) + require.NoError(t, err) + reqStats, ctx := stats.ContextWithEmptyStats(context.Background()) + req = req.WithContext(ctx) + require.NoError(t, rejectQueryOrSetPriority(req, time.Now(), lookbackDelta, testData.limits, "", rejectedQueriesPerTenant)) + assert.Equal(t, testData.expectedMinTime, reqStats.LoadDataSelectMinTime()) + assert.Equal(t, testData.expectedMaxTime, reqStats.LoadDataSelectMaxTime()) + }) + } +} + func Test_rejectQueryOrSetPriorityShouldRejectIfMatches(t *testing.T) { now := time.Now() limits := mockLimits{