Query-frontend: enforce limitsMiddleware for remote read requests

[pracucci]

What this PR does

In this PR I propose to enforce limitsMiddleware for remote read requests too.

Before this PR, -query-frontend.max-query-expression-size-bytes, -query-frontend.max-total-query-length and -querier.max-query-lookback were not enforced for remote read requests in the query-frontend. After this PR, they're enforced in the query-frontend. If a remote read request contains only 1 out of N queries failing because of the limit, the whole remote read request will be rejected (as of today it's quite difficult to only fail 1 out of N queries).

The main change in this PR is about introducing support to manipulate the remote read queries before they're passed to downstream (querier). This is needed by the -querier.max-query-lookback enforcement done by limitsMiddleware.

Which issue(s) this PR fixes or relates to

N/A

Checklist

• Tests updated.
• Documentation added.
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• about-versioning.md updated with experimental features.

[krajorama]

this feels off, couldn't we just update the r.query and convert that into the result with remoteReadToMetricsQueryRequest

[pracucci]

I see your comment and I also don't like much this function, but the WithXXX() functions are expected to return a deep copy without manipulating any of the original request data (that's by contract of these functions). If we do manipulate r.query then we're going to break the contract.

E.g. we take a similar approach in PrometheusRangeQueryRequest too, but there we don't have a complex nested struct like prompb.Query:

mimir/pkg/frontend/querymiddleware/model_extra.go

Lines 158 to 168 in 5571e24

	func (r *PrometheusRangeQueryRequest) WithStartEnd(start int64, end int64) MetricsQueryRequest {
	newRequest := *r
	newRequest.start = start
	newRequest.end = end
	if newRequest.queryExpr != nil {
	newRequest.minT, newRequest.maxT = decodeQueryMinMaxTime(
	newRequest.queryExpr, newRequest.GetStart(), newRequest.GetEnd(), newRequest.GetStep(), newRequest.lookbackDelta,
	)
	}
	return &newRequest
	}

[pracucci]

Discussed offline. I've changed the code to use remoteReadToMetricsQueryRequest().

[pracucci]

Note to reviewers: see #8388

[krajorama]

Don't we have to call req.Body.Close() ?

[pracucci]

I don't think so. The http.Request.Body comment says:

	// For server requests, the Request Body is always non-nil
	// but will return EOF immediately when no body is present.
	// The Server will close the request body. The ServeHTTP
	// Handler does not need to.

[krajorama]

Based on the discussion in prometheus/prometheus#4226 it seems like query [start, end] always includes the [hint start, hint-end] as the query start is for initializing a Querier and the hint is for selecting actual samples.

[krajorama]

nit: now I assume that if someone provided hints, they set a sensible Hints.StartMs. Since we don't have validation midddleware, maybe we do need a zero check

Suggested change

	if clonedQuery.Hints != nil && clonedQuery.Hints.StartMs < start {
	if clonedQuery.Hints != nil && clonedQuery.Hints.StartMs != 0 && clonedQuery.Hints.StartMs < start {

Just to avoid setting StartMs > EndMs

[pracucci]

I'm not super sure about this.

Let's forget for a moment the fact that today we're ignoring Hints in Mimir, and let's assume we'll fix it (as I think we should do).

The reason why I think we should not consider 0 an "invalid" number is because, if hints are populated (non nil), we should just honor values are they're set. Prometheus does the same in its remote read implementation, where ReadHints are converted into SelectHints  and then start/end from SelectHints are just used as is (and we do the same in Mimir for instant/range queries):
https://github.com/prometheus/prometheus/blob/5efc8dd27b6e68d5102b77bc708e52c9821c5101/tsdb/querier.go#L135-L136

[krajorama]

approved, with one nit (which I caused by taking away test for 0)