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 handling of inline scripts in different ES versions #34070

Merged
merged 3 commits into from May 14, 2021

Conversation

Elfo404
Copy link
Member

@Elfo404 Elfo404 commented May 13, 2021

What this PR does / why we need it:
Aggregations' Inline scripts prior to ES 5.6 had to be defined like (VERSION B)

"script": {
  "inline": "script_value"
}

in 5.6, the former was deprecated in favor of:

"script": {
  "source": "script_value"
}

or, as a shorthand: (VERSION A)

"script": "script_value"

In Grafana < 7.4 we were storing the query model like VERSION B, in >=7.4 like VERSION A.

This PR fixes 2 things:

  • Handling different Elasticsearch versions requirements for the script field (What the linked issue is about)
  • Applying the same logic on the backend for alerting queries (it wasn't implemented before)

Which issue(s) this PR fixes:

Fixes #34028
Closes #34030

Special notes for your reviewer:

I created a docker block in devenv for ES 5.0 and moved elastic5 to elastic56 as it was using elasticsearch 5.6. I'm not able to start both at the same time as somehow when starting both one of the nodes elects itself as master and shuts down the other. I tried working around this by setting different configurations but none of them did the job.

I also created a row in http://localhost:3000/d/fuFWehBmk/datasource-tests-elasticsearch-comparison (Datasource tests -> Elasticsearch comparison) that has queries using both the new and old script format.

as mentioned above, running 2 ES version at the same time doesn't work ATM, the best way I can think of to test this is therefore running make devenv sources=elastic5, check that both series are being shown, then make devenv sources=elastic56 and repeat.

Screenshot 2021-05-13 at 15 46 01

Alerting as well works as expected with both formats and both ES version, testing is a bit more trickier as the datasource is templated, so make sure to pick the one you are testing against before testing the alerting rule:
Screenshot 2021-05-13 at 15 52 49 -> Screenshot 2021-05-13 at 15 54 11

Alerting tests:

ES 5.0

New Script format (VERSION A, Query A)

Screenshot 2021-05-13 at 15 56 32

OLD Script format (VERSION A, Query B)

Screenshot 2021-05-13 at 15 58 29

ES 5.6

New Script format (VERSION A, Query A)

Screenshot 2021-05-13 at 16 00 05

Old Script format (VERSION B, Query B)

Screenshot 2021-05-13 at 16 00 37

PS: Don't forget to run ./setup.sh in devenv to get updates to data sources and dashboard :)

@Elfo404 Elfo404 marked this pull request as ready for review May 13, 2021 15:04
@Elfo404 Elfo404 requested a review from a team as a code owner May 13, 2021 15:04
@Elfo404 Elfo404 requested review from aocenas and removed request for a team May 13, 2021 15:04
@Elfo404 Elfo404 changed the title Gio/fix/es script Elasticsearch: Fix handling of inline scripts in different ES versions May 13, 2021
@Elfo404 Elfo404 added this to the 8.0.0-beta2 milestone May 13, 2021
@Chris7
Copy link

Chris7 commented May 13, 2021

Just wanted to say you're awesome. Thanks for the quick turnaround!

@Elfo404 Elfo404 added the old backport v8.0.x Mark PR for automatic backport to v8.0.x label May 13, 2021
Copy link
Member

@aocenas aocenas left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@Elfo404 Elfo404 merged commit 8ec8725 into main May 14, 2021
@Elfo404 Elfo404 deleted the gio/fix/es-script branch May 14, 2021 10:50
grafanabot pushed a commit that referenced this pull request May 14, 2021
#34070)

* Devenv: add block for es 5.0, provisioned datasource & dashboard

* Trasnsform script property based on running ES version

* Handle different scripts format in BE

(cherry picked from commit 8ec8725)
torkelo pushed a commit that referenced this pull request May 14, 2021
#34070) (#34116)

* Devenv: add block for es 5.0, provisioned datasource & dashboard

* Trasnsform script property based on running ES version

* Handle different scripts format in BE

(cherry picked from commit 8ec8725)

Co-authored-by: Giordano Ricci <me@giordanoricci.com>
mortenaa pushed a commit to mortenaa/grafana that referenced this pull request May 25, 2021
grafana#34070)

* Devenv: add block for es 5.0, provisioned datasource & dashboard

* Trasnsform script property based on running ES version

* Handle different scripts format in BE
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 script query is not compatible with datasource's elasticsearch version
4 participants