Implement dedup-ready label sorting in stores #5796

fpetkovski · 2022-10-17T13:33:27Z

This commit adds functionality to all stores to sort series labels in order appropriate for deduplication. This is done by propagating replica labels from the Querier in the Series request. When all stores send a hint that they support sorting labels, we can rely on the k-way merge in the proxy and avoid an expensive global sort.

For backwards compatibility, each Store will send a hint in the SeriesResponse to indicate that it supports this functionality. The hint is sent at the beginning of the stream, and is the first response that each store will send.

The Store Proxy will inspect the hints from each store and detect cases where a store does not support resorting of labels. In that case it will itself send a hint upstream that sorting is not supported.

Fixes #5719
Closes #5742 #5692

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

Changes

Verification

GiedriusS · 2022-10-18T11:03:53Z

Sorry, finally got around to this. I think this should be implemented differently. Ideally, we are able to tell in advance whether resorting is needed. I think that would make the code cleaner. Plus, to achieve full streaming, I suggest removing the temporary buffering stage and passing the proxystore iterator around to other iterators. Here's some untested code that shows my idea:

fpetkovski#3

What do you think about such an idea?

fpetkovski · 2022-10-18T11:32:12Z

The problem is that we cannot know in advance if a store is able to send data sorted for dedup. Since we stream series one by one from store/tsdb.go and store/prometheus.go (receiver, ruler and sidecar), data from these stores will be sorted according to the TSDB specific sort (according to labels.Compare).

Sorting for dedup depends on replica labels, and if a user sets replica labels that are regular series labels in TSDB, these stores will not be able to send series in order required for dedup. So whether a store has this capability or not depends on which labels are set as replica labels.

Also it seems like it's possible to override replica labels on individual requests, which makes it hard to use store info checks to get this information.

bwplotka

Nice, thanks for this! Looks great, and as we discussed. However, indeed there is quite a cognitive load and fragile core in the hot path (heap) for checking those hint that is only because of compatibility ):

Indeed it would be nice to check alternatives if we have any. Last time we checked with @fpetkovski checking if series are sorted is NOT cheap, (probably almost as expensive as full sort due to hashing), so it's not ideal either.

Ideally we have gRPC versioning for this.... Store API v1.1

pkg/store/bucket.go

pkg/store/proxy.go

pkg/store/storepb/rpc.proto

bwplotka · 2022-10-18T16:34:13Z

Actually I like @GiedriusS idea - we really need to tell WHAT gRPC version it is (if new aspect of GRPC feature is supported)... Adding bool to info is enough indeed, but maybe simply add version of gRPC to info? Something naive like that would help us to determine the feature set of store API 🤔

bwplotka · 2022-10-18T16:37:21Z

The problem is that we cannot know in advance if a store is able to send data sorted for dedup. Since we stream series one by one from store/tsdb.go and store/prometheus.go (receiver, ruler and sidecar), data from these stores will be sorted according to the TSDB specific sort (according to labels.Compare).
Sorting for dedup depends on replica labels, and if a user sets replica labels that are regular series labels in TSDB, these stores will not be able to send series in order required for dedup. So whether a store has this capability or not depends on which labels are set as replica labels.

I don't think we have this problem. We want to add guarantee of Store API that if certain field is set data is sorted differently. So we need only simple information - does certain Store API supports this new guarantee or not?

fpetkovski · 2022-10-19T05:20:14Z

@GiedriusS we spoke with @bwplotka in Slack about adding buffering to the sidecar and receiver when replica labels are not external labels, which is the second solution from this issue: #5719.

In this case we will load all series in memory in these components, sort them without taking replica labels into account, and only then send upstream. This will allow us to use the static checks from your suggestion and avoid the complicated response hints.

When replica labels are external labels (which is the most common use case), we won't need to buffer and we can stream everything as we do now. Wdyt?

fpetkovski · 2022-10-19T07:46:10Z

I pushed a new commit that implements this idea. We can add a server proxy that can decide whether to buffer and re-sort or not. This way we keep all stores as they are and avoid various checks that can be error prone and brittle.

pkg/query/query_bench_test.go

GiedriusS · 2022-10-19T09:15:44Z

pkg/store/sorted_series_server.go

+	})
+}
+
+func sortRequired(sortWithoutLabels map[string]struct{}, extLabelsMap map[string]struct{}) bool {


For us this would mean forced buffering on StoreAPI nodes because we have replica labels set on Thanos Query that exist on some nodes and on some do not :/ thus, we'd be forced to set those external labels on all nodes, to some kind of fake values or something. I wonder how common is a setup like ours. 🤔

You are right, this requires us to use the same replica labels everywhere and somehow force external labels on Store Gateway. I actually thought StoreGW always buffers which is why I hardcoded false for passthrough, but after inspecting the code better I see that it's not the case.

We could add a flag in store-gw to indicate what are the replica labels used in the querier, but that will achieve the same effect as setting external labels.

Good point. In such a case we could check if external labels are present in the series. If not, sortWithoutLabels does not change anything, thus we are safe to pass it forward 🤔 or something along those lines.

Actually we can solve this by looking at external labels on individual blocks. Will try to make this change.

I updated store/bucket to sort only if replica labels are not found in at least one block. I think this should over the use case you described.

Hm on second thought, the replica labels can be removed by compaction. In those cases, they won't be found in external labels and store-gateways will be forced to buffer.

I don't know what would be a good solution for this case. We also might be asking for too much from the system without giving it the necessary information to make good decisions.

Could we say that if no label from sortWithoutLabels is present in extLabelsMap that sort is not required?

Unfortunately that would be also true if a user wants to dedup by a label that is not external, and is present in series inside TSDB.

Ah got it now in connection with your comment #5796 (comment), I honestly also wasn't aware of this use case.

pkg/query/querier.go

GiedriusS

Do benchmarks still show a big improvement?

pkg/query/querier.go

pkg/query/query_bench_test.go

bwplotka · 2022-10-19T09:52:18Z

pkg/store/bucket.go

@@ -1026,6 +1026,7 @@ func (s *BucketStore) Series(req *storepb.SeriesRequest, srv storepb.Store_Serie
 		seriesLimiter    = s.seriesLimiterFactory(s.metrics.queriesDropped.WithLabelValues("series"))
 	)

+	sortedSeriesSrv := newSortedSeriesServer(srv, req.SortWithoutLabelSet(), false)


bwplotka · 2022-10-19T09:58:09Z

pkg/store/sorted_series_server.go

+
+	responses           []*storepb.SeriesResponse
+	sortWithoutLabelSet map[string]struct{}
+	passThrough         bool


I feel we could have better naming than passThrough - perhaps something like reSortOnFlushNeeded?

bwplotka · 2022-10-19T10:02:14Z

pkg/store/sorted_series_server.go

+	})
+}
+
+func sortRequired(sortWithoutLabels map[string]struct{}, extLabelsMap map[string]struct{}) bool {


Good point. In such a case we could check if external labels are present in the series. If not, sortWithoutLabels does not change anything, thus we are safe to pass it forward 🤔 or something along those lines.

fpetkovski · 2022-10-19T14:45:00Z

pkg/store/bucket.go

@@ -1101,7 +1111,7 @@ func (s *BucketStore) Series(req *storepb.SeriesRequest, srv storepb.Store_Serie
 				}

 				mtx.Lock()
-				res = append(res, part)
+				res = append(res, newSortedSeriesSet(part, sortWithoutLabelSet))


We also need to send labels at the end here. Basically every time we want to do a merge sort, replica labels need to be at the end of the individual series.

That merge is happening here:

thanos/pkg/store/bucket.go

Lines 1168 to 1170 in 35a3e3d

// NOTE: We "carefully" assume series and chunks are sorted within each SeriesSet. This should be guaranteed by

// blockSeries method. In worst case deduplication logic won't deduplicate correctly, which will be accounted later.

set := storepb.MergeSeriesSets(res...)

fpetkovski · 2022-10-20T06:29:53Z

Here are some benchmarks

Select with 2 replicas

name                                             old time/op    new time/op    delta
QuerySelect/1000000SeriesWith1Samples/select-8      1.10s ± 6%     0.84s ± 2%  -23.52%  (p=0.008 n=5+5)
QuerySelect/100000SeriesWith100Samples/select-8     110ms ± 1%      96ms ± 3%  -12.78%  (p=0.008 n=5+5)
QuerySelect/1SeriesWith10000000Samples/select-8    19.5ms ± 1%    20.3ms ± 1%   +3.99%  (p=0.008 n=5+5)

name                                             old alloc/op   new alloc/op   delta
QuerySelect/1000000SeriesWith1Samples/select-8      818MB ± 0%     743MB ± 0%   -9.20%  (p=0.008 n=5+5)
QuerySelect/100000SeriesWith100Samples/select-8    82.5MB ± 0%    74.4MB ± 0%   -9.88%  (p=0.008 n=5+5)
QuerySelect/1SeriesWith10000000Samples/select-8    18.7MB ± 0%    18.7MB ± 0%   +0.06%  (p=0.008 n=5+5)

name                                             old allocs/op  new allocs/op  delta
QuerySelect/1000000SeriesWith1Samples/select-8      12.5M ± 0%      9.6M ± 0%  -23.15%  (p=0.008 n=5+5)
QuerySelect/100000SeriesWith100Samples/select-8     1.25M ± 0%     0.96M ± 0%  -23.32%  (p=0.008 n=5+5)
QuerySelect/1SeriesWith10000000Samples/select-8      167k ± 0%      167k ± 0%   +0.11%  (p=0.008 n=5+5)

Select with 5 replicas


name                                             old time/op    new time/op    delta
QuerySelect/1000000SeriesWith1Samples/select-8      1.04s ± 6%     0.86s ± 3%  -16.72%  (p=0.008 n=5+5)
QuerySelect/100000SeriesWith100Samples/select-8     105ms ± 2%      97ms ± 1%   -7.52%  (p=0.008 n=5+5)
QuerySelect/1SeriesWith10000000Samples/select-8    19.1ms ± 1%    19.6ms ± 1%   +2.52%  (p=0.008 n=5+5)

name                                             old alloc/op   new alloc/op   delta
QuerySelect/1000000SeriesWith1Samples/select-8      814MB ± 0%     752MB ± 0%   -7.54%  (p=0.008 n=5+5)
QuerySelect/100000SeriesWith100Samples/select-8    82.0MB ± 0%    75.0MB ± 0%   -8.60%  (p=0.008 n=5+5)
QuerySelect/1SeriesWith10000000Samples/select-8    18.7MB ± 0%    18.7MB ± 0%   +0.10%  (p=0.008 n=5+5)

name                                             old allocs/op  new allocs/op  delta
QuerySelect/1000000SeriesWith1Samples/select-8      12.2M ± 0%      9.2M ± 0%  -24.59%  (p=0.008 n=5+5)
QuerySelect/100000SeriesWith100Samples/select-8     1.22M ± 0%     0.92M ± 0%  -24.55%  (p=0.008 n=5+5)
QuerySelect/1SeriesWith10000000Samples/select-8      167k ± 0%      167k ± 0%   +0.18%  (p=0.016 n=5+4)

fpetkovski · 2022-10-22T09:41:10Z

pkg/info/infopb/rpc.proto

    bool  sends_sorted_series = 4;
+    // TODO(fpetkovski): Remove in v1.0
+    bool  sends_sorted_series_without_labels = 5;


Adding a new field here because sends_sorted_series does not mean the same as sending series sorted without certain labels.

fpetkovski · 2022-10-24T07:49:45Z

One thing I realized is that the whole complexity comes from the fact that users are allowed to dedup by a label that is in TSDB and is not necessarily an external label. I wonder if this is even a valid use case, and how many people need to do something like this. Dropping support for this might break some users, but maybe we can add a flag in the querier query.dedup-label-in-tsdb or something similar which can be propagated to stores to tell them to resort the data.

matej-g · 2022-10-24T09:44:45Z

pkg/query/querier_test.go

+	clientsFunc := func() []store.Client {
+		clients := make([]store.Client, 0, len(storeAPI))
+		for _, s := range storeAPI {
+			clients = append(clients, receive.NewLocalClient(storepb.ServerAsClient(s, 0), storesWithSortedSeries, labelsFunc, timeFunc))


Would it be possible to expand inProcessClient to use in here instead or receive client?

What is the difference between those two? Should we maybe converge to a single implementation?

I think inProcessClient is specifically for testing purposes

matej-g · 2022-10-24T13:43:56Z

pkg/store/sorted_series_server.go

+	})
+}
+
+func sortRequired(sortWithoutLabels map[string]struct{}, extLabelsMap map[string]struct{}) bool {


Could we say that if no label from sortWithoutLabels is present in extLabelsMap that sort is not required?

This commit adds functionality to all stores to sort series labels in order appropriate for deduplication. This is done by propagating replica labels from the Querier in the Series request. When all stores send a hint that they support sorting labels, we can rely on the k-way merge in the proxy and avoid an expensive global sort. For backwards compatibility, each Store will send a hint in the SeriesResponse to indicate that it supports this functionality. The hint is sent at the beginning of the stream, and is the first response that each store will send. The Store Proxy will inspect the hints from each store and detect cases where a store does not support resorting of labels. In that case it will itself send a hint upstream that sorting is not supported. Signed-off-by: Filip Petkovski <[email protected]>

bwplotka · 2022-10-25T21:29:15Z

So first of all, we have a bug in the PromQL engine, so had to ensure this PR is correct: #5820

Second is - I think I would be happy to fail or not sort for store API implementation that "label" to sort without is not an external label but actually inside e.g. block. so - for this example:

But in either case we have to check actively sorting, so:

If we want to fail, sidecar/receive/store etc has to actively check if label passed in request is part of replica label or it exists in block etc.
If we want to not sort, querier has to do check if receive sorted stream.

Also - if we don't pass any label and there ARE replica labels for e.g. prometheus+sidecar or receive etc - we return unsorted series, right? so if we don't want to buffer/sort on leafs - we can't really guarantee sorted results on Store API? -- so, is you PR works with no replica labels passed on querier?

Anyway, sorry for back-forth, but it's a big thing spanning multiple components and it's very easy to break other people setups.

bwplotka · 2022-10-25T21:32:38Z

Now I am leaning more into hinting sorting and checking this on querier - sounds like easier way... 🤔

I am preparing for new prompl demo, but will look at this after Wed.

stale · 2023-01-07T21:35:15Z

Hello 👋 Looks like there was no activity on this amazing PR for the last 30 days.
Do you mind updating us on the status? Is there anything we can help with? If you plan to still work on it, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next week, this issue will be closed (we can always reopen a PR if you get back to this!). Alternatively, use remind command if you wish to be reminded at some point in future.

GiedriusS · 2023-02-20T14:18:17Z

I think we can close this since an alternative implementation was merged for solving this problem? 😄

pull-request-size bot added the size/XXL label Oct 17, 2022

fpetkovski force-pushed the store-label-sort branch 3 times, most recently from ae79069 to 6c8259f Compare October 17, 2022 13:45

fpetkovski requested review from bwplotka, GiedriusS and yeya24 and removed request for GiedriusS October 17, 2022 13:45

fpetkovski force-pushed the store-label-sort branch 2 times, most recently from 118007d to 6c8df84 Compare October 18, 2022 05:49

bwplotka reviewed Oct 18, 2022

View reviewed changes

pull-request-size bot added size/XL and removed size/XXL labels Oct 19, 2022

fpetkovski force-pushed the store-label-sort branch 3 times, most recently from 64650f4 to 98775eb Compare October 19, 2022 07:46

fpetkovski force-pushed the store-label-sort branch 7 times, most recently from 1e7ed58 to 5644be4 Compare October 19, 2022 08:28

fpetkovski force-pushed the store-label-sort branch from 5644be4 to 132365f Compare October 19, 2022 08:54

GiedriusS reviewed Oct 19, 2022

View reviewed changes

pkg/query/querier.go Show resolved Hide resolved

pkg/query/query_bench_test.go Outdated Show resolved Hide resolved

fpetkovski force-pushed the store-label-sort branch from 132365f to 6da682d Compare October 19, 2022 11:02

bwplotka reviewed Oct 19, 2022

View reviewed changes

fpetkovski force-pushed the store-label-sort branch from 6da682d to f0a63f2 Compare October 19, 2022 14:37

fpetkovski commented Oct 19, 2022

View reviewed changes

fpetkovski force-pushed the store-label-sort branch 2 times, most recently from c6d62db to 419d323 Compare October 20, 2022 08:45

pull-request-size bot added size/XXL and removed size/XL labels Oct 22, 2022

fpetkovski force-pushed the store-label-sort branch 2 times, most recently from c09f0fa to f224770 Compare October 22, 2022 09:40

fpetkovski commented Oct 22, 2022

View reviewed changes

fpetkovski force-pushed the store-label-sort branch 2 times, most recently from 069f63d to 07bf562 Compare October 22, 2022 09:43

matej-g reviewed Oct 24, 2022

View reviewed changes

fpetkovski force-pushed the store-label-sort branch from 07bf562 to 03409b7 Compare October 25, 2022 14:17

fpetkovski mentioned this pull request Oct 28, 2022

Implement projection pushdown #5832

Open

bwplotka mentioned this pull request Oct 31, 2022

remote write: When ingesting, check that labels are indeed sorted lexicographically prometheus/prometheus#11505

Open

stale bot added the stale label Jan 7, 2023

GiedriusS closed this Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement dedup-ready label sorting in stores #5796

Implement dedup-ready label sorting in stores #5796

fpetkovski commented Oct 17, 2022 •

edited

Loading

GiedriusS commented Oct 18, 2022

fpetkovski commented Oct 18, 2022 •

edited

Loading

bwplotka left a comment

bwplotka commented Oct 18, 2022

bwplotka commented Oct 18, 2022

fpetkovski commented Oct 19, 2022

fpetkovski commented Oct 19, 2022 •

edited

Loading

GiedriusS Oct 19, 2022

fpetkovski Oct 19, 2022 •

edited

Loading

bwplotka Oct 19, 2022

fpetkovski Oct 19, 2022

fpetkovski Oct 19, 2022

fpetkovski Oct 21, 2022

matej-g Oct 24, 2022

fpetkovski Oct 24, 2022

matej-g Oct 24, 2022

GiedriusS left a comment

bwplotka Oct 19, 2022

bwplotka Oct 19, 2022

bwplotka Oct 19, 2022

fpetkovski Oct 19, 2022 •

edited

Loading

fpetkovski commented Oct 20, 2022

fpetkovski Oct 22, 2022

fpetkovski commented Oct 24, 2022

matej-g Oct 24, 2022

fpetkovski Oct 25, 2022

matej-g Oct 25, 2022

matej-g Oct 24, 2022

bwplotka commented Oct 25, 2022

bwplotka commented Oct 25, 2022 •

edited

Loading

stale bot commented Jan 7, 2023

GiedriusS commented Feb 20, 2023

	// NOTE: We "carefully" assume series and chunks are sorted within each SeriesSet. This should be guaranteed by
	// blockSeries method. In worst case deduplication logic won't deduplicate correctly, which will be accounted later.
	set := storepb.MergeSeriesSets(res...)

Implement dedup-ready label sorting in stores #5796

Implement dedup-ready label sorting in stores #5796

Conversation

fpetkovski commented Oct 17, 2022 • edited Loading

Changes

Verification

GiedriusS commented Oct 18, 2022

fpetkovski commented Oct 18, 2022 • edited Loading

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Oct 18, 2022

bwplotka commented Oct 18, 2022

fpetkovski commented Oct 19, 2022

fpetkovski commented Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

fpetkovski Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpetkovski Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

fpetkovski commented Oct 20, 2022

Choose a reason for hiding this comment

fpetkovski commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Oct 25, 2022

bwplotka commented Oct 25, 2022 • edited Loading

stale bot commented Jan 7, 2023

GiedriusS commented Feb 20, 2023

fpetkovski commented Oct 17, 2022 •

edited

Loading

fpetkovski commented Oct 18, 2022 •

edited

Loading

fpetkovski commented Oct 19, 2022 •

edited

Loading

fpetkovski Oct 19, 2022 •

edited

Loading

fpetkovski Oct 19, 2022 •

edited

Loading

bwplotka commented Oct 25, 2022 •

edited

Loading