Fix backend pipeline aggregation query parsing and data frame building #168

fridgepoet · 2023-05-12T14:51:39Z

What this PR does / why we need it:

Queries weren't being parsed correctly for the pipeline aggregation field. The same change as this one in Elasticsearch datasource was applied Elasticsearch: Fix pipeline aggregation queries in backend to pass frontend tests grafana#60337
The time vectors are now appended instead of created with nil placeholders the same length as the response buckets. If an entry was missing, then it left the time for that field “nil”. This would run into the following server-side expression error: https://github.com/grafana/grafana/blob/33ba5310ab82c615b158535d869119a8f3c85224/pkg/expr/mathexp/type_series.go#L93-L92 (This should be the same behavior as currently in the Elasticsearch datasource, by the way)

Which issue(s) this PR fixes:

Fixes #154

Special notes
I still notice an issue with the time series labels are being duplicated. #178

github-actions · 2023-05-12T14:53:01Z

Backend code coverage report for PR #168

Plugin	Main	PR	Difference
opensearch	77.9%	78.6%	.7%

github-actions · 2023-05-12T14:53:02Z

Frontend code coverage report for PR #168
No changes

github-actions · 2023-05-12T14:54:16Z

Levitate is-compatible report:

🔍 Resolving @grafana/data@latest...
🔍 Resolving @grafana/ui@latest...
🔍 Resolving @grafana/runtime@latest...
🔍 Resolving @grafana/e2e-selectors@latest...

🔬 Checking compatibility between ./src/module.ts and @grafana/data@9.5.2...
✔ Found @grafana/data version 9.0.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/ui@9.5.2...
✔ Found @grafana/ui version 9.0.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/runtime@9.5.2...
✔ Found @grafana/runtime version 9.0.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/e2e-selectors@9.5.2...
✔ Found @grafana/e2e-selectors version 9.0.2 locally

✔️ ./src/module.ts appears to be compatible with @grafana/data,@grafana/ui,@grafana/runtime,@grafana/e2e-selectors

…ineAgg Applies fix from grafana/grafana#60337

iwysiu

Nice! I tested it with the reported bug and it works now.

Its not a priority, but I wonder if it would make sense in a future commit to refactor the response_parser.go code to not use the null.Float object to make the syntax more similar

iwysiu · 2023-05-18T19:30:38Z

pkg/opensearch/lucene_handler.go

@@ -149,6 +150,20 @@ func (h *luceneHandler) processQuery(q *Query) error {
 	return nil
 }

+func getPipelineAggField(m *MetricAgg) string {
+	// From https://github.com/grafana/grafana/pull/60337
+	// In frontend we are using Field as pipelineAggField


nit: I don't know if we need to copy the todo comment (I'm assuming that if it's this old we decided that it was fine)

I don't like littering comments either, but in this case if that's alright for you, maybe we should leave this one for now.
We don't yet know the overall impact of this change and I know some tests are still using the old PipelineAggregate input.

I think to remove this function and this comment we should make a decision to deprecate the MetricAgg "PipelineAggregate" field and systematically use Field to get the pipeline aggregate field.

The original issue is "only" about 5 months old -- I think eventually we will clean out a number of comments in this repo and we could include cleaning out this one.

iwysiu · 2023-05-18T19:53:01Z

pkg/opensearch/lucene_handler.go

(I know it's not the point of this pr, but would you mind removing the m := m on line 85 while we're in here)

idastambuk

Clicked around, looks good. Great job on this!

I also see the problem with duplicated labels. I think it's a good idea to open a ticket for this

fridgepoet · 2023-05-22T07:32:49Z

pkg/opensearch/response_parser.go

@@ -588,6 +608,25 @@ func (rp *responseParser) getMetricName(metric string) string {
 	return metric
 }

+func castToFloat(j *simplejson.Json) *float64 {


The originally-used castToNullFloat function below uses an underlying standard library type sql.NullFloat64 which is also a way to distinguish between something "null/nil" or with a float64 value. In this case, we can also just use a *float64 type to distinguish between "nil" and float64 and there is no strong reason to use an imported type for that.

Here's a similar discussion: https://stackoverflow.com/questions/33458439/using-sql-nullfloat64-vs-float64-in-golang

In Elasticsearch I noticed they moved towards *float64 so I copied that; I think that in further refactors the castToNullFloat will no longer be used.

fridgepoet · 2023-05-22T07:38:21Z

pkg/opensearch/lucene_handler.go

@@ -149,6 +150,20 @@ func (h *luceneHandler) processQuery(q *Query) error {
 	return nil
 }

+func getPipelineAggField(m *MetricAgg) string {
+	// From https://github.com/grafana/grafana/pull/60337
+	// In frontend we are using Field as pipelineAggField


I don't like littering comments either, but in this case if that's alright for you, maybe we should leave this one for now.
We don't yet know the overall impact of this change and I know some tests are still using the old PipelineAggregate input.

I think to remove this function and this comment we should make a decision to deprecate the MetricAgg "PipelineAggregate" field and systematically use Field to get the pipeline aggregate field.

The original issue is "only" about 5 months old -- I think eventually we will clean out a number of comments in this repo and we could include cleaning out this one.

fridgepoet force-pushed the change-pipeline-aggregation branch from 1e1e900 to 53fa1dc Compare May 17, 2023 15:51

fridgepoet changed the base branch from main to test-response-parser May 17, 2023 15:52

fridgepoet force-pushed the change-pipeline-aggregation branch 2 times, most recently from da00e24 to 4ed743a Compare May 17, 2023 15:59

fridgepoet changed the title ~~WIP~~ Fix backend pipeline aggregation query parsing and data frame building May 17, 2023

fridgepoet force-pushed the change-pipeline-aggregation branch from 4ed743a to 9093797 Compare May 18, 2023 08:16

Base automatically changed from test-response-parser to main May 18, 2023 15:39

fridgepoet added 5 commits May 18, 2023 17:57

Backend: determine pipelineAggField from field and fall back to pipel…

d57d4ef

…ineAgg Applies fix from grafana/grafana#60337

Backend: Build time vector for frames by appending

edba7ea

Change to []time.Time

5820187

Add test about pipeline field fallback

a3efcf5

Add test for average and derivative

e522eea

fridgepoet force-pushed the change-pipeline-aggregation branch from 2baf2ea to e522eea Compare May 18, 2023 17:49

fridgepoet marked this pull request as ready for review May 18, 2023 18:09

fridgepoet requested a review from a team as a code owner May 18, 2023 18:09

fridgepoet requested review from iwysiu and sarahzinger and removed request for a team May 18, 2023 18:09

iwysiu approved these changes May 18, 2023

View reviewed changes

idastambuk approved these changes May 21, 2023

View reviewed changes

fridgepoet commented May 22, 2023

View reviewed changes

fridgepoet mentioned this pull request May 22, 2023

Backend: Time series legend is duplicating #178

Closed

fridgepoet merged commit b4a6f15 into main May 22, 2023
5 checks passed

fridgepoet deleted the change-pipeline-aggregation branch May 22, 2023 14:25

fridgepoet added the changelog label May 22, 2023

fridgepoet mentioned this pull request May 22, 2023

Prepare v2.5.1 release #179

Merged

fridgepoet mentioned this pull request Oct 2, 2023

Backend: Migrate Lucene metrics to the backend #197

Closed

3 tasks

fridgepoet mentioned this pull request Oct 3, 2023

Explore: Execute Lucene metric queries through the backend and Prepare release v2.13.0 #272

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix backend pipeline aggregation query parsing and data frame building #168

Fix backend pipeline aggregation query parsing and data frame building #168

fridgepoet commented May 12, 2023 •

edited

github-actions bot commented May 12, 2023 •

edited

github-actions bot commented May 12, 2023

github-actions bot commented May 12, 2023 •

edited

iwysiu left a comment

iwysiu May 18, 2023

fridgepoet May 22, 2023

iwysiu May 18, 2023

idastambuk left a comment

fridgepoet May 22, 2023

fridgepoet May 22, 2023

Fix backend pipeline aggregation query parsing and data frame building #168

Fix backend pipeline aggregation query parsing and data frame building #168

Conversation

fridgepoet commented May 12, 2023 • edited

github-actions bot commented May 12, 2023 • edited

github-actions bot commented May 12, 2023

github-actions bot commented May 12, 2023 • edited

iwysiu left a comment

Choose a reason for hiding this comment

iwysiu May 18, 2023

Choose a reason for hiding this comment

fridgepoet May 22, 2023

Choose a reason for hiding this comment

iwysiu May 18, 2023

Choose a reason for hiding this comment

idastambuk left a comment

Choose a reason for hiding this comment

fridgepoet May 22, 2023

Choose a reason for hiding this comment

fridgepoet May 22, 2023

Choose a reason for hiding this comment

fridgepoet commented May 12, 2023 •

edited

github-actions bot commented May 12, 2023 •

edited

github-actions bot commented May 12, 2023 •

edited