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

Add essql search strategy and integrate in canvas #94754

Merged
merged 21 commits into from
Apr 29, 2021

Conversation

poffdeluxe
Copy link
Contributor

@poffdeluxe poffdeluxe commented Mar 16, 2021

Summary

All new Search Service changes (using new search strategy, browser functions) are loaded as a new Lab Project with the new changes active by default. The gameplan is to merge this into 7.x (after 7.13 is cut) with everything active so we can ensure that there aren't any strange side effects that we haven't considered. If all goes well, before 7.14, we will decouple the new changes from the Lab Project, set them to be the default, and remove the legacy server-side functions.

@poffdeluxe poffdeluxe force-pushed the feature/essqlSearchStrategy branch from df67330 to 4094c41 Compare March 18, 2021 15:53
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Left some feedback below, this is a great start! I'm happy to walk through some of the observable/partial results stuff with you if you'd like.

x-pack/plugins/canvas/server/lib/essql_strategy.ts Outdated Show resolved Hide resolved
x-pack/plugins/canvas/server/lib/essql_strategy.ts Outdated Show resolved Hide resolved
x-pack/plugins/canvas/server/lib/essql_strategy.ts Outdated Show resolved Hide resolved
x-pack/plugins/canvas/server/lib/essql_strategy.ts Outdated Show resolved Hide resolved
};
};

return from(searchUntilEnd());
Copy link
Member

Choose a reason for hiding this comment

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

As you mentioned, this is probably a good place to actually give partial results on the observable. I'm happy to walk through this with you.

Also, would a cancel method need to actually close the cursor?

@poffdeluxe poffdeluxe force-pushed the feature/essqlSearchStrategy branch from 7004865 to 10c23ba Compare March 29, 2021 19:37
@poffdeluxe poffdeluxe added auto-backport Deprecated - use backport:version if exact versions are needed Feature:Canvas impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Apr 15, 2021
@poffdeluxe poffdeluxe requested review from a team as code owners April 15, 2021 15:52
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Telemetry changes LGTM

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
canvas 1089 1099 +10

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
presentationUtil 85 94 +9

Async chunks

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

id before after diff
canvas 1.3MB 1.3MB +58.1KB

Page load bundle

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

id before after diff
canvas 518.9KB 532.9KB +14.0KB
presentationUtil 40.5KB 41.1KB +579.0B
total +14.6KB
Unknown metric groups

API count

id before after diff
presentationUtil 87 96 +9

async chunk count

id before after diff
canvas 7 8 +1

History

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

Copy link
Contributor

@cqliu1 cqliu1 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I loaded up all the sample workpads and tested a few SQL queries and it seems to be working the same.

The only thing I noticed was when I used the new param arg for the essql function, this is what the sidebar looks like.

Screen Shot 2021-04-28 at 1 59 42 PM

Not a blocker, but we should consider adding a form control for the new arg, so the user doesn't end up with this warning that forces them to edit their essql query via the expression editor only.

@poffdeluxe poffdeluxe merged commit 2628cea into elastic:master Apr 29, 2021
@poffdeluxe poffdeluxe deleted the feature/essqlSearchStrategy branch April 29, 2021 19:58
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 29, 2021
* Add essql search strategy, new escount temporary function, and new essql temporary function

* Move old es* functions to legacy, add esdocs to use search strategy, add parameter arg for essql

* Clean up

* cleanup

* cleanup

* Move request builder files to common

* cleanup

* add comment

* PR Feedback

* Removing old types

* update type

* Add data.search to labs and fix error messages

* Fix function help type

* Add data service to usage collector types

* Update telemetry

* remove unrelated telemetry change

* Enable multi value leniency for SQL  queries

* Display data service lab project
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

jbudz added a commit that referenced this pull request Apr 29, 2021
@jbudz
Copy link
Member

jbudz commented Apr 29, 2021

This was reverted with 0f15a12. squel was updated to safe-squel in between the time this was last ran and was causing optimization failures.

poffdeluxe added a commit to poffdeluxe/kibana that referenced this pull request Apr 29, 2021
poffdeluxe added a commit that referenced this pull request May 3, 2021
…4754)" (#98841)

* Revert "Revert "Add essql search strategy and integrate in canvas (#94754)""

This reverts commit 0f15a12.

* Update squel usage to safe-squel

Co-authored-by: Kibana Machine <[email protected]>
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 3, 2021
…astic#94754)" (elastic#98841)

* Revert "Revert "Add essql search strategy and integrate in canvas (elastic#94754)""

This reverts commit 0f15a12.

* Update squel usage to safe-squel

Co-authored-by: Kibana Machine <[email protected]>
kibanamachine added a commit that referenced this pull request May 3, 2021
…4754)" (#98841) (#99086)

* Revert "Revert "Add essql search strategy and integrate in canvas (#94754)""

This reverts commit 0f15a12.

* Update squel usage to safe-squel

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Poff Poffenberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Canvas impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants