From c74a050a190486addc1ea1ca4b522462fc7ec680 Mon Sep 17 00:00:00 2001
From: Michael Hoffmann <mhoffm@posteo.de>
Date: Sun, 5 Nov 2023 17:09:23 +0100
Subject: [PATCH] Store: fix returned labels on external label conflict when
 skipping chunks (#6874)

Signed-off-by: Michael Hoffmann <mhoffm@posteo.de>
---
 CHANGELOG.md                 |  2 ++
 pkg/store/acceptance_test.go | 36 ++++++++++++++++++++++++++++++------
 pkg/store/prometheus.go      | 12 +++++++-----
 3 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8f5dd2d4f8..3140315ed8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,8 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 
 ### Fixed
 
+- [#6874](https://github.com/thanos-io/thanos/pull/6874) Sidecar: fix labels returned by 'api/v1/series' in presence of conflicting external and inner labels.
+
 ### Added
 
 ### Changed
diff --git a/pkg/store/acceptance_test.go b/pkg/store/acceptance_test.go
index 9b1e8d49be..2498d571f2 100644
--- a/pkg/store/acceptance_test.go
+++ b/pkg/store/acceptance_test.go
@@ -53,9 +53,10 @@ type labelValuesCallCase struct {
 }
 
 type seriesCallCase struct {
-	matchers []storepb.LabelMatcher
-	start    int64
-	end      int64
+	matchers   []storepb.LabelMatcher
+	start      int64
+	end        int64
+	skipChunks bool
 
 	expectedLabels []labels.Labels
 	expectErr      error
@@ -219,6 +220,28 @@ func testStoreAPIsAcceptance(t *testing.T, startStore func(t *testing.T, extLset
 				},
 			},
 		},
+		{
+			desc: "conflicting internal and external labels when skipping chunks",
+			appendFn: func(app storage.Appender) {
+				_, err := app.Append(0, labels.FromStrings("foo", "bar", "region", "somewhere"), 0, 0)
+				testutil.Ok(t, err)
+
+				testutil.Ok(t, app.Commit())
+			},
+			seriesCalls: []seriesCallCase{
+				{
+					start: timestamp.FromTime(minTime),
+					end:   timestamp.FromTime(maxTime),
+					matchers: []storepb.LabelMatcher{
+						{Type: storepb.LabelMatcher_EQ, Name: "foo", Value: "bar"},
+					},
+					skipChunks: true,
+					expectedLabels: []labels.Labels{
+						labels.FromStrings("foo", "bar", "region", "eu-west"),
+					},
+				},
+			},
+		},
 		{
 			// Testcases taken from https://github.com/prometheus/prometheus/blob/95e705612c1d557f1681bd081a841b78f93ee158/tsdb/querier_test.go#L1898
 			desc: "matching behavior",
@@ -701,9 +724,10 @@ func testStoreAPIsAcceptance(t *testing.T, startStore func(t *testing.T, extLset
 				t.Run("series", func(t *testing.T) {
 					srv := newStoreSeriesServer(context.Background())
 					err := store.Series(&storepb.SeriesRequest{
-						MinTime:  c.start,
-						MaxTime:  c.end,
-						Matchers: c.matchers,
+						MinTime:    c.start,
+						MaxTime:    c.end,
+						Matchers:   c.matchers,
+						SkipChunks: c.skipChunks,
 					}, srv)
 					if c.expectErr != nil {
 						testutil.NotOk(t, err)
diff --git a/pkg/store/prometheus.go b/pkg/store/prometheus.go
index f9e836aca9..05af3b5d93 100644
--- a/pkg/store/prometheus.go
+++ b/pkg/store/prometheus.go
@@ -189,15 +189,17 @@ func (p *PrometheusStore) Series(r *storepb.SeriesRequest, seriesSrv storepb.Sto
 		if err != nil {
 			return err
 		}
+		var b labels.Builder
 		for _, lbm := range labelMaps {
-			lset := make([]labelpb.ZLabel, 0, len(lbm)+finalExtLset.Len())
+			b.Reset(labels.EmptyLabels())
 			for k, v := range lbm {
-				lset = append(lset, labelpb.ZLabel{Name: k, Value: v})
+				b.Set(k, v)
 			}
-			lset = append(lset, labelpb.ZLabelsFromPromLabels(finalExtLset)...)
-			sort.Slice(lset, func(i, j int) bool {
-				return lset[i].Name < lset[j].Name
+			// external labels should take precedence
+			finalExtLset.Range(func(l labels.Label) {
+				b.Set(l.Name, l.Value)
 			})
+			lset := labelpb.ZLabelsFromPromLabels(b.Labels())
 			if err = s.Send(storepb.NewSeriesResponse(&storepb.Series{Labels: lset})); err != nil {
 				return err
 			}