Skip to content

Commit

Permalink
[query] Fix Graphite context time window expansions not being cumulative
Browse files Browse the repository at this point in the history
  • Loading branch information
robskillington authored May 7, 2021
1 parent 83cdc7d commit 4df4556
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 11 deletions.
8 changes: 2 additions & 6 deletions src/query/graphite/common/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,8 @@ func (c *Context) NewChildContext(opts ChildContextOptions) *Context {
child.TimeRangeAdjustment.OriginalEnd = origEnd
child.TimeRangeAdjustment.ShiftStart += opts.adjustment.shiftStart
child.TimeRangeAdjustment.ShiftEnd += opts.adjustment.shiftEnd
if opts.adjustment.expandStart > child.TimeRangeAdjustment.ExpandStart {
child.TimeRangeAdjustment.ExpandStart = opts.adjustment.expandStart
}
if opts.adjustment.expandEnd > child.TimeRangeAdjustment.ExpandEnd {
child.TimeRangeAdjustment.ExpandEnd = opts.adjustment.expandEnd
}
child.TimeRangeAdjustment.ExpandStart += opts.adjustment.expandStart
child.TimeRangeAdjustment.ExpandEnd += opts.adjustment.expandEnd

child.StartTime = origStart.
Add(child.TimeRangeAdjustment.ShiftStart).
Expand Down
16 changes: 11 additions & 5 deletions src/query/graphite/common/test_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,22 @@ func NewConsolidationTestSeries(start, end time.Time, duration time.Duration) (*
}

// CompareOutputsAndExpected compares the actual output with the expected output.
func CompareOutputsAndExpected(t *testing.T, step int, start time.Time, expected []TestSeries,
actual []*ts.Series) {
require.Equal(t, len(expected), len(actual))
func CompareOutputsAndExpected(
t *testing.T,
step int,
start time.Time,
expected []TestSeries,
actual []*ts.Series,
) {
require.Equal(t, len(expected), len(actual), "mismatch series count")
for i := range expected {
i := i // To capture for wrapMsg.
e := expected[i].Data
a := actual[i]
wrapMsg := func(str string) string {
return fmt.Sprintf("%s\nseries=%d\nexpected=%v\nactual=%v",
str, i, e, a.SafeValues())
return fmt.Sprintf("%s\nseries=%d\nexpected=%v\nactual=%v\n"+
"expectedStart=%v\nactualStart=%v\n",
str, i, e, a.SafeValues(), start, a.StartTime())
}
require.Equal(t, expected[i].Name, a.Name())
assert.Equal(t, step, a.MillisPerStep(), wrapMsg(a.Name()+
Expand Down
67 changes: 67 additions & 0 deletions src/query/graphite/native/builtin_functions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,73 @@ func TestMovingSumSuccess(t *testing.T) {
testMovingFunction(t, "movingSum(foo.bar.baz, '30s')", "movingSum(foo.bar.baz,3)", nil, nil, nil)
}

// TestMovingSumOfMovingSum tests that expansion of the time window
// fetched is stacked when child contexts are created, otherwise the child
// functions do not have enough data to work with to satisfy the parent call.
func TestMovingSumOfMovingSum(t *testing.T) {
var (
ctrl = xgomock.NewController(t)
store = storage.NewMockStorage(ctrl)
engine = NewEngine(store, CompileOptions{})
end = time.Now().Truncate(time.Minute)
start = end.Add(-5 * time.Minute)
ctx = common.NewContext(common.ContextOptions{
Start: start,
End: end,
Engine: engine,
})
millisPerStep = 60000
data = []float64{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}
)

defer ctrl.Finish()
defer func() { _ = ctx.Close() }()

store.EXPECT().
FetchByQuery(gomock.Any(), "foo.bar", gomock.Any()).
DoAndReturn(func(
_ *common.Context,
query string,
opts storage.FetchOptions,
) (*storage.FetchResult, error) {
if !opts.EndTime.Equal(end) {
return nil, fmt.Errorf("unexpected end")
}
length := int(opts.EndTime.Sub(opts.StartTime) / time.Minute)
values := data[len(data)-length:]
return &storage.FetchResult{
SeriesList: []*ts.Series{
ts.NewSeries(ctx, query, opts.StartTime,
common.NewTestSeriesValues(ctx, millisPerStep, values)),
},
}, nil
}).
AnyTimes()

target := `movingSum(movingSum(foo.bar,"2min"),"5min")`

phonyContext := common.NewContext(common.ContextOptions{
Start: start,
End: end,
Engine: engine,
})

expr, err := phonyContext.Engine.(*Engine).Compile(target)
require.NoError(t, err)
res, err := expr.Execute(phonyContext)
require.NoError(t, err)

expected := []common.TestSeries{
{
Name: target,
Data: []float64{65, 75, 85, 95, 105},
},
}

common.CompareOutputsAndExpected(t, millisPerStep, start,
expected, res.Values)
}

func TestMovingSumError(t *testing.T) {
testMovingFunctionError(t, "movingSum(foo.bar.baz, '-30s')")
testMovingFunctionError(t, "movingSum(foo.bar.baz, 0)")
Expand Down

0 comments on commit 4df4556

Please sign in to comment.