[query] Add quantile_over_time #1367

benraskin92 · 2019-02-11T01:20:15Z

No description provided.

codecov · 2019-02-11T16:28:53Z

Codecov Report

Merging #1367 into master will decrease coverage by 7.5%.
The diff coverage is 2%.

@@           Coverage Diff            @@
##           master   #1367     +/-   ##
========================================
- Coverage    70.6%     63%   -7.6%     
========================================
  Files         823     817      -6     
  Lines       70369   70004    -365     
========================================
- Hits        49692   44169   -5523     
- Misses      17455   22797   +5342     
+ Partials     3222    3038    -184

Flag	Coverage Δ
#aggregator	`69.2% <ø> (-13.1%)`	⬇️
#cluster	`67.7% <ø> (-18.2%)`	⬇️
#collector	`47.9% <ø> (-15.8%)`	⬇️
#dbnode	`79.9% <ø> (-1%)`	⬇️
#m3em	`66.7% <ø> (-6.5%)`	⬇️
#m3ninx	`70.8% <ø> (-3.5%)`	⬇️
#m3nsch	`28.4% <ø> (-22.8%)`	⬇️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`75.2% <ø> (+0.1%)`	⬆️
#query	`45.7% <2%> (-18.8%)`	⬇️
#x	`71.5% <ø> (-4.6%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f3fd6e...e0532c9. Read the comment docs.

codecov · 2019-02-11T16:28:54Z

Codecov Report

Merging #1367 into master will increase coverage by <.1%.
The diff coverage is 78.1%.

@@           Coverage Diff            @@
##           master   #1367     +/-   ##
========================================
+ Coverage    70.6%   70.6%   +<.1%     
========================================
  Files         823     823             
  Lines       70369   70421     +52     
========================================
+ Hits        49692   49746     +54     
+ Misses      17455   17448      -7     
- Partials     3222    3227      +5

Flag	Coverage Δ
#aggregator	`82.3% <ø> (ø)`	⬆️
#cluster	`85.9% <ø> (ø)`	⬆️
#collector	`63.7% <ø> (ø)`	⬆️
#dbnode	`80.9% <ø> (ø)`	⬆️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.1% <ø> (-0.1%)`	⬇️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`75% <ø> (-0.1%)`	⬇️
#query	`64.5% <78.1%> (ø)`	⬆️
#x	`76% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f3fd6e...75a1262. Read the comment docs.

arnikola · 2019-02-11T19:34:41Z

src/query/functions/temporal/aggregation.go

@@ -85,28 +92,42 @@ func NewAggOp(args []interface{}, optype string) (transform.Params, error) {
 			aggFunc: aggregationFunc,
 		}

+		if optype == QuantileType {
+			if len(args) != 2 {


This is pretty messy; do a similar approach to what's there for holt winters instead

arnikola · 2019-02-11T19:34:57Z

src/query/parser/promql/types.go

@@ -204,7 +204,7 @@ func NewFunctionExpr(

 	case temporal.AvgType, temporal.CountType, temporal.MinType,
 		temporal.MaxType, temporal.SumType, temporal.StdDevType,
-		temporal.StdVarType:
+		temporal.StdVarType, temporal.QuantileType:


Add to parse test

martin-mao · 2019-02-11T20:38:21Z

src/query/functions/temporal/aggregation.go

+	op             baseOp
+	controller     *transform.Controller
+	aggFunc        func([]float64, float64) float64
+	quantileScalar float64


Do we have to add this quantileScalar at the generic agg func layer? Doesn't seem scalable given how many different types of agg funcs we can have. Can we just do the args checking and parsing once we get to the functions that need them?

martin-mao · 2019-02-11T20:39:43Z

src/query/functions/temporal/aggregation.go

 }

-func avgOverTime(values []float64) float64 {
+func avgOverTime(values []float64, _ float64) float64 {


Not great that we modify all the func signatures for a var that isn't used in most of these functions. If it needs to be this way, can we wrap it in an options or something at least so the function signature doesn't have to ever change again? (But only if you have to pass this all the way down, see comment above).

Or consider putting this in some sort of context object that you propagate instead of a specific options for agg functions.

+1. Would it make sense to pass in scalar as a constructor argument to the quantile aggFunc? That way we separate out the "configuration" of the aggFunc from it's actual execution. A note on how that could work:

We can switch aggFunc to be an interface (functions can implement interfaces, so this is actually less work than it seems), to allow for quantile to hold configuration (scalar)

Pass scalar to quantile when we initialize the node instead (threading through NewAggOp and temporal.baseOp.Node)

andrewmains12 · 2019-02-11T21:00:06Z

src/query/functions/temporal/aggregation.go

@@ -192,3 +213,60 @@ func sumAndCount(values []float64) (float64, float64) {

 	return sum, count
 }
+
+func quantileOverTime(values []float64, scalar float64) float64 {
+	valuesSlice := make(valsSlice, 0, len(values))


Add a comment about why copying the array is necessary?

martin-mao · 2019-02-12T18:31:56Z

src/query/functions/temporal/aggregation.go

+	return func(values []float64) float64 {
+		valuesSlice := make(valsSlice, 0, len(values))
+		for _, v := range values {
+			valuesSlice = append(values, v)


What is this loop trying to achieve? This seems to be overwriting valuesSlice on each iteration with the whole of values + the current value from values. You end up with just the values slice + the very last value in values in valuesSlice. Is that what you want?

+1--looks like it should be valuesSlice? You could probably also use copy for this (assuming that's the goal)

martin-mao · 2019-02-12T18:34:26Z

src/query/functions/temporal/base.go

+	if operatorType != QuantileType {
+		durArg = args[0]
+	} else {
+		durArg = args[1]


I know you check outside of this function, but is it worth checking len args in this func just in case? There's some check above for len(args) != 1, but does that cover everything?

Eh I think it's fine (we do the same thing in the Holt-Winters function) + newBaseOp() is a private function

Just expose a new function NewQuantileOp? Spreading this kind of casing across multiple functions confuses the code imo.

arnikola · 2019-02-12T18:40:47Z

src/query/functions/temporal/aggregation.go

@@ -78,6 +82,30 @@ func (a aggProcessor) Init(op baseOp, controller *transform.Controller, opts tra
 	}
 }

+// NewQuantileOp create a new base temporal transform for quantile_over_time func


nit: . at end of comment

arnikola · 2019-02-12T18:41:09Z

src/query/functions/temporal/aggregation.go

@@ -78,6 +82,30 @@ func (a aggProcessor) Init(op baseOp, controller *transform.Controller, opts tra
 	}
 }

+// NewQuantileOp create a new base temporal transform for quantile_over_time func
+func NewQuantileOp(args []interface{}, optype string) (transform.Params, error) {
+	if optype == QuantileType {


Flip the conditional to reduce nesting

arnikola · 2019-02-12T18:42:54Z

src/query/functions/temporal/aggregation.go

+
+		scalar, ok := args[0].(float64)
+		if !ok {
+			return emptyOp, fmt.Errorf("unable to cast to scalar argument: %v for %s", args[1], QuantileType)


This prints args[1] but you're checking args[0]; do we ever use args[1]? Does it need two args?

arnikola · 2019-02-12T18:46:59Z

src/query/functions/temporal/aggregation.go

+	if len(values) == 0 {
+		return math.NaN()
+	}
+	if q < 0 {


nit: newlines

arnikola · 2019-02-12T18:47:52Z

src/query/functions/temporal/aggregation.go

+// If 'values' has zero elements, NaN is returned.
+// If q<0, -Inf is returned.
+// If q>1, +Inf is returned.
+func quantile(q float64, values valsSlice) float64 {


Nit: keep same name for the q variable throughout rather than having it be scalar when you parse it

arnikola · 2019-02-12T18:53:52Z

src/query/functions/temporal/aggregation.go

+			return emptyOp, fmt.Errorf("unable to cast to scalar argument: %v for %s", args[1], QuantileType)
+		}
+
+		aggregationFunc := makeQuantileOverTimeFn(scalar)


You can add an optimization here is scalar < 0 || scalar > 1 by instead having a function that looks like:

func makeInvalidQuantileFn(inf float64) aggFunc { return func(_ []float64) float64 { return inf } }

And passing the correctly signed infinity in

src/query/functions/temporal/aggregation.go

andrewmains12 · 2019-02-12T19:10:30Z

src/query/functions/temporal/base.go

@@ -49,15 +49,27 @@ type baseOp struct {
 // skipping lint check for a single operator type since we will be adding more
 // nolint : unparam
 func newBaseOp(args []interface{}, operatorType string, processorFn MakeProcessor) (baseOp, error) {
-	if operatorType != HoltWintersType && operatorType != PredictLinearType {
+	if operatorType != HoltWintersType && operatorType != PredictLinearType && operatorType != QuantileType {


Seconding +arnikola's earlier comment here; why not handle this in the same way that we do HoltWinters and PredictLinear, and handle this casing in NewFunctionExpr? Handling the argument extraction in 2 places is unnecessarily confusing imo.

andrewmains12 · 2019-02-12T19:11:23Z

src/query/functions/temporal/aggregation_test.go

+		name:   "quantile_over_time",
+		opType: QuantileType,
+		afterBlockOne: [][]float64{
+			{math.NaN(), math.NaN(), math.NaN(), math.NaN(), math.NaN()},


Do we have some tests that don't include NaN's? Might be a useful simple case to cover.

Yeah we should add tests for all of the temporal functions with some NaNs - ill do this in a separate PR.

andrewmains12 · 2019-02-12T19:14:06Z

src/query/functions/temporal/aggregation.go

@@ -78,6 +82,30 @@ func (a aggProcessor) Init(op baseOp, controller *transform.Controller, opts tra
 	}
 }

+// NewQuantileOp create a new base temporal transform for quantile_over_time func
+func NewQuantileOp(args []interface{}, optype string) (transform.Params, error) {
+	if optype == QuantileType {


If this is creating a QuantileOp, why does it need optype as input? Anything other than QuantileType is an error, so it doesn't seem worth having the argument (fwiw, HoltWinters doesn't take one).

andrewmains12 · 2019-02-12T19:14:40Z

src/query/functions/temporal/aggregation.go

@@ -192,3 +220,62 @@ func sumAndCount(values []float64) (float64, float64) {

 	return sum, count
 }
+
+func makeQuantileOverTimeFn(scalar float64) aggFunc {


Thanks for this refactor, much cleaner this way.

arnikola · 2019-02-12T19:28:08Z

src/query/functions/temporal/aggregation.go

+	if q > 1 {
+		return math.Inf(+1)
+	}
+	sort.Sort(values)


There can be a problem with NaNs actually; we don't want them to pollute the output since they don't exist in Prom's implementation. Instead of just doing a sort here, it would be better to put the through a sanitize function that will remove NaNs from the values, then sort.

On the bright side, this means we can use the inbuilt int slice sorter, and not have to worry about the valsSlice type

andrewmains12 · 2019-02-12T19:48:57Z

src/query/functions/temporal/base.go

+	if operatorType != QuantileType {
+		durArg = args[0]
+	} else {
+		durArg = args[1]


Just expose a new function NewQuantileOp? Spreading this kind of casing across multiple functions confuses the code imo.

andrewmains12 · 2019-02-12T21:54:01Z

src/query/functions/temporal/aggregation.go

+
+	return newBaseOp(duration, QuantileType, a)
+}
+
 // NewAggOp creates a new base temporal transform with a specified node.
 func NewAggOp(args []interface{}, optype string) (transform.Params, error) {
 	if aggregationFunc, ok := aggFuncs[optype]; ok {


Nit: flip the conditional, since the if is a lot larger now.

arnikola · 2019-02-12T21:58:22Z

src/query/functions/temporal/base.go

-		return emptyOp, fmt.Errorf("unable to cast to scalar argument: %v for %s", args[0], operatorType)
-	}
-
+func newBaseOp(duration time.Duration, operatorType string, processorFn MakeProcessor) (baseOp, error) {


arnikola · 2019-02-12T22:01:35Z

src/query/functions/temporal/aggregation.go

+
+	duration, ok := args[1].(time.Duration)
+	if !ok {
+		return emptyOp, fmt.Errorf("unable to cast to scalar argument: %v for %s", args[1], QuantileType)


Can we push all of these parsing functions up into promql/types.go, similar to how this does it?

A small gotcha is that any numeric expression is valid in Prom, i.e. quantile_over_time( (1*2)/3, up[5m] ) is a valid query but would fail here and in many of the other temporal functions.

Really don't wanna change this anymore. Also this query works in ours:

quantile_over_time((1*2)/3, service_writeTaggedBatchRaw_latency{instance=~"$instance",quantile="0.99"}[1m])

benraskin92 changed the title ~~[WIP][DON'T REVIEW] Add quantile_over_time~~ Add quantile_over_time Feb 11, 2019

benraskin92 requested review from arnikola and andrewmains12 February 11, 2019 18:33

benraskin92 changed the title ~~Add quantile_over_time~~ [query] Add quantile_over_time Feb 11, 2019

arnikola reviewed Feb 11, 2019

View reviewed changes

martin-mao reviewed Feb 11, 2019

View reviewed changes

andrewmains12 reviewed Feb 11, 2019

View reviewed changes

martin-mao reviewed Feb 12, 2019

View reviewed changes

arnikola reviewed Feb 12, 2019

View reviewed changes

andrewmains12 reviewed Feb 12, 2019

View reviewed changes

src/query/functions/temporal/aggregation.go Outdated Show resolved Hide resolved

andrewmains12 reviewed Feb 12, 2019

View reviewed changes

arnikola reviewed Feb 12, 2019

View reviewed changes

Benjamin Raskin added 7 commits February 12, 2019 16:41

Add quantile_over_time

9a2bd1c

Add tests

3569ce3

Fix comment

158d56e

Address comments

3e705a6

Add tests

26bc536

Address more comments

82adcd3

Fix nan and add tests

bad88d2

benraskin92 force-pushed the braskin/quantile_over_time branch from 714c745 to bad88d2 Compare February 12, 2019 21:41

andrewmains12 approved these changes Feb 12, 2019

View reviewed changes

arnikola reviewed Feb 12, 2019

View reviewed changes

benraskin92 merged commit 7450997 into master Feb 12, 2019

justinjc deleted the braskin/quantile_over_time branch June 17, 2019 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Add quantile_over_time #1367

[query] Add quantile_over_time #1367

benraskin92 commented Feb 11, 2019

codecov bot commented Feb 11, 2019

codecov bot commented Feb 11, 2019 •

edited

Loading

arnikola Feb 11, 2019

arnikola Feb 11, 2019

martin-mao Feb 11, 2019

martin-mao Feb 11, 2019

martin-mao Feb 11, 2019

andrewmains12 Feb 11, 2019

andrewmains12 Feb 11, 2019

martin-mao Feb 12, 2019

andrewmains12 Feb 12, 2019

martin-mao Feb 12, 2019

benraskin92 Feb 12, 2019

andrewmains12 Feb 12, 2019

arnikola Feb 12, 2019

arnikola Feb 12, 2019

arnikola Feb 12, 2019

arnikola Feb 12, 2019

arnikola Feb 12, 2019

arnikola Feb 12, 2019

andrewmains12 Feb 12, 2019

andrewmains12 Feb 12, 2019

benraskin92 Feb 12, 2019

andrewmains12 Feb 12, 2019

andrewmains12 Feb 12, 2019

arnikola Feb 12, 2019

andrewmains12 Feb 12, 2019

andrewmains12 Feb 12, 2019

arnikola Feb 12, 2019

arnikola Feb 12, 2019

benraskin92 Feb 12, 2019

[query] Add quantile_over_time #1367

[query] Add quantile_over_time #1367

Conversation

benraskin92 commented Feb 11, 2019

codecov bot commented Feb 11, 2019

Codecov Report

codecov bot commented Feb 11, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 11, 2019 •

edited

Loading