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: Reduce to apply Mode to instant vector (mathexp.Number) #74859

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

yuri-tceretian
Copy link
Contributor

What is this feature?
Updates reduce command expression to apply modes Drop and Replace to mathexp.Number.
Also, it changes when the warning is returned. If mapping mode is not Strict, the warning will not be added.

Why do we need this feature?
Initially, the expression applied this mode to mathexp.Series (i.e. time-series). However, from user's perspective, the format of data may not be clear and the expression behavior looks different.

Who is this feature for?
Alerting users that query data sources that return vectors and want to drop\map non-numeric values.

Which issue(s) does this PR fix?:

Fixes #74645

Special notes for your reviewer:

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.

@yuri-tceretian yuri-tceretian added add to changelog area/expressions Server Side Expressions (SSE) no-backport Skip backport of PR labels Sep 13, 2023
@yuri-tceretian yuri-tceretian requested a review from a team as a code owner September 13, 2023 16:15
@yuri-tceretian yuri-tceretian self-assigned this Sep 13, 2023
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.2.x milestone Sep 13, 2023
Text: fmt.Sprintf("Reduce operation is not needed. Input query or expression %s is already reduced data.", gr.VarToReduce),
})
copyV.SetValue(value)
if gr.seriesMapper == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is semantically correct because in this case reduce is not a noop anymore.

require.Equal(t, expectedValue, actualValue)
}

t.Run("should add warn notices to every frame", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should see if works with only the first frame, and if it does only put it there. Else were could be repeating this string a lot if there are many items in the response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can address it in a follow-up PR.

@yuri-tceretian yuri-tceretian merged commit 68eda13 into main Sep 14, 2023
12 checks passed
@yuri-tceretian yuri-tceretian deleted the yuri-tceretian/numer-reduce-mapping branch September 14, 2023 16:26
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/backend area/expressions Server Side Expressions (SSE) no-backport Skip backport of PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Drop/Replace Non-Numeric Values in Grafana Alerting does not work.
3 participants