Skip to content

async es|ql search strategy#174246

Merged
ppisljar merged 21 commits intoelastic:mainfrom
ppisljar:esql/async
Jan 29, 2024
Merged

async es|ql search strategy#174246
ppisljar merged 21 commits intoelastic:mainfrom
ppisljar:esql/async

Conversation

@ppisljar
Copy link
Contributor

@ppisljar ppisljar commented Jan 4, 2024

Summary

async es|ql search strategy
@ppisljar ppisljar added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Feature:ES|QL ES|QL related features in Kibana labels Jan 4, 2024
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 4, 2024

/ci
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 13, 2024

/ci
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 15, 2024

/ci
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 15, 2024

/ci
1 similar comment
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 16, 2024

/ci
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 18, 2024

/ci
1 similar comment
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 18, 2024

/ci
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 19, 2024

/ci
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 20, 2024

/ci
1 similar comment
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 21, 2024

/ci
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 22, 2024

/ci
@ppisljar ppisljar marked this pull request as ready for review January 22, 2024 10:57
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
@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.
lukasolson
lukasolson reviewed Jan 23, 2024
View reviewed changes
Copy link
Contributor

@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
Contributor

@lukasolson lukasolson Jan 23, 2024

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
Contributor Author

@ppisljar ppisljar Jan 24, 2024

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/request_utils.ts Outdated
Copy link
Contributor

@lukasolson lukasolson Jan 23, 2024

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
Contributor Author

@ppisljar ppisljar Jan 24, 2024

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
Contributor

@lukasolson lukasolson Jan 23, 2024

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
Contributor Author

@ppisljar ppisljar Jan 24, 2024

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
Contributor

@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
@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
Contributor Author

ppisljar commented Jan 29, 2024

/ci
@ppisljar
Copy link
Contributor Author

ppisljar commented Jan 29, 2024

/ci
@kibana-ci
Copy link

kibana-ci commented Jan 29, 2024

💚 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
@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
Closed
2 tasks
@kertal kertal mentioned this pull request Dec 19, 2024
Merged
1 task
@lukasolson lukasolson mentioned this pull request May 20, 2025
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR 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 Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.13.0

7 participants

@ppisljar @elasticmachine @kibana-ci @dej611 @lukasolson @stratoula @kibanamachine