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: Handle no-index case in backend mode #68534

Merged
merged 2 commits into from May 17, 2023

Conversation

gabor
Copy link
Contributor

@gabor gabor commented May 16, 2023

fixes #68531

when the index-field is not filled out in the elastic datasource config, currently we send the following index-value to the database:
index: [""]

what we should be sending is:
index: [] (maybe even not sending the index-entry at all, but this construct seems to work)

how to test:

  1. adjust/create an elastic datasource where the index is set to empty-string, set pattern to no pattern
  2. save, go to explore
  3. run queries, they should work
  4. (bonus points) verify that the request that is being sent from the go-server to the elastic-database, in fact, has the index: [] part in it
  5. go back and set the index to logs-* (for devenv-elastic). save
  6. verify that it still works in explore

@github-actions
Copy link
Contributor

Backend code coverage report for PR #68534

Plugin Main PR Difference
elasticsearch 86.6% 86.6% 0%

@github-actions
Copy link
Contributor

Frontend code coverage report for PR #68534
No changes

Base automatically changed from gabor/elastic-add-test to main May 16, 2023 15:09
@gabor gabor force-pushed the gabor/elastic-backend-no-index branch from 3f96da6 to f9b7e3e Compare May 16, 2023 15:55
@gabor gabor marked this pull request as ready for review May 16, 2023 15:59
@gabor gabor requested a review from a team as a code owner May 16, 2023 15:59
@gabor gabor requested review from ivanahuckova and removed request for a team May 16, 2023 15:59
@gabor gabor added this to the 10.0.x milestone May 16, 2023
@grafanabot
Copy link
Contributor

Hello @gabor!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

Copy link
Member

@ivanahuckova ivanahuckova left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 38 to 43
name := ip.indexName
if name != "" {
return []string{name}, nil
} else {
return []string{}, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

this is super nit pick, but I don't think name := ip.indexName improves readability and it adds assigning of new variable. So I would suggest to just do

	if ip.indexName != "" {
		return []string{ip.indexName}, nil
	} else {
		return []string{}, nil
	}

Copy link
Contributor

@matyax matyax May 17, 2023

Choose a reason for hiding this comment

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

I'm curious to know if, with Go, you could do both things at the same time with:

   return []string{ip.indexName}, 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.

@matyax unfortunately no, in case of indexName being "" (empty-string), that would return a single-item array where the item is empty-string

Copy link
Contributor

Choose a reason for hiding this comment

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

Right! I still have a hard time understanding this array format. Thanks for the explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivanahuckova agreed, changed 👍

@gabor gabor merged commit fcef387 into main May 17, 2023
11 checks passed
@gabor gabor deleted the gabor/elastic-backend-no-index branch May 17, 2023 13:24
grafanabot pushed a commit that referenced this pull request May 17, 2023
* elastic: backend migration: fix no-index case

* improved code

(cherry picked from commit fcef387)
gabor added a commit that referenced this pull request May 17, 2023
Elasticsearch: Handle no-index case in backend mode (#68534)

* elastic: backend migration: fix no-index case

* improved code

(cherry picked from commit fcef387)

Co-authored-by: Gábor Farkas <gabor.farkas@gmail.com>
@zerok zerok modified the milestones: 10.0.x, 10.0.0-preview, 10.1.x May 31, 2023
@ricky-undeadcoders ricky-undeadcoders removed this from the 10.1.x milestone Aug 1, 2023
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.

Elasticsearch: backend migration: empty index field does not work
6 participants