Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[query] Implemented the Graphite integralByInterval function #2596

Merged
merged 32 commits into from
Sep 21, 2020
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
4e901d7
Added the moving movingMin function
teddywahle Aug 27, 2020
f42685a
Merge branch 'master' into twahle-moving-min
teddywahle Aug 27, 2020
f0df4ce
worked on moving min
teddywahle Aug 27, 2020
bef6377
Merge branch 'twahle-moving-min' of https://github.com/teddywahle/m3 …
teddywahle Aug 27, 2020
8ce6dc3
more work on the moving min func
teddywahle Aug 27, 2020
30ef946
Merge branch 'master' into twahle-moving-min
teddywahle Aug 28, 2020
cf9288f
updated movingMedian
teddywahle Aug 28, 2020
50b0ae9
Merge branch 'twahle-moving-min' of https://github.com/teddywahle/m3 …
teddywahle Aug 28, 2020
1ebe629
Apply suggestions from code review
teddywahle Aug 28, 2020
ebaafce
testMovingFunction
teddywahle Aug 28, 2020
2a0643c
wrote test movingSum function
teddywahle Aug 28, 2020
53222b9
Merge branch 'master' into twahle-moving-min
teddywahle Aug 31, 2020
f45d288
Merge branch 'master' into master
teddywahle Sep 4, 2020
12a47a7
Apply suggestions from code review
teddywahle Sep 4, 2020
67b5378
Apply suggestions from code review
teddywahle Sep 4, 2020
fabdacb
finished basic logic, just need to write tests
teddywahle Sep 4, 2020
58d7914
Merge branch 'master' into graphite-integral-by-interval
teddywahle Sep 4, 2020
70fc626
completed testing for integralByInteral
teddywahle Sep 4, 2020
9083619
Merge branch 'master' into graphite-integral-by-interval
teddywahle Sep 7, 2020
26f1190
Merge branch 'master' into graphite-integral-by-interval
teddywahle Sep 8, 2020
0f425b9
Update src/query/graphite/native/builtin_functions.go
teddywahle Sep 11, 2020
78a46a3
Merge branch 'master' into graphite-integral-by-interval
teddywahle Sep 11, 2020
deb483e
made variables more Go-like
teddywahle Sep 11, 2020
9bde29f
Merge branch 'master' into graphite-integral-by-interval
teddywahle Sep 21, 2020
a912b30
Update src/query/graphite/native/builtin_functions.go
teddywahle Sep 21, 2020
03ad7d5
Apply suggestions from code review
teddywahle Sep 21, 2020
970ed5c
Ran go fmt
teddywahle Sep 21, 2020
b0155f4
Added proper test case from graphite-web source code
teddywahle Sep 21, 2020
6f85d3c
Apply suggestions from code review
teddywahle Sep 21, 2020
678690a
Apply suggestions from code review
teddywahle Sep 21, 2020
6cabbd4
fixed formatting
teddywahle Sep 21, 2020
54d6307
Merge branch 'master' into graphite-integral-by-interval
teddywahle Sep 21, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/query/graphite/native/builtin_functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -1000,6 +1000,43 @@ func integral(ctx *common.Context, input singlePathSpec) (ts.SeriesList, error)
return r, nil
}

// integralByInterval will do the same as integral funcion, except it resets the total to 0
// at the given time in the parameter “from”. Useful for finding totals per hour/day/week.
func integralByInterval(ctx *common.Context, input singlePathSpec, intervalString string) (ts.SeriesList, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be added to builtin_functions.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

intervalUnit, err := common.ParseInterval(intervalString)
if err != nil {
return ts.NewSeriesList(), err
}
results := make([]*ts.Series, 0, len(input.Values))
for _, series := range input.Values {
stepsPerInterval := intervalUnit.Milliseconds() / int64(series.MillisPerStep())
var stepCounter int64 = 0

outvals := ts.NewValues(ctx, series.MillisPerStep(), series.Len())
var currentSum float64
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is more Go-ish to combine these var assignments altogether into a multi line var statement (and leave the zero values unassigned, i.e. stepCounter and currentSum):

var (
  stepsPerInterval = intervalUnit.Milliseconds() /  int64(series.MillisPerStep())
  vals = ts.NewValues(ctx, series.MillisPerStep(), series.Len())
  stepCounter int64
  currentSum float64
)

Also "outvals" should either be camel cased (since it's two words) or just one word (i.e. vals).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! thanks for this.

teddywahle marked this conversation as resolved.
Show resolved Hide resolved
for i := 0; i < series.Len(); i++ {
if stepCounter > stepsPerInterval {
// startNewInterval
stepCounter = 0
currentSum = 0.0
}
n := series.ValueAt(i)
if !math.IsNaN(n) {
currentSum += n
outvals.SetValueAt(i, currentSum)
}
stepCounter += 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it correct to increment step counter if the value is NaN?

Copy link
Contributor Author

@teddywahle teddywahle Sep 21, 2020

Choose a reason for hiding this comment

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

Discussed offline. Answer is yes according to graphite-web source code.

}

newName := fmt.Sprintf("integralByInterval(%s, %s)", series.Name(), intervalString)
results = append(results, ts.NewSeries(ctx, newName, series.StartTime(), outvals))
}

r := ts.SeriesList(input)
r.Values = results
return r, nil
}

// This is the opposite of the integral function. This is useful for taking a
// running total metric and calculating the delta between subsequent data
// points.
Expand Down Expand Up @@ -1979,6 +2016,7 @@ func init() {
MustRegisterFunction(holtWintersForecast)
MustRegisterFunction(identity)
MustRegisterFunction(integral)
MustRegisterFunction(integralByInterval)
MustRegisterFunction(isNonNull)
MustRegisterFunction(keepLastValue).WithDefaultParams(map[uint8]interface{}{
2: -1, // limit
Expand Down
31 changes: 31 additions & 0 deletions src/query/graphite/native/builtin_functions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1736,6 +1736,36 @@ func TestIntegral(t *testing.T) {
}
}

func TestIntegralByInterval(t *testing.T) {
ctx := common.NewTestContext()
defer ctx.Close()

invals := []float64{
0, 1, 2, 3, 4, 5, 6, math.NaN(), 8, math.NaN(),
}

outvals := []float64{
0, 1, 3, 6, 4, 9, 15, math.NaN(), 8, math.NaN(),
}

series := ts.NewSeries(ctx, "hello", time.Now(),
common.NewTestSeriesValues(ctx, 10000, invals))

r, err := integralByInterval(ctx, singlePathSpec{
Values: []*ts.Series{series},
}, "30s")
require.NoError(t, err)

output := r.Values
require.Equal(t, 1, len(output))
assert.Equal(t, "integralByInterval(hello, 30s)", output[0].Name())
assert.Equal(t, series.StartTime(), output[0].StartTime())
require.Equal(t, len(outvals), output[0].Len())
for i, expected := range outvals {
xtest.Equalish(t, expected, output[0].ValueAt(i), "incorrect value at %d", i)
}
}

func TestDerivative(t *testing.T) {
ctx := common.NewTestContext()
defer ctx.Close()
Expand Down Expand Up @@ -3044,6 +3074,7 @@ func TestFunctionsRegistered(t *testing.T) {
"holtWintersForecast",
"identity",
"integral",
"integralByInterval",
"isNonNull",
"keepLastValue",
"legendValue",
Expand Down