-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Search]Add EQL search strategy #78645
Conversation
Since EQL is an x-pack feature, this strategy will live in the x-pack plugin data_enhanced.
Pinging @elastic/kibana-app-arch (Team:AppArch) |
* Ensures that the same variable is not used for both test setup and test assertions * Ensures that mocks are reinstantiated on every test
const eqlClient = context.core.elasticsearch.client.asCurrentUser.eql; | ||
const uiSettingsClient = await context.core.uiSettings.client; | ||
const asyncOptions = { | ||
waitForCompletionTimeout: '100ms', // Wait up to 100ms for the response to return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are needed only if the EQL endpint supports async search
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rylnd is this strategy a copy of the DSL search strategy, just with a different endpoint?
If so, lets think how we can reuse the code. I wouldn't want to maintain this logic twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lizozom I've tried to keep it as similar as possible but there's plenty of deviation. I'll see if I can't extract some of the shared logic to some helpers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was repeated in five places, time to consolidate.
@lizozom I believe this is ready for another 👀 ! I do have a few outstanding questions in the PR description, as well, if you could take a look. |
We export a few new helper functions.
|
||
export const eqlSearchStrategyProvider = ( | ||
logger: Logger | ||
): ISearchStrategy<EqlSearchStrategyRequest, EqlSearchStrategyResponse> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rylnd I still suspect the two strategies are identical, except for the endpoint being used.
Don't you think we could do with adding a parameter to the normal strategy and using that one?
Lets talk about this.
Updates documentation accordingly.
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
We'll extract common code as we go 👍
💚 Build SucceededMetrics [docs]distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
* Add EQL search strategy Since EQL is an x-pack feature, this strategy will live in the x-pack plugin data_enhanced. * Refactor our test setup to minimize shared state * Ensures that the same variable is not used for both test setup and test assertions * Ensures that mocks are reinstantiated on every test * Use explicit top-level exports * Move async search options to a helper function * Move our workaround to a helper function This was repeated in five places, time to consolidate. * Commit documentation changes We export a few new helper functions. * Mark our internal methods as such Updates documentation accordingly. Co-authored-by: Kibana Machine <[email protected]>
* Add EQL search strategy Since EQL is an x-pack feature, this strategy will live in the x-pack plugin data_enhanced. * Refactor our test setup to minimize shared state * Ensures that the same variable is not used for both test setup and test assertions * Ensures that mocks are reinstantiated on every test * Use explicit top-level exports * Move async search options to a helper function * Move our workaround to a helper function This was repeated in five places, time to consolidate. * Commit documentation changes We export a few new helper functions. * Mark our internal methods as such Updates documentation accordingly. Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
The immediate need for this strategy for 7.10 is security_solution and its EQL detection rules. Since EQL is an x-pack feature, this strategy lives in the data_enhanced plugin.
While EQL does not return partial results, I plumbed through as much async functionality as possible (get, search, delete) using the other search strategies as a guide.
Outstanding questions:
timeout
from config do not appear to be supportedmax_concurrent_shard_requests
andtrack_total_hits
do not appear to be supportedquerystring
on options but it's unclear whether I need to do thatChecklist
Delete any items that are not applicable to this PR.
For maintainers