From 1d2a91a5e7c089d7713ff6527e6214681419e480 Mon Sep 17 00:00:00 2001 From: Dieter Plaetinck Date: Fri, 23 Jun 2017 12:27:22 +0200 Subject: [PATCH 1/7] add a test to validate returned target for multiple times same input arg --- expr/data_test.go | 9 ++++++++ expr/func_avgseries_test.go | 46 +++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/expr/data_test.go b/expr/data_test.go index 697c4b27fc..4da41cc5bc 100644 --- a/expr/data_test.go +++ b/expr/data_test.go @@ -24,6 +24,15 @@ var b = []schema.Point{ {Val: math.NaN(), Ts: 60}, } +var avg4a2b = []schema.Point{ + {Val: 0, Ts: 10}, + {Val: math.Inf(0), Ts: 20}, + {Val: math.Inf(0), Ts: 30}, + {Val: math.NaN(), Ts: 40}, + {Val: 1234567890, Ts: 50}, // in accordance with graphite, avg(5,null) = 5 + {Val: 1234567890, Ts: 60}, +} + var c = []schema.Point{ {Val: 0, Ts: 10}, {Val: 0, Ts: 20}, diff --git a/expr/func_avgseries_test.go b/expr/func_avgseries_test.go index cde2907164..93ca47d81a 100644 --- a/expr/func_avgseries_test.go +++ b/expr/func_avgseries_test.go @@ -97,6 +97,52 @@ func TestAvgSeriesMultipleDiffQuery(t *testing.T) { ) } +//mimic target=avgSeries(foo.*,foo.*,a,a) +func TestAvgSeriesMultipleTimesSameInput(t *testing.T) { + testAvgSeries( + "avg-multiple-times-same-input", + [][]models.Series{ + { + { + QueryPatt: "foo.*", + Datapoints: getCopy(a), + }, + { + QueryPatt: "foo.*", + Datapoints: getCopy(b), + }, + }, + { + { + QueryPatt: "foo.*", + Datapoints: getCopy(a), + }, + { + QueryPatt: "foo.*", + Datapoints: getCopy(b), + }, + }, + { + { + QueryPatt: "a", + Datapoints: getCopy(a), + }, + }, + { + { + QueryPatt: "a", + Datapoints: getCopy(a), + }, + }, + }, + models.Series{ + Target: "averageSeries(foo.*,foo.*,a,a)", + Datapoints: getCopy(avg4a2b), + }, + t, + ) +} + func testAvgSeries(name string, in [][]models.Series, out models.Series, t *testing.T) { f := NewAvgSeries() avg := f.(*FuncAvgSeries) From 62a9e891138c267c1dd7832740ea9a9cc9a4e394 Mon Sep 17 00:00:00 2001 From: Dieter Plaetinck Date: Fri, 23 Jun 2017 12:42:01 +0200 Subject: [PATCH 2/7] fix display of sum/avg args with multiple args that may be the same previous approach assumed the same queryPattern was never specified multiple times, which was wrong. this approach is also simpler and faster --- expr/func_avgseries.go | 5 ++++- expr/func_sumseries.go | 5 ++++- expr/helper.go | 20 -------------------- 3 files changed, 8 insertions(+), 22 deletions(-) delete mode 100644 expr/helper.go diff --git a/expr/func_avgseries.go b/expr/func_avgseries.go index 8564253c1a..78e57d6ebb 100644 --- a/expr/func_avgseries.go +++ b/expr/func_avgseries.go @@ -3,6 +3,7 @@ package expr import ( "fmt" "math" + "strings" "github.com/raintank/metrictank/api/models" "gopkg.in/raintank/schema.v1" @@ -28,12 +29,14 @@ func (s *FuncAvgSeries) Context(context Context) Context { func (s *FuncAvgSeries) Exec(cache map[Req][]models.Series) ([]models.Series, error) { var series []models.Series + var queryPatts []string for i := range s.in { in, err := s.in[i].Exec(cache) if err != nil { return nil, err } series = append(series, in...) + queryPatts = append(queryPatts, in[0].QueryPatt) } if len(series) == 0 { @@ -69,7 +72,7 @@ func (s *FuncAvgSeries) Exec(cache map[Req][]models.Series) ([]models.Series, er } cons, queryCons := summarizeCons(series) - name := fmt.Sprintf("averageSeries(%s)", patternsAsArgs(series)) + name := fmt.Sprintf("averageSeries(%s)", strings.Join(queryPatts, ",")) output := models.Series{ Target: name, QueryPatt: name, diff --git a/expr/func_sumseries.go b/expr/func_sumseries.go index d401502d29..a2fbabdc7c 100644 --- a/expr/func_sumseries.go +++ b/expr/func_sumseries.go @@ -3,6 +3,7 @@ package expr import ( "fmt" "math" + "strings" "github.com/raintank/metrictank/api/models" "gopkg.in/raintank/schema.v1" @@ -28,12 +29,14 @@ func (s *FuncSumSeries) Context(context Context) Context { func (s *FuncSumSeries) Exec(cache map[Req][]models.Series) ([]models.Series, error) { var series []models.Series + var queryPatts []string for i := range s.in { in, err := s.in[i].Exec(cache) if err != nil { return nil, err } series = append(series, in...) + queryPatts = append(queryPatts, in[0].QueryPatt) } if len(series) == 0 { @@ -64,7 +67,7 @@ func (s *FuncSumSeries) Exec(cache map[Req][]models.Series) ([]models.Series, er } out = append(out, point) } - name := fmt.Sprintf("sumSeries(%s)", patternsAsArgs(series)) + name := fmt.Sprintf("sumSeries(%s)", strings.Join(queryPatts, ",")) cons, queryCons := summarizeCons(series) output := models.Series{ Target: name, diff --git a/expr/helper.go b/expr/helper.go deleted file mode 100644 index 984f39fbdb..0000000000 --- a/expr/helper.go +++ /dev/null @@ -1,20 +0,0 @@ -package expr - -import ( - "strings" - - "github.com/raintank/metrictank/api/models" -) - -func patternsAsArgs(series []models.Series) string { - var list []string - set := make(map[string]struct{}) - for _, serie := range series { - if _, ok := set[serie.QueryPatt]; ok { - continue - } - list = append(list, serie.QueryPatt) - set[serie.QueryPatt] = struct{}{} - } - return strings.Join(list, ",") -} From 55a499a2bed1c6816a491b140fc36def89d0d4cd Mon Sep 17 00:00:00 2001 From: Dieter Plaetinck Date: Fri, 23 Jun 2017 12:47:06 +0200 Subject: [PATCH 3/7] remove stale prints --- expr/plan_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/expr/plan_test.go b/expr/plan_test.go index 439451ecfe..77d853aaa3 100644 --- a/expr/plan_test.go +++ b/expr/plan_test.go @@ -1,7 +1,6 @@ package expr import ( - "fmt" "reflect" "testing" @@ -269,7 +268,6 @@ func TestConsolidateBy(t *testing.T) { } for i, c := range cases { - fmt.Println("#####", c.in) // for the purpose of this test, we assume ParseMany works fine. exprs, _ := ParseMany([]string{c.in}) plan, err := NewPlan(exprs, from, to, 800, stable, nil) From e4ecb06b4e7dc5041293eee5183e71697bbf50a6 Mon Sep 17 00:00:00 2001 From: Dieter Plaetinck Date: Fri, 23 Jun 2017 12:53:00 +0200 Subject: [PATCH 4/7] simplify/reorder a bit for clarity --- api/graphite.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/api/graphite.go b/api/graphite.go index 999cc66c96..749752e499 100644 --- a/api/graphite.go +++ b/api/graphite.go @@ -129,8 +129,9 @@ func (s *Server) findSeriesRemote(orgId int, patterns []string, seenAfter int64, } func (s *Server) renderMetrics(ctx *middleware.Context, request models.GraphiteRender) { - targets := request.Targets now := time.Now() + defaultFrom := uint32(now.Add(-time.Duration(24) * time.Hour).Unix()) + defaultTo := uint32(now.Unix()) from := request.From to := request.To @@ -138,9 +139,6 @@ func (s *Server) renderMetrics(ctx *middleware.Context, request models.GraphiteR to = request.Until } - defaultFrom := uint32(now.Add(-time.Duration(24) * time.Hour).Unix()) - defaultTo := uint32(now.Unix()) - fromUnix, err := dur.ParseTSpec(from, now, defaultFrom) if err != nil { response.Write(ctx, response.NewError(http.StatusBadRequest, err.Error())) @@ -164,13 +162,13 @@ func (s *Server) renderMetrics(ctx *middleware.Context, request models.GraphiteR fromUnix += 1 toUnix += 1 - exprs, err := expr.ParseMany(targets) + exprs, err := expr.ParseMany(request.Targets) if err != nil { ctx.Error(http.StatusBadRequest, err.Error()) return } - reqRenderTargetCount.Value(len(targets)) + reqRenderTargetCount.Value(len(request.Targets)) if request.Process == "none" { ctx.Req.Request.Body = ctx.Body From 2d9ed2dec043c52bc5c9e9ff9ac7af5dd7d7112d Mon Sep 17 00:00:00 2001 From: Dieter Plaetinck Date: Fri, 23 Jun 2017 13:31:08 +0200 Subject: [PATCH 5/7] avoid slice out of bounds --- expr/plan_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/expr/plan_test.go b/expr/plan_test.go index 77d853aaa3..1779a199d5 100644 --- a/expr/plan_test.go +++ b/expr/plan_test.go @@ -304,10 +304,11 @@ func TestConsolidateBy(t *testing.T) { } if len(out) != len(c.expOut) { t.Errorf("case %d: %q, expected %d series output, not %d", i, c.in, len(c.expOut), len(out)) - } - for j, exp := range c.expOut { - if exp.QueryPatt != out[j].QueryPatt || exp.Consolidator != out[j].Consolidator { - t.Errorf("case %d: %q, output series mismatch at pos %d: expected %v-%v - got %v-%v", i, c.in, j, exp.QueryPatt, exp.Consolidator, out[j].QueryPatt, out[j].Consolidator) + } else { + for j, exp := range c.expOut { + if exp.QueryPatt != out[j].QueryPatt || exp.Consolidator != out[j].Consolidator { + t.Errorf("case %d: %q, output series mismatch at pos %d: expected %v-%v - got %v-%v", i, c.in, j, exp.QueryPatt, exp.Consolidator, out[j].QueryPatt, out[j].Consolidator) + } } } } From d126322702d1a1840a694d6e4cf135266f9b8ab8 Mon Sep 17 00:00:00 2001 From: Dieter Plaetinck Date: Fri, 23 Jun 2017 14:27:00 +0200 Subject: [PATCH 6/7] add new tests for target names after longer function chains see #648 --- expr/plan_test.go | 145 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/expr/plan_test.go b/expr/plan_test.go index 1779a199d5..d91383c5a6 100644 --- a/expr/plan_test.go +++ b/expr/plan_test.go @@ -313,3 +313,148 @@ func TestConsolidateBy(t *testing.T) { } } } + +// TestNamingChains tests whether series names (targets) are correct, after a processing chain of multiple functions +func TestNamingChains(t *testing.T) { + from := uint32(1000) + to := uint32(2000) + stable := true + cases := []struct { + target string // target request "from user" + keys []string + expOut []string + }{ + // the first two are cases that we've seen in the wild + // see https://github.com/raintank/metrictank/issues/648 + { + `aliasSub(perSecond(metrictank.stats.*.*.input.*.metric_invalid.counter32),'.*\.([^\.]+)\.metric_invalid.*', '\1 metric invalid')`, + []string{ + "metrictank.stats.env.instance1.input.carbon.metric_invalid.counter32", + "metrictank.stats.env.instance2.input.carbon.metric_invalid.counter32", + "metrictank.stats.env.instance1.input.kafka.metric_invalid.counter32", + }, + []string{ + "carbon metric invalid", + "carbon metric invalid", + "kafka metric invalid", + }, + }, + { + `aliasSub(sumSeries(perSecond(metrictank.stats.*.*.input.*.metric_invalid.counter32)),'.*\.([^\.]+)\.metric_invalid.*', '\1 metric invalid')`, + []string{ + "metrictank.stats.env.instance1.input.carbon.metric_invalid.counter32", + "metrictank.stats.env.instance2.input.carbon.metric_invalid.counter32", + "metrictank.stats.env.instance1.input.kafka.metric_invalid.counter32", + }, + []string{ + "* metric invalid", + }, + }, + { + `aliasSub(sumSeries(perSecond(metrictank.stats.*.*.input.*.metric_invalid.counter32)),'.*\.([^\.]+)\.metric_invalid.*', '\1 metric invalid')`, + []string{ + "metrictank.stats.env.instance1.input.carbon.metric_invalid.counter32", + }, + []string{ + "* metric invalid", // could be argued that "carbon metric invalid" is more useful, but is less consistent. see #648 + }, + }, + // what follows here is a simplified, but comparable test case, for other alias functions + { + `aliasByNode(perSecond(*.bar), 0)`, + []string{ + "a.bar", + "b.bar", + }, + []string{ + "a", + "b", + }, + }, + { + `aliasByNode(avg(perSecond(*.bar)), 0)`, + []string{ + "a.bar", + "b.bar", + }, + []string{ + "*", + }, + }, + { + `aliasByNode(avg(perSecond(*.bar)), 0)`, + []string{ + "a.bar", + }, + []string{ + "*", // "a" could be more useful but see #648 + }, + }, + { + `alias(perSecond(*.bar), 'a')`, + []string{ + "a.bar", + "b.bar", + }, + []string{ + "a", + "a", + }, + }, + { + `alias(avg(perSecond(*.bar)), 'a')`, + []string{ + "a", + "b", + }, + []string{ + "a", + }, + }, + { + `alias(avg(perSecond(*.bar)), 'a')`, + []string{ + "a", + }, + []string{ + "a", + }, + }, + } + + for i, c := range cases { + exprs, err := ParseMany([]string{c.target}) + if err != nil { + t.Fatal(err) + } + plan, err := NewPlan(exprs, from, to, 800, stable, nil) + if err != nil { + t.Fatal(err) + } + + // create input data + series := make([]models.Series, len(c.keys)) + for j, key := range c.keys { + series[j] = models.Series{ + QueryPatt: plan.Reqs[0].Query, + Target: key, + } + } + input := map[Req][]models.Series{ + plan.Reqs[0]: series, + } + out, err := plan.Run(input) + if err != nil { + t.Fatal(err) + } + if len(out) != len(c.expOut) { + t.Errorf("case %d:\n%q with %d inputs:\nexpected %d series output, not %d", i, c.target, len(c.keys), len(c.expOut), len(out)) + } else { + for j, exp := range c.expOut { + if out[j].Target != exp { + t.Errorf("case %d:\n%q with %d inputs:\noutput series mismatch at pos %d:\nexp: %v\ngot: %v", i, c.target, len(c.keys), j, exp, out[j].Target) + } + } + } + } +} From 15ecc2a588a3c9c4a07c106df75d408ec602195b Mon Sep 17 00:00:00 2001 From: Dieter Plaetinck Date: Fri, 23 Jun 2017 14:53:40 +0200 Subject: [PATCH 7/7] fix/better docs --- expr/NOTES | 24 ++++++++++++------------ expr/plan.go | 6 +++--- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/expr/NOTES b/expr/NOTES index 76c14e46d5..243faba261 100644 --- a/expr/NOTES +++ b/expr/NOTES @@ -93,22 +93,22 @@ see also https://github.com/raintank/metrictank/issues/463#issuecomment-27519988 ## naming -note that when requesting series directly, we want to show the target (to uniquely identify them), not the queryPattern -it follows that the output returned by plan.Run must always use Target attribute to show the proper name, and that any processing functions thus must set that attribute properly. +when requesting series directly, we want to show the target (to uniquely identify each series), not the queryPattern +thus the output returned by plan.Run must always use Target attribute to show the proper name +it follows that any processing functions must set that attribute properly. but some functions (e.g. sumSeries) take queryPattern of their inputs, to show summary query pattern instead of each individual value. -thus, to accomodate wrapping functions as well as lack of wrapping functions (e.g. generating json output), processing functions must set both Target and QueryPatt, and they -should set it to the same value. +thus, to accomodate wrapping functions (which may use QueryPattern) as well as lack of wrapping functions (e.g. generating json output which will look at Target), processing functions must set both Target and QueryPatt, and they should set it to the same value. in a future version we can probably refactor things to make this simpler: fetched data could result in a series with key attribute the graphite key (currently stored in target), and store the query pattern used in the target attribute. functions only set target and only look at target, and at output phase, we just use target. there's only 1 special case of metrics returned without function processing. we could detect this case, or if we have multiple things with same target, print the metric name instead, or something examples: (series are in `series{target,querypatt}` form) -input => results in fetched series output targets +input => results in fetched series output targets ================================================================================================================== -target=a&target=ab => series{a, a }, series{ab,ab} a and ab -target=a* => series{a, a*}, series{ab,a*} a and ab -target=sum(a,ab) => series{a, a }, series{ab,ab} sum(a,ab) -target=sum(a*) => series{a, a*}, series{ab,a*} sum(a*) -target=sum(a,a,ab) => series{a, a }, series{a,a }, series{ab, ab} sum(a,a,ab) (bug in graphite! it has sum(a,ab) ) -target=sum(a,a*) => series{a, a }, series{a,a* }, series{ab, a*} sum(a,a*) -target=sum(a,a,a*) => series{a, a }, series{a,a }, series{a, ab*}, series{ab, ab*} sum(a,a,a*) (bug in graphite! it has sum(a,a*), doesn't even add the extra a) +target=a&target=ab => series{a, a }, series{ab,ab} a and ab +target=a* => series{a, a*}, series{ab,a*} a and ab +target=sum(a,ab) => series{a, a }, series{ab,ab} sum(a,ab) +target=sum(a*) => series{a, a*}, series{ab,a*} sum(a*) +target=sum(a,a,ab) => series{a, a }, series{a,a }, series{ab, ab} sum(a,a,ab) (bug in graphite! it has sum(a,ab) ) +target=sum(a,a*) => series{a, a }, series{a,a* }, series{ab, a*} sum(a,a*) +target=sum(a,a,a*) => series{a, a }, series{a,a }, series{a, a*}, series{ab, a*} sum(a,a,a*) (bug in graphite! it has sum(a,a*), doesn't even add the extra a) diff --git a/expr/plan.go b/expr/plan.go index e3147b3dde..9a6e29bd12 100644 --- a/expr/plan.go +++ b/expr/plan.go @@ -10,6 +10,7 @@ import ( "github.com/raintank/metrictank/consolidation" ) +// Req represents a request for one/more series type Req struct { Query string // whatever was parsed as the query out of a graphite target. e.g. target=sum(foo.{b,a}r.*) -> foo.{b,a}r.* -> this will go straight to index lookup From uint32 @@ -60,9 +61,8 @@ func (p Plan) Dump(w io.Writer) { // which is just a list of requests and the expressions. // traverse tree and as we go down: // * make sure function exists -// * tentative validation pre function call (number of args and type of args, to the extent it can be done in advance), -// * let function validate input arguments further (to the extend it can be done in advance) -// * allow functions to extend the notion of which data is required +// * validation of arguments +// * allow functions to modify the Context (change data range or consolidation) // * future version: allow functions to mark safe to pre-aggregate using consolidateBy or not func NewPlan(exprs []*expr, from, to, mdp uint32, stable bool, reqs []Req) (Plan, error) { var err error