Skip to content

Commit

Permalink
Merge pull request #505 from bookingcom/emohamadi/fix-with-wildcards-…
Browse files Browse the repository at this point in the history 
…functions-panic

Fix with wildcards functions panic
  • Loading branch information
@emadolsky
emadolsky authored May 21, 2024
2 parents 9de3834 + 630cc37 commit ef81a7a
Show file tree
Hide file tree
Showing 7 changed files with 242 additions and 178 deletions.
2 changes: 1 addition & 1 deletion pkg/app/carbonapi/http_handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ func evalExprRender(ctx context.Context, exp parser.Expr, res *([]*types.MetricD
form *renderForm, printErrorStackTrace bool, getTargetData interfaces.GetTargetData) (retErr error) {
defer func() {
if r := recover(); r != nil {
retErr = fmt.Errorf("panic during expr eval: %s", r)
retErr = fmt.Errorf("panic during expr eval: %s\n%s", r, string(debug.Stack()))
if printErrorStackTrace {
debug.PrintStack()
}
Expand Down
65 changes: 6 additions & 59 deletions pkg/expr/functions/averageSeriesWithWildcards/function.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ package averageSeriesWithWildcards

import (
"context"
"fmt"
"strings"

"github.com/bookingcom/carbonapi/pkg/expr/helper"
"github.com/bookingcom/carbonapi/pkg/expr/interfaces"
Expand Down Expand Up @@ -42,64 +40,13 @@ func (f *averageSeriesWithWildcards) Do(ctx context.Context, e parser.Expr, from
return nil, err
}

var results []*types.MetricData

nodeList := []string{}
groups := make(map[string][]*types.MetricData)

for _, a := range args {
metric := helper.ExtractMetric(a.Name)
nodes := strings.Split(metric, ".")
var s []string
// Yes, this is O(n^2), but len(nodes) < 10 and len(fields) < 3
// Iterating an int slice is faster than a map for n ~ 30
// http://www.antoine.im/posts/someone_is_wrong_on_the_internet
for i, n := range nodes {
if !helper.Contains(fields, i) {
s = append(s, n)
}
}

node := strings.Join(s, ".")

if len(groups[node]) == 0 {
nodeList = append(nodeList, node)
}

groups[node] = append(groups[node], a)
}

for _, series := range nodeList {
args := groups[series]
r := *args[0]
r.Name = fmt.Sprintf("averageSeriesWithWildcards(%s)", series)
r.Values = make([]float64, len(args[0].Values))
r.IsAbsent = make([]bool, len(args[0].Values))

length := make([]float64, len(args[0].Values))
atLeastOne := make([]bool, len(args[0].Values))
for _, arg := range args {
for i, v := range arg.Values {
if arg.IsAbsent[i] {
continue
}
atLeastOne[i] = true
length[i]++
r.Values[i] += v
}
return helper.AggregateSeriesWithWildcards("averageSeriesWithWildcards", args, fields, func(values []float64) (float64, bool) {
sum := 0.0
for _, value := range values {
sum += value
}

for i, v := range atLeastOne {
if v {
r.Values[i] = r.Values[i] / length[i]
} else {
r.IsAbsent[i] = true
}
}

results = append(results, &r)
}
return results, nil
return sum / float64(len(values)), false
})
}

// Description is auto-generated description, based on output of https://github.com/graphite-project/graphite-web
Expand Down
68 changes: 6 additions & 62 deletions pkg/expr/functions/multiplySeriesWithWildcards/function.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ package multiplySeriesWithWildcards

import (
"context"
"fmt"
"strings"

"github.com/bookingcom/carbonapi/pkg/expr/helper"
"github.com/bookingcom/carbonapi/pkg/expr/interfaces"
Expand Down Expand Up @@ -43,68 +41,14 @@ func (f *multiplySeriesWithWildcards) Do(ctx context.Context, e parser.Expr, fro
return nil, err
}

var results []*types.MetricData

nodeList := []string{}
groups := make(map[string][]*types.MetricData)

for _, a := range args {
metric := helper.ExtractMetric(a.Name)
nodes := strings.Split(metric, ".")
var s []string
// Yes, this is O(n^2), but len(nodes) < 10 and len(fields) < 3
// Iterating an int slice is faster than a map for n ~ 30
// http://www.antoine.im/posts/someone_is_wrong_on_the_internet
for i, n := range nodes {
if !helper.Contains(fields, i) {
s = append(s, n)
}
}

node := strings.Join(s, ".")

if len(groups[node]) == 0 {
nodeList = append(nodeList, node)
return helper.AggregateSeriesWithWildcards("multiplySeriesWithWildcards", args, fields, func(values []float64) (float64, bool) {
ret := values[0]
for _, value := range values[1:] {
ret *= value
}

groups[node] = append(groups[node], a)
}

for _, series := range nodeList {
args := groups[series]
r := *args[0]
r.Name = fmt.Sprintf("multiplySeriesWithWildcards(%s)", series)
r.Values = make([]float64, len(args[0].Values))
r.IsAbsent = make([]bool, len(args[0].Values))

atLeastOne := make([]bool, len(args[0].Values))
hasVal := make([]bool, len(args[0].Values))

for _, arg := range args {
for i, v := range arg.Values {
if arg.IsAbsent[i] {
continue
}

atLeastOne[i] = true
if !hasVal[i] {
r.Values[i] = v
hasVal[i] = true
} else {
r.Values[i] *= v
}
}
}

for i, v := range atLeastOne {
if !v {
r.IsAbsent[i] = true
}
}

results = append(results, &r)
}
return results, nil
return ret, false
})
}

// Description is auto-generated description, based on output of https://github.com/graphite-project/graphite-web
Expand Down
93 changes: 93 additions & 0 deletions pkg/expr/functions/multiplySeriesWithWildcards/function_test.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package multiplySeriesWithWildcards

import (
"testing"
"time"

"go.uber.org/zap"

"github.com/bookingcom/carbonapi/pkg/expr/helper"
"github.com/bookingcom/carbonapi/pkg/expr/metadata"
"github.com/bookingcom/carbonapi/pkg/expr/types"
"github.com/bookingcom/carbonapi/pkg/parser"
th "github.com/bookingcom/carbonapi/tests"
)

func init() {
md := New("")
evaluator := th.EvaluatorFromFunc(md[0].F)
metadata.SetEvaluator(evaluator)
helper.SetEvaluator(evaluator)
for _, m := range md {
metadata.RegisterFunction(m.Name, m.F, zap.NewNop())
}
}

func TestMultiplySeriesWithWildcards(t *testing.T) {
now32 := int32(time.Now().Unix())

tests := []th.EvalTestItem{
{
"multiplySeriesWithWildcards(empty,0)",
map[parser.MetricRequest][]*types.MetricData{
{"empty", 0, 1}: {
types.MakeMetricData("metric0", []float64{}, 1, now32),
},
},
[]*types.MetricData{types.MakeMetricData("multiplySeriesWithWildcards()",
[]float64{}, 1, now32)},
},
{
"multiplySeriesWithWildcards(*,0)",
map[parser.MetricRequest][]*types.MetricData{
{"*", 0, 1}: {
types.MakeMetricData("m1", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m2", []float64{5, 4, 3, 2, 1}, 1, now32),
},
},
[]*types.MetricData{types.MakeMetricData("multiplySeriesWithWildcards()",
[]float64{5, 8, 9, 8, 5}, 1, now32)},
},
{
"multiplySeriesWithWildcards(m1.*.*,2)",
map[parser.MetricRequest][]*types.MetricData{
{"m1.*.*", 0, 1}: {
types.MakeMetricData("m1.n1.o1", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m1.n2.o2", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m1.n1.o2", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m1.n2.o1", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m1.n2.o3", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m1.n2.o4", []float64{1, 2, 3, 4, 5}, 1, now32),
},
},
[]*types.MetricData{
types.MakeMetricData("multiplySeriesWithWildcards(m1.n1)", []float64{1, 4, 9, 16, 25}, 1, now32),
types.MakeMetricData("multiplySeriesWithWildcards(m1.n2)", []float64{1, 16, 81, 256, 625}, 1, now32),
},
},
{
"multiplySeriesWithWildcards(m1.*.*,1,2)",
map[parser.MetricRequest][]*types.MetricData{
{"m1.*.*", 0, 1}: {
types.MakeMetricData("m1.n1.o1", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m1.n2.o2", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m1.n1.o2", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m1.n2.o1", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m1.n2.o3", []float64{1, 2, 3, 4, 5}, 1, now32),
types.MakeMetricData("m1.n2.o4", []float64{1, 2, 3, 4, 5}, 1, now32),
},
},
[]*types.MetricData{
types.MakeMetricData("multiplySeriesWithWildcards(m1)", []float64{1, 64, 729, 4096, 15625}, 1, now32),
},
},
}

for _, tt := range tests {
copy := tt
testName := tt.Target
t.Run(testName, func(t *testing.T) {
th.TestEvalExpr(t, &copy)
})
}
}
58 changes: 2 additions & 56 deletions pkg/expr/functions/sumSeriesWithWildcards/function.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ package sumSeriesWithWildcards

import (
"context"
"fmt"
"strings"

"github.com/bookingcom/carbonapi/pkg/expr/functions/sum"
"github.com/bookingcom/carbonapi/pkg/expr/helper"
"github.com/bookingcom/carbonapi/pkg/expr/interfaces"
"github.com/bookingcom/carbonapi/pkg/expr/types"
Expand Down Expand Up @@ -42,60 +41,7 @@ func (f *sumSeriesWithWildcards) Do(ctx context.Context, e parser.Expr, from, un
return nil, err
}

var results []*types.MetricData

nodeList := []string{}
groups := make(map[string][]*types.MetricData)

for _, a := range args {
metric := helper.ExtractMetric(a.Name)
nodes := strings.Split(metric, ".")
var s []string
// Yes, this is O(n^2), but len(nodes) < 10 and len(fields) < 3
// Iterating an int slice is faster than a map for n ~ 30
// http://www.antoine.im/posts/someone_is_wrong_on_the_internet
for i, n := range nodes {
if !helper.Contains(fields, i) {
s = append(s, n)
}
}

node := strings.Join(s, ".")

if len(groups[node]) == 0 {
nodeList = append(nodeList, node)
}

groups[node] = append(groups[node], a)
}

for _, series := range nodeList {
args := groups[series]
r := *args[0]
r.Name = fmt.Sprintf("sumSeriesWithWildcards(%s)", series)
r.Values = make([]float64, len(args[0].Values))
r.IsAbsent = make([]bool, len(args[0].Values))

atLeastOne := make([]bool, len(args[0].Values))
for _, arg := range args {
for i, v := range arg.Values {
if arg.IsAbsent[i] {
continue
}
atLeastOne[i] = true
r.Values[i] += v
}
}

for i, v := range atLeastOne {
if !v {
r.IsAbsent[i] = true
}
}

results = append(results, &r)
}
return results, nil
return helper.AggregateSeriesWithWildcards("sumSeriesWithWildcards", args, fields, sum.SumAggregation)
}

// Description is auto-generated description, based on output of https://github.com/graphite-project/graphite-web
Expand Down
Loading

0 comments on commit ef81a7a

Please sign in to comment.