Optimize postings fetching by checking postings and series size

[yeya24]

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

Fixes #6416

The idea behind this optimization is trying to fetch less data if possible. Now we always fetch all postings, then intersect and finally fetch series, chunks, etc.

If a matcher is contains a lot of postings, we are trying to download all postings of this matcher. However, this work might be unnecessary because the final number of series matched is bound by highest selectivity matcher in the query. It might be more efficient to only download series and filter, rather than downloading all postings.

So what we could do is to start from min cardinality posting (highest selectivity), download its series and then apply matchers on the series.

If 4 * minPostingCardinality + estimatesSeriesSize * minPostingCardinality < totalPostingCardinality * 4 + estimatesSeriesSize * finalSeriesMatched, then we know it is cheaper to fetch minPostings.

We can know the minPostingCardinality and totalPostingCardinality by checking posting offset table. We use the max series size added in block meta.json as the estimatesSeriesSize.

We don't have a way to know finalSeriesMatched but in worst case we can assume it is 0.

Then we have a basic cost model to decide which way is better.

In the actual implementation, I used similar idea to implement an algorithm which finds the best way to download postings/series based on heuristics.

Verification

Need to add tests and benchmark performance.

[GiedriusS]

We don't have a way to know finalSeriesMatched but in worst case we can assume it is 0.

Isn't that the best case that we don't have to fetch any series? Perhaps the worst case is when we have to fetch all of the series identified by each fetched series reference? But yeah, your formula sounds about right.

[yeya24]

@GiedriusS Yeah you got what I meant. Worst/best case is relative to the two plans.

[yeya24]

I am going to rebase this on top of #6532

[yeya24]

@GiedriusS I updated the pr a little bit more. Still need to add more test cases but it would be really appreciated if you could help review the algorithm first.
The main algorithm of picking postings are in lazy_postings.go file

[alanprot]

Do we need to worry about the matchers now on the merge function?
Ex: cpu_utilization{pod="a.+", pod!="aaabb"} will be converted in one postingGroup but i dont see we are merging those matchers?

thanos/pkg/store/bucket.go

Line 2415 in 266a760

func (pg postingGroup) merge(other *postingGroup) *postingGroup {

[yeya24]

I merged the matchers in https://github.com/thanos-io/thanos/pull/6465/files#diff-3e2896fafa6ff73509c77df2c4389b68828e02575bb4fb78b6c34bcfb922a7ceR2625

[alanprot]

oh ok..

Would it make more sense to add those matchers on the merge function? cause now you have a resulting merged posting group that is incomplete?

[yeya24]

Good catch. I don't know if there is a good way to merge matchers as it requires additional sorting and probably a map to dedup.

27d04c8 I have changed the method to only merge keys

[GiedriusS]

Is this ready for a review?

[yeya24]

@GiedriusS Yes it is ready for review.
I will share some load test number in the pr later

[yeya24]

We were running with lazy expanded postings enabled (this pr) before 14:45, which is the time of the spikes. We can ignore the spikes itself, it is due to my deployment.

So when running with this pr, we consumed slightly more CPU, about 3% ~ 4%. But memory usage is much better: consumed about 2GB max with this pr while main branch consumes 5GB max.
Bandwidth usage is also better, especially on bandwidth received.

Other stats about store gateway.
With this pr, postings size touched is only 1 MBps while main version touched 950 MBps postings, this may vary due to different test data and query but it should fetch less postings.

It touched more series, which is expected. But we just fetch 100 MBps series more. Compared to savings from postings side it is still better.

Query latency improved a little bit. Improved P99 from 3.5s to 2.5s so 1s faster.

[GiedriusS]

A few comments, still haven't finished digging into the algorithm

[GiedriusS]

-4 because the postings length is encoded in the 4 bytes, right? Perhaps worth adding a comment here?

[yeya24]

Added a comment about the 4 bytes

[yeya24]

@MichaHoffmann I also enabled lazy expanded postings in the labels acceptance test.
The test suite helped me find one bug. After fixing that, all tests passed. If you are interested please also help me take a look. Thanks

[yeya24]

Added 2 more metrics.
thanos_bucket_store_lazy_expanded_posting_size_bytes_total is used to count number of bytes of postings we skipped fetching because of the lazy optimization.

thanos_bucket_store_lazy_expanded_posting_series_overfetched_size_bytes_total is used to count number of bytes of series we overfetched because of the lazy optimization.

thanos_bucket_store_lazy_expanded_posting_size_bytes_total - thanos_bucket_store_lazy_expanded_posting_series_overfetched_size_bytes_total should tell how much bytes you saved due to the lazy expansion.

[MichaHoffmann]

lgtm!

[yeya24]

Hi @GiedriusS, maybe you wanna take another look before I merge it?

[GiedriusS]

Great improvement, LGTM. Thank you for the comments in the code explaining what is happening

[GiedriusS]

Perhaps in the future we could use some kind of heuristics to set this automatically 🤔