Thanos store: improve series-sample-limit

[pracucci]

The Thanos store supports the -store.grpc.series-sample-limit CLI flag, whose description is:

Maximum amount of samples returned via a single Series call. 0 means no limit. NOTE: For efficiency we take 120 as the number of samples in chunk (it cannot be bigger than that), so the actual number of samples might be lower, even though the maximum could be hit.

However, looking at the implementation, it doesn't limit the "samples returned via a single Series call" but the samples read from each block. If a single Series() call fetch samples from N blocks, the limit is checked for each block and the actual number of samples returned could be up to N * limit (where N is the number of blocks queried).

I would like to propose to improve it.

Proposal

I propose to internally change SampleLimiter into ChunksLimiter, limiting the number of chunks instead of samples (which is how it's actually implemented, because the number of samples per chunks is just estimated). The ChunksLimiter interface is as follow:

type ChunksLimiter interface {
        // Reserve num chunks out of the total number of chunks.
	Reserve(num uint64) error
}

The Reserve() function increases by num the number of chunks fetched, so that multiple calls to Reserve() (one for each block) will increase the total number of chunks fetched until it will eventually hit the limit. A new limiter is created for each Series() call, so that limits are isolated to individual Series() calls.

The -store.grpc.series-sample-limit flag value (if > 0) will be used to compute the actual number of chunks to limit to, which is maxChunks = (maxSampleCount / maxSamplesPerChunk) + 1.

[bwplotka]

LGTM, this is what we were planning to do anyway. Also one note: Let's make sure we adapt limits on series during posting phase without fetching chunks if not needed (:

[pracucci]

Let's make sure we adapt limits on series during posting phase without fetching chunks if not needed

Postings are expanded on a per-block basis (each block is processed in a dedicated goroutine). I agree we can check the limit while iterating postings, but another goroutine may have already started fetching chunks so there's no guarantee no chunks will be fetched at all when a Series() execution hits the limit.

so there's no guarantee no chunks will be fetched at all

To introduce this guarantee, we would have to split the BucketStore.Series() into two phases:

1. Concurrently expand postings from all blocks (limit checked here)
2. Wait until (1) is done for all blocks and then start to concurrently fetch chunks

This two phases could introduce a performance penalty (because we would reduce concurrency effectiveness) and I'm not sure would be worth just to enforce the limit.

WDYT? Am I missing anything?