execution: fix label handling in histogram_quantile (#336)

Signed-off-by: Michael Hoffmann <[email protected]>
thanos-io · Nov 29, 2023 · 191359d · 191359d
1 parent f3dcefe
commit 191359d
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 30 deletions.
diff --git a/engine/engine_test.go b/engine/engine_test.go
@@ -3010,6 +3010,13 @@ min without () (
 			queryTime: time.Unix(0, 0),
 			query:     "sort_desc(http_requests_total)",
 		},
+		{
+			name: "histogram_quantile with mock duplicate labels",
+			load: `load 30s
+               http_requests_total{pod="nginx-2", route="/"}  0+0.14x40`,
+			queryTime: time.Unix(600, 0),
+			query:     `histogram_quantile(10, -http_requests_total or http_requests_total)`,
+		},
 		{
 			name: "quantile by pod",
 			load: `load 30s

diff --git a/execution/binary/vector.go b/execution/binary/vector.go
@@ -98,9 +98,9 @@ func (o *vectorOperator) Analyze() (model.OperatorTelemetry, []model.ObservableV
 
 func (o *vectorOperator) Explain() (me string, next []model.VectorOperator) {
 	if o.matching.On {
-		return fmt.Sprintf("[*vectorOperator] %s %v on %v group %v", parser.ItemTypeStr[o.opType], o.matching.Card.String(), o.matching.MatchingLabels, o.matching.Include), []model.VectorOperator{o.lhs, o.rhs}
+		return fmt.Sprintf("[*vectorOperator] %s - %v, on: %v, group: %v", parser.ItemTypeStr[o.opType], o.matching.Card.String(), o.matching.MatchingLabels, o.matching.Include), []model.VectorOperator{o.lhs, o.rhs}
 	}
-	return fmt.Sprintf("[*vectorOperator] %s %v ignoring %v group %v", parser.ItemTypeStr[o.opType], o.matching.Card.String(), o.matching.On, o.matching.Include), []model.VectorOperator{o.lhs, o.rhs}
+	return fmt.Sprintf("[*vectorOperator] %s - %v, ignoring: %v, group: %v", parser.ItemTypeStr[o.opType], o.matching.Card.String(), o.matching.On, o.matching.Include), []model.VectorOperator{o.lhs, o.rhs}
 }
 
 func (o *vectorOperator) Series(ctx context.Context) ([]labels.Labels, error) {

diff --git a/execution/function/histogram.go b/execution/function/histogram.go
@@ -184,9 +184,6 @@ func (o *histogramOperator) loadSeries(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
-	if extlabels.ContainsDuplicateLabelSetAfterDroppingName(series) {
-		return extlabels.ErrDuplicateLabelSet
-	}
 
 	var (
 		hashBuf      = make([]byte, 0, 256)
@@ -205,13 +202,17 @@ func (o *histogramOperator) loadSeries(ctx context.Context) error {
 			hasBucketValue = false
 		}
 
-		lbls, _ = extlabels.DropMetricName(lbls, b)
 		hasher.Reset()
 		hashBuf = lbls.Bytes(hashBuf)
 		if _, err := hasher.Write(hashBuf); err != nil {
 			return err
 		}
 
+		// We check for duplicate series after dropped labels when
+		// showing the result of the query. Series that are equal after
+		// dropping name should not hash to the same bucket here.
+		lbls, _ = extlabels.DropMetricName(lbls, b)
+
 		seriesHash := hasher.Sum64()
 		seriesID, ok := seriesHashes[seriesHash]
 		if !ok {

diff --git a/extlabels/labels.go b/extlabels/labels.go
@@ -4,7 +4,6 @@
 package extlabels
 
 import (
-	"github.com/cespare/xxhash/v2"
 	"github.com/efficientgo/core/errors"
 	"github.com/prometheus/prometheus/model/labels"
 )
@@ -13,29 +12,6 @@ var (
 	ErrDuplicateLabelSet = errors.New("vector cannot contain metrics with the same labelset")
 )
 
-func ContainsDuplicateLabelSetAfterDroppingName(series []labels.Labels) bool {
-	var (
-		buf  = make([]byte, 0, 256)
-		seen = make(map[uint64]struct{}, len(series))
-	)
-
-	b := labels.ScratchBuilder{}
-	for _, s := range series {
-		b.Reset()
-		buf = buf[:0]
-
-		lbls, _ := DropMetricName(s, b)
-		buf = lbls.Bytes(buf)
-
-		h := xxhash.Sum64(lbls.Bytes(buf))
-		if _, ok := seen[h]; ok {
-			return true
-		}
-		seen[h] = struct{}{}
-	}
-	return false
-}
-
 // DropMetricName removes the __name__ label and returns the dropped name and remaining labels.
 func DropMetricName(l labels.Labels, b labels.ScratchBuilder) (labels.Labels, labels.Label) {
 	return DropLabel(l, labels.MetricName, b)