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
Alerting: Fix mathexp.NoData cannot be reduced #55347
Conversation
6b6c0f9
to
18ef349
Compare
pkg/expr/commands.go
Outdated
@@ -171,6 +171,8 @@ func (gr *ReduceCommand) Execute(_ context.Context, vars mathexp.Vars) (mathexp. | |||
Text: fmt.Sprintf("Reduce operation is not needed. Input query or expression %s is already reduced data.", gr.VarToReduce), | |||
}) | |||
newRes.Values = append(newRes.Values, copyV) | |||
case mathexp.NoData: | |||
newRes.Values = append(newRes.Values, v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that for mathexp.Number
we create a copy called copyV
. I'm not sure we need to do that here unless we also need to copy the metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copying is so pointers don't accidentally mutate data in upstream operations (since the result of each operation can be returned in SSE).
Metadata is still a bit wonky and undefined. But in general, for the output of a non-datasource query operation such as reduce, it is probably better to create a NewNoData() that creates a Frame with no fields.
Side Note: My idea with "NoData" is "untyped nodata". In the future, I could see adding NoDataTimeSeries etc.
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34016 |
18ef349
to
906b940
Compare
This commit fixes a bug where queries from datasources such as InfluxDB that returned no data would not create a DatasourceNoData alert, but instead an error "can only reduce type series, got type noData".
906b940
to
e44aa69
Compare
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34049 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We need the same for Classic condition as well
This commit fixes a bug where queries from datasources such as InfluxDB that returned no data would not create a DatasourceNoData alert, but instead an error "can only reduce type series, got type noData". (cherry picked from commit 7d20766)
This commit fixes a bug where queries from datasources such as InfluxDB that returned no data would not create a DatasourceNoData alert, but instead an error "can only reduce type series, got type noData". (cherry picked from commit 7d20766) Co-authored-by: George Robinson <george.robinson@grafana.com>
What this PR does / why we need it:
This is one of two possible fixes for #55085. It changes the reduce function to reduce
mathexp.NoData
to a result with no values instead of an error "can only reduce type series, got type noData".Which issue(s) this PR fixes:
Fixes #55085
Special notes for your reviewer: