Improve search typescript #69333

lizozom · 2020-06-16T20:28:45Z

Summary

This PR removes the concept of "Search Strategies" from the front end of Kibana.

Instead, it incorporates the search logic within the search_interceptors.
The OSS interceptor is responsible for synchronous querying, while the x-pack interceptor adds capabilities such as async queries, cancellation, running beyond timeout etc.

This PR also reduces the waitForCompletion timeout to 100ms (instead of 1s). This is especially important moving forward to async search, as this determins the earliest time we get the asyncID from ES.

Dev Docs

This PR deprecates the front end search strategy concept removes the following API methods from the data.search plugin:

registerSearchStrategy
getSearchStrategy

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

…on the server

…-side-search-strategy

src/plugins/data/public/search/search_interceptor.ts

src/plugins/data/public/search/i_search.ts

src/plugins/data/public/search/search_interceptor.ts

src/plugins/data/public/search/search_service.ts

...evelopment/plugins/data/public/kibana-plugin-plugins-data-public.searchinterceptor.search.md

…-side-search-strategy

lizozom · 2020-06-25T20:55:43Z

@elasticmachine merge upstream

…-side-search-strategy

delete search explorer

…om/kibana into search/server-side-search-strategy

lizozom · 2020-06-29T21:10:22Z

x-pack/plugins/data_enhanced/server/search/es_search_strategy.ts

    ...request.params,
  };

+  params.trackTotalHits = true; // Get the exact count of hits
+
  // If we have an ID, then just poll for that ID, otherwise send the entire request body
  const { body = undefined, index = undefined, ...queryParams } = request.id ? {} : params;

  const method = request.id ? 'GET' : 'POST';
  const path = encodeURI(request.id ? `/_async_search/${request.id}` : `/${index}/_async_search`);

  // Wait up to 1s for the response to return


delete comment

x-pack/plugins/data_enhanced/server/search/es_search_strategy.ts

Dosant · 2020-07-03T09:40:08Z

src/plugins/data/common/utils/abort_utils.ts

+export function toPromise(signal: AbortSignal): Promise<never> {
+  return new Promise((resolve, reject) => {
+    if (signal.aborted) reject(new AbortError());
+    signal.addEventListener('abort', () => reject(new AbortError()));


I think every time we addEventListener we also have to have removeEventListener somewhere.
Otherwise we are spamming memory leaks.

Just double checked this is required, for example, fetch polyfill does this: https://github.com/github/fetch/blob/master/fetch.js#L534

I guess should be fixed in separate pr, since it isn't something new introduced with the refactor.

Ahh, turns out I mentioned it here: #65051.
Added fetch example there in description

Added a removeEventListener

It won't always work. We also need to remove listener after corresponding request is successfully finished.
That is why they do it in finalise callback. https://github.com/github/fetch/blob/master/fetch.js#L534
Should be tacked properly in scope of #65051

src/plugins/data/public/search/search_interceptor.ts

…-side-search-strategy

lizozom · 2020-07-05T14:07:08Z

@elasticmachine merge upstream

weltenwort · 2020-07-06T11:13:58Z

Could you explain how this changes the scope of the functionality that the data plugin offers? Does this remove the ability for plugins to register and use their own search strategies? Seeing how the examples were removed, will there be new documentation in their place to explain the usage?

lizozom · 2020-07-06T11:23:51Z

@elasticmachine merge upstream

jasonrhodes · 2020-07-06T13:38:16Z

x-pack/plugins/infra/public/utils/logs_overview_fetchers.ts

  return new Promise((resolve, reject) => {
-    esSearcher
+    dataPlugin.search


Is there some documentation for how to interact with this API? How can I confirm that dataPlugin.search.search is the correct API function etc?

The docs are in the making.
Synced with @weltenwort to confirm the changes suit your needs.

OK, in the meantime without docs, can you confirm that the API is definitely dataPlugin.search.search vs dataPlugin.search? And what are the list of available keys and methods on dataPlugin?

Data plugin contains several services

query (everything that has to do with the search bar - time range, query string, filters)

search (which contains the search function and the aggs helpers

index patterns

field formatting

autocompletion

The method relevant to you is indeed dataStart.search.search()

lizozom · 2020-07-07T09:37:42Z

@elasticmachine merge upstream

jasonrhodes

LGTM thanks for the explanations

…-side-search-strategy

Dosant

I partially tested using flights dashboard (with x-pack)
Focused on 2 things:

Cancelations are working as expected when clicking refresh fast. (this was flaky in that past)
I set latency to 15s, and made sure I can cancel or run beyond timeout

With that didn't find any issues.
But this is just artificial client side tests and I didn't test with data to trigger proper async search flow.

…om/kibana into search/server-side-search-strategy

kibanamachine · 2020-07-07T17:36:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d3b5eeb

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
data	202	-2	204
dataEnhanced	5	-1	6
total	-	-3	-

Saved Objects .kibana field count

id	value	diff	baseline
action	5	+5	-
action_task_params	3	+3	-
alert	22	+22	-
apm-indices	8	+8	-
apm-telemetry	238	+238	-
application_usage_transactional	2	+2	-
canvas-element	8	+8	-
canvas-workpad	5	+5	-
canvas-workpad-template	8	+8	-
cases	32	+32	-
cases-comments	17	+17	-
cases-configure	14	+14	-
cases-user-actions	10	+10	-
config	2	+2	-
dashboard	17	+17	-
endpoint:user-artifact	10	+10	-
endpoint:user-artifact-manifest	3	+3	-
epm-packages	8	+8	-
exception-list	35	+35	-
exception-list-agnostic	35	+35	-
file-upload-telemetry	2	+2	-
fleet-agent-actions	6	+6	-
fleet-agent-events	11	+11	-
fleet-agents	23	+23	-
fleet-enrollment-api-keys	10	+10	-
graph-workspace	9	+9	-
index-pattern	10	+10	-
infrastructure-ui-source	21	+21	-
ingest_manager_settings	5	+5	-
ingest-agent-configs	11	+11	-
ingest-outputs	10	+10	-
ingest-package-configs	35	+35	-
inventory-view	40	+40	-
kql-telemetry	3	+3	-
lens	7	+7	-
lens-ui-telemetry	5	+5	-
map	7	+7	-
metrics-explorer-view	22	+22	-
ml-telemetry	3	+3	-
namespace	1	+1	-
namespaces	1	+1	-
query	6	+6	-
references	4	+4	-
sample-data-telemetry	3	+3	-
search	9	+9	-
siem-detection-engine-rule-actions	8	+8	-
siem-detection-engine-rule-status	12	+12	-
siem-ui-timeline	86	+86	-
siem-ui-timeline-note	8	+8	-
siem-ui-timeline-pinned-event	7	+7	-
space	9	+9	-
telemetry	9	+9	-
timelion-sheet	13	+13	-
tsvb-validation-telemetry	2	+2	-
type	1	+1	-
ui-metric	2	+2	-
updated_at	1	+1	-
upgrade-assistant-reindex-operation	15	+15	-
upgrade-assistant-telemetry	13	+13	-
uptime-dynamic-settings	4	+4	-
url	6	+6	-
visualization	9	+9	-
total	-	+951	-

History

💚 Build #59303 succeeded a625e0b
💚 Build #58891 succeeded bc4fb7b
💔 Build #58766 failed eff55c5
💔 Build #58764 failed f6a0fa7
💔 Build #58760 failed 6e2180d

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

* [search] Refactor the way search strategies are registered/retrieved on the server * Fix types and tests and update docs * Fix failing test * Fix build of example plugin * Fix functional test * Make server strategies sync * Move strategy name into options * docs * Remove FE strategies * TypeScript of hell delete search explorer * Fix search interceptor OSS tests * typos * test cleanup * Delete search example fix interceptor async tests to use fake timers * docs * fix * return search wrapper * Update search interceptor tests and abort utils * ts * jest test fix * code review * change how logs consume search API Co-authored-by: Lukas Olson <olson.lukas@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # src/plugins/data/public/public.api.md

* master: (36 commits) fixed api url in example plugin (elastic#70934) [data.search.aggs]: Remove remaining client dependencies (elastic#70251) [Security Solution][Endpoint] Fix base64 download bug and adopt new user artifact/manifest format (elastic#70998) [Security Solution][Exceptions] - Exception Modal Part I (elastic#70639) [SIEM][Detection Engine][Lists] Adds additional data types to value based lists [SIEM][Detection Engine][Lists] Removes feature flag for lists [APM] Show license callout in ML settings (elastic#70959) Migrate service settings test to jest (elastic#70992) [APM] Add cloud attributes to data telemetry (elastic#71008) Fix breadcrumb on panels for visibility / round corners (elastic#71010) Improve search typescript (elastic#69333) [savedObjects field count] run in baseline job (elastic#70999) [Security Solution] [Timeline] Timeline manager tweaks (elastic#69988) [Endpoint] Support redirect from Policy Details to Ingest when user initiates Edit Policy from Datasource Edit page (elastic#70874) [APM] Add API tests (elastic#70740) [Security Solution][Exceptions] - Tie server and client code together (elastic#70918) [Audit Logging] Add AuditTrail service (elastic#69278) [Usage Collection] Ensure no type duplicates (elastic#70946) [Security Solution] [Timeline] Bugfix for timeline row actions disappear sometimes (elastic#70958) [CI] Add pipeline task queue framework and merge workers into one (elastic#64011) ...

* Improve search typescript (#69333) * [search] Refactor the way search strategies are registered/retrieved on the server * Fix types and tests and update docs * Fix failing test * Fix build of example plugin * Fix functional test * Make server strategies sync * Move strategy name into options * docs * Remove FE strategies * TypeScript of hell delete search explorer * Fix search interceptor OSS tests * typos * test cleanup * Delete search example fix interceptor async tests to use fake timers * docs * fix * return search wrapper * Update search interceptor tests and abort utils * ts * jest test fix * code review * change how logs consume search API Co-authored-by: Lukas Olson <olson.lukas@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # src/plugins/data/public/public.api.md * docs * Fix merge Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

lukasolson and others added 11 commits June 8, 2020 11:35

[search] Refactor the way search strategies are registered/retrieved …

d964c84

…on the server

Fix types and tests and update docs

1452ed5

Fix failing test

c7d6b61

Fix build of example plugin

955bd88

Merge branch 'master' into refactorServerGetStrategy

93b41fe

Fix functional test

b0be2df

Merge branch 'master' of github.com:elastic/kibana into pr/68452

e09429a

Merge branch 'master' of github.com:elastic/kibana into pr/68452

4eb9dd1

Make server strategies sync

b536e74

Merge branch 'master' of github.com:elastic/kibana into search/server…

1c79c1f

…-side-search-strategy

Move strategy name into options

97c456e

lizozom self-assigned this Jun 16, 2020

docs

239e3a5