Store: store responses should always be sorted

[MichaHoffmann]

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

Changes

• always resort store responses
• compare labelsets without dropping replica labels in proxy heap

Verification

Not verified yet

[vCra]

Tested this out in our test environment:

After deployment number of 422s and 503s disappear completely

And we no longer see any of the Error executing query: vector cannot contain metrics with the same labelset issues seen in #6677

[tekicode]

Thanks for your effort on this PR @MichaHoffmann I'm running this in our testing environment as well and the labelset error I reported in #6677 is resolved

[saswatamcode]

LGTM, seems like this also preserves the optimization

@fpetkovski @GiedriusS maybe we can merge and cut a patch?

[GiedriusS]

Thanks for working on this but I think that this is a wrong approach to take because it just papers over the problem namely that StoreAPIs don't uphold the invariant of sending a sorted response set. I think that for project's long term health we should focus our efforts toward fixing that.

I think this is related: prometheus/prometheus#12605. This is because we actually implement a very similar logic regards external labels.

Essentially, I think that external labels functionality breaks this invariant when the internal TSDB has overlapping labels with different values. Everywhere we use this logic:

• Select() or, in other words, get a sorted series set
• Overwrite labels with the external labels

But if the 1st operation has a mix of time series with and without labels that are part of external labels, problems might ensue. Because time series with less labels come before those with more labels, it means that the end result might be unsorted if the first series in the result do not have external labels in the TSDB and the latter ones do.

I'm a bit busy but I could try writing a test for this case in all components to show what I'm talking about. I'll try to do that over the next few working days.

Also, I think it's correct to only compare raw labels and the previous change regarding comparing external labels first tries to workaround the same problem that I mentioned here

[MichaHoffmann]

Just to state: I didnt intend this as the solution to the problem; more as a short term ad-hoc fix so we can uphold the invariant and solve the problem properly in subsequent PRs.

[GiedriusS]

It's all good, no worries. I was thinking about this and perhaps this problem could be temporarily fixed by forcing resorting if there's an intersection between the external labelset and the label names filter? This would allow us to check if such a possibility is even possible before starting to read the series.

[MichaHoffmann]

It's all good, no worries. I was thinking about this and perhaps this problem could be temporarily fixed by forcing resorting if there's an intersection between the external labelset and the label names filter? This would allow us to check if such a possibility is even possible before starting to read the series.

If we get respSet A and B we would need to check if labelnames of store B intersect extlabels of A and vice versa; in which case we would need to resort i think. But that seems way more complicated then forcing sort on store APIs for first iteration

[douglascamata]

Could we commit to merging this as a hotfix and work on improvements or a "proper fix" in follow ups? This dedup issue is plaguing Thanos releases since 6 months, blocking many people out there from upgrading to take advantage of new features because of uncertainty to whether there will be issues or not with queries. Nobody will care if the memory improvements introduced by the big query sorting change are completely negated for them to be able to always have correct query results and the ability to upgrade their Thanos. Query dedup logic is clearly broken and it has to be fixed. Query accuracy is a pilar of any observability software.

@MichaHoffmann is putting a lot of hard effort and time into this working on fixes, automated tests, testing things in general, etc. I'm sure he has many plans or ideas to improve this further down the line. Others could and are very welcome to help too.

But let's get this working first, shall we? We gotta move forward and, most importantly, move over this bug.

[GiedriusS]

We've moved this discussion a bit into #prometheus-dev. It seems like in most cases it is not safe to extend a sorted series set with external labels because it might lead an unsorted series set. Maybe someone will have some ideas there. In general it looks like external labels is a broken functionality. Anyway... if nothing comes up, we will probably be forced to revert back buffering and resorting in the querier :'/

[fpetkovski]

Isn't it better to resort in the store instead of centralizing a compute intensive operation in a single component?

[MichaHoffmann]

Isn't it better to resort in the store instead of centralizing a compute intensive operation in a single component?

The worry is that sidecar gets expensive i think. We could introduce a slight expansion of the protocol where we generally assume that store responses are sorted except if we get a hint that its not ( and sidecar would send that hint maybe )

[fpetkovski]

It would be great if we can leave this out to the user to decide. Our performance will drop significantly if we had to sort in the querier again.

[MichaHoffmann]

It would be great if we can leave this out to the user to decide. Our performance will drop significantly if we had to sort in the querier again.

I added a sortingStrategy thats currently hard coded in the different stores. I would like to move making it configurable ( to opt-out or to sort in the querier if wanted ) into a followup PR though since strictly speaking its an optimziation and this is already getting kinda big

[GiedriusS]

Adding test on Prometheus side to show the issue: prometheus/prometheus#12822.

[fpetkovski]

Do we want a sortingStrategyQuery?

[MichaHoffmann]

yeah i wanted to add it in followup pr

[fpetkovski]

Hm, does it mean we will resort again in the querier? Maybe we want the strategy to be set in the querier and propagated to the store. This way we can decide in both places whether to resort or not.

[MichaHoffmann]

Querier should use lazyresponse set always right?

[fpetkovski]

As I understand it, if sorting strategy querier, then we need to use eager in the querier and stream from the store. Not sure if that gets added as a follow up though.

[MichaHoffmann]

Ah yeah but if we use querier sorting strategy we should resort here i think; otherwise we could use sorting strategy none right?

[fpetkovski]

Yes, so:

• sorting strategy querier = eager in querier, lazy in stores.
• sorting strategy store = eager in store, lazy in querier.

[mhoffm-aiven]

yeah precisely ( but i would like to add querier later since its more complicated change )

[GiedriusS]

Agreed, we will need this but maybe for a separate PR.

[MichaHoffmann]

@GiedriusS is this PR acceptable?

[GiedriusS]

Let's also remove the paragraph https://thanos.io/tip/components/query.md/#deduplication-on-non-external-labels from here regarding cuckoo filter

[GiedriusS]

Why doesn't this need resorting? 🤔

[MichaHoffmann]

block responses are sorted because of eager retrieval there; basically we put sorted stuff in dedup proxy heap and get sorted series set back out and dont need to sort it again before returning it

[MichaHoffmann]

all the dropping and extlabel juggling is done on block level already so after we joined it we can just pass it along

[fpetkovski]

Given all the edge cases we have ran into so far, I don't see other options than to eventually resort the dataset.

[saswatamcode]

LGTM!