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

kql, lucene and timerange functions #93043

Merged
merged 15 commits into from
Mar 17, 2021

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Mar 1, 2021

Summary

This PR adds kql, lucene and timerange expression functions as well as queryToAst and timerangeToAst helper functions and makes the use of them in kibana_context expression function

resolves #67895

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@ppisljar ppisljar added WIP Work in progress Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) v8.0.0 Team:AppServices v7.13.0 labels Mar 1, 2021
@ppisljar ppisljar requested a review from a team March 1, 2021 15:19
@ppisljar ppisljar requested a review from a team as a code owner March 1, 2021 15:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@ppisljar ppisljar added the release_note:skip Skip the PR/issue when compiling release notes label Mar 1, 2021
@ppisljar
Copy link
Member Author

ppisljar commented Mar 2, 2021

@elasticmachine merge upstream

@ppisljar ppisljar force-pushed the expressions/kibana_context branch from 566aac0 to ad76bb3 Compare March 8, 2021 17:25
@ppisljar
Copy link
Member Author

ppisljar commented Mar 9, 2021

@elasticmachine merge upstream

@ppisljar ppisljar force-pushed the expressions/kibana_context branch from 4873635 to 07373da Compare March 9, 2021 12:28
@ppisljar ppisljar added review and removed WIP Work in progress labels Mar 11, 2021
@ppisljar ppisljar force-pushed the expressions/kibana_context branch from d6c41d3 to 3de7093 Compare March 16, 2021 10:06
Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

Checked in Chrome/Mac, LGTM. Dashboards work as expected and checked the new functions in the expression sample plugin, both work.

@@ -61,7 +63,7 @@ export const kibanaContextFunction: ExpressionFunctionKibanaContext = {
}),
},
timeRange: {
types: ['string', 'null'],
types: ['timerange', 'null'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess these two type changes are breaking changes, so we need a breaking change note.

Copy link
Member Author

Choose a reason for hiding this comment

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

as we never store the expression (except in canvas), nor expose expression to the user (except in canvas) and this function is not used in canvas I think we are good

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

I don't think this works fully as intended. I tested Visualize with a KQL query and it's serialized to a lucene expression function:
Screenshot 2021-03-16 at 13 58 24
Screenshot 2021-03-16 at 13 58 05

Pretty sure it's caused by this line: https://github.com/elastic/kibana/pull/93043/files#diff-329955dd8d6110b5e8c0eaf934dd0ce1f3357dfd682b43a19652e8274cc916efR15

language is actually kuery, not kql.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

I tested this PR and if a kql query is added to a visualization, it won't be applied on dashboard:

  • Go to visualize, create metric vis and add geo.src: "CN" kql query
  • Save
  • Filter is not part of the request on the dashboard (and count is wrong)

It's part of the ast though

@ppisljar ppisljar force-pushed the expressions/kibana_context branch from 7020ef5 to 0425a5a Compare March 17, 2021 13:43
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Visualize changes LGTM, work as expected for kql and lucene

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 607 612 +5

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
data 211.4KB 211.4KB +2.0B
visualizations 46.2KB 46.2KB +2.0B
total +4.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 807.9KB 811.8KB +3.9KB
visualizations 114.8KB 115.1KB +282.0B
total +4.2KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ppisljar ppisljar merged commit 4db72b9 into elastic:master Mar 17, 2021
ppisljar added a commit to ppisljar/kibana that referenced this pull request Mar 17, 2021
ppisljar added a commit that referenced this pull request Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes review v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data.query.query] Create expression functions for KQL/Lucene & add toAst method
5 participants