From f5084ab1c5a8767020356ee0706b145a8f98fe22 Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Thu, 21 Apr 2016 16:52:33 +0100 Subject: [PATCH 1/7] Add tests for group_left/group_right --- promql/testdata/operators.test | 35 ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/promql/testdata/operators.test b/promql/testdata/operators.test index e8d39dc19..c85984dfb 100644 --- a/promql/testdata/operators.test +++ b/promql/testdata/operators.test @@ -170,3 +170,38 @@ eval instant at 50m 0 == bool 1 eval instant at 50m 1 == bool 1 1 + +# group_left/group_right. + +clear + +load 5m + node_var{instance="abc",job="node"} 2 + node_role{instance="abc",job="node",role="prometheus"} 1 + +load 5m + node_cpu{instance="abc",job="node",mode="idle"} 3 + node_cpu{instance="abc",job="node",mode="user"} 1 + node_cpu{instance="def",job="node",mode="idle"} 8 + node_cpu{instance="def",job="node",mode="user"} 2 + +# Copy machine role to node variable. +eval instant at 5m node_role * on (instance,job) group_left (role) node_var + {instance="abc",job="node",role="prometheus"} 2 + +eval instant at 5m node_var * on (instance,job) group_right (role) node_role + {instance="abc",job="node",role="prometheus"} 2 + +# Ratio of total. +eval instant at 5m node_cpu / on (instance,job) group_left (mode) sum by (instance,job)(node_cpu) + {instance="abc",job="node",mode="idle"} .75 + {instance="abc",job="node",mode="user"} .25 + {instance="def",job="node",mode="idle"} .80 + {instance="def",job="node",mode="user"} .20 + +eval instant at 5m sum by (mode, job)(node_cpu) / on (job) group_left (mode) sum by (job)(node_cpu) + {job="node",mode="idle"} 0.7857142857142857 + {job="node",mode="user"} 0.21428571428571427 + +eval instant at 5m sum(sum by (mode, job)(node_cpu) / on (job) group_left (mode) sum by (job)(node_cpu)) + {} 1.0 From 1d08c4fef012e169aa5c6f22c2729b7175fc2123 Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Thu, 21 Apr 2016 11:45:06 +0100 Subject: [PATCH 2/7] Add 'ignoring' as modifier for binops. Where 'on' uses the given labels to match, 'ignoring' uses all other labels to match. group_left/right is not supported yet. --- promql/ast.go | 3 +++ promql/engine.go | 31 ++++++++++++++++----------- promql/lex.go | 2 ++ promql/lex_test.go | 3 +++ promql/parse.go | 9 +++++++- promql/parse_test.go | 30 +++++++++++++++++++++++++++ promql/printer.go | 6 +++++- promql/printer_test.go | 6 ++++++ promql/testdata/operators.test | 38 ++++++++++++++++++++++++++++++++++ 9 files changed, 114 insertions(+), 14 deletions(-) diff --git a/promql/ast.go b/promql/ast.go index 9606bb2e8..5a7e24caf 100644 --- a/promql/ast.go +++ b/promql/ast.go @@ -231,6 +231,9 @@ type VectorMatching struct { // On contains the labels which define equality of a pair // of elements from the vectors. On model.LabelNames + // Ignoring excludes the given label names from matching, + // rather than only using them. + Ignoring bool // Include contains additional labels that should be included in // the result from the side with the higher cardinality. Include model.LabelNames diff --git a/promql/engine.go b/promql/engine.go index 5005f6a6f..2dd010b88 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.On...) + sigf := signatureFunc(matching.Ignoring, matching.On...) 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.On...) + sigf := signatureFunc(matching.Ignoring, matching.On...) 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.On...) + sigf := signatureFunc(matching.Ignoring, matching.On...) 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.On...) + sigf = signatureFunc(matching.Ignoring, matching.On...) resultLabels = append(matching.On, matching.Include...) ) @@ -851,7 +851,7 @@ func (ev *evaluator) vectorBinop(op itemType, lhs, rhs vector, matching *VectorM } else if !keep { continue } - metric := resultMetric(ls.Metric, op, resultLabels...) + metric := resultMetric(ls.Metric, op, matching.Ignoring, resultLabels...) insertedSigs, exists := matchedSigs[sig] if matching.Card == CardOneToOne { @@ -883,12 +883,16 @@ 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. -func signatureFunc(labels ...model.LabelName) func(m metric.Metric) uint64 { - if len(labels) == 0 { +// 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 { return func(m metric.Metric) uint64 { - m.Del(model.MetricNameLabel) - return uint64(m.Metric.Fingerprint()) + tmp := m.Metric.Clone() + for _, l := range labels { + delete(tmp, l) + } + delete(tmp, model.MetricNameLabel) + return uint64(tmp.Fingerprint()) } } return func(m metric.Metric) uint64 { @@ -898,11 +902,14 @@ func signatureFunc(labels ...model.LabelName) func(m metric.Metric) uint64 { // resultMetric returns the metric for the given sample(s) based on the vector // binary operation and the matching options. -func resultMetric(met metric.Metric, op itemType, labels ...model.LabelName) metric.Metric { - if len(labels) == 0 { +func resultMetric(met metric.Metric, op itemType, ignoring bool, labels ...model.LabelName) metric.Metric { + if len(labels) == 0 || ignoring { if shouldDropMetricName(op) { met.Del(model.MetricNameLabel) } + for _, l := range labels { + met.Del(l) + } return met } // As we definitely write, creating a new metric is the easiest solution. diff --git a/promql/lex.go b/promql/lex.go index fbd999352..cbfa877b2 100644 --- a/promql/lex.go +++ b/promql/lex.go @@ -172,6 +172,7 @@ const ( itemBy itemWithout itemOn + itemIgnoring itemGroupLeft itemGroupRight itemBool @@ -209,6 +210,7 @@ var key = map[string]itemType{ "keeping_extra": itemKeepCommon, "keep_common": itemKeepCommon, "on": itemOn, + "ignoring": itemIgnoring, "group_left": itemGroupLeft, "group_right": itemGroupRight, "bool": itemBool, diff --git a/promql/lex_test.go b/promql/lex_test.go index 549b24f86..49ab711f9 100644 --- a/promql/lex_test.go +++ b/promql/lex_test.go @@ -278,6 +278,9 @@ var tests = []struct { }, { input: "on", expected: []item{{itemOn, 0, "on"}}, + }, { + input: "ignoring", + expected: []item{{itemIgnoring, 0, "ignoring"}}, }, { input: "group_left", expected: []item{{itemGroupLeft, 0, "group_left"}}, diff --git a/promql/parse.go b/promql/parse.go index 125e1060b..ba246d424 100644 --- a/promql/parse.go +++ b/promql/parse.go @@ -462,7 +462,10 @@ func (p *parser) expr() Expr { } // Parse ON clause. - if p.peek().typ == itemOn { + if p.peek().typ == itemOn || p.peek().typ == itemIgnoring { + if p.peek().typ == itemIgnoring { + vecMatching.Ignoring = true + } p.next() vecMatching.On = p.labels() @@ -478,6 +481,10 @@ func (p *parser) expr() Expr { } } + if vecMatching.Ignoring && (vecMatching.Card == CardManyToOne || vecMatching.Card == CardOneToMany) { + p.errorf("IGNORING not permitted with many to one matching") + } + for _, ln := range vecMatching.On { for _, ln2 := range vecMatching.Include { if ln == ln2 { diff --git a/promql/parse_test.go b/promql/parse_test.go index c400ba077..22a07a138 100644 --- a/promql/parse_test.go +++ b/promql/parse_test.go @@ -250,6 +250,14 @@ var testExpr = []struct { input: "1 offset 1d", fail: true, errMsg: "offset modifier must be preceded by an instant or range selector", + }, { + input: "a - on(b) ignoring(c) d", + fail: true, + errMsg: "parse error at char 11: no valid expression found", + }, { + input: "a - ignoring(b) group_left(c) d", + fail: true, + errMsg: "parse error at char 29: IGNORING not permitted with many to one matching", }, // Vector binary operations. { @@ -517,6 +525,28 @@ var testExpr = []struct { On: model.LabelNames{"test", "blub"}, }, }, + }, { + input: "foo and ignoring(test,blub) 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, + On: model.LabelNames{"test", "blub"}, + Ignoring: true, + }, + }, }, { input: "foo unless on(bar) baz", expected: &BinaryExpr{ diff --git a/promql/printer.go b/promql/printer.go index 748d77461..06b810b90 100644 --- a/promql/printer.go +++ b/promql/printer.go @@ -160,7 +160,11 @@ func (node *BinaryExpr) String() string { matching := "" vm := node.VectorMatching if vm != nil && len(vm.On) > 0 { - matching = fmt.Sprintf(" ON(%s)", vm.On) + if vm.Ignoring { + matching = fmt.Sprintf(" IGNORING(%s)", vm.On) + } else { + matching = fmt.Sprintf(" ON(%s)", vm.On) + } if vm.Card == CardManyToOne { matching += fmt.Sprintf(" GROUP_LEFT(%s)", vm.Include) } diff --git a/promql/printer_test.go b/promql/printer_test.go index 29b275e0b..e17774ac8 100644 --- a/promql/printer_test.go +++ b/promql/printer_test.go @@ -36,6 +36,12 @@ func TestExprString(t *testing.T) { { in: `sum(task:errors:rate10s{job="s"}) WITHOUT (instance)`, }, + { + in: `a - ON(b) c`, + }, + { + in: `a - IGNORING(b) c`, + }, { in: `up > BOOL 0`, }, diff --git a/promql/testdata/operators.test b/promql/testdata/operators.test index c85984dfb..1c095ebae 100644 --- a/promql/testdata/operators.test +++ b/promql/testdata/operators.test @@ -79,6 +79,14 @@ eval instant at 50m (http_requests{group="canary"} + 1) and on(instance) http_re {group="canary", instance="0", job="api-server"} 301 {group="canary", instance="0", job="app-server"} 701 +eval instant at 50m (http_requests{group="canary"} + 1) and ignoring(group) http_requests{instance="0", group="production"} + {group="canary", instance="0", job="api-server"} 301 + {group="canary", instance="0", job="app-server"} 701 + +eval instant at 50m (http_requests{group="canary"} + 1) and ignoring(group, job) http_requests{instance="0", group="production"} + {group="canary", instance="0", job="api-server"} 301 + {group="canary", instance="0", job="app-server"} 701 + eval instant at 50m http_requests{group="canary"} or http_requests{group="production"} http_requests{group="canary", instance="0", job="api-server"} 300 http_requests{group="canary", instance="0", job="app-server"} 700 @@ -109,6 +117,14 @@ eval instant at 50m (http_requests{group="canary"} + 1) or on(instance) (http_re vector_matching_a{l="x"} 10 vector_matching_a{l="y"} 20 +eval instant at 50m (http_requests{group="canary"} + 1) or ignoring(l, group, job) (http_requests or cpu_count or vector_matching_a) + {group="canary", instance="0", job="api-server"} 301 + {group="canary", instance="0", job="app-server"} 701 + {group="canary", instance="1", job="api-server"} 401 + {group="canary", instance="1", job="app-server"} 801 + vector_matching_a{l="x"} 10 + vector_matching_a{l="y"} 20 + eval instant at 50m http_requests{group="canary"} unless http_requests{instance="0"} http_requests{group="canary", instance="1", job="api-server"} 400 http_requests{group="canary", instance="1", job="app-server"} 800 @@ -125,6 +141,18 @@ eval instant at 50m http_requests{group="canary"} / on(instance,job) http_reques {instance="1", job="api-server"} 2 {instance="1", job="app-server"} 1.3333333333333333 +eval instant at 50m http_requests{group="canary"} unless ignoring(group, instance) http_requests{instance="0"} + +eval instant at 50m http_requests{group="canary"} unless ignoring(group) http_requests{instance="0"} + http_requests{group="canary", instance="1", job="api-server"} 400 + http_requests{group="canary", instance="1", job="app-server"} 800 + +eval instant at 50m http_requests{group="canary"} / ignoring(group) http_requests{group="production"} + {instance="0", job="api-server"} 3 + {instance="0", job="app-server"} 1.4 + {instance="1", job="api-server"} 2 + {instance="1", job="app-server"} 1.3333333333333333 + # https://github.com/prometheus/prometheus/issues/1489 eval instant at 50m http_requests AND ON (dummy) vector(1) http_requests{group="canary", instance="0", job="api-server"} 300 @@ -136,6 +164,16 @@ eval instant at 50m http_requests AND ON (dummy) vector(1) http_requests{group="production", instance="1", job="api-server"} 200 http_requests{group="production", instance="1", job="app-server"} 600 +eval instant at 50m http_requests AND IGNORING (group, instance, job) vector(1) + http_requests{group="canary", instance="0", job="api-server"} 300 + http_requests{group="canary", instance="0", job="app-server"} 700 + http_requests{group="canary", instance="1", job="api-server"} 400 + http_requests{group="canary", instance="1", job="app-server"} 800 + http_requests{group="production", instance="0", job="api-server"} 100 + http_requests{group="production", instance="0", job="app-server"} 500 + http_requests{group="production", instance="1", job="api-server"} 200 + http_requests{group="production", instance="1", job="app-server"} 600 + # Comparisons. eval instant at 50m SUM(http_requests) BY (job) > 1000 From d1edfb25b3dcea198658e6a6a0984a7be00e1007 Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Thu, 21 Apr 2016 15:53:14 +0100 Subject: [PATCH 3/7] Add support for OneToMany with IGNORING. The labels listed in the group_ modifier will be copied from the one side to the many side. It will be valid to specify no labels. This is intended to replace the existing ON/GROUP_* support., --- promql/engine.go | 49 +++++++++++++++------ promql/parse.go | 26 +++++------ promql/parse_test.go | 79 +++++++++++++++++++++++++++++++--- promql/testdata/operators.test | 43 ++++++++++++++++++ 4 files changed, 165 insertions(+), 32 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 2dd010b88..d5b4cf801 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -795,9 +795,8 @@ func (ev *evaluator) vectorBinop(op itemType, lhs, rhs vector, matching *VectorM panic("many-to-many only allowed for set operators") } var ( - result = vector{} - sigf = signatureFunc(matching.Ignoring, matching.On...) - resultLabels = append(matching.On, matching.Include...) + result = vector{} + sigf = signatureFunc(matching.Ignoring, matching.On...) ) // The control flow below handles one-to-one or many-to-one matching. @@ -851,7 +850,7 @@ func (ev *evaluator) vectorBinop(op itemType, lhs, rhs vector, matching *VectorM } else if !keep { continue } - metric := resultMetric(ls.Metric, op, matching.Ignoring, resultLabels...) + metric := resultMetric(ls.Metric, rs.Metric, op, matching) insertedSigs, exists := matchedSigs[sig] if matching.Card == CardOneToOne { @@ -863,7 +862,7 @@ func (ev *evaluator) vectorBinop(op itemType, lhs, rhs vector, matching *VectorM // In many-to-one matching the grouping labels have to ensure a unique metric // for the result vector. Check whether those labels have already been added for // the same matching labels. - insertSig := model.SignatureForLabels(metric.Metric, matching.Include...) + insertSig := uint64(metric.Metric.Fingerprint()) if !exists { insertedSigs = map[uint64]struct{}{} matchedSigs[sig] = insertedSigs @@ -902,21 +901,43 @@ func signatureFunc(ignoring bool, labels ...model.LabelName) func(m metric.Metri // resultMetric returns the metric for the given sample(s) based on the vector // binary operation and the matching options. -func resultMetric(met metric.Metric, op itemType, ignoring bool, labels ...model.LabelName) metric.Metric { - if len(labels) == 0 || ignoring { +func resultMetric(lhs, rhs metric.Metric, op itemType, matching *VectorMatching) metric.Metric { + if len(matching.On)+len(matching.Include) == 0 { if shouldDropMetricName(op) { - met.Del(model.MetricNameLabel) + lhs.Del(model.MetricNameLabel) } - for _, l := range labels { - met.Del(l) + return lhs + } + if matching.Ignoring { + if shouldDropMetricName(op) { + lhs.Del(model.MetricNameLabel) } - return met + if matching.Card == CardOneToOne { + for _, l := range matching.On { + lhs.Del(l) + } + } + for _, ln := range matching.Include { + // Included labels from the `group_x` modifier are taken from the one side with Ignoring. + value := rhs.Metric[ln] + if value != "" { + lhs.Set(ln, rhs.Metric[ln]) + } else { + lhs.Del(ln) + } + } + return lhs } // As we definitely write, creating a new metric is the easiest solution. m := model.Metric{} - for _, ln := range labels { - // Included labels from the `group_x` modifier are taken from the "many"-side. - if v, ok := met.Metric[ln]; ok { + for _, ln := range matching.On { + if v, ok := lhs.Metric[ln]; ok { + m[ln] = v + } + } + for _, ln := range matching.Include { + // Included labels from the `group_x` modifier are taken from the "many"-side with On. + if v, ok := lhs.Metric[ln]; ok { m[ln] = v } } diff --git a/promql/parse.go b/promql/parse.go index ba246d424..b724b56d5 100644 --- a/promql/parse.go +++ b/promql/parse.go @@ -461,7 +461,7 @@ func (p *parser) expr() Expr { returnBool = true } - // Parse ON clause. + // Parse ON/IGNORING clause. if p.peek().typ == itemOn || p.peek().typ == itemIgnoring { if p.peek().typ == itemIgnoring { vecMatching.Ignoring = true @@ -470,24 +470,24 @@ func (p *parser) expr() Expr { vecMatching.On = p.labels() // Parse grouping. - if t := p.peek().typ; t == itemGroupLeft { + if t := p.peek().typ; t == itemGroupLeft || t == itemGroupRight { p.next() - vecMatching.Card = CardManyToOne - vecMatching.Include = p.labels() - } else if t == itemGroupRight { - p.next() - vecMatching.Card = CardOneToMany - vecMatching.Include = p.labels() + if t == itemGroupLeft { + vecMatching.Card = CardManyToOne + } else { + vecMatching.Card = CardOneToMany + } + if p.peek().typ == itemLeftParen { + vecMatching.Include = p.labels() + } else if !vecMatching.Ignoring { + p.errorf("must specify labels in INCLUDE clause when using ON") + } } } - if vecMatching.Ignoring && (vecMatching.Card == CardManyToOne || vecMatching.Card == CardOneToMany) { - p.errorf("IGNORING not permitted with many to one matching") - } - for _, ln := range vecMatching.On { for _, ln2 := range vecMatching.Include { - if ln == ln2 { + if ln == ln2 && !vecMatching.Ignoring { p.errorf("label %q must not occur in ON and INCLUDE clause at once", ln) } } diff --git a/promql/parse_test.go b/promql/parse_test.go index 22a07a138..db39ece5a 100644 --- a/promql/parse_test.go +++ b/promql/parse_test.go @@ -250,14 +250,14 @@ var testExpr = []struct { input: "1 offset 1d", fail: true, errMsg: "offset modifier must be preceded by an instant or range selector", + }, { + input: "a - on(b) group_left d", + fail: true, + errMsg: "must specify labels in INCLUDE clause when using ON", }, { input: "a - on(b) ignoring(c) d", fail: true, errMsg: "parse error at char 11: no valid expression found", - }, { - input: "a - ignoring(b) group_left(c) d", - fail: true, - errMsg: "parse error at char 29: IGNORING not permitted with many to one matching", }, // Vector binary operations. { @@ -590,6 +590,52 @@ var testExpr = []struct { Include: model.LabelNames{"bar"}, }, }, + }, { + input: "foo / ignoring(test,blub) group_left(blub) bar", + expected: &BinaryExpr{ + Op: itemDIV, + 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: CardManyToOne, + On: model.LabelNames{"test", "blub"}, + Include: model.LabelNames{"blub"}, + Ignoring: true, + }, + }, + }, { + input: "foo / ignoring(test,blub) group_left(bar) bar", + expected: &BinaryExpr{ + Op: itemDIV, + 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: CardManyToOne, + On: model.LabelNames{"test", "blub"}, + Include: model.LabelNames{"bar"}, + Ignoring: true, + }, + }, }, { input: "foo - on(test,blub) group_right(bar,foo) bar", expected: &BinaryExpr{ @@ -612,6 +658,29 @@ var testExpr = []struct { Include: model.LabelNames{"bar", "foo"}, }, }, + }, { + input: "foo - ignoring(test,blub) group_right(bar,foo) bar", + expected: &BinaryExpr{ + Op: itemSUB, + 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: CardOneToMany, + On: model.LabelNames{"test", "blub"}, + Include: model.LabelNames{"bar", "foo"}, + Ignoring: true, + }, + }, }, { input: "foo and 1", fail: true, @@ -671,7 +740,7 @@ var testExpr = []struct { }, { input: `http_requests{group="production"} / on(instance) group_left cpu_count{type="smp"}`, fail: true, - errMsg: "unexpected identifier \"cpu_count\" in grouping opts, expected \"(\"", + errMsg: "parse error at char 61: must specify labels in INCLUDE clause when using ON", }, { input: `http_requests{group="production"} + on(instance) group_left(job,instance) cpu_count{type="smp"}`, fail: true, diff --git a/promql/testdata/operators.test b/promql/testdata/operators.test index 1c095ebae..4b505374c 100644 --- a/promql/testdata/operators.test +++ b/promql/testdata/operators.test @@ -223,6 +223,9 @@ load 5m node_cpu{instance="def",job="node",mode="idle"} 8 node_cpu{instance="def",job="node",mode="user"} 2 +load 5m + random{foo="bar"} 1 + # Copy machine role to node variable. eval instant at 5m node_role * on (instance,job) group_left (role) node_var {instance="abc",job="node",role="prometheus"} 2 @@ -230,6 +233,18 @@ eval instant at 5m node_role * on (instance,job) group_left (role) node_var eval instant at 5m node_var * on (instance,job) group_right (role) node_role {instance="abc",job="node",role="prometheus"} 2 +eval instant at 5m node_var * ignoring (role) group_left (role) node_role + {instance="abc",job="node",role="prometheus"} 2 + +eval instant at 5m node_role * ignoring (role) group_right (role) node_var + {instance="abc",job="node",role="prometheus"} 2 + +# Copy machine role to node variable with instrumentation labels. +eval instant at 5m node_cpu * ignoring (role, mode) group_left (role) node_role + {instance="abc",job="node",mode="idle",role="prometheus"} 3 + {instance="abc",job="node",mode="user",role="prometheus"} 1 + + # Ratio of total. eval instant at 5m node_cpu / on (instance,job) group_left (mode) sum by (instance,job)(node_cpu) {instance="abc",job="node",mode="idle"} .75 @@ -243,3 +258,31 @@ eval instant at 5m sum by (mode, job)(node_cpu) / on (job) group_left (mode) sum eval instant at 5m sum(sum by (mode, job)(node_cpu) / on (job) group_left (mode) sum by (job)(node_cpu)) {} 1.0 + + +eval instant at 5m node_cpu / ignoring (mode) group_left sum without (mode)(node_cpu) + {instance="abc",job="node",mode="idle"} .75 + {instance="abc",job="node",mode="user"} .25 + {instance="def",job="node",mode="idle"} .80 + {instance="def",job="node",mode="user"} .20 + +eval instant at 5m node_cpu / ignoring (mode) group_left(dummy) sum without (mode)(node_cpu) + {instance="abc",job="node",mode="idle"} .75 + {instance="abc",job="node",mode="user"} .25 + {instance="def",job="node",mode="idle"} .80 + {instance="def",job="node",mode="user"} .20 + +eval instant at 5m sum without (instance)(node_cpu) / ignoring (mode) group_left sum without (instance, mode)(node_cpu) + {job="node",mode="idle"} 0.7857142857142857 + {job="node",mode="user"} 0.21428571428571427 + +eval instant at 5m sum(sum without (instance)(node_cpu) / ignoring (mode) group_left sum without (instance, mode)(node_cpu)) + {} 1.0 + + +# Copy over label from metric with no matching labels, without having to list cross-job target labels ('job' here). +#eval instant at 5m node_cpu + on(dummy) group_left(foo) random +# {instance="abc",job="node",mode="idle",foo="bar"} 3 +# {instance="abc",job="node",mode="user",foo="bar"} 1 +# {instance="def",job="node",mode="idle",foo="bar"} 8 +# {instance="def",job="node",mode="user",foo="bar"} 2 From 768d09fd2af923c1b3f0d63ffa5109a32fc2fdfe Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Thu, 21 Apr 2016 18:41:27 +0100 Subject: [PATCH 4/7] Change on+group_* to take copy from the one side. If the label doesn't exist on the one side, it's not copied. All labels on the many inside are included, this is a breaking change but likely low impact. --- promql/engine.go | 27 +++++++++++++++------------ promql/testdata/operators.test | 10 +++++----- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index d5b4cf801..141aa630c 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -902,23 +902,20 @@ func signatureFunc(ignoring bool, labels ...model.LabelName) func(m metric.Metri // resultMetric returns the metric for the given sample(s) based on the vector // binary operation and the matching options. func resultMetric(lhs, rhs metric.Metric, op itemType, matching *VectorMatching) metric.Metric { + if shouldDropMetricName(op) { + lhs.Del(model.MetricNameLabel) + } if len(matching.On)+len(matching.Include) == 0 { - if shouldDropMetricName(op) { - lhs.Del(model.MetricNameLabel) - } return lhs } if matching.Ignoring { - if shouldDropMetricName(op) { - lhs.Del(model.MetricNameLabel) - } if matching.Card == CardOneToOne { for _, l := range matching.On { lhs.Del(l) } } for _, ln := range matching.Include { - // Included labels from the `group_x` modifier are taken from the one side with Ignoring. + // Included labels from the `group_x` modifier are taken from the "one"-side. value := rhs.Metric[ln] if value != "" { lhs.Set(ln, rhs.Metric[ln]) @@ -930,14 +927,20 @@ func resultMetric(lhs, rhs metric.Metric, op itemType, matching *VectorMatching) } // As we definitely write, creating a new metric is the easiest solution. m := model.Metric{} - for _, ln := range matching.On { - if v, ok := lhs.Metric[ln]; ok { - m[ln] = v + if matching.Card == CardOneToOne { + for _, ln := range matching.On { + if v, ok := lhs.Metric[ln]; ok { + m[ln] = v + } + } + } else { + for k, v := range lhs.Metric { + m[k] = v } } for _, ln := range matching.Include { - // Included labels from the `group_x` modifier are taken from the "many"-side with On. - if v, ok := lhs.Metric[ln]; ok { + // Included labels from the `group_x` modifier are taken from the "one"-side . + if v, ok := rhs.Metric[ln]; ok { m[ln] = v } } diff --git a/promql/testdata/operators.test b/promql/testdata/operators.test index 4b505374c..83ec8879f 100644 --- a/promql/testdata/operators.test +++ b/promql/testdata/operators.test @@ -281,8 +281,8 @@ eval instant at 5m sum(sum without (instance)(node_cpu) / ignoring (mode) group_ # Copy over label from metric with no matching labels, without having to list cross-job target labels ('job' here). -#eval instant at 5m node_cpu + on(dummy) group_left(foo) random -# {instance="abc",job="node",mode="idle",foo="bar"} 3 -# {instance="abc",job="node",mode="user",foo="bar"} 1 -# {instance="def",job="node",mode="idle",foo="bar"} 8 -# {instance="def",job="node",mode="user",foo="bar"} 2 +eval instant at 5m node_cpu + on(dummy) group_left(foo) random*0 + {instance="abc",job="node",mode="idle",foo="bar"} 3 + {instance="abc",job="node",mode="user",foo="bar"} 1 + {instance="def",job="node",mode="idle",foo="bar"} 8 + {instance="def",job="node",mode="user",foo="bar"} 2 From d991f0cf47fd00f55c893043dbad2c4d34423c94 Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Thu, 21 Apr 2016 19:03:10 +0100 Subject: [PATCH 5/7] For many-to-one matches, always copy label from one side. This is a breaking change for everyone using the machine roles labeling approach. --- promql/engine.go | 2 ++ promql/parse.go | 2 -- promql/parse_test.go | 29 +++++++++++++++++++++-------- promql/testdata/operators.test | 14 +++++++++----- 4 files changed, 32 insertions(+), 15 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 141aa630c..6d1148fe5 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -942,6 +942,8 @@ func resultMetric(lhs, rhs metric.Metric, op itemType, matching *VectorMatching) // Included labels from the `group_x` modifier are taken from the "one"-side . if v, ok := rhs.Metric[ln]; ok { m[ln] = v + } else { + delete(m, ln) } } return metric.Metric{Metric: m, Copied: false} diff --git a/promql/parse.go b/promql/parse.go index b724b56d5..c4b055f37 100644 --- a/promql/parse.go +++ b/promql/parse.go @@ -479,8 +479,6 @@ func (p *parser) expr() Expr { } if p.peek().typ == itemLeftParen { vecMatching.Include = p.labels() - } else if !vecMatching.Ignoring { - p.errorf("must specify labels in INCLUDE clause when using ON") } } } diff --git a/promql/parse_test.go b/promql/parse_test.go index db39ece5a..d7c5a4dab 100644 --- a/promql/parse_test.go +++ b/promql/parse_test.go @@ -250,10 +250,6 @@ var testExpr = []struct { input: "1 offset 1d", fail: true, errMsg: "offset modifier must be preceded by an instant or range selector", - }, { - input: "a - on(b) group_left d", - fail: true, - errMsg: "must specify labels in INCLUDE clause when using ON", }, { input: "a - on(b) ignoring(c) d", fail: true, @@ -504,6 +500,27 @@ var testExpr = []struct { On: model.LabelNames{"test", "blub"}, }, }, + }, { + input: "foo * on(test,blub) group_left bar", + expected: &BinaryExpr{ + Op: itemMUL, + 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: CardManyToOne, + On: model.LabelNames{"test", "blub"}, + }, + }, }, { input: "foo and on(test,blub) bar", expected: &BinaryExpr{ @@ -737,10 +754,6 @@ var testExpr = []struct { input: "foo unless on(bar) group_right(baz) bar", fail: true, errMsg: "no grouping allowed for \"unless\" operation", - }, { - input: `http_requests{group="production"} / on(instance) group_left cpu_count{type="smp"}`, - fail: true, - errMsg: "parse error at char 61: must specify labels in INCLUDE clause when using ON", }, { input: `http_requests{group="production"} + on(instance) group_left(job,instance) cpu_count{type="smp"}`, fail: true, diff --git a/promql/testdata/operators.test b/promql/testdata/operators.test index 83ec8879f..b070a7281 100644 --- a/promql/testdata/operators.test +++ b/promql/testdata/operators.test @@ -227,10 +227,10 @@ load 5m random{foo="bar"} 1 # Copy machine role to node variable. -eval instant at 5m node_role * on (instance,job) group_left (role) node_var +eval instant at 5m node_role * on (instance) group_right (role) node_var {instance="abc",job="node",role="prometheus"} 2 -eval instant at 5m node_var * on (instance,job) group_right (role) node_role +eval instant at 5m node_var * on (instance) group_left (role) node_role {instance="abc",job="node",role="prometheus"} 2 eval instant at 5m node_var * ignoring (role) group_left (role) node_role @@ -244,19 +244,23 @@ eval instant at 5m node_cpu * ignoring (role, mode) group_left (role) node_role {instance="abc",job="node",mode="idle",role="prometheus"} 3 {instance="abc",job="node",mode="user",role="prometheus"} 1 +eval instant at 5m node_cpu * on (instance) group_left (role) node_role + {instance="abc",job="node",mode="idle",role="prometheus"} 3 + {instance="abc",job="node",mode="user",role="prometheus"} 1 + # Ratio of total. -eval instant at 5m node_cpu / on (instance,job) group_left (mode) sum by (instance,job)(node_cpu) +eval instant at 5m node_cpu / on (instance) group_left sum by (instance,job)(node_cpu) {instance="abc",job="node",mode="idle"} .75 {instance="abc",job="node",mode="user"} .25 {instance="def",job="node",mode="idle"} .80 {instance="def",job="node",mode="user"} .20 -eval instant at 5m sum by (mode, job)(node_cpu) / on (job) group_left (mode) sum by (job)(node_cpu) +eval instant at 5m sum by (mode, job)(node_cpu) / on (job) group_left sum by (job)(node_cpu) {job="node",mode="idle"} 0.7857142857142857 {job="node",mode="user"} 0.21428571428571427 -eval instant at 5m sum(sum by (mode, job)(node_cpu) / on (job) group_left (mode) sum by (job)(node_cpu)) +eval instant at 5m sum(sum by (mode, job)(node_cpu) / on (job) group_left sum by (job)(node_cpu)) {} 1.0 From 7201c010c4688fb5d43f2761ce2a799218504870 Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Tue, 26 Apr 2016 14:28:36 +0100 Subject: [PATCH 6/7] Rename On to MatchingLabels --- promql/ast.go | 6 ++-- promql/engine.go | 14 ++++----- promql/parse.go | 6 ++-- promql/parse_test.go | 68 ++++++++++++++++++++++---------------------- promql/printer.go | 6 ++-- 5 files changed, 50 insertions(+), 50 deletions(-) diff --git a/promql/ast.go b/promql/ast.go index 5a7e24caf..35719afc2 100644 --- a/promql/ast.go +++ b/promql/ast.go @@ -228,9 +228,9 @@ func (vmc VectorMatchCardinality) String() string { type VectorMatching struct { // The cardinality of the two vectors. Card VectorMatchCardinality - // On contains the labels which define equality of a pair - // of elements from the vectors. - On model.LabelNames + // 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 diff --git a/promql/engine.go b/promql/engine.go index 6d1148fe5..b0742ad8d 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.On...) + sigf := signatureFunc(matching.Ignoring, 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.On...) + sigf := signatureFunc(matching.Ignoring, 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.On...) + sigf := signatureFunc(matching.Ignoring, 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.On...) + sigf = signatureFunc(matching.Ignoring, matching.MatchingLabels...) ) // The control flow below handles one-to-one or many-to-one matching. @@ -905,12 +905,12 @@ func resultMetric(lhs, rhs metric.Metric, op itemType, matching *VectorMatching) if shouldDropMetricName(op) { lhs.Del(model.MetricNameLabel) } - if len(matching.On)+len(matching.Include) == 0 { + if len(matching.MatchingLabels)+len(matching.Include) == 0 { return lhs } if matching.Ignoring { if matching.Card == CardOneToOne { - for _, l := range matching.On { + for _, l := range matching.MatchingLabels { lhs.Del(l) } } @@ -928,7 +928,7 @@ func resultMetric(lhs, rhs metric.Metric, op itemType, matching *VectorMatching) // As we definitely write, creating a new metric is the easiest solution. m := model.Metric{} if matching.Card == CardOneToOne { - for _, ln := range matching.On { + for _, ln := range matching.MatchingLabels { if v, ok := lhs.Metric[ln]; ok { m[ln] = v } diff --git a/promql/parse.go b/promql/parse.go index c4b055f37..79cd37aeb 100644 --- a/promql/parse.go +++ b/promql/parse.go @@ -467,7 +467,7 @@ func (p *parser) expr() Expr { vecMatching.Ignoring = true } p.next() - vecMatching.On = p.labels() + vecMatching.MatchingLabels = p.labels() // Parse grouping. if t := p.peek().typ; t == itemGroupLeft || t == itemGroupRight { @@ -483,7 +483,7 @@ func (p *parser) expr() Expr { } } - for _, ln := range vecMatching.On { + for _, ln := range vecMatching.MatchingLabels { for _, ln2 := range vecMatching.Include { if ln == ln2 && !vecMatching.Ignoring { p.errorf("label %q must not occur in ON and INCLUDE clause at once", ln) @@ -1052,7 +1052,7 @@ func (p *parser) checkType(node Node) (typ model.ValueType) { } if (lt != model.ValVector || rt != model.ValVector) && n.VectorMatching != nil { - if len(n.VectorMatching.On) > 0 { + if len(n.VectorMatching.MatchingLabels) > 0 { p.errorf("vector matching only allowed between vectors") } n.VectorMatching = nil diff --git a/promql/parse_test.go b/promql/parse_test.go index d7c5a4dab..94f06d7e0 100644 --- a/promql/parse_test.go +++ b/promql/parse_test.go @@ -469,14 +469,14 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardOneToMany, - On: model.LabelNames{"baz", "buz"}, - Include: model.LabelNames{"test"}, + Card: CardOneToMany, + MatchingLabels: model.LabelNames{"baz", "buz"}, + Include: model.LabelNames{"test"}, }, }, VectorMatching: &VectorMatching{ - Card: CardOneToOne, - On: model.LabelNames{"foo"}, + Card: CardOneToOne, + MatchingLabels: model.LabelNames{"foo"}, }, }, }, { @@ -496,8 +496,8 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardOneToOne, - On: model.LabelNames{"test", "blub"}, + Card: CardOneToOne, + MatchingLabels: model.LabelNames{"test", "blub"}, }, }, }, { @@ -517,8 +517,8 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardManyToOne, - On: model.LabelNames{"test", "blub"}, + Card: CardManyToOne, + MatchingLabels: model.LabelNames{"test", "blub"}, }, }, }, { @@ -538,8 +538,8 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardManyToMany, - On: model.LabelNames{"test", "blub"}, + Card: CardManyToMany, + MatchingLabels: model.LabelNames{"test", "blub"}, }, }, }, { @@ -559,9 +559,9 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardManyToMany, - On: model.LabelNames{"test", "blub"}, - Ignoring: true, + Card: CardManyToMany, + MatchingLabels: model.LabelNames{"test", "blub"}, + Ignoring: true, }, }, }, { @@ -581,8 +581,8 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardManyToMany, - On: model.LabelNames{"bar"}, + Card: CardManyToMany, + MatchingLabels: model.LabelNames{"bar"}, }, }, }, { @@ -602,9 +602,9 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardManyToOne, - On: model.LabelNames{"test", "blub"}, - Include: model.LabelNames{"bar"}, + Card: CardManyToOne, + MatchingLabels: model.LabelNames{"test", "blub"}, + Include: model.LabelNames{"bar"}, }, }, }, { @@ -624,10 +624,10 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardManyToOne, - On: model.LabelNames{"test", "blub"}, - Include: model.LabelNames{"blub"}, - Ignoring: true, + Card: CardManyToOne, + MatchingLabels: model.LabelNames{"test", "blub"}, + Include: model.LabelNames{"blub"}, + Ignoring: true, }, }, }, { @@ -647,10 +647,10 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardManyToOne, - On: model.LabelNames{"test", "blub"}, - Include: model.LabelNames{"bar"}, - Ignoring: true, + Card: CardManyToOne, + MatchingLabels: model.LabelNames{"test", "blub"}, + Include: model.LabelNames{"bar"}, + Ignoring: true, }, }, }, { @@ -670,9 +670,9 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardOneToMany, - On: model.LabelNames{"test", "blub"}, - Include: model.LabelNames{"bar", "foo"}, + Card: CardOneToMany, + MatchingLabels: model.LabelNames{"test", "blub"}, + Include: model.LabelNames{"bar", "foo"}, }, }, }, { @@ -692,10 +692,10 @@ var testExpr = []struct { }, }, VectorMatching: &VectorMatching{ - Card: CardOneToMany, - On: model.LabelNames{"test", "blub"}, - Include: model.LabelNames{"bar", "foo"}, - Ignoring: true, + 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 06b810b90..82ac53812 100644 --- a/promql/printer.go +++ b/promql/printer.go @@ -159,11 +159,11 @@ func (node *BinaryExpr) String() string { matching := "" vm := node.VectorMatching - if vm != nil && len(vm.On) > 0 { + if vm != nil && len(vm.MatchingLabels) > 0 { if vm.Ignoring { - matching = fmt.Sprintf(" IGNORING(%s)", vm.On) + matching = fmt.Sprintf(" IGNORING(%s)", vm.MatchingLabels) } else { - matching = fmt.Sprintf(" ON(%s)", vm.On) + matching = fmt.Sprintf(" ON(%s)", vm.MatchingLabels) } if vm.Card == CardManyToOne { matching += fmt.Sprintf(" GROUP_LEFT(%s)", vm.Include) From 68e70d992a304ea692a496fe43a3ad2f7af14cc0 Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Tue, 26 Apr 2016 14:31:00 +0100 Subject: [PATCH 7/7] Clarify error message around on(x) group_left(x) --- promql/ast.go | 2 +- promql/parse.go | 2 +- promql/parse_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/promql/ast.go b/promql/ast.go index 35719afc2..f8eaa3e15 100644 --- a/promql/ast.go +++ b/promql/ast.go @@ -235,7 +235,7 @@ type VectorMatching struct { // rather than only using them. Ignoring bool // Include contains additional labels that should be included in - // the result from the side with the higher cardinality. + // the result from the side with the lower cardinality. Include model.LabelNames } diff --git a/promql/parse.go b/promql/parse.go index 79cd37aeb..38911775f 100644 --- a/promql/parse.go +++ b/promql/parse.go @@ -486,7 +486,7 @@ func (p *parser) expr() Expr { for _, ln := range vecMatching.MatchingLabels { for _, ln2 := range vecMatching.Include { if ln == ln2 && !vecMatching.Ignoring { - p.errorf("label %q must not occur in ON and INCLUDE clause at once", ln) + 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 94f06d7e0..e5d1ea8a3 100644 --- a/promql/parse_test.go +++ b/promql/parse_test.go @@ -757,7 +757,7 @@ var testExpr = []struct { }, { input: `http_requests{group="production"} + on(instance) group_left(job,instance) cpu_count{type="smp"}`, fail: true, - errMsg: "label \"instance\" must not occur in ON and INCLUDE clause at once", + errMsg: "label \"instance\" must not occur in ON and GROUP clause at once", }, { input: "foo + bool bar", fail: true,