StoreAPIs be clear about Series() invariants / SeriesSet invariant

[bwplotka]

Just want to make sure we are all on the same page on one of the invariants/requirements of StoreAPI.Series API.

Currently, our API assumes that each Series frame will hold ALL the chunks for the certain series from a given source. This means that chain of frames like below from single StoreAPI is NOT expected:

&testClient{
					StoreClient: &mockedStoreAPI{
						RespSeries: []*storepb.SeriesResponse{
							storeSeriesResponse(t, labels.FromStrings("a", "a"), []sample{{0, 0}, {2, 1}, {3, 2}}, []sample{{4, 3}}),
							storeSeriesResponse(t, labels.FromStrings("a", "a"), []sample{{5, 4}}), // Continuations of the same series. This is against our API!
							storepb.NewWarnSeriesResponse(errors.New("warning")),
							storeSeriesResponse(t, labels.FromStrings("a", "b"), []sample{{2, 2}, {3, 3}, {4, 4}}),
						},
					},
				},

The same is assumed (implicitly) by SeriesSet interfaces e.g here: https://github.com/thanos-io/thanos/blob/master/pkg/store/storepb/custom.go#L80 This invariant is assumed by PromQL engine (@brian-brazil right?) and so far also respected by all our StoreAPI implementations (thanks to https://github.com/thanos-io/thanos/blob/master/pkg/store/storepb/custom.go#L63). However, we never explicitly said that on the API level.

What makes it tricky is our recently added remote read streaming DOES allow "continuation of the same series": prometheus/prometheus#5703 Remote read decision to not stick to strict SeriesSet behavior was driven by the goal of having constant memory frames while marshaling/unmarshalling protobuf. Imagine a case when you run a query for 2y against sidecar + Prometheus. With being able to chunk single series into pieces we can guarantee constant memory usage and don't need any sample limit etc. I think that's fair.

Now we have this inconsistency between remote read and StoreAPI (spotted in this PR), where StoreAPI is potentially stricter. Now we have 3 options:

1. Change Prometheus remote read to be strict as well. I think this is not as great as constant memory reason is quite awesome.
2. Add logic in Thanos sidecar to compose multiple series into a single frame to match the StoreAPI invariant. This is easy to do, but we are losing constant memory guarantee which is quite crucial, especially for the sidecar.
3. Lose this requirement on StoreAPI and handle that properly on Querier (before PromQL). Still have this requirement for internall Series Set interfaces to not confuse. I think that is the right solution.

Thanks @krasi-georgiev for spotting this.

Thoughts @brancz @brian-brazil @povilasv @GiedriusS @miekg @gouthamve @krasi-georgiev ?

It's worth to note that the recent optimizations for potential caching layer thanks to @tomwilkie are splitting the query on PromAPI level into days no matter what. This makes this decision less relevant/important as most likely all chunks will fit into the single frame easily.

[krasi-georgiev]

Lose this requirement on StoreAPI and handle that properly on Querier (before PromQL). Still have this requirement for internall Series Set interfaces to not confuse. I think that is the right solution.

I also think that this is the right solution.
tsdb does something similar with the vertical compaction(although it matches by overlapping time range and not by duplicate series.)

[brian-brazil]

This invariant is assumed by PromQL engine (@brian-brazil right?)

PromQL yes, however the remote read code on the Prometheus end should be handling this already. It's expected that a series will be spread across frames for constant memory reasons.

[bwplotka]

Cool, looks we want to state the same on StoreAPI then. 👍

[brancz]

Yes we want to split them in the StoreAPI responses, the only place where everything needs to be available is when PromQL executes, and with the Cortex query optimizations that means at max each split of queries will at most have to have 1 day worth of data in memory.

tl;dr yes loosen StoreAPI.

[bwplotka]

Cool. This issue (being clear about loosing StoreAPI) is fixed then here where we update StoreAPI proto docs.