Skip to content

Commit

Permalink
[Search] Remove long-running query pop-up (#75385)
Browse files Browse the repository at this point in the history
* [Search] Remove long-running query pop-up

* Don't timeout if requestTimeout isn't configured

* Remove unused kibanaUtils

* Remove unused kibanaReact

* Re-add reference to kibanaUtils

* Remove unused translations and update documentation

* Add new x-pack advanced setting searchTimeout and use it in the EnhancedSearchInterceptor

* docs

* Re-add toast when queries time out

* Fix types

* Update error message with capabilities

* Update docs

* Update docs

* Move search server routes into a directory.

* Add internal/_msearch route.

* Remove legacy search API, rewrite default search strategy to use internal route.

* Remove legacy es_client code.

* Handle msearch options on server.

* Remove elasticsearch-browser dependency.

* Update generated docs.

* Rely on server timeout in OSS (?)
Use UI setting in xpack.

* Rename function

* Add features to dependencies

* Undefined check

* doc

* Code review fixes

* code review

* doc

* loading count

* simplify code review and fix jest tets

* type check

* Remove esShard from client

* cleanup request parameters from FE

* doc

* doc

* Align request parameters on server,
Remove leftover parameters from client
Shim responses for search and msearch routes

* docs
Stop using toSnakeCase
Updates jest tests

* add management docs

* docs

* Remove import

* Break circular dep + fix msearch test

* Remove deleted type

* Fix jest

* Bring toSnakeCase back

* docs

* fix jest

* Add new x-pack advanced setting searchTimeout and use it in the EnhancedSearchInterceptor

* docs

* Rely on server timeout in OSS (?)
Use UI setting in xpack.

* Rename function

* doc

* Remove esShard from client

* cleanup request parameters from FE

* doc

* doc

* Align request parameters on server,
Remove leftover parameters from client
Shim responses for search and msearch routes

* docs
Stop using toSnakeCase
Updates jest tests

* add management docs

* docs

* Remove import

* Break circular dep + fix msearch test

* Remove deleted type

* Fix jest

* Bring toSnakeCase back

* docs

* fix jest

* Fix merge

* Fix types

* Allow timeout to be undefined

* Fix jest test

* Upldate docs

* Fix msearch jest

* Merge correction

* docs

* Fix rollup search merge

* Fix merge

* Use i18n

Co-authored-by: Liza K <liza.katz@elastic.co>
Co-authored-by: Luke Elmers <luke.elmers@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
  • Loading branch information
4 people committed Sep 13, 2020
1 parent 00737fe commit 38807ac
Show file tree
Hide file tree
Showing 18 changed files with 110 additions and 418 deletions.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ export declare class SearchInterceptor
| Property | Modifiers | Type | Description |
| --- | --- | --- | --- |
| [deps](./kibana-plugin-plugins-data-public.searchinterceptor.deps.md) | | <code>SearchInterceptorDeps</code> | |
| [showTimeoutError](./kibana-plugin-plugins-data-public.searchinterceptor.showtimeouterror.md) | | <code>((e: Error) =&gt; void) &amp; import(&quot;lodash&quot;).Cancelable</code> | |

## Methods

| Method | Modifiers | Description |
| --- | --- | --- |
| [getPendingCount$()](./kibana-plugin-plugins-data-public.searchinterceptor.getpendingcount_.md) | | Returns an <code>Observable</code> over the current number of pending searches. This could mean that one of the search requests is still in flight, or that it has only received partial responses. |
| [search(request, options)](./kibana-plugin-plugins-data-public.searchinterceptor.search.md) | | Searches using the given <code>search</code> method. Overrides the <code>AbortSignal</code> with one that will abort either when <code>cancelPending</code> is called, when the request times out, or when the original <code>AbortSignal</code> is aborted. Updates <code>pendingCount$</code> when the request is started/finalized. |

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-plugins-data-public](./kibana-plugin-plugins-data-public.md) &gt; [SearchInterceptor](./kibana-plugin-plugins-data-public.searchinterceptor.md) &gt; [showTimeoutError](./kibana-plugin-plugins-data-public.searchinterceptor.showtimeouterror.md)

## SearchInterceptor.showTimeoutError property

<b>Signature:</b>

```typescript
protected showTimeoutError: ((e: Error) => void) & import("lodash").Cancelable;
```
10 changes: 2 additions & 8 deletions src/plugins/data/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ import { Search } from '@elastic/elasticsearch/api/requestParams';
import { SearchResponse } from 'elasticsearch';
import { SerializedFieldFormat as SerializedFieldFormat_2 } from 'src/plugins/expressions/common';
import { Subscription } from 'rxjs';
import { Toast } from 'kibana/public';
import { ToastInputFields } from 'src/core/public/notifications';
import { ToastsSetup } from 'kibana/public';
import { TransportRequestOptions } from '@elastic/elasticsearch/lib/Transport';
Expand Down Expand Up @@ -1740,11 +1739,6 @@ export class SearchInterceptor {
protected application: CoreStart['application'];
// (undocumented)
protected readonly deps: SearchInterceptorDeps;
getPendingCount$(): Observable<number>;
// @internal (undocumented)
protected hideToast: () => void;
// @internal
protected longRunningToast?: Toast;
// @internal
protected pendingCount$: BehaviorSubject<number>;
// @internal (undocumented)
Expand All @@ -1758,8 +1752,8 @@ export class SearchInterceptor {
combinedSignal: AbortSignal;
cleanup: () => void;
};
// @internal (undocumented)
protected showToast: () => void;
// (undocumented)
protected showTimeoutError: ((e: Error) => void) & import("lodash").Cancelable;
// @internal
protected timeoutSubscriptions: Subscription;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,31 +63,4 @@ describe('Search Usage Collector', () => {
SEARCH_EVENT_TYPE.QUERIES_CANCELLED
);
});

test('tracks long popups', async () => {
await usageCollector.trackLongQueryPopupShown();
expect(mockUsageCollectionSetup.reportUiStats).toHaveBeenCalled();
expect(mockUsageCollectionSetup.reportUiStats.mock.calls[0][1]).toBe(METRIC_TYPE.LOADED);
expect(mockUsageCollectionSetup.reportUiStats.mock.calls[0][2]).toBe(
SEARCH_EVENT_TYPE.LONG_QUERY_POPUP_SHOWN
);
});

test('tracks long popups dismissed', async () => {
await usageCollector.trackLongQueryDialogDismissed();
expect(mockUsageCollectionSetup.reportUiStats).toHaveBeenCalled();
expect(mockUsageCollectionSetup.reportUiStats.mock.calls[0][1]).toBe(METRIC_TYPE.CLICK);
expect(mockUsageCollectionSetup.reportUiStats.mock.calls[0][2]).toBe(
SEARCH_EVENT_TYPE.LONG_QUERY_DIALOG_DISMISSED
);
});

test('tracks run query beyond timeout', async () => {
await usageCollector.trackLongQueryRunBeyondTimeout();
expect(mockUsageCollectionSetup.reportUiStats).toHaveBeenCalled();
expect(mockUsageCollectionSetup.reportUiStats.mock.calls[0][1]).toBe(METRIC_TYPE.CLICK);
expect(mockUsageCollectionSetup.reportUiStats.mock.calls[0][2]).toBe(
SEARCH_EVENT_TYPE.LONG_QUERY_RUN_BEYOND_TIMEOUT
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -48,29 +48,5 @@ export const createUsageCollector = (
SEARCH_EVENT_TYPE.QUERIES_CANCELLED
);
},
trackLongQueryPopupShown: async () => {
const currentApp = await getCurrentApp();
return usageCollection?.reportUiStats(
currentApp!,
METRIC_TYPE.LOADED,
SEARCH_EVENT_TYPE.LONG_QUERY_POPUP_SHOWN
);
},
trackLongQueryDialogDismissed: async () => {
const currentApp = await getCurrentApp();
return usageCollection?.reportUiStats(
currentApp!,
METRIC_TYPE.CLICK,
SEARCH_EVENT_TYPE.LONG_QUERY_DIALOG_DISMISSED
);
},
trackLongQueryRunBeyondTimeout: async () => {
const currentApp = await getCurrentApp();
return usageCollection?.reportUiStats(
currentApp!,
METRIC_TYPE.CLICK,
SEARCH_EVENT_TYPE.LONG_QUERY_RUN_BEYOND_TIMEOUT
);
},
};
};
6 changes: 0 additions & 6 deletions src/plugins/data/public/search/collectors/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,9 @@
export enum SEARCH_EVENT_TYPE {
QUERY_TIMED_OUT = 'queryTimedOut',
QUERIES_CANCELLED = 'queriesCancelled',
LONG_QUERY_POPUP_SHOWN = 'longQueryPopupShown',
LONG_QUERY_DIALOG_DISMISSED = 'longQueryDialogDismissed',
LONG_QUERY_RUN_BEYOND_TIMEOUT = 'longQueryRunBeyondTimeout',
}

export interface SearchUsageCollector {
trackQueryTimedOut: () => Promise<void>;
trackQueriesCancelled: () => Promise<void>;
trackLongQueryPopupShown: () => Promise<void>;
trackLongQueryDialogDismissed: () => Promise<void>;
trackLongQueryRunBeyondTimeout: () => Promise<void>;
}
59 changes: 0 additions & 59 deletions src/plugins/data/public/search/long_query_notification.tsx

This file was deleted.

75 changes: 34 additions & 41 deletions src/plugins/data/public/search/search_interceptor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,39 @@ describe('SearchInterceptor', () => {
await flushPromises();
});

test('Should not timeout if requestTimeout is undefined', async () => {
searchInterceptor = new SearchInterceptor({
startServices: mockCoreSetup.getStartServices(),
uiSettings: mockCoreSetup.uiSettings,
http: mockCoreSetup.http,
toasts: mockCoreSetup.notifications.toasts,
});
mockCoreSetup.http.fetch.mockImplementationOnce((options: any) => {
return new Promise((resolve, reject) => {
options.signal.addEventListener('abort', () => {
reject(new AbortError());
});

setTimeout(resolve, 5000);
});
});
const mockRequest: IEsSearchRequest = {
params: {},
};
const response = searchInterceptor.search(mockRequest);

expect.assertions(1);
const next = jest.fn();
const complete = () => {
expect(next).toBeCalled();
};
response.subscribe({ next, complete });

jest.advanceTimersByTime(5000);

await flushPromises();
});

test('Observable should fail if user aborts (test merged signal)', async () => {
const abortController = new AbortController();
mockCoreSetup.http.fetch.mockImplementationOnce((options: any) => {
Expand Down Expand Up @@ -125,7 +158,7 @@ describe('SearchInterceptor', () => {
await flushPromises();
});

test('Immediatelly aborts if passed an aborted abort signal', async (done) => {
test('Immediately aborts if passed an aborted abort signal', async (done) => {
const abort = new AbortController();
const mockRequest: IEsSearchRequest = {
params: {},
Expand All @@ -141,44 +174,4 @@ describe('SearchInterceptor', () => {
response.subscribe({ error });
});
});

describe('getPendingCount$', () => {
test('should observe the number of pending requests', () => {
const pendingCount$ = searchInterceptor.getPendingCount$();
const pendingNext = jest.fn();
pendingCount$.subscribe(pendingNext);

const mockResponse: any = { result: 200 };
mockCoreSetup.http.fetch.mockResolvedValue(mockResponse);
const mockRequest: IEsSearchRequest = {
params: {},
};
const response = searchInterceptor.search(mockRequest);

response.subscribe({
complete: () => {
expect(pendingNext.mock.calls).toEqual([[0], [1], [0]]);
},
});
});

test('should observe the number of pending requests on error', () => {
const pendingCount$ = searchInterceptor.getPendingCount$();
const pendingNext = jest.fn();
pendingCount$.subscribe(pendingNext);

const mockResponse: any = { result: 500 };
mockCoreSetup.http.fetch.mockRejectedValue(mockResponse);
const mockRequest: IEsSearchRequest = {
params: {},
};
const response = searchInterceptor.search(mockRequest);

response.subscribe({
complete: () => {
expect(pendingNext.mock.calls).toEqual([[0], [1], [0]]);
},
});
});
});
});
Loading

0 comments on commit 38807ac

Please sign in to comment.