From 2e2c014d52756a8379381c838b80f84d12f6d91b Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 7 Jun 2022 05:38:27 +0100 Subject: [PATCH] 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 * 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 * 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 --- model/labels/labels.go | 57 ++++++++++---- model/labels/labels_test.go | 151 ++++++++++++++++++++---------------- promql/engine.go | 26 +++---- promql/functions.go | 11 +-- 4 files changed, 135 insertions(+), 110 deletions(-) diff --git a/model/labels/labels.go b/model/labels/labels.go index 29b7385c8..27423d19c 100644 --- a/model/labels/labels.go +++ b/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 { - ret := make([]Label, 0, len(ls)) - +func (ls Labels) BytesWithLabels(buf []byte, names ...string) []byte { + 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 { - ret := make([]Label, 0, len(ls)) - +func (ls Labels) BytesWithoutLabels(buf []byte, names ...string) []byte { + 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]) + } + b.WriteString(ls[i].Name) + b.WriteByte(seps[0]) + b.WriteString(ls[i].Value) } - return ret + 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) } - res = append(res, b.add...) - sort.Sort(res) - + 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 } diff --git a/model/labels/labels_test.go b/model/labels/labels_test.go index da4d80b58..5d5fdd475 100644 --- a/model/labels/labels_test.go +++ b/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) { - require.Equal(t, Labels{{"aaa", "111"}, {"bbb", "222"}}, Labels{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}}.WithLabels("aaa", "bbb")) +func TestLabels_BytesWithLabels(t *testing.T) { + 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) { - require.Equal(t, Labels{{"aaa", "111"}}, Labels{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}}.WithoutLabels("bbb", "ccc")) - require.Equal(t, Labels{{"aaa", "111"}}, Labels{{"aaa", "111"}, {"bbb", "222"}, {MetricName, "333"}}.WithoutLabels("bbb")) +func TestLabels_BytesWithoutLabels(t *testing.T) { + 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"}}.Bytes(nil), Labels{{MetricName, "333"}, {"aaa", "111"}, {"bbb", "222"}}.BytesWithoutLabels(nil, MetricName, "bbb")) } -func TestBulider_NewBulider(t *testing.T) { - require.Equal( - t, - &Builder{ - base: Labels{{"aaa", "111"}}, - del: []string{}, - add: []Label{}, +func TestBuilder(t *testing.T) { + for i, tcase := range []struct { + base Labels + del []string + keep []string + set []Label + want Labels + }{ + { + base: FromStrings("aaa", "111"), + want: FromStrings("aaa", "111"), }, - NewBuilder(Labels{{"aaa", "111"}}), - ) -} - -func TestBuilder_Del(t *testing.T) { - require.Equal( - t, - &Builder{ - del: []string{"bbb"}, - add: []Label{{"aaa", "111"}, {"ccc", "333"}}, - }, - (&Builder{ - del: []string{}, - add: []Label{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}}, - }).Del("bbb"), - ) -} - -func TestBuilder_Set(t *testing.T) { - require.Equal( - t, - &Builder{ - base: Labels{{"aaa", "111"}}, - del: []string{}, - add: []Label{{"bbb", "222"}}, - }, - (&Builder{ - base: Labels{{"aaa", "111"}}, - del: []string{}, - 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"}}, + { + base: FromStrings("aaa", "111", "bbb", "222", "ccc", "333"), del: []string{"bbb"}, - add: []Label{{"ddd", "444"}}, - }).Labels(), - ) + want: FromStrings("aaa", "111", "ccc", "333"), + }, + { + base: nil, + set: []Label{{"aaa", "111"}, {"bbb", "222"}, {"ccc", "333"}}, + del: []string{"bbb"}, + want: FromStrings("aaa", "111", "ccc", "333"), + }, + { + base: FromStrings("aaa", "111"), + set: []Label{{"bbb", "222"}}, + want: FromStrings("aaa", "111", "bbb", "222"), + }, + { + base: FromStrings("aaa", "111"), + set: []Label{{"bbb", "222"}, {"bbb", "333"}}, + want: FromStrings("aaa", "111", "bbb", "333"), + }, + { + base: FromStrings("aaa", "111", "bbb", "222", "ccc", "333"), + del: []string{"bbb"}, + set: []Label{{"ddd", "444"}}, + 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) { diff --git a/promql/engine.go b/promql/engine.go index 8e5fcea95..7711223d6 100644 --- a/promql/engine.go +++ b/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: - for _, l := range lhs { - for _, n := range matching.MatchingLabels { - if l.Name == n { - continue Outer - } - } - enh.lb.Del(l.Name) - } + enh.lb.Keep(matching.MatchingLabels...) } 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 - + lb.Reset(metric) if without { - lb.Reset(metric) 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, diff --git a/promql/functions.go b/promql/functions.go index 0b3a04251..18c8e63a3 100644 --- a/promql/functions.go +++ b/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) - // Add the metric name (which is always removed) to the signature to prevent combining multiple histograms - // 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] + enh.lblBuf = el.Metric.BytesWithoutLabels(enh.lblBuf, labels.BucketLabel) + mb, ok := enh.signatureToMetricWithBuckets[string(enh.lblBuf)] 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}) }