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

SavedObjectClient#find with KQL expression strings #78348

Closed
kobelb opened this issue Sep 23, 2020 · 7 comments
Closed

SavedObjectClient#find with KQL expression strings #78348

kobelb opened this issue Sep 23, 2020 · 7 comments
Labels
discuss enhancement New value added to drive a business result performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@kobelb
Copy link
Contributor

kobelb commented Sep 23, 2020

KQL expression string parsing is slow. We've seen a complex KQL string take up to 250ms to parse. #76811 exists to discuss the KQL expression string parsing being slow and determine whether there are any performance improvements we can make. Assuming we're not able to get KQL expression string parsing performance where we need it to be, we'll instead need to minimize where it's performed.

The SavedObjectsClient#find method currently allow a filter to be specified that is either a KQL expression string or a KueryNode:

filter?: string | KueryNode;

I think that we should change this to only accept a KueryNode. Specifying a KQL expression string is easier for the developer, so they will be prone to defaulting to using a string. If we only allow them to specify a KueryNode, it will hopefully prompt them to constructing the KueryNode manually, which is much more performant.

However, There are some situations where we need to use a KQL expression string:

  1. When the end-user specifies the KQL expression string
  2. In the SavedObjects find object API

When the end-user is specifying the KQL expression string, developers can use esKuery.fromQueryExpression directly to get the KueryNode which can then be passed to SavedObjectsClient#find.

The find route-handler will need to be changed to use esKuery.fromQueryExpression, before calling SavedObjectsClient#find.

@kobelb kobelb added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Sep 23, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@kobelb kobelb added chore performance bug Fixes for quality problems that affect the customer experience and removed chore labels Sep 23, 2020
@pgayvallet
Copy link
Contributor

I think that we should change this to only accept a KueryNode

However, There are some situations where we need to use a KQL expression string:

Any idea on how we should change the public API if there some use cases where it should be able to call it with a string filter?

Maybe we could split filter into two mutually exclusive fields, filter?: KueryNode; rawFilter?:string, while documenting that using rawFilter is discouraged, but I don't see how we can do better than that?

@rudolf
Copy link
Contributor

rudolf commented Sep 28, 2020

Any idea on how we should change the public API if there some use cases where it should be able to call it with a string filter?

I think @kobelb suggested removing string completely from the SavedObjectsClient API, if there is an API which needs to accept strings (like the HTTP saved objects find API) then creating the KeuryNode from a string needs to happen inside the route handler (or any level above the saved objects client).

@lukeelmers
Copy link
Member

Would it be worth spending the time to address #55485 first, and then make these changes to require a KueryNode from the data plugin instead?

Otherwise y'all will be implementing the changes described here, only to have them removed again when we address that other issue.

@kobelb
Copy link
Contributor Author

kobelb commented Oct 7, 2020

To address #55485, I thought we were just going to pull the "es_query" code out into a place where both core and the data plugin can import it. Wouldn't this just change the location where we're importing these types and classes? Is there some intricacy when I'm overlooking?

@lukeelmers
Copy link
Member

Ah, sorry @kobelb, my comment above was assuming the original plan we had outlined in that issue, however I had missed your subsequent comment describing the challenges of that approach.

The only issue with re-creating the es_query package is that we will run into the same issue that drove us to merge it into the data plugin in the first place: It depends on type interfaces from data like IIndexPattern, IFieldType, Query, plus also now some from the kibana_utils plugin.

I don't want to distract from the main topic of this issue, so I'll go ahead and comment on #55485

@pgayvallet pgayvallet added discuss enhancement New value added to drive a business result and removed bug Fixes for quality problems that affect the customer experience labels Mar 31, 2021
@lizozom
Copy link
Contributor

lizozom commented Apr 18, 2022

Closing this issue due to inactivity.
Feel free to reopen if needed 🙏🏻

@lizozom lizozom closed this as completed Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss enhancement New value added to drive a business result performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

6 participants