Ignore common label-values in ProxyResponseHeap sort function #6299
Conversation
wallee94
force-pushed
the
change-proxy-response-heap-less-func
branch
from
258fdcd
to
422865b
Compare
Signed-off-by: Walther Lee <walthere.lee@gmail.com>
wallee94
force-pushed
the
change-proxy-response-heap-less-func
branch
from
422865b
to
d75fbe5
Compare
There was a problem hiding this comment.
Thanks for taking a stab at this!
I think a better way would be to exclude external labels from the comparison function since they are the ones that affect the sorting order. Because they are know in advance, we can construct a set of labels once per series request instead of once per comparison. The latter will likely be far more expensive.
@GiedriusS any thoughts on this solution?
|
Is this and #6296 achieving the same outcome, but through different means? 🤔 |
|
I think we need both for this issue |
Signed-off-by: Walther Lee <walthere.lee@gmail.com>
|
Ah I didn't realize the first time that |
pkg/store/proxy_heap_test.go
Outdated
| @@ -22,6 +22,132 @@ func TestRmLabelsCornerCases(t *testing.T) { | |||
| }), labels.Labels{}) | |||
| } | |||
|
|
|||
| type testingRespSet struct { | |||
There was a problem hiding this comment.
Can we reuse the mocks and stubs from the testutil package? You can see an example usage here.
There was a problem hiding this comment.
I could not find an implementation of respSet in testutil, but I changed it to use one from proxy_head.go.
Signed-off-by: Walther Lee <walthere.lee@gmail.com>
Signed-off-by: Walther Lee <walthere.lee@gmail.com>
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
|
Thanks for the changes 👍 I ran the proxy benchmark locally using If we want to completely the impact of ignoring external labels, we could probably add them in a separate field when sending data from stores, but for now we should be okay with a small regression. |
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
…-func Reuse buffers for label comparison
Signed-off-by: Walther Lee <walthere.lee@gmail.com>
|
Oh wow, I didn't expect so much memory regression. The new changes look great to me, I've merged them and the latest changes in main. Thanks again @fpetkovski ! |
Signed-off-by: Walther Lee <walthere.lee@gmail.com>
|
@wallee94 have you checked whether this change solves your problem? |
|
@fpetkovski Yes, I deployed the branch today to our dev cluster. We were seeing duplicated series from the ruler and sidecars, and this change fixes the issue |
In thanos-io#6299 we changed the comparison algorithm in the store proxy to ignore store-specific external labels. This causes problems during long rollouts because old stores and new stores will sort data differently, leading to incorrectly deduplicated results. This commit adds a new flag to the store info message to indicate whether the store sorts data with or without external labels. The querier uses this flag to decide whether to resort the store response set before starting a merge. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
In thanos-io#6299 we changed the comparison algorithm in the store proxy to ignore store-specific external labels. This causes problems during long rollouts because old stores and new stores will sort data differently, leading to incorrectly deduplicated results. This commit adds a new flag to the store info message to indicate whether the store sorts data with or without external labels. The querier uses this flag to decide whether to resort the store response set before starting a merge. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
Changes
Change
ProxyResponseHeap.Lessfunction to compare only the label-values that are not in both series.Some components are sorting the response before adding external labels, returning sets that might not be sorted after that. Because common label-values don't make a difference when comparing two label sets, ignoring these common external labels can help sort the response in the heap again.
I described the bug and how to replicate in #6257 (comment)
Verification
I added a few tests for
ProxyResponseHeapthat I used to replicate the bug and test the changes.