github-mirrors/kibana
1
0
Fork 0
mirror of https://github.com/elastic/kibana.git synced 2025-04-23 17:28:26 -04:00
Code Issues Projects Releases Packages Wiki Activity

[Discover] Fix performance when switching to text based query (#159903)

Browse source 
Optimize switch to text-based queries by reducing unnecessary EuiDataGrid renderings and addressing performance regression. Introduce a PARTIAL state emission in the document$ observable for text-based query results. This state indicates ongoing loading in the UI. The appState to URL changes hook completes with the COMPLETE state submission to document$, enabling UI rendering.

Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
This commit is contained in:
Matthias Wilhelm 2023-06-23 20:59:03 +02:00 • committed by GitHub
parent c3b8ed278d
commit 43b460c72b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 97 additions and 51 deletions
Download patch file Download diff file Expand all files Collapse all files

29
src/plugins/discover/public/application/main/components/layout/discover_documents.tsx
View file

@ -115,17 +115,22 @@ function DiscoverDocumentsComponent({
const sampleSize = useMemo(() => uiSettings.get(SAMPLE_SIZE_SETTING), [uiSettings]);
const documentState = useDataState(documents$);
const isDataLoading = documentState.fetchStatus === FetchStatus.LOADING;
const isDataLoading =
documentState.fetchStatus === FetchStatus.LOADING ||
documentState.fetchStatus === FetchStatus.PARTIAL;
const isTextBasedQuery = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]);
// This is needed to prevent EuiDataGrid pushing onSort because the data view has been switched.
// It's just necessary for non-text-based query lang requests since they don't have a partial result state, that's
// considered as loading state in the Component.
// 1. When switching the data view, the sorting in the URL is reset to the default sorting of the selected data view.
// 2. The new sort param is already available in this component and propagated to the EuiDataGrid.
// 3. currentColumns are still referring to the old state
// 4. since the new sort by field isn't available in currentColumns EuiDataGrid is emitting a 'onSort', which is unsorting the grid
// 5. this is propagated to Discover's URL and causes an unwanted change of state to an unsorted state
// This solution switches to the loading state in this component when the URL index doesn't match the dataView.id
const isDataViewLoading = index && dataView.id && index !== dataView.id;
const isEmptyDataResult = !documentState.result || documentState.result.length === 0;
const isPlainRecord = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]);
const isDataViewLoading = !isTextBasedQuery && dataView.id && index !== dataView.id;
const isEmptyDataResult =
isTextBasedQuery || !documentState.result || documentState.result.length === 0;
const rows = useMemo(() => documentState.result || [], [documentState.result]);
const {
@ -173,10 +178,10 @@ function DiscoverDocumentsComponent({
const showTimeCol = useMemo(
() =>
!isPlainRecord &&
!isTextBasedQuery &&
!uiSettings.get(DOC_HIDE_TIME_COLUMN_SETTING, false) &&
!!dataView.timeFieldName,
[isPlainRecord, uiSettings, dataView.timeFieldName]
[isTextBasedQuery, uiSettings, dataView.timeFieldName]
);
if (isDataViewLoading || (isEmptyDataResult && isDataLoading)) {
@ -209,12 +214,12 @@ function DiscoverDocumentsComponent({
isLoading={isDataLoading}
searchDescription={savedSearch.description}
sharedItemTitle={savedSearch.title}
isPlainRecord={isPlainRecord}
isPlainRecord={isTextBasedQuery}
onAddColumn={onAddColumn}
onFilter={onAddFilter as DocViewFilterFn}
onMoveColumn={onMoveColumn}
onRemoveColumn={onRemoveColumn}
onSort={!isPlainRecord ? onSort : undefined}
onSort={!isTextBasedQuery ? onSort : undefined}
useNewFieldsApi={useNewFieldsApi}
dataTestSubj="discoverDocTable"
DocViewer={DocViewer}
@ -223,8 +228,8 @@ function DiscoverDocumentsComponent({
)}
{!isLegacy && (
<>
{!hideAnnouncements && !isPlainRecord && (
<DiscoverTourProvider isPlainRecord={isPlainRecord}>
{!hideAnnouncements && !isTextBasedQuery && (
<DiscoverTourProvider isPlainRecord={isTextBasedQuery}>
<DocumentExplorerUpdateCallout />
</DiscoverTourProvider>
)}
@ -250,13 +255,13 @@ function DiscoverDocumentsComponent({
onFilter={onAddFilter as DocViewFilterFn}
onRemoveColumn={onRemoveColumn}
onSetColumns={onSetColumns}
onSort={!isPlainRecord ? onSort : undefined}
onSort={!isTextBasedQuery ? onSort : undefined}
onResize={onResizeDataGrid}
useNewFieldsApi={useNewFieldsApi}
rowHeightState={rowHeight}
onUpdateRowHeight={onUpdateRowHeight}
isSortEnabled={true}
isPlainRecord={isPlainRecord}
isPlainRecord={isTextBasedQuery}
query={query}
rowsPerPageState={rowsPerPage}
onUpdateRowsPerPage={onUpdateRowsPerPage}

6
src/plugins/discover/public/application/main/components/no_results/no_results_suggestions/no_results_suggestions.tsx
View file

@ -18,6 +18,7 @@ import {
type Filter,
} from '@kbn/es-query';
import { FormattedMessage } from '@kbn/i18n-react';
import { isTextBasedQuery } from '../../../utils/is_text_based_query';
import { NoResultsSuggestionDefault } from './no_results_suggestion_default';
import {
NoResultsSuggestionWhenFilters,
@ -76,7 +77,8 @@ export const NoResultsSuggestions: React.FC<NoResultsSuggestionProps> = ({
}
};
const canExtendTimeRange = Boolean(occurrencesRange?.from && occurrencesRange.to);
const canExtendTimeRange =
!isTextBasedQuery(query) && Boolean(occurrencesRange?.from && occurrencesRange.to);
const canAdjustSearchCriteria = isTimeBased || hasFilters || hasQuery;
const body = canAdjustSearchCriteria ? (
@ -135,7 +137,7 @@ export const NoResultsSuggestions: React.FC<NoResultsSuggestionProps> = ({
`}
>
{typeof occurrencesRange === 'undefined' ? (
<EuiLoadingSpinner />
!isTextBasedQuery(query) && <EuiLoadingSpinner />
) : canExtendTimeRange ? (
<EuiButton
color="primary"

5
src/plugins/discover/public/application/main/components/no_results/no_results_suggestions/use_fetch_occurances_range.ts
View file

@ -15,6 +15,7 @@ import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser';
import type { AggregationsSingleMetricAggregateBase } from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
import { buildEsQuery } from '@kbn/es-query';
import { getEsQueryConfig } from '@kbn/data-plugin/common';
import { isTextBasedQuery } from '../../../utils/is_text_based_query';
export interface Params {
dataView?: DataView;
@ -86,7 +87,9 @@ export const useFetchOccurrencesRange = (params: Params): Result => {
}, [abortControllerRef, mountedRef]);
useEffect(() => {
fetchOccurrences(params.dataView, params.query, params.filters);
if (!isTextBasedQuery(params.query)) {
fetchOccurrences(params.dataView, params.query, params.filters);
}
}, [fetchOccurrences, params.query, params.filters, params.dataView]);
return {

24
src/plugins/discover/public/application/main/hooks/use_test_based_query_language.test.tsx
View file

@ -38,7 +38,7 @@ function getHookProps(
const msgLoading = {
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.LOADING,
fetchStatus: FetchStatus.PARTIAL,
query,
};
stateContainer.dataState.data$.documents$.next(msgLoading);
@ -53,7 +53,7 @@ function getHookProps(
const query = { sql: 'SELECT * from the-data-view-title' };
const msgComplete = {
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
@ -137,7 +137,7 @@ describe('useTextBasedQueryLanguage', () => {
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
@ -167,7 +167,7 @@ describe('useTextBasedQueryLanguage', () => {
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
@ -182,7 +182,7 @@ describe('useTextBasedQueryLanguage', () => {
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
@ -205,7 +205,7 @@ describe('useTextBasedQueryLanguage', () => {
documents$.next({
recordRawType: RecordRawType.DOCUMENT,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
@ -217,7 +217,7 @@ describe('useTextBasedQueryLanguage', () => {
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
@ -245,7 +245,7 @@ describe('useTextBasedQueryLanguage', () => {
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
@ -258,7 +258,7 @@ describe('useTextBasedQueryLanguage', () => {
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
@ -289,7 +289,7 @@ describe('useTextBasedQueryLanguage', () => {
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
@ -324,7 +324,7 @@ describe('useTextBasedQueryLanguage', () => {
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
@ -355,7 +355,7 @@ describe('useTextBasedQueryLanguage', () => {
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',

17
src/plugins/discover/public/application/main/hooks/use_text_based_query_language.ts
View file

@ -47,6 +47,7 @@ export function useTextBasedQueryLanguage({
columns: [],
query: undefined,
};
indexTitle.current = '';
}
}, []);
@ -56,14 +57,23 @@ export function useTextBasedQueryLanguage({
if (!query || next.fetchStatus === FetchStatus.ERROR) {
return;
}
const sendComplete = () => {
stateContainer.dataState.data$.documents$.next({
...next,
fetchStatus: FetchStatus.COMPLETE,
});
};
const { columns: stateColumns, index, viewMode } = stateContainer.appState.getState();
let nextColumns: string[] = [];
const isTextBasedQueryLang =
recordRawType === 'plain' && isOfAggregateQueryType(query) && 'sql' in query;
const hasResults = next.result?.length && next.fetchStatus === FetchStatus.COMPLETE;
const hasResults = Boolean(next.result?.length);
const initialFetch = !prev.current.columns.length;
if (isTextBasedQueryLang) {
if (next.fetchStatus !== FetchStatus.PARTIAL) {
return;
}
if (hasResults) {
// check if state needs to contain column transformation due to a different columns in the resultset
const firstRow = next.result![0];
@ -92,13 +102,13 @@ export function useTextBasedQueryLanguage({
const addDataViewToState = Boolean(dataViewObj?.id !== index) || initialFetch;
const queryChanged = indexPatternFromQuery !== indexTitle.current;
if (!addColumnsToState && !queryChanged) {
sendComplete();
return;
}
if (queryChanged) {
indexTitle.current = indexPatternFromQuery;
}
const nextState = {
...(addDataViewToState && { index: dataViewObj.id }),
...(addColumnsToState && { columns: nextColumns }),
@ -106,7 +116,8 @@ export function useTextBasedQueryLanguage({
viewMode: getValidViewMode({ viewMode, isTextBasedQueryMode: true }),
}),
};
stateContainer.appState.replaceUrlState(nextState);
await stateContainer.appState.replaceUrlState(nextState);
sendComplete();
} else {
// cleanup for a "regular" query
cleanup();

25
src/plugins/discover/public/application/main/hooks/utils/build_state_subscribe.ts
View file

@ -44,20 +44,32 @@ export const buildStateSubscribe =
}) =>
async (nextState: DiscoverAppState) => {
const prevState = appState.getPrevious();
const nextQuery = nextState.query;
const savedSearch = savedSearchState.getState();
if (isEqualState(prevState, nextState)) {
const prevQuery = savedSearch.searchSource.getField('query');
const queryChanged = !isEqual(nextQuery, prevQuery) || !isEqual(nextQuery, prevState.query);
if (isEqualState(prevState, nextState) && !queryChanged) {
addLog('[appstate] subscribe update ignored due to no changes', { prevState, nextState });
return;
}
addLog('[appstate] subscribe triggered', nextState);
const { hideChart, interval, breakdownField, sort, index } = appState.getPrevious();
const { hideChart, interval, breakdownField, sort, index } = prevState;
const isTextBasedQueryLang = isTextBasedQuery(nextQuery);
if (isTextBasedQueryLang) {
const isTextBasedQueryLangPrev = isTextBasedQuery(prevQuery);
if (!isTextBasedQueryLangPrev) {
savedSearchState.update({ nextState });
dataState.reset(savedSearch);
}
}
// Cast to boolean to avoid false positives when comparing
// undefined and false, which would trigger a refetch
const chartDisplayChanged = Boolean(nextState.hideChart) !== Boolean(hideChart);
const chartIntervalChanged = nextState.interval !== interval;
const chartIntervalChanged = nextState.interval !== interval && !isTextBasedQueryLang;
const breakdownFieldChanged = nextState.breakdownField !== breakdownField;
const docTableSortChanged = !isEqual(nextState.sort, sort);
const dataViewChanged = !isEqual(nextState.index, index);
const docTableSortChanged = !isEqual(nextState.sort, sort) && !isTextBasedQueryLang;
const dataViewChanged = !isEqual(nextState.index, index) && !isTextBasedQueryLang;
let savedSearchDataView;
// NOTE: this is also called when navigating from discover app to context app
if (nextState.index && dataViewChanged) {
@ -90,7 +102,8 @@ export const buildStateSubscribe =
chartIntervalChanged ||
breakdownFieldChanged ||
docTableSortChanged ||
dataViewChanged
dataViewChanged ||
queryChanged
) {
addLog('[appstate] subscribe triggers data fetching');
dataState.fetch();

2
src/plugins/discover/public/application/main/services/discover_app_state_container.ts
View file

@ -54,7 +54,7 @@ export interface DiscoverAppStateContainer extends ReduxLikeStateContainer<Disco
* @param newState
* @param merge if true, the given state is merged with the current state
*/
replaceUrlState: (newPartial: DiscoverAppState, merge?: boolean) => void;
replaceUrlState: (newPartial: DiscoverAppState, merge?: boolean) => Promise<void>;
/**
* Resets the current state to the initial state
*/

5
src/plugins/discover/public/application/main/services/discover_state.ts
View file

@ -351,10 +351,7 @@ export function getDiscoverStateContainer({
const unsubscribeData = dataStateContainer.subscribe();
// updates saved search when query or filters change, triggers data fetching
const filterUnsubscribe = merge(
services.data.query.queryString.getUpdates$(),
services.filterManager.getFetches$()
).subscribe(async () => {
const filterUnsubscribe = merge(services.filterManager.getFetches$()).subscribe(async () => {
await savedSearchContainer.update({
nextDataView: internalStateContainer.getState().dataView,
nextState: appStateContainer.getState(),

2
src/plugins/discover/public/application/main/utils/fetch_all.test.ts
View file

@ -266,7 +266,7 @@ describe('test fetchAll', () => {
{ fetchStatus: FetchStatus.UNINITIALIZED },
{ fetchStatus: FetchStatus.LOADING, recordRawType: 'plain', query },
{
fetchStatus: FetchStatus.COMPLETE,
fetchStatus: FetchStatus.PARTIAL,
recordRawType: 'plain',
result: documents,
textBasedQueryColumns: [{ id: '1', name: 'test1', meta: { type: 'number' } }],

17
src/plugins/discover/public/application/main/utils/fetch_all.ts
View file

@ -8,7 +8,8 @@
import { Adapters } from '@kbn/inspector-plugin/common';
import type { SavedSearch, SortOrder } from '@kbn/saved-search-plugin/public';
import { BehaviorSubject, filter, firstValueFrom, map, merge, scan } from 'rxjs';
import { DiscoverAppState } from '../services/discover_app_state_container';
import { isEqual } from 'lodash';
import type { DiscoverAppState } from '../services/discover_app_state_container';
import { updateVolatileSearchSource } from './update_search_source';
import { getRawRecordType } from './get_raw_record_type';
import {
@ -55,6 +56,7 @@ export function fetchAll(
try {
const dataView = searchSource.getField('index')!;
const query = getAppState().query;
const prevQuery = dataSubjects.documents$.getValue().query;
const recordRawType = getRawRecordType(query);
if (reset) {
sendResetMsg(dataSubjects, initialFetchStatus, recordRawType);
@ -93,9 +95,20 @@ export function fetchAll(
recordRawType,
});
}
/**
* The partial state for text based query languages is necessary in case the query has changed
* In the follow up useTextBasedQueryLanguage hook in this case new columns are added to AppState
* So the data table shows the new columns of the table. The partial state was introduced to prevent
* To frequent change of state causing the table to re-render to often, which causes race conditions
* So it takes too long, a bad user experience, also a potential flakniess in tests
*/
const fetchStatus =
useSql && (!prevQuery || !isEqual(query, prevQuery))
? FetchStatus.PARTIAL
: FetchStatus.COMPLETE;
dataSubjects.documents$.next({
fetchStatus: FetchStatus.COMPLETE,
fetchStatus,
result: records,
textBasedQueryColumns,
recordRawType,

1
src/plugins/discover/public/application/main/utils/get_fetch_observable.ts
View file

@ -53,7 +53,6 @@ export function getFetch$({
);
})
),
data.query.queryString.getUpdates$(),
searchSessionManager.newSearchSessionIdFromURL$.pipe(filter((sessionId) => !!sessionId))
).pipe(debounceTime(100));
}

8
src/plugins/discover/public/components/discover_grid/discover_grid.tsx
View file

@ -22,6 +22,7 @@ import {
EuiLoadingSpinner,
EuiIcon,
EuiDataGridRefProps,
EuiDataGridInMemory,
} from '@elastic/eui';
import type { DataView } from '@kbn/data-views-plugin/public';
import type { SortOrder } from '@kbn/saved-search-plugin/public';
@ -227,7 +228,6 @@ export interface DiscoverGridProps {
toastNotifications: ToastsStart;
};
}
export const EuiDataGridMemoized = React.memo(EuiDataGrid);
const CONTROL_COLUMN_IDS_DEFAULT = ['openDetails', 'select'];
@ -583,6 +583,10 @@ export const DiscoverGrid = ({
[onUpdateRowHeight]
);
const inMemory = useMemo(() => {
return isPlainRecord ? ({ level: 'sorting' } as EuiDataGridInMemory) : undefined;
}, [isPlainRecord]);
const toolbarVisibility = useMemo(
() =>
defaultColumns
@ -685,7 +689,7 @@ export const DiscoverGrid = ({
sorting={sorting as EuiDataGridSorting}
toolbarVisibility={toolbarVisibility}
rowHeightsOptions={rowHeightsOptions}
inMemory={isPlainRecord ? { level: 'sorting' } : undefined}
inMemory={inMemory}
gridStyle={GRID_STYLE}
/>
</div>

4
test/functional/apps/discover/group3/_sidebar.ts
View file

@ -52,8 +52,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
await PageObjects.unifiedFieldList.cleanSidebarLocalStorage();
});
// FLAKY: https://github.com/elastic/kibana/issues/159167
describe.skip('field filtering', function () {
describe('field filtering', function () {
it('should reveal and hide the filter form when the toggle is clicked', async function () {
await PageObjects.unifiedFieldList.openSidebarFieldFilter();
await PageObjects.unifiedFieldList.closeSidebarFieldFilter();
@ -377,6 +376,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
);
await PageObjects.discover.selectTextBaseLang('SQL');
await PageObjects.unifiedFieldList.waitUntilSidebarHasLoaded();
expect(await PageObjects.unifiedFieldList.getSidebarAriaDescription()).to.be(
'50 selected fields. 51 available fields.'

3
x-pack/test/functional/apps/discover/visualize_field.ts
View file

@ -38,8 +38,7 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) {
await PageObjects.timePicker.setDefaultAbsoluteRange();
}
// FLAKY: https://github.com/elastic/kibana/issues/148225
describe.skip('discover field visualize button', () => {
describe('discover field visualize button', () => {
before(async () => {
await kibanaServer.uiSettings.replace(defaultSettings);
});