Additional series limiter for actual series matched

[yeya24]

Is your proposal related to a problem?

Related issue: #6743

Current series limiter checks limits after expanding postings at https://github.com/thanos-io/thanos/blob/main/pkg/store/bucket.go#L1096. This makes total sense because current series limiter limits the series touched. Touched means total series fetched from S3 and cache.

However, for scenario like vertical sharding and lazy postings, the number of series finally matched is usually much less than the number of series touched, because of additional filtering happened when fetching series. https://github.com/thanos-io/thanos/blob/main/pkg/store/bucket.go#L1208

With the existing series limiter that applies for number of series touched, it makes it harder to enable vertical sharding and lazy postings because you might need to set the limit to a very high number in order to not hit the limit.

Describe the solution you'd like

To overcome the issue mentioned above, we can have an additional series limiter that applies limit for series finally matched. This can be enabled via a new flag store.limits.request-matched-series. For lazy postings use case, user can turn on the new flag and disables the old series limit flag.

The issue of the new limit is that the limit will be applied after you fetch some series rather than before fetching any series. So there might be some additional cost.

Describe alternatives you've considered

The alternative would be to change the existing series limiter to apply its limit after series is matched. But this changes existing behavior.

Additional context

(Write your answer here.)

[yeya24]

Would love to get some feedbacks. @fpetkovski @MichaHoffmann @saswatamcode @GiedriusS

[yeya24]

We can also apply series limit after expanding postings if it is not vertical sharding or lazy postings.
If the request is vertical sharding or lazy postings, then we can apply lazily when fetching series

[MichaHoffmann]

More flags that change meaning depending on other flags ( if lazy postings are enabled or not ) seem very hard to reason about. If we configure the limits to protect the store and the lazy postings expansion fetches more series then we should keep the behavior as is probably right?

[yeya24]

Thanks for the reply @MichaHoffmann. I totally agree with you. The existing limiter indeed protects SG from touching too many series.
But if I don't care about series touched, but cares about actual series matched, then another series limiter that applies lazily is more suitable. Because actual series matched will download chunks, which is sometimes more important than series touched.

I agree it is not good to change existing series limiter. Then I think it is probably better to have another series limiter without changing the existing one. It can be hidden if users really have that usecase.

[MichaHoffmann]

Another limiter that protects us from sending too many series might be sensible, but then again; the consumer can just not read the stream at some point; this feels like a setting in querier to me maybe

[yeya24]

Created #6972 to address this.

Note that stores other than store gateway, like sidecar, rule, querier, receiver all apply series limits when streaming series. https://github.com/thanos-io/thanos/blob/main/pkg/store/limiter.go#L171

I think it should also make sense to enable similar behavior in SG and users can choose if they want to.