From ae0befe2df75d40b34f8989fa90174c487f2d7f5 Mon Sep 17 00:00:00 2001 From: Evan Yin Date: Tue, 25 May 2021 23:29:49 -0700 Subject: [PATCH 1/3] [query] Update hitcount to support alignToInterval (needs to fix tests) --- .../graphite/native/builtin_functions.go | 64 +++++++++++++++++-- .../graphite/native/builtin_functions_test.go | 33 ++++++++-- 2 files changed, 85 insertions(+), 12 deletions(-) diff --git a/src/query/graphite/native/builtin_functions.go b/src/query/graphite/native/builtin_functions.go index 34b5f154c0..c7bd8c145a 100644 --- a/src/query/graphite/native/builtin_functions.go +++ b/src/query/graphite/native/builtin_functions.go @@ -1531,18 +1531,70 @@ func durationToSeconds(d time.Duration) int { // hitcount estimates hit counts from a list of time series. This function assumes the values in each time // series represent hits per second. It calculates hits per some larger interval such as per day or per hour. -// NB(xichen): it doesn't support the alignToInterval parameter because no one seems to be using that. -func hitcount(ctx *common.Context, seriesList singlePathSpec, intervalString string) (ts.SeriesList, error) { +func hitcount(ctx *common.Context, seriesList singlePathSpec, intervalString string, alignToInterval bool) (*unaryContextShifter, error) { interval, err := common.ParseInterval(intervalString) if err != nil { - return ts.NewSeriesList(), err + return nil, err } - intervalInSeconds := durationToSeconds(interval) if intervalInSeconds <= 0 { - return ts.NewSeriesList(), common.ErrInvalidIntervalFormat + return nil, common.ErrInvalidIntervalFormat + } + + var shiftDuration = time.Second * 0 // default to no shift. + if alignToInterval { + // follow graphite implementation: only handle minutes, hours and days. + origStartTime := ctx.StartTime + if interval.Hours() >= 24 { // interval is in days + // truncate to days. + newStartTime := time.Date( + origStartTime.Year(), + origStartTime.Month(), + origStartTime.Day(), 0, 0, 0, 0, origStartTime.Location()) + shiftDuration = newStartTime.Sub(origStartTime) + } else if interval.Hours() >=1 { // interval is in hrs + newStartTime := time.Date( + origStartTime.Year(), + origStartTime.Month(), + origStartTime.Day(), + origStartTime.Hour(), 0, 0, 0, origStartTime.Location()) + shiftDuration = newStartTime.Sub(origStartTime) + } else if interval.Minutes() >=1 { // interval is in minutes. + newStartTime := time.Date( + origStartTime.Year(), + origStartTime.Month(), + origStartTime.Day(), + origStartTime.Hour(), + origStartTime.Minute(), 0, 0, origStartTime.Location()) + shiftDuration = newStartTime.Sub(origStartTime) + } + } + contextShiftingFn := func(c *common.Context) *common.Context { + opts := common.NewChildContextOptions() + opts.AdjustTimeRange(shiftDuration, 0, 0, 0) + childCtx := c.NewChildContext(opts) + return childCtx + } + + transformerFn := func(input ts.SeriesList) (ts.SeriesList, error) { + r := hitCountImpl(ctx, seriesList, intervalString, interval, intervalInSeconds) + return r, nil } + + return &unaryContextShifter{ + ContextShiftFunc: contextShiftingFn, + UnaryTransformer: transformerFn, + }, nil +} + +func hitCountImpl( + ctx *common.Context, + seriesList singlePathSpec, + intervalString string, + interval time.Duration, + intervalInSeconds int) ts.SeriesList { + resultSeries := make([]*ts.Series, len(seriesList.Values)) for index, series := range seriesList.Values { step := time.Duration(series.MillisPerStep()) * time.Millisecond @@ -1585,7 +1637,7 @@ func hitcount(ctx *common.Context, seriesList singlePathSpec, intervalString str r := ts.SeriesList(seriesList) r.Values = resultSeries - return r, nil + return r } func safeIndex(len, index int) int { diff --git a/src/query/graphite/native/builtin_functions_test.go b/src/query/graphite/native/builtin_functions_test.go index d28607679f..96a1a85e8a 100644 --- a/src/query/graphite/native/builtin_functions_test.go +++ b/src/query/graphite/native/builtin_functions_test.go @@ -3078,8 +3078,14 @@ func TestLimitSortStable(t *testing.T) { } func TestHitCount(t *testing.T) { - ctx := common.NewTestContext() - defer func() { _ = ctx.Close() }() + ctrl := xgomock.NewController(t) + defer ctrl.Finish() + + store := storage.NewMockStorage(ctrl) + engine := NewEngine(store, CompileOptions{}) + + //ctx := common.NewTestContext() + //defer func() { _ = ctx.Close() }() now := time.Now() tests := []struct { @@ -3115,22 +3121,37 @@ func TestHitCount(t *testing.T) { } for _, input := range tests { + ctx := common.NewContext(common.ContextOptions{ + Start: input.startTime, + End: input.startTime.Add(time.Second * 10), + Engine: engine}) + defer func() { _ = ctx.Close() }() + series := ts.NewSeries( ctx, input.name, input.startTime, common.NewTestSeriesValues(ctx, input.stepInMilli, input.values), ) - results, err := hitcount(ctx, singlePathSpec{ - Values: []*ts.Series{series}, - }, input.intervalString) + //results, err := hitcount(ctx, singlePathSpec{ + // Values: []*ts.Series{series}, + //}, input.intervalString, false) + target := fmt.Sprintf("hitcount(%v, %v,false)", input.name, input.intervalString) + testSeriesFn := func(context.Context, string, storage.FetchOptions) (*storage.FetchResult, error) { + return &storage.FetchResult{SeriesList: []*ts.Series{series}}, nil + } + store.EXPECT().FetchByQuery(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(testSeriesFn).AnyTimes() + expr, err := engine.Compile(target) + require.NoError(t, err) + res, err := expr.Execute(ctx) + require.NoError(t, err) expected := common.TestSeries{ Name: fmt.Sprintf(`hitcount(%s, %q)`, input.name, input.intervalString), Data: input.output, } require.Nil(t, err) common.CompareOutputsAndExpected(t, input.newStep, input.newStartTime, - []common.TestSeries{expected}, results.Values) + []common.TestSeries{expected}, res.Values) } } From 954a03b246065fc0c818f07dd5ab410f69838307 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Sat, 10 Jul 2021 19:44:11 -0400 Subject: [PATCH 2/3] Fix tests and use bootstrapped instead of input for transform function --- .../graphite/native/builtin_functions.go | 32 ++++--- .../graphite/native/builtin_functions_test.go | 84 ++++++++++--------- 2 files changed, 62 insertions(+), 54 deletions(-) diff --git a/src/query/graphite/native/builtin_functions.go b/src/query/graphite/native/builtin_functions.go index 7903104864..343bbb7b0b 100644 --- a/src/query/graphite/native/builtin_functions.go +++ b/src/query/graphite/native/builtin_functions.go @@ -1598,11 +1598,17 @@ func durationToSeconds(d time.Duration) int { // hitcount estimates hit counts from a list of time series. This function assumes the values in each time // series represent hits per second. It calculates hits per some larger interval such as per day or per hour. -func hitcount(ctx *common.Context, seriesList singlePathSpec, intervalString string, alignToInterval bool) (*unaryContextShifter, error) { +func hitcount( + ctx *common.Context, + _ singlePathSpec, + intervalString string, + alignToInterval bool, +) (*unaryContextShifter, error) { interval, err := common.ParseInterval(intervalString) if err != nil { return nil, err } + intervalInSeconds := durationToSeconds(interval) if intervalInSeconds <= 0 { return nil, common.ErrInvalidIntervalFormat @@ -1619,14 +1625,14 @@ func hitcount(ctx *common.Context, seriesList singlePathSpec, intervalString str origStartTime.Month(), origStartTime.Day(), 0, 0, 0, 0, origStartTime.Location()) shiftDuration = newStartTime.Sub(origStartTime) - } else if interval.Hours() >=1 { // interval is in hrs + } else if interval.Hours() >= 1 { // interval is in hrs newStartTime := time.Date( origStartTime.Year(), origStartTime.Month(), origStartTime.Day(), origStartTime.Hour(), 0, 0, 0, origStartTime.Location()) shiftDuration = newStartTime.Sub(origStartTime) - } else if interval.Minutes() >=1 { // interval is in minutes. + } else if interval.Minutes() >= 1 { // interval is in minutes. newStartTime := time.Date( origStartTime.Year(), origStartTime.Month(), @@ -1636,6 +1642,7 @@ func hitcount(ctx *common.Context, seriesList singlePathSpec, intervalString str shiftDuration = newStartTime.Sub(origStartTime) } } + contextShiftingFn := func(c *common.Context) *common.Context { opts := common.NewChildContextOptions() opts.AdjustTimeRange(shiftDuration, 0, 0, 0) @@ -1643,12 +1650,11 @@ func hitcount(ctx *common.Context, seriesList singlePathSpec, intervalString str return childCtx } - transformerFn := func(input ts.SeriesList) (ts.SeriesList, error) { - r := hitCountImpl(ctx, seriesList, intervalString, interval, intervalInSeconds) + transformerFn := func(bootstrappedSeries ts.SeriesList) (ts.SeriesList, error) { + r := hitCountImpl(ctx, bootstrappedSeries, intervalString, interval, intervalInSeconds) return r, nil } - return &unaryContextShifter{ ContextShiftFunc: contextShiftingFn, UnaryTransformer: transformerFn, @@ -1657,13 +1663,13 @@ func hitcount(ctx *common.Context, seriesList singlePathSpec, intervalString str func hitCountImpl( ctx *common.Context, - seriesList singlePathSpec, + seriesList ts.SeriesList, intervalString string, interval time.Duration, - intervalInSeconds int) ts.SeriesList { - - resultSeries := make([]*ts.Series, len(seriesList.Values)) - for index, series := range seriesList.Values { + intervalInSeconds int, +) ts.SeriesList { + resultSeries := make([]*ts.Series, 0, len(seriesList.Values)) + for _, series := range seriesList.Values { step := time.Duration(series.MillisPerStep()) * time.Millisecond bucketCount := int(math.Ceil(float64(series.EndTime().Sub(series.StartTime())) / float64(interval))) buckets := ts.NewValues(ctx, int(interval/time.Millisecond), bucketCount) @@ -1697,9 +1703,9 @@ func hitCountImpl( } } } - newName := fmt.Sprintf("hitcount(%s, %q)", series.Name(), intervalString) + newName := fmt.Sprintf("hitcount(%s,%q)", series.Name(), intervalString) newSeries := ts.NewSeries(ctx, newName, newStart, buckets) - resultSeries[index] = newSeries + resultSeries = append(resultSeries, newSeries) } r := ts.SeriesList(seriesList) diff --git a/src/query/graphite/native/builtin_functions_test.go b/src/query/graphite/native/builtin_functions_test.go index e8f5a466d6..22ef4e18b9 100644 --- a/src/query/graphite/native/builtin_functions_test.go +++ b/src/query/graphite/native/builtin_functions_test.go @@ -3231,16 +3231,7 @@ func TestLimitSortStable(t *testing.T) { } -func TestHitCount(t *testing.T) { - ctrl := xgomock.NewController(t) - defer ctrl.Finish() - - store := storage.NewMockStorage(ctrl) - engine := NewEngine(store, CompileOptions{}) - - //ctx := common.NewTestContext() - //defer func() { _ = ctx.Close() }() - +func TestHitcount(t *testing.T) { now := time.Now() tests := []struct { name string @@ -3274,38 +3265,49 @@ func TestHitCount(t *testing.T) { }, } - for _, input := range tests { - ctx := common.NewContext(common.ContextOptions{ - Start: input.startTime, - End: input.startTime.Add(time.Second * 10), - Engine: engine}) - defer func() { _ = ctx.Close() }() + for i, input := range tests { + t.Run(fmt.Sprintf("test_%d_%s", i, input.name), func(t *testing.T) { + ctrl := xgomock.NewController(t) + defer ctrl.Finish() - series := ts.NewSeries( - ctx, - input.name, - input.startTime, - common.NewTestSeriesValues(ctx, input.stepInMilli, input.values), - ) - //results, err := hitcount(ctx, singlePathSpec{ - // Values: []*ts.Series{series}, - //}, input.intervalString, false) - target := fmt.Sprintf("hitcount(%v, %v,false)", input.name, input.intervalString) - testSeriesFn := func(context.Context, string, storage.FetchOptions) (*storage.FetchResult, error) { - return &storage.FetchResult{SeriesList: []*ts.Series{series}}, nil - } - store.EXPECT().FetchByQuery(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(testSeriesFn).AnyTimes() - expr, err := engine.Compile(target) - require.NoError(t, err) - res, err := expr.Execute(ctx) - require.NoError(t, err) - expected := common.TestSeries{ - Name: fmt.Sprintf(`hitcount(%s, %q)`, input.name, input.intervalString), - Data: input.output, - } - require.Nil(t, err) - common.CompareOutputsAndExpected(t, input.newStep, input.newStartTime, - []common.TestSeries{expected}, res.Values) + store := storage.NewMockStorage(ctrl) + engine := NewEngine(store, CompileOptions{}) + + ctx := common.NewContext(common.ContextOptions{ + Start: input.startTime, + End: input.startTime.Add(time.Second * 10), + Engine: engine, + }) + defer func() { _ = ctx.Close() }() + + series := ts.NewSeries(ctx, input.name, input.startTime, + common.NewTestSeriesValues(ctx, input.stepInMilli, input.values)) + + target := fmt.Sprintf("hitcount(%s,%q,false)", input.name, input.intervalString) + testSeriesFn := func( + *common.Context, + string, + storage.FetchOptions, + ) (*storage.FetchResult, error) { + return &storage.FetchResult{SeriesList: []*ts.Series{series}}, nil + } + + store.EXPECT(). + FetchByQuery(gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(testSeriesFn). + AnyTimes() + expr, err := engine.Compile(target) + require.NoError(t, err) + res, err := expr.Execute(ctx) + require.NoError(t, err) + expected := common.TestSeries{ + Name: fmt.Sprintf("hitcount(%s,%q)", input.name, input.intervalString), + Data: input.output, + } + require.NoError(t, err) + common.CompareOutputsAndExpected(t, input.newStep, input.newStartTime, + []common.TestSeries{expected}, res.Values) + }) } } From e74df3be29642d5e2a1a76b9d812f9299e530e73 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Sat, 10 Jul 2021 20:31:30 -0400 Subject: [PATCH 3/3] Fix lint --- src/query/graphite/native/builtin_functions.go | 15 +++++++++------ .../graphite/native/builtin_functions_test.go | 1 + 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/query/graphite/native/builtin_functions.go b/src/query/graphite/native/builtin_functions.go index 343bbb7b0b..c8203b41a0 100644 --- a/src/query/graphite/native/builtin_functions.go +++ b/src/query/graphite/native/builtin_functions.go @@ -1614,25 +1614,28 @@ func hitcount( return nil, common.ErrInvalidIntervalFormat } - var shiftDuration = time.Second * 0 // default to no shift. + shiftDuration := time.Second * 0 // Default to no shift. if alignToInterval { - // follow graphite implementation: only handle minutes, hours and days. + // Follow graphite implementation: only handle minutes, hours and days. origStartTime := ctx.StartTime - if interval.Hours() >= 24 { // interval is in days - // truncate to days. + switch { + case interval.Hours() >= 24: + // Interval in days, truncate to days. newStartTime := time.Date( origStartTime.Year(), origStartTime.Month(), origStartTime.Day(), 0, 0, 0, 0, origStartTime.Location()) shiftDuration = newStartTime.Sub(origStartTime) - } else if interval.Hours() >= 1 { // interval is in hrs + case interval.Hours() >= 1: + // Interval is in hrs. newStartTime := time.Date( origStartTime.Year(), origStartTime.Month(), origStartTime.Day(), origStartTime.Hour(), 0, 0, 0, origStartTime.Location()) shiftDuration = newStartTime.Sub(origStartTime) - } else if interval.Minutes() >= 1 { // interval is in minutes. + case interval.Minutes() >= 1: + // Interval is in minutes. newStartTime := time.Date( origStartTime.Year(), origStartTime.Month(), diff --git a/src/query/graphite/native/builtin_functions_test.go b/src/query/graphite/native/builtin_functions_test.go index 22ef4e18b9..f70c675c0c 100644 --- a/src/query/graphite/native/builtin_functions_test.go +++ b/src/query/graphite/native/builtin_functions_test.go @@ -3266,6 +3266,7 @@ func TestHitcount(t *testing.T) { } for i, input := range tests { + input := input t.Run(fmt.Sprintf("test_%d_%s", i, input.name), func(t *testing.T) { ctrl := xgomock.NewController(t) defer ctrl.Finish()