From 645cf57bed9f9edb52d6ee72b5380788b3e057d7 Mon Sep 17 00:00:00 2001 From: Julius Volz Date: Sat, 21 Feb 2015 14:05:50 +0100 Subject: [PATCH 1/3] Fix aggregation grouping key calculation. --- rules/ast/ast.go | 1 + rules/helpers_test.go | 21 +++++++++++++++++++++ rules/rules_test.go | 10 ++++++++++ 3 files changed, 32 insertions(+) diff --git a/rules/ast/ast.go b/rules/ast/ast.go index a6f9e0195..7e891edd8 100644 --- a/rules/ast/ast.go +++ b/rules/ast/ast.go @@ -376,6 +376,7 @@ func (node *VectorAggregation) labelsToGroupingKey(labels clientmodel.Metric) ui summer := fnv.New64a() for _, label := range node.groupBy { fmt.Fprint(summer, labels[label]) + fmt.Fprint(summer, []byte{0}) } return summer.Sum64() diff --git a/rules/helpers_test.go b/rules/helpers_test.go index ab1da04e8..867f66891 100644 --- a/rules/helpers_test.go +++ b/rules/helpers_test.go @@ -184,6 +184,27 @@ var testMatrix = ast.Matrix{ }, Values: append(getTestValueStream(0, 90, 10, testStartTime), getTestValueStream(0, 0, 10, testStartTime.Add(testSampleInterval*10))...), }, + // For label-key grouping regression test. + { + Metric: clientmodel.COWMetric{ + Metric: clientmodel.Metric{ + clientmodel.MetricNameLabel: "label_grouping_test", + "a": "aa", + "b": "bb", + }, + }, + Values: getTestValueStream(0, 100, 10, testStartTime), + }, + { + Metric: clientmodel.COWMetric{ + Metric: clientmodel.Metric{ + clientmodel.MetricNameLabel: "label_grouping_test", + "a": "a", + "b": "abb", + }, + }, + Values: getTestValueStream(0, 200, 20, testStartTime), + }, } var testVector = getTestVectorFromTestMatrix(testMatrix) diff --git a/rules/rules_test.go b/rules/rules_test.go index 2661e24ab..61f3b9c4e 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -553,6 +553,8 @@ func TestExpressions(t *testing.T) { `testcounter_reset_end => 0 @[%v]`, `testcounter_reset_middle => 50 @[%v]`, `x{y="testvalue"} => 100 @[%v]`, + `label_grouping_test{a="a", b="abb"} => 200 @[%v]`, + `label_grouping_test{a="aa", b="bb"} => 100 @[%v]`, }, }, { @@ -641,6 +643,14 @@ func TestExpressions(t *testing.T) { expr: `sum(http_requests) offset 5m`, shouldFail: true, }, + // Regression test for missing separator byte in labelsToGroupingKey. + { + expr: `sum(label_grouping_test) by (a, b)`, + output: []string{ + `{a="a", b="abb"} => 200 @[%v]`, + `{a="aa", b="bb"} => 100 @[%v]`, + }, + }, } storage, closer := newTestStorage(t) From 7fefccd92996688f9c1582762c497b699166541c Mon Sep 17 00:00:00 2001 From: Julius Volz Date: Sat, 21 Feb 2015 17:19:13 +0100 Subject: [PATCH 2/3] Write() directly into hash and use model.SeparatorByte. --- rules/ast/ast.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules/ast/ast.go b/rules/ast/ast.go index 7e891edd8..8eae7359a 100644 --- a/rules/ast/ast.go +++ b/rules/ast/ast.go @@ -375,8 +375,8 @@ func (node *ScalarFunctionCall) Eval(timestamp clientmodel.Timestamp) clientmode func (node *VectorAggregation) labelsToGroupingKey(labels clientmodel.Metric) uint64 { summer := fnv.New64a() for _, label := range node.groupBy { - fmt.Fprint(summer, labels[label]) - fmt.Fprint(summer, []byte{0}) + summer.Write([]byte(labels[label])) + summer.Write([]byte{clientmodel.SeparatorByte}) } return summer.Sum64() From 42601acfdeda9422d6941f29efddcdb84acc8dff Mon Sep 17 00:00:00 2001 From: Julius Volz Date: Sat, 21 Feb 2015 17:45:47 +0100 Subject: [PATCH 3/3] Replace labelsToKey() with metric Fingerprint (fixes grouping bug). --- rules/ast/ast.go | 32 ++++------------------- rules/rules_test.go | 64 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 27 deletions(-) diff --git a/rules/ast/ast.go b/rules/ast/ast.go index 8eae7359a..a3eb25788 100644 --- a/rules/ast/ast.go +++ b/rules/ast/ast.go @@ -19,7 +19,6 @@ import ( "fmt" "hash/fnv" "math" - "sort" "time" clientmodel "github.com/prometheus/client_golang/model" @@ -382,27 +381,6 @@ func (node *VectorAggregation) labelsToGroupingKey(labels clientmodel.Metric) ui return summer.Sum64() } -func labelsToKey(labels clientmodel.Metric) uint64 { - pairs := metric.LabelPairs{} - - for label, value := range labels { - pairs = append(pairs, &metric.LabelPair{ - Name: label, - Value: value, - }) - } - - sort.Sort(pairs) - - summer := fnv.New64a() - - for _, pair := range pairs { - fmt.Fprint(summer, pair.Name, pair.Value) - } - - return summer.Sum64() -} - // EvalVectorInstant evaluates a VectorNode with an instant query. func EvalVectorInstant(node VectorNode, timestamp clientmodel.Timestamp, storage local.Storage, queryStats *stats.TimerGroup) (Vector, error) { totalEvalTimer := queryStats.GetTimer(stats.TotalEvalTime).Start() @@ -436,7 +414,7 @@ func EvalVectorRange(node VectorNode, start clientmodel.Timestamp, end clientmod defer closer.Close() evalTimer := queryStats.GetTimer(stats.InnerEvalTime).Start() - sampleStreams := map[uint64]*SampleStream{} + sampleStreams := map[clientmodel.Fingerprint]*SampleStream{} for t := start; !t.After(end); t = t.Add(interval) { if et := totalEvalTimer.ElapsedTime(); et > *queryTimeout { evalTimer.Stop() @@ -448,14 +426,14 @@ func EvalVectorRange(node VectorNode, start clientmodel.Timestamp, end clientmod Value: sample.Value, Timestamp: sample.Timestamp, } - groupingKey := labelsToKey(sample.Metric.Metric) - if sampleStreams[groupingKey] == nil { - sampleStreams[groupingKey] = &SampleStream{ + fp := sample.Metric.Metric.Fingerprint() + if sampleStreams[fp] == nil { + sampleStreams[fp] = &SampleStream{ Metric: sample.Metric, Values: metric.Values{samplePair}, } } else { - sampleStreams[groupingKey].Values = append(sampleStreams[groupingKey].Values, samplePair) + sampleStreams[fp].Values = append(sampleStreams[fp].Values, samplePair) } } } diff --git a/rules/rules_test.go b/rules/rules_test.go index 61f3b9c4e..d25cd2501 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -860,6 +860,70 @@ func TestRangedEvaluationRegressions(t *testing.T) { }, expr: "sum(testmetric) keeping_extra", }, + { + // Testing metric fingerprint grouping behavior. + in: ast.Matrix{ + { + Metric: clientmodel.COWMetric{ + Metric: clientmodel.Metric{ + clientmodel.MetricNameLabel: "testmetric", + "aa": "bb", + }, + }, + Values: metric.Values{ + { + Timestamp: testStartTime, + Value: 1, + }, + }, + }, + { + Metric: clientmodel.COWMetric{ + Metric: clientmodel.Metric{ + clientmodel.MetricNameLabel: "testmetric", + "a": "abb", + }, + }, + Values: metric.Values{ + { + Timestamp: testStartTime, + Value: 2, + }, + }, + }, + }, + out: ast.Matrix{ + { + Metric: clientmodel.COWMetric{ + Metric: clientmodel.Metric{ + clientmodel.MetricNameLabel: "testmetric", + "aa": "bb", + }, + }, + Values: metric.Values{ + { + Timestamp: testStartTime, + Value: 1, + }, + }, + }, + { + Metric: clientmodel.COWMetric{ + Metric: clientmodel.Metric{ + clientmodel.MetricNameLabel: "testmetric", + "a": "abb", + }, + }, + Values: metric.Values{ + { + Timestamp: testStartTime, + Value: 2, + }, + }, + }, + }, + expr: "testmetric", + }, } for i, s := range scenarios {