Labels: optimise creation of signature with/without labels (#10667)

* Labels: create signature with/without labels

Instead of creating a new Labels slice then converting to signature,
go directly to the signature and save time.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

* Labels: refactor Builder tests

Have one test with a range of cases, and have them check the final
output rather than checking the internal structure of the Builder.

Also add a couple of cases where the value is "", which should be
interpreted as 'delete'.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

* Labels: add 'Keep' function to Builder

This lets us replace `Labels.WithLabels` with the more general `Builder`.

In `engine.resultMetric()` we can call `Keep()` instead of checking
and calling `Del()`.

Avoid calling `Sort()` in `Builder.Labels()` if we didn't add anything,
so that `Keep()` has the same performance as `WithLabels()`.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

model/labels/labels.go

@@ -202,11 +202,11 @@ func (ls Labels) HashWithoutLabels(b []byte, names ...string) (uint64, []byte) {
 	return xxhash.Sum64(b), b
 }
 
-// WithLabels returns a new labels.Labels from ls that only contains labels matching names.
+// BytesWithLabels is just as Bytes(), but only for labels matching names.
 // 'names' have to be sorted in ascending order.
-func (ls Labels) WithLabels(names ...string) Labels {
+func (ls Labels) BytesWithLabels(buf []byte, names ...string) []byte {
-	ret := make([]Label, 0, len(ls))
+	b := bytes.NewBuffer(buf[:0])
-
+	b.WriteByte(labelSep)
 	i, j := 0, 0
 	for i < len(ls) && j < len(names) {
 		if names[j] < ls[i].Name {
@@ -214,30 +214,40 @@ func (ls Labels) WithLabels(names ...string) Labels {
 		} else if ls[i].Name < names[j] {
 			i++
 		} else {
-			ret = append(ret, ls[i])
+			if b.Len() > 1 {
+				b.WriteByte(seps[0])
+			}
+			b.WriteString(ls[i].Name)
+			b.WriteByte(seps[0])
+			b.WriteString(ls[i].Value)
 			i++
 			j++
 		}
 	}
-	return ret
+	return b.Bytes()
 }
 
-// WithoutLabels returns a new labels.Labels from ls that contains labels not matching names.
+// BytesWithoutLabels is just as Bytes(), but only for labels not matching names.
 // 'names' have to be sorted in ascending order.
-func (ls Labels) WithoutLabels(names ...string) Labels {
+func (ls Labels) BytesWithoutLabels(buf []byte, names ...string) []byte {
-	ret := make([]Label, 0, len(ls))
+	b := bytes.NewBuffer(buf[:0])
-
+	b.WriteByte(labelSep)
 	j := 0
 	for i := range ls {
 		for j < len(names) && names[j] < ls[i].Name {
 			j++
 		}
-		if ls[i].Name == MetricName || (j < len(names) && ls[i].Name == names[j]) {
+		if j < len(names) && ls[i].Name == names[j] {
 			continue
 		}
-		ret = append(ret, ls[i])
+		if b.Len() > 1 {
+			b.WriteByte(seps[0])
 		}
-	return ret
+		b.WriteString(ls[i].Name)
+		b.WriteByte(seps[0])
+		b.WriteString(ls[i].Value)
+	}
+	return b.Bytes()
 }
 
 // Copy returns a copy of the labels.
@@ -426,6 +436,20 @@ func (b *Builder) Del(ns ...string) *Builder {
 	return b
 }
 
+// Keep removes all labels from the base except those with the given names.
+func (b *Builder) Keep(ns ...string) *Builder {
+Outer:
+	for _, l := range b.base {
+		for _, n := range ns {
+			if l.Name == n {
+				continue Outer
+			}
+		}
+		b.del = append(b.del, l.Name)
+	}
+	return b
+}
+
 // Set the name/value pair as a label.
 func (b *Builder) Set(n, v string) *Builder {
 	if v == "" {
@@ -467,8 +491,9 @@ Outer:
 		}
 		res = append(res, l)
 	}
+	if len(b.add) > 0 { // Base is already in order, so we only need to sort if we add to it.
 		res = append(res, b.add...)
 		sort.Sort(res)
-
+	}
 	return res
 }

model/labels/labels_test.go

@@ -624,81 +624,94 @@ func TestLabels_Map(t *testing.T) {
 	require.Equal(t, map[string]string{"aaa": "111", "bbb": "222"}, Labels{{"aaa", "111"}, {"bbb", "222"}}.Map())
 }
 
-func TestLabels_WithLabels(t *testing.T) {
+func TestLabels_BytesWithLabels(t *testing.T) {
-	require.Equal(t, Labels{{"aaa", "111"}, {"bbb", "222"}}, Labels{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}}.WithLabels("aaa", "bbb"))
+	require.Equal(t, Labels{{"aaa", "111"}, {"bbb", "222"}}.Bytes(nil), Labels{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}}.BytesWithLabels(nil, "aaa", "bbb"))
+	require.Equal(t, Labels{}.Bytes(nil), Labels{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}}.BytesWithLabels(nil))
 }
 
-func TestLabels_WithoutLabels(t *testing.T) {
+func TestLabels_BytesWithoutLabels(t *testing.T) {
-	require.Equal(t, Labels{{"aaa", "111"}}, Labels{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}}.WithoutLabels("bbb", "ccc"))
+	require.Equal(t, Labels{{"aaa", "111"}}.Bytes(nil), Labels{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}}.BytesWithoutLabels(nil, "bbb", "ccc"))
-	require.Equal(t, Labels{{"aaa", "111"}}, Labels{{"aaa", "111"}, {"bbb", "222"}, {MetricName, "333"}}.WithoutLabels("bbb"))
+	require.Equal(t, Labels{{"aaa", "111"}}.Bytes(nil), Labels{{MetricName, "333"}, {"aaa", "111"}, {"bbb", "222"}}.BytesWithoutLabels(nil, MetricName, "bbb"))
 }
 
-func TestBulider_NewBulider(t *testing.T) {
+func TestBuilder(t *testing.T) {
-	require.Equal(
+	for i, tcase := range []struct {
-		t,
+		base Labels
-		&Builder{
+		del  []string
-			base: Labels{{"aaa", "111"}},
+		keep []string
-			del:  []string{},
+		set  []Label
-			add:  []Label{},
+		want Labels
+	}{
+		{
+			base: FromStrings("aaa", "111"),
+			want: FromStrings("aaa", "111"),
 		},
-		NewBuilder(Labels{{"aaa", "111"}}),
+		{
-	)
+			base: FromStrings("aaa", "111", "bbb", "222", "ccc", "333"),
-}
-
-func TestBuilder_Del(t *testing.T) {
-	require.Equal(
-		t,
-		&Builder{
 			del:  []string{"bbb"},
-			add: []Label{{"aaa", "111"}, {"ccc", "333"}},
+			want: FromStrings("aaa", "111", "ccc", "333"),
 		},
-		(&Builder{
+		{
-			del: []string{},
+			base: nil,
-			add: []Label{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}},
+			set:  []Label{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}},
-		}).Del("bbb"),
+			del:  []string{"bbb"},
-	)
+			want: FromStrings("aaa", "111", "ccc", "333"),
-}
+		},
-
+		{
-func TestBuilder_Set(t *testing.T) {
+			base: FromStrings("aaa", "111"),
-	require.Equal(
+			set:  []Label{{"bbb", "222"}},
-		t,
+			want: FromStrings("aaa", "111", "bbb", "222"),
-		&Builder{
+		},
-			base: Labels{{"aaa", "111"}},
+		{
-			del:  []string{},
+			base: FromStrings("aaa", "111"),
-			add:  []Label{{"bbb", "222"}},
+			set:  []Label{{"bbb", "222"}, {"bbb", "333"}},
-		},
+			want: FromStrings("aaa", "111", "bbb", "333"),
-		(&Builder{
+		},
-			base: Labels{{"aaa", "111"}},
+		{
-			del:  []string{},
+			base: FromStrings("aaa", "111", "bbb", "222", "ccc", "333"),
-			add:  []Label{},
-		}).Set("bbb", "222"),
-	)
-
-	require.Equal(
-		t,
-		&Builder{
-			base: Labels{{"aaa", "111"}},
-			del:  []string{},
-			add:  []Label{{"bbb", "333"}},
-		},
-		(&Builder{
-			base: Labels{{"aaa", "111"}},
-			del:  []string{},
-			add:  []Label{{"bbb", "222"}},
-		}).Set("bbb", "333"),
-	)
-}
-
-func TestBuilder_Labels(t *testing.T) {
-	require.Equal(
-		t,
-		Labels{{"aaa", "111"}, {"ccc", "333"}, {"ddd", "444"}},
-		(&Builder{
-			base: Labels{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}},
 			del:  []string{"bbb"},
-			add:  []Label{{"ddd", "444"}},
+			set:  []Label{{"ddd", "444"}},
-		}).Labels(),
+			want: FromStrings("aaa", "111", "ccc", "333", "ddd", "444"),
-	)
+		},
+		{ // Blank value is interpreted as delete.
+			base: FromStrings("aaa", "111", "bbb", "", "ccc", "333"),
+			want: FromStrings("aaa", "111", "ccc", "333"),
+		},
+		{
+			base: FromStrings("aaa", "111", "bbb", "222", "ccc", "333"),
+			set:  []Label{{"bbb", ""}},
+			want: FromStrings("aaa", "111", "ccc", "333"),
+		},
+		{
+			base: FromStrings("aaa", "111", "bbb", "222", "ccc", "333"),
+			keep: []string{"bbb"},
+			want: FromStrings("bbb", "222"),
+		},
+		{
+			base: FromStrings("aaa", "111", "bbb", "222", "ccc", "333"),
+			keep: []string{"aaa", "ccc"},
+			want: FromStrings("aaa", "111", "ccc", "333"),
+		},
+		{
+			base: FromStrings("aaa", "111", "bbb", "222", "ccc", "333"),
+			del:  []string{"bbb"},
+			set:  []Label{{"ddd", "444"}},
+			keep: []string{"aaa", "ddd"},
+			want: FromStrings("aaa", "111", "ddd", "444"),
+		},
+	} {
+		t.Run(fmt.Sprint(i), func(t *testing.T) {
+			b := NewBuilder(tcase.base)
+			for _, lbl := range tcase.set {
+				b.Set(lbl.Name, lbl.Value)
+			}
+			if len(tcase.keep) > 0 {
+				b.Keep(tcase.keep...)
+			}
+			b.Del(tcase.del...)
+			require.Equal(t, tcase.want, b.Labels())
+		})
+	}
 }
 
 func TestLabels_Hash(t *testing.T) {

promql/engine.go

@@ -2050,14 +2050,16 @@ func (ev *evaluator) VectorBinop(op parser.ItemType, lhs, rhs Vector, matching *
 }
 
 func signatureFunc(on bool, b []byte, names ...string) func(labels.Labels) string {
-	sort.Strings(names)
 	if on {
+		sort.Strings(names)
 		return func(lset labels.Labels) string {
-			return string(lset.WithLabels(names...).Bytes(b))
+			return string(lset.BytesWithLabels(b, names...))
 		}
 	}
+	names = append([]string{labels.MetricName}, names...)
+	sort.Strings(names)
 	return func(lset labels.Labels) string {
-		return string(lset.WithoutLabels(names...).Bytes(b))
+		return string(lset.BytesWithoutLabels(b, names...))
 	}
 }
 
@@ -2092,15 +2094,7 @@ func resultMetric(lhs, rhs labels.Labels, op parser.ItemType, matching *parser.V
 
 	if matching.Card == parser.CardOneToOne {
 		if matching.On {
-		Outer:
+			enh.lb.Keep(matching.MatchingLabels...)
-			for _, l := range lhs {
-				for _, n := range matching.MatchingLabels {
-					if l.Name == n {
-						continue Outer
-					}
-				}
-				enh.lb.Del(l.Name)
-			}
 		} else {
 			enh.lb.Del(matching.MatchingLabels...)
 		}
@@ -2295,16 +2289,14 @@ func (ev *evaluator) aggregation(op parser.ItemType, grouping []string, without
 		group, ok := result[groupingKey]
 		// Add a new group if it doesn't exist.
 		if !ok {
-			var m labels.Labels
-
-			if without {
 			lb.Reset(metric)
+			if without {
 				lb.Del(grouping...)
 				lb.Del(labels.MetricName)
-				m = lb.Labels()
 			} else {
-				m = metric.WithLabels(grouping...)
+				lb.Keep(grouping...)
 			}
+			m := lb.Labels()
 			newAgg := &groupedAggregation{
 				labels:     m,
 				value:      s.V,

promql/functions.go

@@ -791,7 +791,6 @@ func funcPredictLinear(vals []parser.Value, args parser.Expressions, enh *EvalNo
 func funcHistogramQuantile(vals []parser.Value, args parser.Expressions, enh *EvalNodeHelper) Vector {
 	q := vals[0].(Vector)[0].V
 	inVec := vals[1].(Vector)
-	sigf := signatureFunc(false, enh.lblBuf, labels.BucketLabel)
 
 	if enh.signatureToMetricWithBuckets == nil {
 		enh.signatureToMetricWithBuckets = map[string]*metricWithBuckets{}
@@ -809,19 +808,15 @@ func funcHistogramQuantile(vals []parser.Value, args parser.Expressions, enh *Ev
 			// TODO(beorn7): Issue a warning somehow.
 			continue
 		}
-		l := sigf(el.Metric)
+		enh.lblBuf = el.Metric.BytesWithoutLabels(enh.lblBuf, labels.BucketLabel)
-		// Add the metric name (which is always removed) to the signature to prevent combining multiple histograms
+		mb, ok := enh.signatureToMetricWithBuckets[string(enh.lblBuf)]
-		// with the same label set. See https://github.com/prometheus/prometheus/issues/9910
-		l = l + el.Metric.Get(model.MetricNameLabel)
-
-		mb, ok := enh.signatureToMetricWithBuckets[l]
 		if !ok {
 			el.Metric = labels.NewBuilder(el.Metric).
 				Del(excludedLabels...).
 				Labels()
 
 			mb = &metricWithBuckets{el.Metric, nil}
-			enh.signatureToMetricWithBuckets[l] = mb
+			enh.signatureToMetricWithBuckets[string(enh.lblBuf)] = mb
 		}
 		mb.buckets = append(mb.buckets, bucket{upperBound, el.V})
 	}