diff --git a/CHANGELOG.md b/CHANGELOG.md index b4d407ee27..0da9148564 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re ### Fixed +- [#7011](https://github.com/thanos-io/thanos/pull/7011) Query Frontend: queries with negative offset should check whether it is cacheable or not. - [#6874](https://github.com/thanos-io/thanos/pull/6874) Sidecar: fix labels returned by 'api/v1/series' in presence of conflicting external and inner labels. - [#7009](https://github.com/thanos-io/thanos/pull/7009) Rule: Fix spacing error in URL. diff --git a/internal/cortex/querier/queryrange/results_cache.go b/internal/cortex/querier/queryrange/results_cache.go index d29f5c3305..de0f7e8149 100644 --- a/internal/cortex/querier/queryrange/results_cache.go +++ b/internal/cortex/querier/queryrange/results_cache.go @@ -282,6 +282,9 @@ func (s resultsCache) shouldCacheResponse(ctx context.Context, req Request, r Re if !s.isAtModifierCachable(req, maxCacheTime) { return false } + if !s.isOffsetCachable(req) { + return false + } if s.cacheGenNumberLoader == nil { return true @@ -305,11 +308,10 @@ func (s resultsCache) shouldCacheResponse(ctx context.Context, req Request, r Re return true } -var errAtModifierAfterEnd = errors.New("at modifier after end") - // isAtModifierCachable returns true if the @ modifier result // is safe to cache. func (s resultsCache) isAtModifierCachable(r Request, maxCacheTime int64) bool { + var errAtModifierAfterEnd = errors.New("at modifier after end") // There are 2 cases when @ modifier is not safe to cache: // 1. When @ modifier points to time beyond the maxCacheTime. // 2. If the @ modifier time is > the query range end while being @@ -357,6 +359,46 @@ func (s resultsCache) isAtModifierCachable(r Request, maxCacheTime int64) bool { return atModCachable } +// isOffsetCachable returns true if the offset is positive, result is safe to cache. +// and false when offset is negative, result is not cached. +func (s resultsCache) isOffsetCachable(r Request) bool { + var errNegativeOffset = errors.New("negative offset") + query := r.GetQuery() + if !strings.Contains(query, "offset") { + return true + } + expr, err := parser.ParseExpr(query) + if err != nil { + level.Warn(s.logger).Log("msg", "failed to parse query, considering offset as not cachable", "query", query, "err", err) + return false + } + + offsetCachable := true + parser.Inspect(expr, func(n parser.Node, _ []parser.Node) error { + switch e := n.(type) { + case *parser.VectorSelector: + if e.OriginalOffset < 0 { + offsetCachable = false + return errNegativeOffset + } + case *parser.MatrixSelector: + offset := e.VectorSelector.(*parser.VectorSelector).OriginalOffset + if offset < 0 { + offsetCachable = false + return errNegativeOffset + } + case *parser.SubqueryExpr: + if e.OriginalOffset < 0 { + offsetCachable = false + return errNegativeOffset + } + } + return nil + }) + + return offsetCachable +} + func getHeaderValuesWithName(r Response, headerName string) (headerValues []string) { for _, hv := range r.GetHeaders() { if hv.GetName() != headerName { diff --git a/internal/cortex/querier/queryrange/results_cache_test.go b/internal/cortex/querier/queryrange/results_cache_test.go index cb7d04fc95..cb12d5cb23 100644 --- a/internal/cortex/querier/queryrange/results_cache_test.go +++ b/internal/cortex/querier/queryrange/results_cache_test.go @@ -532,6 +532,45 @@ func TestShouldCache(t *testing.T) { input: Response(&PrometheusResponse{}), expected: false, }, + // offset on vector selectors. + { + name: "positive offset on vector selector", + request: &PrometheusRequest{Query: "metric offset 10ms", End: 125000}, + input: Response(&PrometheusResponse{}), + expected: true, + }, + { + name: "negative offset on vector selector", + request: &PrometheusRequest{Query: "metric offset -10ms", End: 125000}, + input: Response(&PrometheusResponse{}), + expected: false, + }, + // offset on matrix selectors. + { + name: "positive offset on matrix selector", + request: &PrometheusRequest{Query: "rate(metric[5m] offset 10ms)", End: 125000}, + input: Response(&PrometheusResponse{}), + expected: true, + }, + { + name: "negative offset on matrix selector", + request: &PrometheusRequest{Query: "rate(metric[5m] offset -10ms)", End: 125000}, + input: Response(&PrometheusResponse{}), + expected: false, + }, + // offset on subqueries. + { + name: "positive offset on subqueries", + request: &PrometheusRequest{Query: "sum_over_time(rate(metric[1m])[10m:1m] offset 10ms)", End: 125000}, + input: Response(&PrometheusResponse{}), + expected: true, + }, + { + name: "negative offset on subqueries", + request: &PrometheusRequest{Query: "sum_over_time(rate(metric[1m])[10m:1m] offset -10ms)", End: 125000}, + input: Response(&PrometheusResponse{}), + expected: false, + }, } { { t.Run(tc.name, func(t *testing.T) {