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

Fix: Add fields to frame if it does not already exist when grouping by multiple terms #392

Merged
merged 3 commits into from
May 24, 2024

Conversation

kevinwcyu
Copy link
Contributor

What this PR does / why we need it:

Multiple group by terms would only have the final group by term processed and added to the frame when executing on the backend.

This PR updates the way the frame is built up when processing aggregations. If the frame does not have any fields yet, the propKeys are added as fields (this will include all of the terms except the final term), otherwise if the frame already has fields, it will use those existing fields.

Which issue(s) this PR fixes:

Fixes #391

Special notes for your reviewer:

Sending queries to the backend needs to be reenabled for this to work. Change the following condition in datasource.ts

      request.targets.every(
        (target) =>
          target.metrics?.every(
            (metric) =>
              metric.type === 'raw_data' ||
              metric.type === 'raw_document' ||
              (request.app === CoreApp.Explore && target.queryType === QueryType.Lucene)
          ) ||
          (request.app === CoreApp.Explore && target.queryType === QueryType.PPL) ||
          target.luceneQueryType === LuceneQueryType.Traces
      )

to the following

      request.targets.every(
        (target) =>
          !(target.queryType === QueryType.PPL && target.format === 'time_series' && request.app === CoreApp.Dashboard)
      )

We can reenable sending queries to the backend after the remaining backend migration related bugs are fixed.

Here's an example of a query you can test with. It just needs to have at least 2 group by terms.

Screenshot 2024-05-22 at 2 54 32 PM

@kevinwcyu kevinwcyu requested a review from a team as a code owner May 22, 2024 21:57
@kevinwcyu kevinwcyu requested review from idastambuk and njvrzm and removed request for a team May 22, 2024 21:57
@idastambuk
Copy link
Contributor

Tested it, looks good!
The only thing is that sometimes Im getting a too many buckets error and it's not passed on to the user (all we get in the UI is unknown error. I created this to make sure we don't forget it.

Copy link
Contributor

@njvrzm njvrzm left a comment

Choose a reason for hiding this comment

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

Small nit, one minor comment and one question but: 👍🏻

@@ -999,7 +999,11 @@ func (rp *responseParser) processAggregationDocs(esAgg *simplejson.Json, aggDef
frames := data.Frames{}
var fields []*data.Field

if queryResult.Frames == nil {
if queryResult.Frames != nil && len(queryResult.Frames) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: you can just write if len(queryResult.Frames) != 0 here - you'd only need the nil check if queryResult.Frames was a pointer.

@@ -1012,7 +1016,8 @@ func (rp *responseParser) processAggregationDocs(esAgg *simplejson.Json, aggDef
for _, e := range fields {
for _, propKey := range propKeys {
if e.Name == propKey {
e.Append(props[propKey])
value := props[propKey]
e.Append(&value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious - did something in the sdk or elsewhere change, or did this code path never get executed before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this bit of the code used to build the results the same way as the frontend flow (building a table), but was refactored to use data frames instead. that refactoring was done by following elasticsearch's pr (which had the same bug that this PR is addressing). so it probably wasn't being executed from that point onwards. but we didn't notice because the backend flow would've only been run for alerts or in explore and it would've only been an issue if someone was using multiple group by terms.

pkg/opensearch/response_parser.go Outdated Show resolved Hide resolved
kevinwcyu and others added 2 commits May 23, 2024 09:12
Co-authored-by: Nathan Vērzemnieks <[email protected]>
@kevinwcyu kevinwcyu merged commit 23e0a22 into main May 24, 2024
4 checks passed
@kevinwcyu kevinwcyu deleted the kevinwcyu/391-group-by-queries-on-backend branch May 24, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backend migration: When adding multiple groupBy terms, only the last group works
3 participants