From 246a817300bb253a51e0171691201416dc650842 Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Thu, 23 Jun 2016 17:23:44 +0100 Subject: [PATCH 1/2] Flip vector matching to be ignoring by default. This is a noop semantically. --- promql/ast.go | 6 +++--- promql/engine.go | 14 +++++++------- promql/parse.go | 6 +++--- promql/parse_test.go | 12 ++++++++---- promql/printer.go | 6 +++--- 5 files changed, 24 insertions(+), 20 deletions(-) diff --git a/promql/ast.go b/promql/ast.go index fa9812fdd5..3f929bbe26 100644 --- a/promql/ast.go +++ b/promql/ast.go @@ -231,9 +231,9 @@ type VectorMatching struct { // MatchingLabels contains the labels which define equality of a pair of // elements from the vectors. MatchingLabels model.LabelNames - // Ignoring excludes the given label names from matching, - // rather than only using them. - Ignoring bool + // On includes the given label names from matching, + // rather than excluding them. + On bool // Include contains additional labels that should be included in // the result from the side with the lower cardinality. Include model.LabelNames diff --git a/promql/engine.go b/promql/engine.go index 8a7ad6d665..92da4a41d5 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -728,7 +728,7 @@ func (ev *evaluator) vectorAnd(lhs, rhs vector, matching *VectorMatching) vector if matching.Card != CardManyToMany { panic("set operations must only use many-to-many matching") } - sigf := signatureFunc(matching.Ignoring, matching.MatchingLabels...) + sigf := signatureFunc(matching.On, matching.MatchingLabels...) var result vector // The set of signatures for the right-hand side vector. @@ -751,7 +751,7 @@ func (ev *evaluator) vectorOr(lhs, rhs vector, matching *VectorMatching) vector if matching.Card != CardManyToMany { panic("set operations must only use many-to-many matching") } - sigf := signatureFunc(matching.Ignoring, matching.MatchingLabels...) + sigf := signatureFunc(matching.On, matching.MatchingLabels...) var result vector leftSigs := map[uint64]struct{}{} @@ -773,7 +773,7 @@ func (ev *evaluator) vectorUnless(lhs, rhs vector, matching *VectorMatching) vec if matching.Card != CardManyToMany { panic("set operations must only use many-to-many matching") } - sigf := signatureFunc(matching.Ignoring, matching.MatchingLabels...) + sigf := signatureFunc(matching.On, matching.MatchingLabels...) rightSigs := map[uint64]struct{}{} for _, rs := range rhs { @@ -796,7 +796,7 @@ func (ev *evaluator) vectorBinop(op itemType, lhs, rhs vector, matching *VectorM } var ( result = vector{} - sigf = signatureFunc(matching.Ignoring, matching.MatchingLabels...) + sigf = signatureFunc(matching.On, matching.MatchingLabels...) ) // The control flow below handles one-to-one or many-to-one matching. @@ -883,8 +883,8 @@ func (ev *evaluator) vectorBinop(op itemType, lhs, rhs vector, matching *VectorM // signatureFunc returns a function that calculates the signature for a metric // based on the provided labels. If ignoring, then the given labels are ignored instead. -func signatureFunc(ignoring bool, labels ...model.LabelName) func(m metric.Metric) uint64 { - if len(labels) == 0 || ignoring { +func signatureFunc(on bool, labels ...model.LabelName) func(m metric.Metric) uint64 { + if len(labels) == 0 || !on { return func(m metric.Metric) uint64 { tmp := m.Metric.Clone() for _, l := range labels { @@ -908,7 +908,7 @@ func resultMetric(lhs, rhs metric.Metric, op itemType, matching *VectorMatching) if len(matching.MatchingLabels)+len(matching.Include) == 0 { return lhs } - if matching.Ignoring { + if !matching.On { if matching.Card == CardOneToOne { for _, l := range matching.MatchingLabels { lhs.Del(l) diff --git a/promql/parse.go b/promql/parse.go index e2e03b18f5..057237daed 100644 --- a/promql/parse.go +++ b/promql/parse.go @@ -463,8 +463,8 @@ func (p *parser) expr() Expr { // Parse ON/IGNORING clause. if p.peek().typ == itemOn || p.peek().typ == itemIgnoring { - if p.peek().typ == itemIgnoring { - vecMatching.Ignoring = true + if p.peek().typ == itemOn { + vecMatching.On = true } p.next() vecMatching.MatchingLabels = p.labels() @@ -485,7 +485,7 @@ func (p *parser) expr() Expr { for _, ln := range vecMatching.MatchingLabels { for _, ln2 := range vecMatching.Include { - if ln == ln2 && !vecMatching.Ignoring { + if ln == ln2 && vecMatching.On { p.errorf("label %q must not occur in ON and GROUP clause at once", ln) } } diff --git a/promql/parse_test.go b/promql/parse_test.go index 4b9791fe24..16be4382cf 100644 --- a/promql/parse_test.go +++ b/promql/parse_test.go @@ -471,12 +471,14 @@ var testExpr = []struct { VectorMatching: &VectorMatching{ Card: CardOneToMany, MatchingLabels: model.LabelNames{"baz", "buz"}, + On: true, Include: model.LabelNames{"test"}, }, }, VectorMatching: &VectorMatching{ Card: CardOneToOne, MatchingLabels: model.LabelNames{"foo"}, + On: true, }, }, }, { @@ -498,6 +500,7 @@ var testExpr = []struct { VectorMatching: &VectorMatching{ Card: CardOneToOne, MatchingLabels: model.LabelNames{"test", "blub"}, + On: true, }, }, }, { @@ -519,6 +522,7 @@ var testExpr = []struct { VectorMatching: &VectorMatching{ Card: CardManyToOne, MatchingLabels: model.LabelNames{"test", "blub"}, + On: true, }, }, }, { @@ -540,6 +544,7 @@ var testExpr = []struct { VectorMatching: &VectorMatching{ Card: CardManyToMany, MatchingLabels: model.LabelNames{"test", "blub"}, + On: true, }, }, }, { @@ -561,7 +566,6 @@ var testExpr = []struct { VectorMatching: &VectorMatching{ Card: CardManyToMany, MatchingLabels: model.LabelNames{"test", "blub"}, - Ignoring: true, }, }, }, { @@ -583,6 +587,7 @@ var testExpr = []struct { VectorMatching: &VectorMatching{ Card: CardManyToMany, MatchingLabels: model.LabelNames{"bar"}, + On: true, }, }, }, { @@ -604,6 +609,7 @@ var testExpr = []struct { VectorMatching: &VectorMatching{ Card: CardManyToOne, MatchingLabels: model.LabelNames{"test", "blub"}, + On: true, Include: model.LabelNames{"bar"}, }, }, @@ -627,7 +633,6 @@ var testExpr = []struct { Card: CardManyToOne, MatchingLabels: model.LabelNames{"test", "blub"}, Include: model.LabelNames{"blub"}, - Ignoring: true, }, }, }, { @@ -650,7 +655,6 @@ var testExpr = []struct { Card: CardManyToOne, MatchingLabels: model.LabelNames{"test", "blub"}, Include: model.LabelNames{"bar"}, - Ignoring: true, }, }, }, { @@ -673,6 +677,7 @@ var testExpr = []struct { Card: CardOneToMany, MatchingLabels: model.LabelNames{"test", "blub"}, Include: model.LabelNames{"bar", "foo"}, + On: true, }, }, }, { @@ -695,7 +700,6 @@ var testExpr = []struct { Card: CardOneToMany, MatchingLabels: model.LabelNames{"test", "blub"}, Include: model.LabelNames{"bar", "foo"}, - Ignoring: true, }, }, }, { diff --git a/promql/printer.go b/promql/printer.go index 5d395bf6b5..8d93af29ea 100644 --- a/promql/printer.go +++ b/promql/printer.go @@ -160,10 +160,10 @@ func (node *BinaryExpr) String() string { matching := "" vm := node.VectorMatching if vm != nil && len(vm.MatchingLabels) > 0 { - if vm.Ignoring { - matching = fmt.Sprintf(" IGNORING(%s)", vm.MatchingLabels) - } else { + if vm.On { matching = fmt.Sprintf(" ON(%s)", vm.MatchingLabels) + } else { + matching = fmt.Sprintf(" IGNORING(%s)", vm.MatchingLabels) } if vm.Card == CardManyToOne || vm.Card == CardOneToMany { matching += " GROUP_" From 3b89616d82110b2c843cd87bc9a1da4fb8c0bd34 Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Thu, 23 Jun 2016 17:49:22 +0100 Subject: [PATCH 2/2] Allow on, ignoring, by and without wit empty laberls. This offers new semantics in allowing on() for matching two single-element vectors with no known common labels. Previosuly this was often done using on(dummy). This also allows making it explict that you meant to do an aggregation without labels via by(). Fixes #1597. --- promql/engine.go | 7 ++-- promql/parse.go | 14 ++++---- promql/parse_test.go | 59 +++++++++++++++++++++++++++++--- promql/testdata/aggregators.test | 13 +++++++ promql/testdata/operators.test | 18 ++++++++++ 5 files changed, 96 insertions(+), 15 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 92da4a41d5..3a9fc8fac4 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -882,9 +882,9 @@ func (ev *evaluator) vectorBinop(op itemType, lhs, rhs vector, matching *VectorM } // signatureFunc returns a function that calculates the signature for a metric -// based on the provided labels. If ignoring, then the given labels are ignored instead. +// ignoring the provided labels. If on, then the given labels are only used instead. func signatureFunc(on bool, labels ...model.LabelName) func(m metric.Metric) uint64 { - if len(labels) == 0 || !on { + if !on { return func(m metric.Metric) uint64 { tmp := m.Metric.Clone() for _, l := range labels { @@ -905,9 +905,6 @@ func resultMetric(lhs, rhs metric.Metric, op itemType, matching *VectorMatching) if shouldDropMetricName(op) { lhs.Del(model.MetricNameLabel) } - if len(matching.MatchingLabels)+len(matching.Include) == 0 { - return lhs - } if !matching.On { if matching.Card == CardOneToOne { for _, l := range matching.MatchingLabels { diff --git a/promql/parse.go b/promql/parse.go index 057237daed..da9c383b3f 100644 --- a/promql/parse.go +++ b/promql/parse.go @@ -671,14 +671,16 @@ func (p *parser) labels() model.LabelNames { p.expect(itemLeftParen, ctx) labels := model.LabelNames{} - for { - id := p.expect(itemIdentifier, ctx) - labels = append(labels, model.LabelName(id.val)) + if p.peek().typ != itemRightParen { + for { + id := p.expect(itemIdentifier, ctx) + labels = append(labels, model.LabelName(id.val)) - if p.peek().typ != itemComma { - break + if p.peek().typ != itemComma { + break + } + p.next() } - p.next() } p.expect(itemRightParen, ctx) diff --git a/promql/parse_test.go b/promql/parse_test.go index 16be4382cf..5e65bfd98e 100644 --- a/promql/parse_test.go +++ b/promql/parse_test.go @@ -547,6 +547,28 @@ var testExpr = []struct { On: true, }, }, + }, { + input: "foo and on() bar", + expected: &BinaryExpr{ + Op: itemLAND, + LHS: &VectorSelector{ + Name: "foo", + LabelMatchers: metric.LabelMatchers{ + {Type: metric.Equal, Name: model.MetricNameLabel, Value: "foo"}, + }, + }, + RHS: &VectorSelector{ + Name: "bar", + LabelMatchers: metric.LabelMatchers{ + {Type: metric.Equal, Name: model.MetricNameLabel, Value: "bar"}, + }, + }, + VectorMatching: &VectorMatching{ + Card: CardManyToMany, + MatchingLabels: model.LabelNames{}, + On: true, + }, + }, }, { input: "foo and ignoring(test,blub) bar", expected: &BinaryExpr{ @@ -568,6 +590,27 @@ var testExpr = []struct { MatchingLabels: model.LabelNames{"test", "blub"}, }, }, + }, { + input: "foo and ignoring() bar", + expected: &BinaryExpr{ + Op: itemLAND, + LHS: &VectorSelector{ + Name: "foo", + LabelMatchers: metric.LabelMatchers{ + {Type: metric.Equal, Name: model.MetricNameLabel, Value: "foo"}, + }, + }, + RHS: &VectorSelector{ + Name: "bar", + LabelMatchers: metric.LabelMatchers{ + {Type: metric.Equal, Name: model.MetricNameLabel, Value: "bar"}, + }, + }, + VectorMatching: &VectorMatching{ + Card: CardManyToMany, + MatchingLabels: model.LabelNames{}, + }, + }, }, { input: "foo unless on(bar) baz", expected: &BinaryExpr{ @@ -1146,6 +1189,18 @@ var testExpr = []struct { }, Grouping: model.LabelNames{"foo"}, }, + }, { + input: "sum by ()(some_metric)", + expected: &AggregateExpr{ + Op: itemSum, + Expr: &VectorSelector{ + Name: "some_metric", + LabelMatchers: metric.LabelMatchers{ + {Type: metric.Equal, Name: model.MetricNameLabel, Value: "some_metric"}, + }, + }, + Grouping: model.LabelNames{}, + }, }, { input: `sum some_metric by (test)`, fail: true, @@ -1154,10 +1209,6 @@ var testExpr = []struct { input: `sum (some_metric) by test`, fail: true, errMsg: "unexpected identifier \"test\" in grouping opts, expected \"(\"", - }, { - input: `sum (some_metric) by ()`, - fail: true, - errMsg: "unexpected \")\" in grouping opts, expected identifier", }, { input: `sum (some_metric) by test`, fail: true, diff --git a/promql/testdata/aggregators.test b/promql/testdata/aggregators.test index de4dd1e837..78bf5192d7 100644 --- a/promql/testdata/aggregators.test +++ b/promql/testdata/aggregators.test @@ -37,6 +37,19 @@ eval instant at 50m sum without (instance) (http_requests{job="api-server"}) {group="canary",job="api-server"} 700 {group="production",job="api-server"} 300 +# Empty by. +eval instant at 50m sum by () (http_requests{job="api-server"}) + {} 1000 + +# No by/without. +eval instant at 50m sum (http_requests{job="api-server"}) + {} 1000 + +# Empty without. +eval instant at 50m sum without () (http_requests{job="api-server",group="production"}) + {group="production",job="api-server",instance="0"} 100 + {group="production",job="api-server",instance="1"} 200 + # 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 diff --git a/promql/testdata/operators.test b/promql/testdata/operators.test index 8169ff6f46..2075e77d75 100644 --- a/promql/testdata/operators.test +++ b/promql/testdata/operators.test @@ -329,3 +329,21 @@ eval instant at 5m node_cpu > on(job, instance) group_left(target) (threshold or node_cpu{instance="abc",job="node",mode="user",target="a@b.com"} 1 node_cpu{instance="def",job="node",mode="idle"} 8 node_cpu{instance="def",job="node",mode="user"} 2 + +clear + +load 5m + random{foo="bar"} 2 + metricA{baz="meh"} 3 + metricB{baz="meh"} 4 + +# On with no labels, for metrics with no common labels. +eval instant at 5m random + on() metricA + {} 5 + +# Ignoring with no labels is the same as no ignoring. +eval instant at 5m metricA + ignoring() metricB + {baz="meh"} 7 + +eval instant at 5m metricA + metricB + {baz="meh"} 7