Add irate #897

benraskin92 · 2018-09-11T14:08:49Z

No description provided.

arnikola · 2018-09-11T14:32:59Z

src/query/functions/temporal/rate_test.go

+	}
+}
+
+// func TestUnknownAggregation(t *testing.T) {


arnikola · 2018-09-11T14:36:25Z

src/query/functions/temporal/rate.go

+		return math.NaN()
+	}
+
+	// {0, 1, 2, 3, 4},


codecov · 2018-09-12T18:14:37Z

Codecov Report

Merging #897 into master will increase coverage by 0.1%.
The diff coverage is 87.23%.

@@            Coverage Diff            @@
##           master     #897     +/-   ##
=========================================
+ Coverage   78.29%   78.39%   +0.1%     
=========================================
  Files         397      398      +1     
  Lines       33799    33844     +45     
=========================================
+ Hits        26462    26533     +71     
+ Misses       5520     5501     -19     
+ Partials     1817     1810      -7

Flag	Coverage Δ
#dbnode	`81.51% <ø> (+0.12%)`	⬆️
#m3ninx	`71.93% <ø> (ø)`	⬆️
#query	`67.84% <87.23%> (+0.12%)`	⬆️

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 721ac92...8774436. Read the comment docs.

nikunjgit · 2018-09-11T14:31:56Z

src/query/functions/temporal/rate.go

+}
+
+func (r *rateNode) Process(values []float64) float64 {
+	switch r.op.operatorType {


instead of doing this, the node can just store a boolean isRate. In Process, just pass that boolean in. This way you don't have to look at operatorType in node

nikunjgit · 2018-09-13T17:11:48Z

src/query/functions/temporal/rate.go

+)
+
+const (
+	// IRateTemporalType calculates the per-second instant rate of increase of the time series


fix comment ? we don't have instant/ range vector concept.

nikunjgit · 2018-09-13T17:15:55Z

src/query/functions/temporal/rate.go

+	previousSample := values[nonNanIdx]
+
+	var resultValue float64
+	if isRate && lastSample < previousSample {


i think you need reset for both types ?

Not according to Prom logic

arnikola · 2018-09-11T14:36:34Z

src/query/functions/temporal/rate.go

+}
+
+func instantValue(values []float64, isRate bool, stepSize time.Duration) float64 {
+	fmt.Println(values)


arnikola · 2018-09-11T14:37:23Z

src/query/functions/temporal/rate.go

+}
+
+func (r *rateNode) Process(values []float64) float64 {
+	switch r.op.operatorType {


No need to check operator type in the process function.

arnikola · 2018-09-11T14:42:07Z

src/query/functions/temporal/rate.go

+	lastSample := values[valuesLen-1]
+	previousSample := values[valuesLen-2]
+
+	fmt.Println(lastSample, previousSample)


arnikola · 2018-09-13T17:34:02Z

src/query/functions/temporal/rate.go

+			return i
+		}
+	}
+	return -1


nit newline

arnikola · 2018-09-13T17:35:12Z

src/query/functions/temporal/rate.go

+	if nonNanIdx < 1 {
+		return math.NaN()
+	}
+	lastSample := values[nonNanIdx]


nit newline

arnikola · 2018-09-13T17:35:15Z

src/query/functions/temporal/rate.go

+	if nonNanIdx == -1 {
+		return math.NaN()
+	}
+	previousSample := values[nonNanIdx]


nit newline

Think this is fine :)

I thought we were add newlines after each closing bracket with lines following it

arnikola · 2018-09-13T17:42:03Z

src/query/functions/temporal/rate.go

+	}
+
+	if isRate {
+		resultValue /= float64(stepSize) / math.Pow10(9)


Two things here;

I think this is wrong; you should be multiplying stepSize by the difference between indices for lastSample and previousSample

It's a bit confusing to do x /= y / z, better to write this as
resultValue *= float64(time.Second)
resultValue /= float64(stepSize * (indexLast - indexPrevious))

arnikola · 2018-09-14T15:43:07Z

src/query/functions/temporal/rate_test.go

+	}
+}
+
+func TestFindNonNanIdx(t *testing.T) {


Either add test or delete this function

arnikola · 2018-09-14T15:45:02Z

src/query/functions/temporal/rate.go

+		return math.NaN()
+	}
+
+	var indexLast int


Don't need this, just initialize the var when you need it

arnikola · 2018-09-14T15:45:49Z

src/query/functions/temporal/rate.go

+
+	nonNanIdx := valuesLen - 1
+	// find idx for last non-NaN value
+	nonNanIdx = findNonNanIdx(values, nonNanIdx)


You can doindexLast := findNonNanIdx(values, nonNanIdx)instead

arnikola · 2018-09-14T15:49:19Z

src/query/functions/temporal/rate.go

+	if nonNanIdx < 1 {
+		return math.NaN()
+	}
+	indexLast = nonNanIdx


nit: newline

arnikola · 2018-09-14T15:50:47Z

src/query/functions/temporal/rate.go

+	return -1
+}
+
+func instantValue(values []float64, isRate bool, stepSize time.Duration) float64 {


This function isn't necessary, you can move the logic into the Process() function instead

arnikola · 2018-09-14T20:12:57Z

src/query/functions/temporal/rate.go

+)
+
+const (
+	// IRateTemporalType calculates the per-second rate of increase of the time series


IRateType is fine since it's in temporal, otherwise you have temporal.IRateTemporal

arnikola · 2018-09-14T20:13:04Z

src/query/functions/temporal/rate.go

+
+	// IDeltaTemporalType calculates the difference between the last two values in the time series.
+	// IDeltaTemporalType should only be used with gauges.
+	IDeltaTemporalType = "idelta"


same comment here

arnikola · 2018-09-14T20:14:20Z

src/query/functions/temporal/rate.go

+}
+
+func newRateNode(op baseOp, controller *transform.Controller, opts transform.Options) Processor {
+	var isRate bool


isRate := op.operatorType == IRateTemporalType

arnikola · 2018-09-14T20:15:12Z

src/query/functions/temporal/rate.go

+		return math.NaN()
+	}
+
+	lastSample := values[indexLast]


nit: since you keep track of this index, you can define lastSample lower down, closer to where you use it

arnikola reviewed Sep 11, 2018

View reviewed changes

src/query/functions/temporal/rate_test.go Outdated

}

}

// func TestUnknownAggregation(t *testing.T) {

Copy link

Collaborator

arnikola Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit delete

arnikola reviewed Sep 11, 2018

View reviewed changes

src/query/functions/temporal/rate.go Outdated

return math.NaN()

}

// {0, 1, 2, 3, 4},

Copy link

Collaborator

arnikola Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

richardartoul force-pushed the master branch from 2883443 to 2cf04be Compare September 11, 2018 18:16

Benjamin Raskin added 3 commits September 12, 2018 13:57

Add irate

094ff72

Changes

2f65324

Add tests

ec8739b

benraskin92 force-pushed the braskin/add_irate branch from 5256f8f to ec8739b Compare September 12, 2018 17:57

Updates

00e9058

Add comments

8774436

benraskin92 changed the title ~~[WIP] Add irate~~ Add irate Sep 12, 2018

benraskin92 requested a review from nikunjgit September 12, 2018 19:03

nikunjgit reviewed Sep 13, 2018

View reviewed changes

arnikola reviewed Sep 13, 2018

View reviewed changes

Address comments

3ecae22

nikunjgit previously approved these changes Sep 14, 2018

View reviewed changes

arnikola reviewed Sep 14, 2018

View reviewed changes

src/query/functions/temporal/rate.go Outdated

if nonNanIdx < 1 {

return math.NaN()

}

indexLast = nonNanIdx

Copy link

Collaborator

arnikola Sep 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: newline

arnikola reviewed Sep 14, 2018

View reviewed changes

Address comments

907cf81

benraskin92 dismissed nikunjgit’s stale review via 907cf81 September 14, 2018 18:10

Fix comment

6f7cf85

arnikola reviewed Sep 14, 2018

View reviewed changes

Benjamin Raskin added 2 commits September 14, 2018 16:15

Update naming

545ef99

Address comments

9fc4947

arnikola approved these changes Sep 14, 2018

View reviewed changes

benraskin92 merged commit 7adcd64 into master Sep 14, 2018

prateek deleted the braskin/add_irate branch September 29, 2018 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add irate #897

Add irate #897

benraskin92 commented Sep 11, 2018

arnikola Sep 11, 2018

arnikola Sep 11, 2018

codecov bot commented Sep 12, 2018 •

edited

Loading

nikunjgit Sep 11, 2018

nikunjgit Sep 13, 2018

nikunjgit Sep 13, 2018

benraskin92 Sep 14, 2018

arnikola Sep 11, 2018

arnikola Sep 11, 2018

arnikola Sep 11, 2018

arnikola Sep 13, 2018

arnikola Sep 13, 2018

arnikola Sep 13, 2018

benraskin92 Sep 14, 2018

arnikola Sep 14, 2018

arnikola Sep 13, 2018

arnikola Sep 14, 2018

arnikola Sep 14, 2018 •

edited

Loading

arnikola Sep 14, 2018

arnikola Sep 14, 2018

arnikola Sep 14, 2018

arnikola Sep 14, 2018

arnikola Sep 14, 2018

arnikola Sep 14, 2018

arnikola Sep 14, 2018

Add irate #897

Add irate #897

Conversation

benraskin92 commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2018 • 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

arnikola Sep 14, 2018 • edited Loading

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 Sep 12, 2018 •

edited

Loading

arnikola Sep 14, 2018 •

edited

Loading