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

Elasticsearch: Fix processing of response with multiple group by for alerting #65165

Merged
merged 4 commits into from
Mar 22, 2023

Conversation

ivanahuckova
Copy link
Member

@ivanahuckova ivanahuckova commented Mar 22, 2023

This PR fixes processing of response with multiple group by on backend. On backend, we were missing using fields from frames if there already were any, and add fields from propKeys if there weren't any. This is correctly done on frontend implementation where we check if we have any columns and if yes, we use them instead of adding propKeys.

Fixes: #64727
Part of #54011

How to test:

  1. I added test for this scenario, so first of all, it should pass 🙂
  2. With feature toggle elasticsearchBackendMigration=true run make devenv sources=elastic
  3. Open Explore and run query following query with 2 terms group by

image

4. There should be a correct response in table

@github-actions
Copy link
Contributor

Backend code coverage report for PR #65165

Plugin Main PR Difference
elasticsearch 88.1% 88.9% .8%

@github-actions
Copy link
Contributor

Frontend code coverage report for PR #65165
No changes

Copy link
Contributor

@gabor gabor left a comment

Choose a reason for hiding this comment

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

LGTM 👍 (left one nitpick comment)

pkg/tsdb/elasticsearch/response_parser.go Outdated Show resolved Hide resolved
@ivanahuckova ivanahuckova enabled auto-merge (squash) March 22, 2023 16:34
@ivanahuckova ivanahuckova merged commit ae0f9fc into main Mar 22, 2023
@ivanahuckova ivanahuckova deleted the ivana/es-group-by-multiple branch March 22, 2023 16:53
gotjosh pushed a commit that referenced this pull request Mar 27, 2023
…alerting (#65165)

* Elasticsearch: Fix processing of response with multiple group by on backend

* Remove unused param from createFields

* Switch comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alerting using Elasticsearch as datasource is broken for grouping by multiple fields
3 participants