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

SSE: Warn on dropped items in Union in Math Operation #72682

Merged
merged 9 commits into from
Aug 3, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
121 changes: 112 additions & 9 deletions pkg/expr/mathexp/exp.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"math"
"reflect"
"runtime"
"strings"

"github.com/grafana/grafana-plugin-sdk-go/data"

Expand All @@ -25,7 +26,9 @@ type State struct {
// Could hold more properties that change behavior around:
// - Unions (How many result A and many Result B in case A + B are joined)
// - NaN/Null behavior
RefID string
RefID string
Drops map[string]map[string][]data.Labels // binary node text -> LH/RH -> Drop Labels
DropCount int64

tracer tracing.Tracer
}
Expand Down Expand Up @@ -61,6 +64,7 @@ func (e *Expr) Execute(refID string, vars Vars, tracer tracing.Tracer) (r Result
func (e *Expr) executeState(s *State) (r Results, err error) {
defer errRecover(&err, s)
r, err = s.walk(e.Tree.Root)
s.addDropNotices(&r)
return
}

Expand Down Expand Up @@ -198,25 +202,63 @@ type Union struct {
// within a collection of Series or Numbers. The Unions are used with binary
// operations. The labels of the Union will the taken from result with a greater
// number of tags.
func union(aResults, bResults Results) []*Union {
func (e *State) union(aResults, bResults Results, biNode *parse.BinaryNode) []*Union {
unions := []*Union{}
appendUnions := func(u *Union) {
unions = append(unions, u)
}

aVar := biNode.Args[0].String()
bVar := biNode.Args[1].String()

aMatched := make([]bool, len(aResults.Values))
bMatched := make([]bool, len(bResults.Values))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand the approach. I wonder if we can hold only indices of the unmatched. In this case, we will not have to pre-allocate slices, which can be pretty big (I've seen 10k dimensions) but act more lazily because dropped metrics are not something that appears often during regular rule evaluation.

This will require no-data case to be handled separately but I think it will help readability.

Copy link
Contributor Author

@kylebrandt kylebrandt Aug 3, 2023

Choose a reason for hiding this comment

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

This is what I tried at first but ran into difficulty. With the nested loop, we loop over B results multiple times. So during that loop - it is not known if a result will be matched in a future loop. So I ended up creating bool mask where I mark as matched during the loop iteration, and then take the unmatched items.

collectDrops := func() {
check := func(v string, matchArray []bool, r *Results) {
for i, b := range matchArray {
if !b {
kylebrandt marked this conversation as resolved.
Show resolved Hide resolved
if e.Drops == nil {
e.Drops = make(map[string]map[string][]data.Labels)
}
if e.Drops[biNode.String()] == nil {
e.Drops[biNode.String()] = make(map[string][]data.Labels)
}

if r.Values[i].Type() == parse.TypeNoData {
continue
}

e.DropCount++
e.Drops[biNode.String()][v] = append(e.Drops[biNode.String()][v], r.Values[i].GetLabels())
}
}
}
check(aVar, aMatched, &aResults)
check(bVar, bMatched, &bResults)
}

aValueLen := len(aResults.Values)
bValueLen := len(bResults.Values)
if aValueLen == 0 || bValueLen == 0 {
return unions
}

if aValueLen == 1 || bValueLen == 1 {
if aResults.Values[0].Type() == parse.TypeNoData || bResults.Values[0].Type() == parse.TypeNoData {
unions = append(unions, &Union{
aNoData := aResults.Values[0].Type() == parse.TypeNoData
bNoData := bResults.Values[0].Type() == parse.TypeNoData
if aNoData || bNoData {
appendUnions(&Union{
Labels: nil,
A: aResults.Values[0],
B: bResults.Values[0],
})
collectDrops()
return unions
}
}
for _, a := range aResults.Values {
for _, b := range bResults.Values {

for iA, a := range aResults.Values {
for iB, b := range bResults.Values {
var labels data.Labels
aLabels := a.GetLabels()
bLabels := b.GetLabels()
Expand All @@ -241,20 +283,25 @@ func union(aResults, bResults Results) []*Union {
A: a,
B: b,
}
unions = append(unions, u)
appendUnions(u)
aMatched[iA] = true
bMatched[iB] = true
}
}

if len(unions) == 0 && len(aResults.Values) == 1 && len(bResults.Values) == 1 {
// In the case of only 1 thing on each side of the operator, we combine them
// and strip the tags.
// This isn't ideal for understanding behavior, but will make more stuff work when
// combining different datasources without munging.
// This choice is highly questionable in the long term.
unions = append(unions, &Union{
appendUnions(&Union{
A: aResults.Values[0],
B: bResults.Values[0],
})
}

collectDrops()
return unions
}

Expand All @@ -268,7 +315,7 @@ func (e *State) walkBinary(node *parse.BinaryNode) (Results, error) {
if err != nil {
return res, err
}
unions := union(ar, br)
unions := e.union(ar, br, node)
for _, uni := range unions {
var value Value
switch at := uni.A.(type) {
Expand Down Expand Up @@ -548,3 +595,59 @@ func (e *State) walkFunc(node *parse.FuncNode) (Results, error) {
}
return res, nil
}

func (e *State) addDropNotices(r *Results) {
nT := strings.Builder{}

if e.DropCount > 0 && len(r.Values) > 0 {
itemsPerNodeLimit := 5 // Limit on dropped items shown per each node in the binary node

nT.WriteString(fmt.Sprintf("%v items dropped from union(s)", e.DropCount))
if len(e.Drops) > 0 {
nT.WriteString(": ")

biNodeDropCount := 0
for biNodeText, biNodeDrops := range e.Drops {
nT.WriteString(fmt.Sprintf(`["%s": `, biNodeText))

nodeCount := 0
for inputNode, droppedItems := range biNodeDrops {
nT.WriteString(fmt.Sprintf("(%s: ", inputNode))

itemCount := 0
for _, item := range droppedItems {
nT.WriteString(fmt.Sprintf("{%s}", item))

itemCount++
if itemCount == itemsPerNodeLimit {
nT.WriteString(fmt.Sprintf("...%v more...", len(droppedItems)-itemsPerNodeLimit))
break
}
if itemCount < len(droppedItems) {
nT.WriteString(" ")
}
}

nT.WriteString(")")

nodeCount++
if nodeCount < len(biNodeDrops) {
nT.WriteString(" ")
}
}

nT.WriteString("]")

biNodeDropCount++
if biNodeDropCount < len(biNodeDrops) {
nT.WriteString(" ")
}
}
}

r.Values[0].AddNotice(data.Notice{
Severity: data.NoticeSeverityWarning,
Text: nT.String(),
})
}
}
7 changes: 4 additions & 3 deletions pkg/expr/mathexp/exp_nan_null_val_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/grafana/grafana-plugin-sdk-go/data"
"github.com/grafana/grafana/pkg/expr/mathexp/parse"
"github.com/grafana/grafana/pkg/infra/tracing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -491,21 +492,21 @@ func TestNoData(t *testing.T) {
res, err := e.Execute("", makeVars(NewNoData(), NewNoData()), tracing.NewFakeTracer())
require.NoError(t, err)
require.Len(t, res.Values, 1)
require.Equal(t, NewNoData(), res.Values[0])
require.Equal(t, parse.TypeNoData, res.Values[0].Type())
})

t.Run("$A=nodata, $B=series", func(t *testing.T) {
res, err := e.Execute("", makeVars(NewNoData(), series), tracing.NewFakeTracer())
require.NoError(t, err)
require.Len(t, res.Values, 1)
require.Equal(t, NewNoData(), res.Values[0])
require.Equal(t, parse.TypeNoData, res.Values[0].Type())
})

t.Run("$A=series, $B=nodata", func(t *testing.T) {
res, err := e.Execute("", makeVars(NewNoData(), series), tracing.NewFakeTracer())
require.NoError(t, err)
require.Len(t, res.Values, 1)
require.Equal(t, NewNoData(), res.Values[0])
require.Equal(t, parse.TypeNoData, res.Values[0].Type())
})
}
})
Expand Down
2 changes: 2 additions & 0 deletions pkg/expr/mathexp/parse/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ func (t NodeType) String() string {
return "NodeString"
case NodeNumber:
return "NodeNumber"
case NodeVar:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

random fix, can go in another PR if this isn't merged

return "NodeVar"
default:
return "NodeUnknown"
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/expr/mathexp/union_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"

"github.com/grafana/grafana-plugin-sdk-go/data"
"github.com/grafana/grafana/pkg/expr/mathexp/parse"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -293,7 +294,8 @@ func Test_union(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
unions := union(tt.aResults, tt.bResults)
fakeNode := &parse.BinaryNode{Args: [2]parse.Node{&parse.VarNode{}, &parse.VarNode{}}}
unions := (&State{}).union(tt.aResults, tt.bResults, fakeNode)
tt.unionsAre(t, tt.unions, unions)
})
}
Expand Down