From d35bafef16f17d06a8bb1fc3fc58a1fc31d33c02 Mon Sep 17 00:00:00 2001 From: Evan Yin Date: Sat, 31 Jul 2021 11:43:11 -0700 Subject: [PATCH] [query] Fix groupByNodes to get proper first path (#3640) --- .../graphite/native/aggregation_functions.go | 42 ++++++++----------- .../native/aggregation_functions_test.go | 11 ++++- .../graphite/native/builtin_functions.go | 10 ++++- 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/src/query/graphite/native/aggregation_functions.go b/src/query/graphite/native/aggregation_functions.go index 051dcb39ab..eade8eaf01 100644 --- a/src/query/graphite/native/aggregation_functions.go +++ b/src/query/graphite/native/aggregation_functions.go @@ -604,29 +604,15 @@ func groupByNode(ctx *common.Context, series singlePathSpec, node int, fname str return groupByNodes(ctx, series, fname, []int{node}...) } -func findFirstMetricExpression(seriesName string) (string, bool) { - idxOfRightParen := strings.Index(seriesName, ")") - if idxOfRightParen == -1 { - return "", false - } - substring := seriesName[:idxOfRightParen] - idxOfLeftParen := strings.LastIndex(substring, "(") - if idxOfLeftParen == -1 { - return "", false - } - return seriesName[idxOfLeftParen+1 : idxOfRightParen], true -} - -func getParts(series *ts.Series) []string { +func getAggregationKey(series *ts.Series, nodes []int) (string, error) { seriesName := series.Name() - if metricExpr, ok := findFirstMetricExpression(seriesName); ok { - seriesName = metricExpr + metricsPath, err := getFirstPathExpression(seriesName) + if err != nil { + return "", err } - return strings.Split(seriesName, ".") -} + seriesName = metricsPath -func getAggregationKey(series *ts.Series, nodes []int) string { - parts := getParts(series) + parts := strings.Split(seriesName, ".") keys := make([]string, 0, len(nodes)) for _, n := range nodes { @@ -640,20 +626,23 @@ func getAggregationKey(series *ts.Series, nodes []int) string { keys = append(keys, "") } } - return strings.Join(keys, ".") + return strings.Join(keys, "."), nil } -func getMetaSeriesGrouping(seriesList singlePathSpec, nodes []int) map[string][]*ts.Series { +func getMetaSeriesGrouping(seriesList singlePathSpec, nodes []int) (map[string][]*ts.Series, error) { metaSeries := make(map[string][]*ts.Series) if len(nodes) > 0 { for _, s := range seriesList.Values { - key := getAggregationKey(s, nodes) + key, err := getAggregationKey(s, nodes) + if err != nil { + return metaSeries, err + } metaSeries[key] = append(metaSeries[key], s) } } - return metaSeries + return metaSeries, nil } // Takes a serieslist and maps a callback to subgroups within as defined by multiple nodes @@ -668,7 +657,10 @@ func getMetaSeriesGrouping(seriesList singlePathSpec, nodes []int) map[string][] // // NOTE: if len(nodes) = 0, aggregate all series into 1 series. func groupByNodes(ctx *common.Context, seriesList singlePathSpec, fname string, nodes ...int) (ts.SeriesList, error) { - metaSeries := getMetaSeriesGrouping(seriesList, nodes) + metaSeries, err := getMetaSeriesGrouping(seriesList, nodes) + if err != nil { + return ts.NewSeriesList(), err + } if len(metaSeries) == 0 { // if nodes is an empty slice or every node in nodes exceeds the number diff --git a/src/query/graphite/native/aggregation_functions_test.go b/src/query/graphite/native/aggregation_functions_test.go index 0623f3dc3e..d4c211b6cc 100644 --- a/src/query/graphite/native/aggregation_functions_test.go +++ b/src/query/graphite/native/aggregation_functions_test.go @@ -776,7 +776,7 @@ func TestGroupByNodes(t *testing.T) { inputs = []*ts.Series{ ts.NewSeries(ctx, "transformNull(servers.foo-1.pod1.status.500)", start, ts.NewConstantValues(ctx, 2, 12, 10000)), - ts.NewSeries(ctx, "servers.foo-2.pod1.status.500", start, + ts.NewSeries(ctx, "scaleToSeconds(servers.foo-2.pod1.status.500,1)", start, ts.NewConstantValues(ctx, 4, 12, 10000)), ts.NewSeries(ctx, "servers.foo-3.pod1.status.500", start, ts.NewConstantValues(ctx, 6, 12, 10000)), @@ -836,7 +836,14 @@ func TestGroupByNodes(t *testing.T) { {"pod2.500", 8 * 12}, }}, {"sum", []int{}, []result{ // test empty slice handing. - {"sumSeries(transformNull(servers.foo-1.pod1.status.500),servers.foo-2.pod1.status.500,servers.foo-3.pod1.status.500,servers.foo-1.pod2.status.500,servers.foo-2.pod2.status.500,servers.foo-1.pod1.status.400,servers.foo-2.pod1.status.400,servers.foo-3.pod2.status.400)", (2 + 4 + 6 + 8 + 10 + 20 + 30 + 40) * 12}, + { + "sumSeries(transformNull(servers.foo-1.pod1.status.500)," + + "scaleToSeconds(servers.foo-2.pod1.status.500,1)," + + "servers.foo-3.pod1.status.500,servers.foo-1.pod2.status.500," + + "servers.foo-2.pod2.status.500,servers.foo-1.pod1.status.400," + + "servers.foo-2.pod1.status.400,servers.foo-3.pod2.status.400)", + (2 + 4 + 6 + 8 + 10 + 20 + 30 + 40) * 12, + }, }}, {"sum", []int{100}, []result{ // test all nodes out of bounds {"", (2 + 4 + 6 + 8 + 10 + 20 + 30 + 40) * 12}, diff --git a/src/query/graphite/native/builtin_functions.go b/src/query/graphite/native/builtin_functions.go index da2201bcde..4ed51d9a69 100644 --- a/src/query/graphite/native/builtin_functions.go +++ b/src/query/graphite/native/builtin_functions.go @@ -1033,7 +1033,10 @@ func asPercent(ctx *common.Context, input singlePathSpec, total genericInterface } if len(nodes) > 0 { - metaSeries := getMetaSeriesGrouping(input, nodes) + metaSeries, err := getMetaSeriesGrouping(input, nodes) + if err != nil { + return ts.NewSeriesList(), err + } totalSeries := make(map[string]*ts.Series) switch totalArg := total.(type) { @@ -1057,7 +1060,10 @@ func asPercent(ctx *common.Context, input singlePathSpec, total genericInterface case singlePathSpec: total = ts.SeriesList(v) } - totalGroups := getMetaSeriesGrouping(singlePathSpec(total), nodes) + totalGroups, err := getMetaSeriesGrouping(singlePathSpec(total), nodes) + if err != nil { + return ts.NewSeriesList(), err + } for k, series := range totalGroups { if len(series) == 1 { totalSeries[k] = series[0]