[querier] honor querier mint,maxt if no SelectHints are passed to Select

[aallawala]

What this PR does:
This PR attempts to address an issue our org is seeing where each user's Rules Manager is unable to restore the FOR state, as intended via this Prometheus PR prometheus/prometheus#4061

What happens instead is, since the Rules Manager initiates a Select query with no SelectHints (even though the querier that is initialized via https://github.com/prometheus/prometheus/blob/b1ed4a0a663d/rules/manager.go#L710 bounds itself down to just the OutageTolerance window set within the ManagerOpts), the Cortex querier treats this as a series search instead of a metrics search. The current Cortex querier implementation looks for a nil SelectHints OR a SelectHints.Func == "series" to apply a Labels search instead of a time series search. This Labels search then strips out any samples via series.MetricsToSeriesSet before sending the result back to the caller.

The Ruler's RestoreForState expects a time series search as the samples are later used to restore the FOR state even though a nil is passed for SelectHints. Since the querier will return the MetricSeries without samples, the RestoreForState restores to the current timestamp, effectively resetting all progress that was done by the prior owning Ruler instance.

I believe this change is alright given that the time bounds of the Select query are checked further down in the querier's Select block. https://github.com/cortexproject/cortex/blob/master/pkg/querier/querier.go#L320-L323

I'm not entirely familiar with this code base, so there will probably be something I'm missing. All feedback is appreciated!

cc'ing the maintainers whom I've seen commits from WRT this, @pracucci @gouthamve @codesome

Which issue(s) this PR fixes:
Fixes # I didn't find a corresponding issue but please do update if there is one.

Checklist

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

[bboreham]

Thanks for this. What you say seems plausible, but can we go a bit further and remove all assumptions about what the caller meant?
Comments previously talked about a "kludge" and "later versions of prometheus", but we know exactly which version of prometheus this code is linked against, so we ought to be able to check, no?

[aallawala]

Thanks for this. What you say seems plausible, but can we go a bit further and remove all assumptions about what the caller meant?
Comments previously talked about a "kludge" and "later versions of prometheus", but we know exactly which version of prometheus this code is linked against, so we ought to be able to check, no?

thank you for taking a look, @bboreham.

I dug into this a little bit and saw that the "kludge" comment was first introduced via a prometheus upgrade done via PR: #802 . Looking at the Gopkg dep list, that PR brought in the prometheus commit, prometheus/prometheus@c47fbcb, which was right before the prom 2.3.0 release.

The next comment, "later versions of prometheus", was added via PR: to specifically bring in the change where prometheus now submits a select hint with func="series" when initiating a series search versus passing a nil.

The prometheus version that's pinned against Cortex now does inject a SelectHint.func="series" when submitting a series search. I am unable to find any other instances where prometheus submits a nil for a series search.

[bboreham]

Sorry, I think I have to retract my previous point - looking at prometheus/prometheus#8050 it talks about remote endpoints, so probably we can receive the request and its hints from different versions of Prometheus via remote-read.

That said, should the Rules Manager be changed to pass the series hint?

[aallawala]

so probably we can receive the request and its hints from different versions of Prometheus via remote-read

Correct me if I'm wrong - Even if cortex can receive a read request and its hints from different versions of Prometheus via remote-read, isn't the entrypoint still gated via the api/vi/api.go? All series requests that go through the v1 API now have a SelectHint object initialized with a start/stop and func=series, versus having it be set to nil.

That said, should the Rules Manager be changed to pass the series hint?

I don't disagree that the Rules Manager should pass a series hint as well - but in this instance, rather than manipulating the core Rules Manager logic, this PR attempts to match the behavior of the querier implementations in Prometheus itself. The two main Prometheus querier implementations, blockQuerier and blockChunkQuerier, both will set the minT and maxT to the querier itself before utilizing the selectHints, should they exist.

Meanwhile, currently there are three Cortex querier implementations (with the fourth being the top-level querier that will fan out to the three queriers), the blocksStoreQuerier, the chunkStoreQuerier, and the distributorQuerier. Each handles the time range of the query in their own way.

The blocksStoreQuerier will honor the Prometheus behavior where it will first set the minT,maxT to the querier and utilize the SelectHints, should they exist.

The chunkStoreQuerier will only utilize the time range in the SelectHints. Should they not exist (or if the func="series"), the querier will return an empty set.

The distributorQuerier is similar to the chunkStoreQuerier in that it will only utilize the time range in the SelectHints. However, should they not exist (or if func="series"), the querier will process the query as a labels search instead of a time series search.

What do you think, @bboreham?

[pracucci]

Very interesting!

I tend to agree with @aallawala here. I think we should match the current Prometheus logic and so when SelectHints is nil we should consider it as a normal query and not a "series" query. It's a series query only when hints are specified and hints.Func is "series".

That being said, the place where you changed the logic is not the only place where we check if hints == nil, so I think there are places to be changed. I suggest you to look for all references of storage.SelectHints.

[aallawala]

Very interesting!

I tend to agree with @aallawala here. I think we should match the current Prometheus logic and so when SelectHints is nil we should consider it as a normal query and not a "series" query. It's a series query only when hints are specified and hints.Func is "series".

That being said, the place where you changed the logic is not the only place where we check if hints == nil, so I think there are places to be changed. I suggest you to look for all references of storage.SelectHints.

Thank you for taking a look, @pracucci.

I went ahead and updated the remaining two base querier implementations, chunksStoreQuerier and distributorQueryable. The blocksStoreQuerier already matches the Prometheus behavior where it will first set the minT,maxT to the querier and utilize the SelectHints, should they exist.

Would there happen to be any other places that should be updated?

[bboreham]

LGTM

Note there are a couple of merge conflicts, which I would typically rebase for but you can try doing a merge if you like. If you do it in the GitHub UI it may lack the DCO sign-off.

[aallawala]

LGTM

Note there are a couple of merge conflicts, which I would typically rebase for but you can try doing a merge if you like. If you do it in the GitHub UI it may lack the DCO sign-off.

Thank you @bboreham for taking another look! I rebased to address the conflicts upstream. PTAL and let me know if there are other changes to be made

[aallawala]

Hi, could I get some eyes on this please?

[alvinlin123]

Thanks! LGTM.