From 9d0112d7cfe8c1c36c862f7c8bf77111c2ae04ff Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Sun, 7 Feb 2016 18:03:16 +0000 Subject: [PATCH] Add without aggregator modifier. This has the advantage that the user doesn't need to list all labels they want to keep (as with "by") but without having to worry about inconsistent labels as when there's only one time series (as with "keeping_common"). Almost all aggregation should use this rather than the existing two options as it's much less error prone and easier to maintain due to not having to always add in "job" plus whatever other common job-level labels you have like "region". --- promql/ast.go | 1 + promql/engine.go | 21 +++++++++++++++--- promql/lex.go | 2 ++ promql/lex_test.go | 3 +++ promql/parse.go | 17 +++++++++++--- promql/parse_test.go | 38 ++++++++++++++++++++++++++++++++ promql/printer.go | 7 +++++- promql/printer_test.go | 3 +++ promql/testdata/aggregators.test | 19 ++++++++++++++++ 9 files changed, 104 insertions(+), 7 deletions(-) diff --git a/promql/ast.go b/promql/ast.go index 09c6fd44d7..bc19d76f1a 100644 --- a/promql/ast.go +++ b/promql/ast.go @@ -104,6 +104,7 @@ type AggregateExpr struct { Op itemType // The used aggregation operation. Expr Expr // The vector expression over which is aggregated. Grouping model.LabelNames // The labels by which to group the vector. + Without bool // Whether to drop the given labels rather than keep them. KeepExtraLabels bool // Whether to keep extra labels common among result elements. } diff --git a/promql/engine.go b/promql/engine.go index c1cceb0485..f52bbc71ad 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -619,7 +619,7 @@ func (ev *evaluator) eval(expr Expr) model.Value { switch e := expr.(type) { case *AggregateExpr: vector := ev.evalVector(e.Expr) - return ev.aggregation(e.Op, e.Grouping, e.KeepExtraLabels, vector) + return ev.aggregation(e.Op, e.Grouping, e.Without, e.KeepExtraLabels, vector) case *BinaryExpr: lhs := ev.evalOneOf(e.LHS, model.ValScalar, model.ValVector) @@ -1039,12 +1039,25 @@ type groupedAggregation struct { } // aggregation evaluates an aggregation operation on a vector. -func (ev *evaluator) aggregation(op itemType, grouping model.LabelNames, keepExtra bool, vec vector) vector { +func (ev *evaluator) aggregation(op itemType, grouping model.LabelNames, without bool, keepExtra bool, vec vector) vector { result := map[uint64]*groupedAggregation{} for _, sample := range vec { - groupingKey := model.SignatureForLabels(sample.Metric.Metric, grouping...) + withoutMetric := sample.Metric + if without { + for _, l := range grouping { + withoutMetric.Del(l) + } + withoutMetric.Del(model.MetricNameLabel) + } + + var groupingKey uint64 + if without { + groupingKey = uint64(withoutMetric.Metric.Fingerprint()) + } else { + groupingKey = model.SignatureForLabels(sample.Metric.Metric, grouping...) + } groupedResult, ok := result[groupingKey] // Add a new group if it doesn't exist. @@ -1053,6 +1066,8 @@ func (ev *evaluator) aggregation(op itemType, grouping model.LabelNames, keepExt if keepExtra { m = sample.Metric m.Del(model.MetricNameLabel) + } else if without { + m = withoutMetric } else { m = metric.Metric{ Metric: model.Metric{}, diff --git a/promql/lex.go b/promql/lex.go index 7817d34216..90f82f3736 100644 --- a/promql/lex.go +++ b/promql/lex.go @@ -158,6 +158,7 @@ const ( itemKeepCommon itemOffset itemBy + itemWithout itemOn itemGroupLeft itemGroupRight @@ -192,6 +193,7 @@ var key = map[string]itemType{ "annotations": itemAnnotations, "offset": itemOffset, "by": itemBy, + "without": itemWithout, "keeping_extra": itemKeepCommon, "keep_common": itemKeepCommon, "on": itemOn, diff --git a/promql/lex_test.go b/promql/lex_test.go index e3f0b4c4e7..cb1642aecc 100644 --- a/promql/lex_test.go +++ b/promql/lex_test.go @@ -261,6 +261,9 @@ var tests = []struct { }, { input: "by", expected: []item{{itemBy, 0, "by"}}, + }, { + input: "without", + expected: []item{{itemWithout, 0, "without"}}, }, { input: "on", expected: []item{{itemOn, 0, "on"}}, diff --git a/promql/parse.go b/promql/parse.go index a239869734..5912809bc9 100644 --- a/promql/parse.go +++ b/promql/parse.go @@ -732,11 +732,14 @@ func (p *parser) aggrExpr() *AggregateExpr { p.errorf("expected aggregation operator but got %s", agop) } var grouping model.LabelNames - var keepExtra bool + var keepExtra, without bool modifiersFirst := false - if p.peek().typ == itemBy { + if t := p.peek().typ; t == itemBy || t == itemWithout { + if t == itemWithout { + without = true + } p.next() grouping = p.labels() modifiersFirst = true @@ -752,10 +755,13 @@ func (p *parser) aggrExpr() *AggregateExpr { p.expect(itemRightParen, ctx) if !modifiersFirst { - if p.peek().typ == itemBy { + if t := p.peek().typ; t == itemBy || t == itemWithout { if len(grouping) > 0 { p.errorf("aggregation must only contain one grouping clause") } + if t == itemWithout { + without = true + } p.next() grouping = p.labels() } @@ -765,10 +771,15 @@ func (p *parser) aggrExpr() *AggregateExpr { } } + if keepExtra && without { + p.errorf("cannot use 'keep_common' with 'without'") + } + return &AggregateExpr{ Op: agop.typ, Expr: e, Grouping: grouping, + Without: without, KeepExtraLabels: keepExtra, } } diff --git a/promql/parse_test.go b/promql/parse_test.go index 21744f6d2d..07e5ce0c0d 100644 --- a/promql/parse_test.go +++ b/promql/parse_test.go @@ -869,6 +869,32 @@ var testExpr = []struct { }, Grouping: model.LabelNames{"foo"}, }, + }, { + input: "sum without (foo) (some_metric)", + expected: &AggregateExpr{ + Op: itemSum, + Without: true, + Expr: &VectorSelector{ + Name: "some_metric", + LabelMatchers: metric.LabelMatchers{ + {Type: metric.Equal, Name: model.MetricNameLabel, Value: "some_metric"}, + }, + }, + Grouping: model.LabelNames{"foo"}, + }, + }, { + input: "sum (some_metric) without (foo)", + expected: &AggregateExpr{ + Op: itemSum, + Without: true, + Expr: &VectorSelector{ + Name: "some_metric", + LabelMatchers: metric.LabelMatchers{ + {Type: metric.Equal, Name: model.MetricNameLabel, Value: "some_metric"}, + }, + }, + Grouping: model.LabelNames{"foo"}, + }, }, { input: "stddev(some_metric)", expected: &AggregateExpr{ @@ -920,6 +946,18 @@ var testExpr = []struct { input: "MIN by(test) (some_metric) keep_common", fail: true, errMsg: "could not parse remaining input \"keep_common\"...", + }, { + input: `sum (some_metric) without (test) keep_common`, + fail: true, + errMsg: "cannot use 'keep_common' with 'without'", + }, { + input: `sum (some_metric) without (test) by (test)`, + fail: true, + errMsg: "could not parse remaining input \"by (test)\"...", + }, { + input: `sum without (test) (some_metric) by (test)`, + fail: true, + errMsg: "could not parse remaining input \"by (test)\"...", }, // Test function calls. { diff --git a/promql/printer.go b/promql/printer.go index 7be0de04e4..e35699d7d1 100644 --- a/promql/printer.go +++ b/promql/printer.go @@ -137,7 +137,12 @@ func (es Expressions) String() (s string) { func (node *AggregateExpr) String() string { aggrString := fmt.Sprintf("%s(%s)", node.Op, node.Expr) if len(node.Grouping) > 0 { - format := "%s BY (%s)" + var format string + if node.Without { + format = "%s WITHOUT (%s)" + } else { + format = "%s BY (%s)" + } if node.KeepExtraLabels { format += " KEEP_COMMON" } diff --git a/promql/printer_test.go b/promql/printer_test.go index 473305b614..890adc661c 100644 --- a/promql/printer_test.go +++ b/promql/printer_test.go @@ -30,6 +30,9 @@ func TestExprString(t *testing.T) { { in: `sum(task:errors:rate10s{job="s"}) BY (code) KEEP_COMMON`, }, + { + in: `sum(task:errors:rate10s{job="s"}) WITHOUT (instance)`, + }, { in: `up > BOOL 0`, }, diff --git a/promql/testdata/aggregators.test b/promql/testdata/aggregators.test index 4e71cd08a0..de4dd1e837 100644 --- a/promql/testdata/aggregators.test +++ b/promql/testdata/aggregators.test @@ -8,6 +8,10 @@ load 5m http_requests{job="app-server", instance="0", group="canary"} 0+70x10 http_requests{job="app-server", instance="1", group="canary"} 0+80x10 +load 5m + foo{job="api-server", instance="0", region="europe"} 0+90x10 + foo{job="api-server"} 0+100x10 + # Simple sum. eval instant at 50m SUM BY (group) (http_requests{job="api-server"}) {group="canary"} 700 @@ -28,6 +32,18 @@ eval instant at 50m count by (group) (http_requests{job="api-server"}) {group="canary"} 2 {group="production"} 2 +# Simple without. +eval instant at 50m sum without (instance) (http_requests{job="api-server"}) + {group="canary",job="api-server"} 700 + {group="production",job="api-server"} 300 + +# Without with mismatched and missing labels. Do not do this. +eval instant at 50m sum without (instance) (http_requests{job="api-server"} or foo) + {group="canary",job="api-server"} 700 + {group="production",job="api-server"} 300 + {region="europe",job="api-server"} 900 + {job="api-server"} 1000 + # Lower-cased aggregation operators should work too. eval instant at 50m sum(http_requests) by (job) + min(http_requests) by (job) + max(http_requests) by (job) + avg(http_requests) by (job) {job="app-server"} 4550 @@ -45,6 +61,7 @@ eval instant at 50m sum(sum by (group) keep_common (http_requests{job="api-serve {job="api-server"} 1000 + # Standard deviation and variance. eval instant at 50m stddev(http_requests) {} 229.12878474779 @@ -61,6 +78,7 @@ eval instant at 50m stdvar by (instance)(http_requests) {instance="1"} 50000 + # Regression test for missing separator byte in labelsToGroupingKey. clear load 5m @@ -72,6 +90,7 @@ eval instant at 50m sum(label_grouping_test) by (a, b) {a="aa", b="bb"} 100 + # Tests for min/max. clear load 5m