Merge pull request #667 from raintank/expr-naming

fixes for expr naming

api/graphite.go

@@ -129,18 +129,16 @@ 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
 	if to == "" {
 		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

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)

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},

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,

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)

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,

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