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

Conversation

kylebrandt
Copy link
Contributor

@kylebrandt kylebrandt commented Aug 1, 2023

What is this feature?

Unions in SSE operations currently drop items when doing a binary operation (e.g. $A + $B) from either A or B when there isn't a label "match" between them.

This is confusing, so to aid in this, we warn and display the count of items that where dropped, and which items they were.

image

image

Fixes #

Special notes for your reviewer:

Design wise, we really need to revisit "Union". In short, Math should probably have a dropdown that changes what happens with a binary operation (e.g. $a OP $b) in terms of union/join set operations based on labels. So this is more in the direction of "Informational based on the currently (and only mode)" so as not to create a future problem when tackling this in a broader sense.

The output format is pretty dense. The way it works is from Left to Right, so - ((A union B) union C). So things can be dropped between union of A and B, as well as between the result of of the the union between A and B then having a Union with C. To put it another way, with $A + $B + $C, if there is a match between A and C, but not between A and B, the item will be dropped.

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@kylebrandt kylebrandt added the area/expressions Server Side Expressions (SSE) label Aug 1, 2023
@kylebrandt kylebrandt requested a review from a team as a code owner August 1, 2023 14:16
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.2.x milestone Aug 1, 2023
@@ -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

@kylebrandt kylebrandt added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Aug 1, 2023
@kylebrandt kylebrandt changed the title SSE: (WIP) Warn on dropped items in Union SSE: Warn on dropped items in Union in Math Operation Aug 1, 2023
pkg/expr/mathexp/exp.go Outdated Show resolved Hide resolved
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.

@kylebrandt kylebrandt enabled auto-merge (squash) August 3, 2023 18:01
Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

LGTM.
Not in this PR, but I think it would be great to have different modes of SSE evals: normal and diagnostics. In the diag mode each node could emit additional information that could be exposed to front-end and presented in some fancy UI. In normal mode it can be evaluated by alerting, so it does not have to spend time on collecting diag info.

@kylebrandt kylebrandt merged commit d0ad4fc into main Aug 3, 2023
12 checks passed
@kylebrandt kylebrandt deleted the sseUnionDropWarning branch August 3, 2023 18:23
aishyandapalli pushed a commit to aishyandapalli/grafana that referenced this pull request Aug 16, 2023
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
@zerok zerok removed this from the 10.2.x milestone Oct 23, 2023
@bobemoe
Copy link
Contributor

bobemoe commented Mar 9, 2024

I think these are erroneously appearing on my Graphite Math. Any way to hide them? See above linked issue for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend area/expressions Server Side Expressions (SSE) no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants