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.

Sign up for GitHub

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

Jump to bottom

async es|ql search strategy #174246

Merged
merged 21 commits into from
Jan 29, 2024
Merged

async es|ql search strategy #174246

merged 21 commits into from
Jan 29, 2024

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Jan 4, 2024

Summary

async es|ql search strategy

@ppisljar ppisljar added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Feature:ES|QL ES|QL related features in Kibana labels Jan 4, 2024
@ppisljar
Copy link
Member Author

ppisljar commented Jan 4, 2024

/ci

@ppisljar
Copy link
Member Author

/ci

@ppisljar
Copy link
Member Author

/ci

@ppisljar
Copy link
Member Author

/ci

1 similar comment
@ppisljar
Copy link
Member Author

/ci

@ppisljar
Copy link
Member Author

/ci

1 similar comment
@ppisljar
Copy link
Member Author

/ci

@ppisljar
Copy link
Member Author

/ci

@ppisljar
Copy link
Member Author

/ci

1 similar comment
@ppisljar
Copy link
Member Author

/ci

@ppisljar
Copy link
Member Author

/ci

@ppisljar ppisljar marked this pull request as ready for review January 22, 2024 10:57
@ppisljar ppisljar requested a review from a team as a code owner January 22, 2024 10:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

stratoula
stratoula approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This works great! I tested both Discover and Lens, everything works as expected. @lukasolson a review from you would be awesome though as this is not an area I shine

src/plugins/data/server/search/strategies/esql_async_search/esql_asyn_search_strategy.test.ts Outdated Show resolved Hide resolved
@stratoula stratoula mentioned this pull request Jan 23, 2024
Merged
1 task
@kertal kertal requested a review from lukasolson January 23, 2024 11:42
dej611
dej611 reviewed Jan 23, 2024
View reviewed changes
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Had a quick code review and left few comments. Will test it locally.

src/plugins/data/server/search/strategies/esql_async_search/request_utils.ts Outdated Show resolved Hide resolved
src/plugins/data/server/search/strategies/esql_async_search/request_utils.ts Outdated Show resolved Hide resolved
lukasolson
lukasolson reviewed Jan 23, 2024
View reviewed changes
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.

Looking good, just some minor feedback about reusing the existing utils

src/plugins/data/server/search/strategies/esql_async_search/esql_async_search_strategy.ts
IKibanaSearchResponse<SqlGetAsyncResponse>
> => {
function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) {
const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser;
Copy link
Member

Choose a reason for hiding this comment

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

useInternalUser shouldn't be necessary, it was only necessary for the default search strategy. (It can be removed from the non-async ESQL strategy as well, but that can be done separately if you'd like)

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll leave it for now

src/plugins/data/server/search/strategies/esql_async_search/esql_async_search_strategy.ts Outdated Show resolved Hide resolved
src/plugins/data/server/search/strategies/esql_async_search/esql_async_search_strategy.ts Outdated Show resolved Hide resolved
src/plugins/data/server/search/strategies/esql_async_search/esql_async_search_strategy.ts Outdated Show resolved Hide resolved
src/plugins/data/server/search/strategies/esql_async_search/request_utils.ts Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Is this file necessary or can we reuse the existing functions from src/plugins/data/server/search/strategies/ese_search/request_utils.ts? They are already being used by the EQL strategy. We should probably move them to the common/ directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, its different due to different params on the esql request

src/plugins/data/server/search/strategies/esql_async_search/response_utils.ts
Copy link
Member

Choose a reason for hiding this comment

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

Same question here as above, can we reuse the existing functions

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, its different due to different params on the esql response

ppisljar and others added 3 commits January 24, 2024 10:13
@ppisljar @lukasolson @dej611
Apply suggestions from code review
1f10771 
Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
@ppisljar
applying review feedback
b2f6e32
@ppisljar
Merge branch 'main' into esql/async
75daa34
@stratoula stratoula mentioned this pull request Jan 24, 2024
Closed
lukasolson
lukasolson approved these changes Jan 25, 2024
View reviewed changes
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, just hoping for a little bit of cleanup

src/plugins/data/server/search/strategies/esql_async_search/response_utils.ts Outdated Show resolved Hide resolved
src/plugins/data/server/search/strategies/esql_async_search/esql_async_search_strategy.ts Outdated Show resolved Hide resolved
@ppisljar @lukasolson
Update src/plugins/data/server/search/strategies/esql_async_search/re…
c23d246 
…sponse_utils.ts

Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
@ppisljar
Copy link
Member Author

/ci

@ppisljar
Copy link
Member Author

/ci

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

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
data 2582 2583 +1

Page load bundle

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

id before after diff
data 415.1KB 415.2KB +112.0B
Unknown metric groups

API count

id before after diff
data 3233 3234 +1

History

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

cc @ppisljar

@ppisljar ppisljar merged commit 9908db4 into elastic:main Jan 29, 2024
20 checks passed
@stratoula stratoula mentioned this pull request Jan 30, 2024
Closed
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
@ppisljar @CoenWarmer
async es|ql search strategy (elastic#174246)
b6c4894
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
@ppisljar @fkanout
async es|ql search strategy (elastic#174246)
3d7552e
@jughosta jughosta mentioned this pull request Apr 18, 2024
Draft
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dej611 dej611 dej611 left review comments

@stratoula stratoula stratoula approved these changes

@lukasolson lukasolson lukasolson approved these changes

Assignees

@ppisljar ppisljar

Labels
backport:skip This commit does not require backporting Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ppisljar @elasticmachine @kibana-ci @dej611 @lukasolson @stratoula @kibanamachine