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

[searchService] Dedupe shard error toasts #131776

Merged
merged 31 commits into from
Oct 18, 2022
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
1b82948
dedupe shard error toasts
mattkime May 7, 2022
95c884b
Merge branch 'main' into dedupe_shard_toasts
mattkime May 7, 2022
b5b1932
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine May 7, 2022
b0d5cb1
fix debounce import
mattkime May 7, 2022
6987ec6
fix debounce import
mattkime May 7, 2022
b6a9f46
fix debounce import
mattkime May 7, 2022
4966f26
Merge branch 'main' into dedupe_shard_toasts
mattkime May 9, 2022
3cad151
set fallback for uiSettings
mattkime May 9, 2022
9281831
check ui settings in try/catch
mattkime May 9, 2022
2cd1d1e
bind this
mattkime May 9, 2022
986d91c
Merge branch 'main' into dedupe_shard_toasts
mattkime May 9, 2022
3fca1f1
Update handle_response.tsx
mattkime May 10, 2022
0997750
Merge branch 'main' into dedupe_shard_toasts
mattkime Sep 23, 2022
b8ffad0
refactor for handle warning
mattkime Sep 23, 2022
7aa32db
fix jest test
mattkime Sep 23, 2022
a01de85
Merge branch 'main' into dedupe_shard_toasts
mattkime Sep 25, 2022
cd46eef
Merge branch 'main' into dedupe_shard_toasts
mattkime Oct 10, 2022
d54b3ec
Merge branch 'main' into dedupe_shard_toasts
mattkime Oct 11, 2022
d4685dd
dedupe and add tests, dedupe based on sessionId
mattkime Oct 12, 2022
f72751e
Merge branch 'main' into dedupe_shard_toasts
mattkime Oct 12, 2022
c5656b5
fix some tests
mattkime Oct 13, 2022
bb912be
Merge branch 'main' into dedupe_shard_toasts
mattkime Oct 13, 2022
a120ab4
Merge branch 'main' into dedupe_shard_toasts
mattkime Oct 14, 2022
c581b6a
bind addWarning
mattkime Oct 15, 2022
0e36324
fix functional tests
mattkime Oct 15, 2022
830b932
Merge branch 'main' into dedupe_shard_toasts
mattkime Oct 15, 2022
9c5833e
Merge branch 'main' into dedupe_shard_toasts
mattkime Oct 17, 2022
1a9baa5
debounce to 30s
mattkime Oct 17, 2022
9b3032b
Merge branch 'dedupe_shard_toasts' of github.com:mattkime/kibana into…
mattkime Oct 17, 2022
0fd3182
advance timers by 30s
mattkime Oct 17, 2022
d182486
functional test fix
mattkime Oct 17, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 28 additions & 6 deletions src/plugins/data/public/search/fetch/handle_warnings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,17 @@ const warnings: SearchResponseWarning[] = [
},
];

const sessionId = 'abcd';

describe('Filtering and showing warnings', () => {
const notifications = notificationServiceMock.createStartContract();
jest.useFakeTimers('modern');

describe('handleWarnings', () => {
const request = { body: {} };
beforeEach(() => {
jest.resetAllMocks();
jest.advanceTimersByTime(10000);
setNotifications(notifications);
(notifications.toasts.addWarning as jest.Mock).mockReset();
(extract.extractWarnings as jest.Mock).mockImplementation(() => warnings);
Expand All @@ -60,43 +64,61 @@ describe('Filtering and showing warnings', () => {
test('should notify if timed out', () => {
(extract.extractWarnings as jest.Mock).mockImplementation(() => [warnings[0]]);
const response = { rawResponse: { timed_out: true } } as unknown as estypes.SearchResponse;
handleWarnings(request, response, theme);
handleWarnings({ request, response, theme });
// test debounce, addWarning should only be called once
handleWarnings({ request, response, theme });

expect(notifications.toasts.addWarning).toBeCalledTimes(1);
expect(notifications.toasts.addWarning).toBeCalledWith({ title: 'Something timed out!' });

// test debounce, call addWarning again due to sessionId
handleWarnings({ request, response, theme, sessionId });
expect(notifications.toasts.addWarning).toBeCalledTimes(2);
});

test('should notify if shards failed for unknown type/reason', () => {
(extract.extractWarnings as jest.Mock).mockImplementation(() => [warnings[2]]);
const response = {
rawResponse: { _shards: { failed: 1, total: 2, successful: 1, skipped: 1 } },
} as unknown as estypes.SearchResponse;
handleWarnings(request, response, theme);
handleWarnings({ request, response, theme });
// test debounce, addWarning should only be called once
handleWarnings({ request, response, theme });

expect(notifications.toasts.addWarning).toBeCalledTimes(1);
expect(notifications.toasts.addWarning).toBeCalledWith({ title: 'Some shards failed!' });

// test debounce, call addWarning again due to sessionId
handleWarnings({ request, response, theme, sessionId });
expect(notifications.toasts.addWarning).toBeCalledTimes(2);
});

test('should add mount point for shard modal failure button if warning.text is provided', () => {
(extract.extractWarnings as jest.Mock).mockImplementation(() => [warnings[1]]);
const response = {
rawResponse: { _shards: { failed: 1, total: 2, successful: 1, skipped: 1 } },
} as unknown as estypes.SearchResponse;
handleWarnings(request, response, theme);
handleWarnings({ request, response, theme });
// test debounce, addWarning should only be called once
handleWarnings({ request, response, theme });

expect(notifications.toasts.addWarning).toBeCalledTimes(1);
expect(notifications.toasts.addWarning).toBeCalledWith({
title: 'Some shards failed!',
text: expect.any(Function),
});

// test debounce, call addWarning again due to sessionId
handleWarnings({ request, response, theme, sessionId });
expect(notifications.toasts.addWarning).toBeCalledTimes(2);
});

test('should notify once if the response contains multiple failures', () => {
(extract.extractWarnings as jest.Mock).mockImplementation(() => [warnings[1], warnings[2]]);
const response = {
rawResponse: { _shards: { failed: 1, total: 2, successful: 1, skipped: 1 } },
} as unknown as estypes.SearchResponse;
handleWarnings(request, response, theme);
handleWarnings({ request, response, theme });

expect(notifications.toasts.addWarning).toBeCalledTimes(1);
expect(notifications.toasts.addWarning).toBeCalledWith({
Expand All @@ -111,7 +133,7 @@ describe('Filtering and showing warnings', () => {
const response = {
rawResponse: { _shards: { failed: 1, total: 2, successful: 1, skipped: 1 } },
} as unknown as estypes.SearchResponse;
handleWarnings(request, response, theme, callback);
handleWarnings({ request, response, theme, callback });

expect(notifications.toasts.addWarning).toBeCalledTimes(1);
expect(notifications.toasts.addWarning).toBeCalledWith({ title: 'Some shards failed!' });
Expand All @@ -122,7 +144,7 @@ describe('Filtering and showing warnings', () => {
const response = {
rawResponse: { _shards: { failed: 1, total: 2, successful: 1, skipped: 1 } },
} as unknown as estypes.SearchResponse;
handleWarnings(request, response, theme, callback);
handleWarnings({ request, response, theme, callback });

expect(notifications.toasts.addWarning).toBeCalledTimes(0);
});
Expand Down
49 changes: 37 additions & 12 deletions src/plugins/data/public/search/fetch/handle_warnings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
*/

import { estypes } from '@elastic/elasticsearch';
import { debounce } from 'lodash';
import { EuiSpacer } from '@elastic/eui';
import { ThemeServiceStart } from '@kbn/core/public';
import { toMountPoint } from '@kbn/kibana-react-plugin/public';
import React from 'react';
import type { MountPoint } from '@kbn/core/public';
import { SearchRequest } from '..';
import { getNotifications } from '../../services';
import { ShardFailureOpenModalButton, ShardFailureRequest } from '../../shard_failure_modal';
Expand All @@ -21,33 +23,56 @@ import {
} from '../types';
import { extractWarnings } from './extract_warnings';

const getDebouncedWarning = () => {
const addWarning = () => debounce(getNotifications().toasts.addWarning, 10000, { leading: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that regardless of the deduping, we'll still only show a max of one warning every 10 seconds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debounce is applied for a given debounceKey. Different debounceKeys within 10s would result in multiple warnings. The same debounceKey within 10s will result in only one warning, past 10s an additional warning is shown.

const memory: Record<string, ReturnType<typeof addWarning>> = {};

return (
debounceKey: string,
mattkime marked this conversation as resolved.
Show resolved Hide resolved
title: string,
text?: string | MountPoint<HTMLElement> | undefined
) => {
memory[debounceKey] = memory[debounceKey] || addWarning();
return memory[debounceKey]({ title, text });
};
};

const debouncedWarningWithoutReason = getDebouncedWarning();
const debouncedTimeoutWarning = getDebouncedWarning();
const debouncedWarning = getDebouncedWarning();

/**
* @internal
* All warnings are expected to come from the same response. Therefore all "text" properties, which contain the
* response, will be the same.
*/
export function handleWarnings(
request: SearchRequest,
response: estypes.SearchResponse,
theme: ThemeServiceStart,
cb?: WarningHandlerCallback
) {
export function handleWarnings({
request,
response,
theme,
callback,
sessionId = '',
}: {
request: SearchRequest;
response: estypes.SearchResponse;
theme: ThemeServiceStart;
callback?: WarningHandlerCallback;
sessionId?: string;
}) {
const warnings = extractWarnings(response);
if (warnings.length === 0) {
return;
}

const internal = cb ? filterWarnings(warnings, cb) : warnings;
const internal = callback ? filterWarnings(warnings, callback) : warnings;
if (internal.length === 0) {
return;
}

// timeout notification
const [timeout] = internal.filter((w) => w.type === 'timed_out');
if (timeout) {
getNotifications().toasts.addWarning({
title: timeout.message,
});
debouncedTimeoutWarning(sessionId + timeout.message, timeout.message);
}

// shard warning failure notification
Expand Down Expand Up @@ -75,12 +100,12 @@ export function handleWarnings(
{ theme$: theme.theme$ }
);

getNotifications().toasts.addWarning({ title, text });
debouncedWarning(sessionId + warning.text, title, text);
return;
}

// timeout warning, or shard warning with no failure reason
getNotifications().toasts.addWarning({ title });
debouncedWarningWithoutReason(sessionId + title, title);
}

/**
Expand Down
13 changes: 10 additions & 3 deletions src/plugins/data/public/search/search_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ describe('Search service', () => {
let mockCoreSetup: MockedKeys<CoreSetup>;
let mockCoreStart: MockedKeys<CoreStart>;
const initializerContext = coreMock.createPluginInitializerContext();
jest.useFakeTimers('modern');
initializerContext.config.get = jest.fn().mockReturnValue({
search: { aggs: { shardDelay: { enabled: false } }, sessions: { enabled: true } },
});
Expand All @@ -35,6 +36,7 @@ describe('Search service', () => {
mockCoreSetup = coreMock.createSetup();
mockCoreStart = coreMock.createStart();
searchService = new SearchService(initializerContext);
jest.advanceTimersByTime(10000);
});

describe('setup()', () => {
Expand Down Expand Up @@ -217,7 +219,13 @@ describe('Search service', () => {
const responder1 = inspector.adapter.start('request1');
const responder2 = inspector.adapter.start('request2');
responder1.ok(getMockResponseWithShards(shards));
responder2.ok(getMockResponseWithShards(shards));
responder2.ok({
json: {
rawResponse: {
timed_out: true,
},
},
});

data.showWarnings(inspector.adapter, callback);

Expand All @@ -227,8 +235,7 @@ describe('Search service', () => {
text: expect.any(Function),
});
expect(notifications.toasts.addWarning).nthCalledWith(2, {
title: '2 of 4 shards failed',
text: expect.any(Function),
title: 'Data might be incomplete because your request timed out',
});
});
});
Expand Down
16 changes: 13 additions & 3 deletions src/plugins/data/public/search/search_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,12 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> {
onResponse: (request, response, options) => {
if (!options.disableShardFailureWarning) {
const { rawResponse } = response;
handleWarnings(request.body, rawResponse, theme);
handleWarnings({
request: request.body,
response: rawResponse,
theme,
sessionId: options.sessionId,
});
}
return response;
},
Expand Down Expand Up @@ -271,7 +276,7 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> {
showError: (e) => {
this.searchInterceptor.showError(e);
},
showWarnings: (adapter, cb) => {
showWarnings: (adapter, callback) => {
adapter?.getRequests().forEach((request) => {
const rawResponse = (
request.response?.json as { rawResponse: estypes.SearchResponse | undefined }
Expand All @@ -281,7 +286,12 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> {
return;
}

handleWarnings(request.json as SearchRequest, rawResponse, theme, cb);
handleWarnings({
request: request.json as SearchRequest,
response: rawResponse,
theme,
callback,
});
});
},
session: this.sessionService,
Expand Down
4 changes: 2 additions & 2 deletions test/examples/search/warnings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

// toasts
const toasts = await find.allByCssSelector(toastsSelector);
expect(toasts.length).to.be(2);
expect(toasts.length).to.be(1);
const expects = ['2 of 4 shards failed', 'Query result'];
await asyncForEach(toasts, async (t, index) => {
expect(await t.getVisibleText()).to.eql(expects[index]);
Expand Down Expand Up @@ -157,7 +157,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
let toasts: WebElementWrapper[] = [];
await retry.try(async () => {
toasts = await find.allByCssSelector(toastsSelector);
expect(toasts.length).to.be(2);
expect(toasts.length).to.be(1);
});
const expects = ['2 of 4 shards failed', 'Query result'];
await asyncForEach(toasts, async (t, index) => {
Expand Down