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

[Discover] When using ES|QL prevent propagation of index state variable to URL (#177241)

Browse source 
By removing redundant propagating state (index, viewMode) to URL for ES|QL, it leads to less render cycles.

This is especially valuable when switching from DataView to ES|QL mode showing performance gains of about 25 - 30%.

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
This commit is contained in:
Matthias Wilhelm 2024-03-18 15:19:45 +01:00 • committed by GitHub
parent 902ba553d5
commit 187a8f5fab
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 141 additions and 92 deletions
Download patch file Download diff file Expand all files Collapse all files

5
src/plugins/discover/public/application/main/components/layout/discover_layout.tsx
View file

@ -83,8 +83,10 @@ export function DiscoverLayout({ stateContainer }: DiscoverLayoutProps) {
state.columns,
state.sort,
]);
const isPlainRecord = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]);
const viewMode: VIEW_MODE = useAppStateSelector((state) => {
if (uiSettings.get(SHOW_FIELD_STATISTICS) !== true) return VIEW_MODE.DOCUMENT_LEVEL;
if (uiSettings.get(SHOW_FIELD_STATISTICS) !== true || isPlainRecord)
return VIEW_MODE.DOCUMENT_LEVEL;
return state.viewMode ?? VIEW_MODE.DOCUMENT_LEVEL;
});
const [dataView, dataViewLoading] = useInternalStateSelector((state) => [
@ -112,7 +114,6 @@ export function DiscoverLayout({ stateContainer }: DiscoverLayoutProps) {
const useNewFieldsApi = useMemo(() => !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE), [uiSettings]);
const isPlainRecord = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]);
const resultState = useMemo(
() => getResultState(dataState.fetchStatus, dataState.foundDocuments ?? false),
[dataState.fetchStatus, dataState.foundDocuments]

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

@ -102,36 +102,25 @@ describe('useTextBasedQueryLanguage', () => {
test('a text based query should change state when loading and finished', async () => {
const { replaceUrlState, stateContainer } = renderHookWithContext(true);
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
await waitFor(() => {
expect(replaceUrlState).toHaveBeenCalledWith({
index: 'the-data-view-id',
columns: [],
});
});
replaceUrlState.mockReset();
stateContainer.dataState.data$.documents$.next(msgComplete);
expect(replaceUrlState).toHaveBeenCalledTimes(0);
});
test('should change viewMode to DOCUMENT_LEVEL if it was AGGREGATED_LEVEL', async () => {
test('should change viewMode to undefined (default) if it was AGGREGATED_LEVEL', async () => {
const { replaceUrlState } = renderHookWithContext(false, {
viewMode: VIEW_MODE.AGGREGATED_LEVEL,
});
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
expect(replaceUrlState).toHaveBeenCalledWith({
index: 'the-data-view-id',
viewMode: VIEW_MODE.DOCUMENT_LEVEL,
columns: [],
viewMode: undefined,
});
});
test('changing a text based query with different result columns should change state when loading and finished', async () => {
const { replaceUrlState, stateContainer } = renderHookWithContext(false);
const documents$ = stateContainer.dataState.data$.documents$;
stateContainer.dataState.data$.documents$.next(msgComplete);
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
replaceUrlState.mockReset();
documents$.next({
@ -151,18 +140,15 @@ describe('useTextBasedQueryLanguage', () => {
await waitFor(() => {
expect(replaceUrlState).toHaveBeenCalledWith({
index: 'the-data-view-id',
columns: ['field1'],
});
});
});
test('changing a text based query with same result columns should change state when loading and finished', async () => {
test('changing a text based query with same result columns should not change state when loading and finished', async () => {
const { replaceUrlState, stateContainer } = renderHookWithContext(false);
const documents$ = stateContainer.dataState.data$.documents$;
stateContainer.dataState.data$.documents$.next(msgComplete);
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
replaceUrlState.mockReset();
documents$.next({
recordRawType: RecordRawType.PLAIN,
@ -176,52 +162,16 @@ describe('useTextBasedQueryLanguage', () => {
],
query: { esql: 'from the-data-view-2' },
});
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
await waitFor(() => {
expect(replaceUrlState).toHaveBeenCalledWith({
index: 'the-data-view-id',
columns: [],
});
});
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(0));
});
test('changing a text based query with no transformational commands should only change dataview state when loading and finished', async () => {
const { replaceUrlState, stateContainer } = renderHookWithContext(false);
const documents$ = stateContainer.dataState.data$.documents$;
stateContainer.dataState.data$.documents$.next(msgComplete);
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
replaceUrlState.mockReset();
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.PARTIAL,
result: [
{
id: '1',
raw: { field1: 1 },
flattened: { field1: 1 },
} as unknown as DataTableRecord,
],
// non transformational command
query: { esql: 'from the-data-view-title | where field1 > 0' },
});
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
await waitFor(() => {
expect(replaceUrlState).toHaveBeenCalledWith({
index: 'the-data-view-id',
columns: [],
});
});
});
test('only changing a text based query with same result columns should not change columns', async () => {
const { replaceUrlState, stateContainer } = renderHookWithContext(false);
const documents$ = stateContainer.dataState.data$.documents$;
documents$.next(msgComplete);
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(0));
replaceUrlState.mockReset();
documents$.next({
@ -237,6 +187,11 @@ describe('useTextBasedQueryLanguage', () => {
query: { esql: 'from the-data-view-title | keep field1' },
});
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
await waitFor(() => {
expect(replaceUrlState).toHaveBeenCalledWith({
columns: ['field1'],
});
});
replaceUrlState.mockReset();
documents$.next({
@ -252,14 +207,14 @@ describe('useTextBasedQueryLanguage', () => {
query: { esql: 'from the-data-view-title | keep field 1 | WHERE field1=1' },
});
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(0));
});
test('if its not a text based query coming along, it should be ignored', async () => {
const { replaceUrlState, stateContainer } = renderHookWithContext(false);
const documents$ = stateContainer.dataState.data$.documents$;
documents$.next(msgComplete);
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(0));
replaceUrlState.mockReset();
documents$.next({
@ -289,7 +244,6 @@ describe('useTextBasedQueryLanguage', () => {
await waitFor(() => {
expect(replaceUrlState).toHaveBeenCalledWith({
index: 'the-data-view-id',
columns: ['field1'],
});
});
@ -298,9 +252,9 @@ describe('useTextBasedQueryLanguage', () => {
test('it should not overwrite existing state columns on initial fetch', async () => {
const { replaceUrlState, stateContainer } = renderHookWithContext(false, {
columns: ['field1'],
index: 'the-data-view-id',
});
const documents$ = stateContainer.dataState.data$.documents$;
expect(replaceUrlState).toHaveBeenCalledTimes(0);
documents$.next({
recordRawType: RecordRawType.PLAIN,
@ -315,6 +269,14 @@ describe('useTextBasedQueryLanguage', () => {
query: { esql: 'from the-data-view-title | keep field 1 | WHERE field1=1' },
});
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
await waitFor(() => {
expect(replaceUrlState).toHaveBeenCalledWith({
columns: ['field1', 'field2'],
});
});
replaceUrlState.mockReset();
documents$.next({
recordRawType: RecordRawType.PLAIN,
fetchStatus: FetchStatus.PARTIAL,
@ -336,7 +298,6 @@ describe('useTextBasedQueryLanguage', () => {
test('it should not overwrite existing state columns on initial fetch and non transformational commands', async () => {
const { replaceUrlState, stateContainer } = renderHookWithContext(false, {
columns: ['field1'],
index: 'the-data-view-id',
});
const documents$ = stateContainer.dataState.data$.documents$;
@ -356,9 +317,8 @@ describe('useTextBasedQueryLanguage', () => {
});
test('it should overwrite existing state columns on transitioning from a query with non transformational commands to a query with transformational', async () => {
const { replaceUrlState, stateContainer } = renderHookWithContext(false, {
index: 'the-data-view-id',
});
const { replaceUrlState, stateContainer } = renderHookWithContext(false, {});
const documents$ = stateContainer.dataState.data$.documents$;
documents$.next({
@ -395,7 +355,6 @@ describe('useTextBasedQueryLanguage', () => {
test('it should not overwrite state column when successfully fetching after an error fetch', async () => {
const { replaceUrlState, stateContainer } = renderHookWithContext(false, {
columns: [],
index: 'the-data-view-id',
});
const documents$ = stateContainer.dataState.data$.documents$;
@ -468,7 +427,7 @@ describe('useTextBasedQueryLanguage', () => {
renderHook(() => useTextBasedQueryLanguage(props), { wrapper: getHookContext(stateContainer) });
documents$.next(msgComplete);
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(0));
replaceUrlState.mockReset();
documents$.next({
@ -488,7 +447,6 @@ describe('useTextBasedQueryLanguage', () => {
await waitFor(() => {
expect(replaceUrlState).toHaveBeenCalledWith({
index: 'the-data-view-id',
columns: ['field1'],
});
});

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

@ -9,7 +9,6 @@ import { isEqual } from 'lodash';
import { isOfAggregateQueryType, getAggregateQueryMode } from '@kbn/es-query';
import { useCallback, useEffect, useRef } from 'react';
import type { DataViewsContract } from '@kbn/data-views-plugin/public';
import { VIEW_MODE } from '@kbn/saved-search-plugin/public';
import { switchMap } from 'rxjs';
import { useSavedSearchInitial } from '../services/discover_state_provider';
import type { DiscoverStateContainer } from '../services/discover_state';
@ -87,7 +86,7 @@ export function useTextBasedQueryLanguage({
if (next.fetchStatus !== FetchStatus.PARTIAL) {
return;
}
const dataViewObj = stateContainer.internalState.getState().dataView!;
const dataViewObj = stateContainer.internalState.getState().dataView;
if (hasResults) {
// check if state needs to contain column transformation due to a different columns in the resultset
@ -110,8 +109,10 @@ export function useTextBasedQueryLanguage({
const addColumnsToState = !isEqual(nextColumns, prev.current.columns);
const queryChanged = query[language] !== prev.current.query;
// no need to reset index to state if it hasn't changed
const addDataViewToState = Boolean(dataViewObj?.id !== index);
if (!queryChanged || (!addDataViewToState && !addColumnsToState)) {
const addDataViewToState = index !== undefined;
const changeViewMode =
viewMode !== getValidViewMode({ viewMode, isTextBasedQueryMode: true });
if (!queryChanged || (!addDataViewToState && !addColumnsToState && !changeViewMode)) {
sendComplete();
return;
}
@ -120,14 +121,16 @@ export function useTextBasedQueryLanguage({
prev.current.query = query[language];
prev.current.columns = nextColumns;
}
const nextState = {
...(addDataViewToState && { index: dataViewObj.id }),
...((addColumnsToState || queryChanged) && { columns: nextColumns }),
...(viewMode === VIEW_MODE.AGGREGATED_LEVEL && {
viewMode: getValidViewMode({ viewMode, isTextBasedQueryMode: true }),
}),
};
await stateContainer.appState.replaceUrlState(nextState);
// just change URL state if necessary
if (addDataViewToState || addColumnsToState || changeViewMode) {
const nextState = {
...(addDataViewToState && { index: undefined }),
...(addColumnsToState && { columns: nextColumns }),
...(changeViewMode && { viewMode: undefined }),
};
await stateContainer.appState.replaceUrlState(nextState);
}
sendComplete();
} else {
// cleanup for a "regular" query

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

@ -47,7 +47,19 @@ export const buildStateSubscribe =
const nextQuery = nextState.query;
const savedSearch = savedSearchState.getState();
const prevQuery = savedSearch.searchSource.getField('query');
const isTextBasedQueryLang = isTextBasedQuery(nextQuery);
const queryChanged = !isEqual(nextQuery, prevQuery) || !isEqual(nextQuery, prevState.query);
if (
isTextBasedQueryLang &&
isEqualState(prevState, nextState, ['index', 'viewMode']) &&
!queryChanged
) {
// When there's a switch from data view to es|ql, this just leads to a cleanup of index and viewMode
// And there's no subsequent action in this function required
addLog('[appstate] subscribe update ignored for es|ql', { prevState, nextState });
return;
}
if (isEqualState(prevState, nextState) && !queryChanged) {
addLog('[appstate] subscribe update ignored due to no changes', { prevState, nextState });
return;
@ -55,7 +67,6 @@ export const buildStateSubscribe =
addLog('[appstate] subscribe triggered', nextState);
const { hideChart, interval, breakdownField, sampleSize, sort, index } = prevState;
const isTextBasedQueryLang = isTextBasedQuery(nextQuery);
if (isTextBasedQueryLang) {
const isTextBasedQueryLangPrev = isTextBasedQuery(prevQuery);
if (!isTextBasedQueryLangPrev) {

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

@ -11,14 +11,14 @@ import { dataViewMock } from '@kbn/discover-utils/src/__mocks__';
import { createKbnUrlStateStorage, withNotifyOnErrors } from '@kbn/kibana-utils-plugin/public';
import type { Filter } from '@kbn/es-query';
import { History } from 'history';
import { savedSearchMock } from '../../../__mocks__/saved_search';
import { discoverServiceMock } from '../../../__mocks__/services';
import {
DiscoverAppStateContainer,
getDiscoverAppStateContainer,
isEqualState,
} from './discover_app_state_container';
import { SavedSearch } from '@kbn/saved-search-plugin/common';
import { SavedSearch, VIEW_MODE } from '@kbn/saved-search-plugin/common';
let history: History;
let state: DiscoverAppStateContainer;
@ -161,4 +161,66 @@ describe('Test discover app state container', () => {
);
});
});
describe('isEqualState', () => {
const initialState = {
index: 'the-index',
columns: ['the-column'],
sort: [],
query: { query: 'the-query', language: 'kuery' },
filters: [],
interval: 'auto',
hideChart: true,
sampleSize: 100,
viewMode: VIEW_MODE.DOCUMENT_LEVEL,
savedQuery: undefined,
hideAggregatedPreview: true,
rowHeight: 25,
headerRowHeight: 25,
grid: {},
breakdownField: 'the-breakdown-field',
};
test('returns true if the states are equal', () => {
expect(isEqualState(initialState, { ...initialState })).toBeTruthy();
});
test('handles the special filter change case correctly ', () => {
// this is some sort of legacy behavior, especially for the filter case
const previousState = { initialState, filters: [{ index: 'test', meta: {} }] };
const nextState = {
initialState,
filters: [{ index: 'test', meta: {}, $$hashKey: 'hi' }],
};
expect(isEqualState(previousState, nextState)).toBeTruthy();
});
test('returns true if the states are not equal', () => {
const changedParams = [
{ index: 'the-new-index' },
{ columns: ['newColumns'] },
{ sort: [['column', 'desc']] },
{ query: { query: 'ok computer', language: 'pirate-english' } },
{ filters: [{ index: 'test', meta: {} }] },
{ interval: 'eternity' },
{ hideChart: undefined },
{ sampleSize: 1 },
{ viewMode: undefined },
{ savedQuery: 'sdsd' },
{ hideAggregatedPreview: false },
{ rowHeight: 100 },
{ headerRowHeight: 1 },
{ grid: { test: 'test' } },
{ breakdownField: 'new-breakdown-field' },
];
changedParams.forEach((param) => {
expect(isEqualState(initialState, { ...initialState, ...param })).toBeFalsy();
});
});
test('allows to exclude variables from comparison', () => {
expect(
isEqualState(initialState, { ...initialState, index: undefined }, ['index'])
).toBeTruthy();
});
});
});

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

@ -22,7 +22,7 @@ import {
} from '@kbn/es-query';
import { SavedSearch, VIEW_MODE } from '@kbn/saved-search-plugin/public';
import { IKbnUrlStateStorage, ISyncStateRef, syncState } from '@kbn/kibana-utils-plugin/public';
import { isEqual } from 'lodash';
import { isEqual, omit } from 'lodash';
import { connectToQueryState, syncGlobalQueryStateWithUrl } from '@kbn/data-plugin/public';
import type { DiscoverGridSettings } from '@kbn/saved-search-plugin/common';
import type { DiscoverServices } from '../../../build_services';
@ -350,13 +350,19 @@ export function isEqualFilters(
* Helper function to compare 2 different state, is needed since comparing filters
* works differently
*/
export function isEqualState(stateA: DiscoverAppState, stateB: DiscoverAppState) {
export function isEqualState(
stateA: DiscoverAppState,
stateB: DiscoverAppState,
exclude: string[] = []
) {
if (!stateA && !stateB) {
return true;
} else if (!stateA || !stateB) {
return false;
}
const { filters: stateAFilters = [], ...stateAPartial } = stateA;
const { filters: stateBFilters = [], ...stateBPartial } = stateB;
const { filters: stateAFilters = [], ...stateAPartial } = omit(stateA, exclude);
const { filters: stateBFilters = [], ...stateBPartial } = omit(stateB, exclude);
return isEqual(stateAPartial, stateBPartial) && isEqualFilters(stateAFilters, stateBFilters);
}

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

@ -444,7 +444,6 @@ describe('Test discover state actions', () => {
"sampleSize": undefined,
"sort": Array [],
"timeRange": undefined,
"usesAdHocDataView": false,
}
`);
expect(searchSource.getField('index')?.id).toEqual('the-data-view-id');

4
src/plugins/discover/public/application/main/utils/get_state_defaults.test.ts
View file

@ -108,6 +108,7 @@ describe('getStateDefaults', () => {
},
});
expect(actualForTextBasedWithValidViewMode.viewMode).toBe(VIEW_MODE.DOCUMENT_LEVEL);
expect(actualForTextBasedWithValidViewMode.index).toBe(undefined);
const actualForWithValidViewMode = getStateDefaults({
services: discoverServiceMock,
@ -117,5 +118,8 @@ describe('getStateDefaults', () => {
},
});
expect(actualForWithValidViewMode.viewMode).toBe(VIEW_MODE.AGGREGATED_LEVEL);
expect(actualForWithValidViewMode.index).toBe(
savedSearchMock.searchSource.getField('index')?.id
);
});
});

6
src/plugins/discover/public/application/main/utils/get_state_defaults.ts
View file

@ -47,6 +47,7 @@ export function getStateDefaults({
const dataView = searchSource.getField('index');
const query = searchSource.getField('query') || data.query.queryString.getDefaultQuery();
const isTextBasedQueryMode = isTextBasedQuery(query);
const sort = getSortArray(savedSearch.sort ?? [], dataView!);
const columns = getDefaultColumns(savedSearch, uiSettings);
const chartHidden = getChartHidden(storage, 'discover');
@ -61,7 +62,7 @@ export function getStateDefaults({
)
: sort,
columns,
index: dataView?.id,
index: isTextBasedQueryMode ? undefined : dataView?.id,
interval: 'auto',
filters: cloneDeep(searchSource.getOwnField('filter')) as DiscoverAppState['filters'],
hideChart: typeof chartHidden === 'boolean' ? chartHidden : undefined,
@ -75,6 +76,7 @@ export function getStateDefaults({
grid: undefined,
breakdownField: undefined,
};
if (savedSearch.grid) {
defaultState.grid = savedSearch.grid;
}
@ -90,7 +92,7 @@ export function getStateDefaults({
if (savedSearch.viewMode) {
defaultState.viewMode = getValidViewMode({
viewMode: savedSearch.viewMode,
isTextBasedQueryMode: isTextBasedQuery(query),
isTextBasedQueryMode,
});
}
if (savedSearch.hideAggregatedPreview) {

7
src/plugins/discover/public/application/main/utils/update_saved_search.ts
View file

@ -20,6 +20,7 @@ import type { DiscoverGlobalStateContainer } from '../services/discover_global_s
* @param savedSearch
* @param dataView
* @param state
* @param globalStateContainer
* @param services
* @param useFilterAndQueryServices - when true data services are being used for updating filter + query
*/
@ -38,9 +39,11 @@ export function updateSavedSearch({
services: DiscoverServices;
useFilterAndQueryServices?: boolean;
}) {
if (dataView) {
if (dataView && savedSearch.searchSource.getField('index')?.id !== dataView.id) {
savedSearch.searchSource.setField('index', dataView);
savedSearch.usesAdHocDataView = !dataView.isPersisted();
if (!dataView.isPersisted()) {
savedSearch.usesAdHocDataView = true;
}
}
if (useFilterAndQueryServices) {
savedSearch.searchSource