From 869ceec5ca8a1156d077bb2a888a91ef73e30511 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Fri, 11 Oct 2024 20:18:11 +0200 Subject: [PATCH] [ML] Transforms: Improve data grid memoization. (#195394) ## Summary Part of #178606 and #151664. - Removes some unused code related to identifying populated index fields. - Changes `useIndexData()` to accept just one config options arg instead of individual args. - Improves data grid memoziation. Improvements tested locally: #### `many_fields` dataset (no timestamp) - `main`: `~22s` and 10 data grid rerenders until many_fields data set loaded. The transform config dropdown are hardly usable and super slow, each edit causes 3 data grid rerenders. - This PR: `~4.5s` and 7 data grid rerenders until many_fields data set loaded. The transform config dropdowns are a bit slow but usable! #### `kibana_sample_data_logs` dataset (whole dataset in the past to test rerenders on load without data) - `main`: 5 rerenders. - This PR: 3 rerenders ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --- .../ml/data_grid/components/data_grid.tsx | 4 +- .../ml/data_grid/hooks/use_data_grid.tsx | 81 +++++++----- .../public/app/__mocks__/app_dependencies.tsx | 3 + .../public/app/hooks/use_index_data.test.tsx | 102 ++++++++------- .../public/app/hooks/use_index_data.ts | 123 +++++------------- .../step_define/step_define_form.tsx | 21 ++- 6 files changed, 161 insertions(+), 173 deletions(-) diff --git a/x-pack/packages/ml/data_grid/components/data_grid.tsx b/x-pack/packages/ml/data_grid/components/data_grid.tsx index c5c16249a6a89..0ea2fea5e8229 100644 --- a/x-pack/packages/ml/data_grid/components/data_grid.tsx +++ b/x-pack/packages/ml/data_grid/components/data_grid.tsx @@ -154,7 +154,9 @@ export const DataGrid: FC = memo( const wrapperEl = useRef(null); - if (status === INDEX_STATUS.LOADED && data.length === 0) { + // `UNUSED` occurs if we were not able to identify populated fields, because + // then the query to fetch actual docs would not have triggered yet. + if ((status === INDEX_STATUS.UNUSED || status === INDEX_STATUS.LOADED) && data.length === 0) { return (
{isWithHeader(props) && } diff --git a/x-pack/packages/ml/data_grid/hooks/use_data_grid.tsx b/x-pack/packages/ml/data_grid/hooks/use_data_grid.tsx index cb575a27847f8..dc87c68cc9b7c 100644 --- a/x-pack/packages/ml/data_grid/hooks/use_data_grid.tsx +++ b/x-pack/packages/ml/data_grid/hooks/use_data_grid.tsx @@ -168,35 +168,54 @@ export const useDataGrid = ( } }, [chartsVisible, rowCount, rowCountRelation]); - return { - ccsWarning, - chartsVisible, - chartsButtonVisible: true, - columnsWithCharts, - errorMessage, - invalidSortingColumnns, - noDataMessage, - onChangeItemsPerPage, - onChangePage, - onSort, - pagination, - resetPagination, - rowCount, - rowCountRelation, - setColumnCharts, - setCcsWarning, - setErrorMessage, - setNoDataMessage, - setPagination, - setRowCountInfo, - setSortingColumns, - setStatus, - setTableItems, - setVisibleColumns, - sortingColumns, - status, - tableItems, - toggleChartVisibility, - visibleColumns, - }; + return useMemo( + () => ({ + ccsWarning, + chartsVisible, + chartsButtonVisible: true, + columnsWithCharts, + errorMessage, + invalidSortingColumnns, + noDataMessage, + onChangeItemsPerPage, + onChangePage, + onSort, + pagination, + resetPagination, + rowCount, + rowCountRelation, + setColumnCharts, + setCcsWarning, + setErrorMessage, + setNoDataMessage, + setPagination, + setRowCountInfo, + setSortingColumns, + setStatus, + setTableItems, + setVisibleColumns, + sortingColumns, + status, + tableItems, + toggleChartVisibility, + visibleColumns, + }), + // custom comparison + // eslint-disable-next-line react-hooks/exhaustive-deps + [ + ccsWarning, + chartsVisible, + columnsWithCharts, + errorMessage, + invalidSortingColumnns, + noDataMessage, + pagination, + rowCount, + rowCountRelation, + sortingColumns, + status, + tableItems, + visibleColumns, + ] + ); }; diff --git a/x-pack/plugins/transform/public/app/__mocks__/app_dependencies.tsx b/x-pack/plugins/transform/public/app/__mocks__/app_dependencies.tsx index e0536a3e1b4d6..cdb159c158d10 100644 --- a/x-pack/plugins/transform/public/app/__mocks__/app_dependencies.tsx +++ b/x-pack/plugins/transform/public/app/__mocks__/app_dependencies.tsx @@ -66,6 +66,9 @@ dataStart.search.search = jest.fn(({ params }: IKibanaSearchRequest) => { }); }) as ISearchGeneric; +// Replace mock to support tests for `use_index_data`. +coreSetup.http.post = jest.fn().mockResolvedValue([]); + const appDependencies: AppDependencies = { analytics: coreStart.analytics, application: coreStart.application, diff --git a/x-pack/plugins/transform/public/app/hooks/use_index_data.test.tsx b/x-pack/plugins/transform/public/app/hooks/use_index_data.test.tsx index ee7e2ca1e7256..37e8096f355ed 100644 --- a/x-pack/plugins/transform/public/app/hooks/use_index_data.test.tsx +++ b/x-pack/plugins/transform/public/app/hooks/use_index_data.test.tsx @@ -12,7 +12,7 @@ import { renderHook } from '@testing-library/react-hooks'; import { __IntlProvider as IntlProvider } from '@kbn/i18n-react'; import type { CoreSetup } from '@kbn/core/public'; -import { DataGrid, type UseIndexDataReturnType } from '@kbn/ml-data-grid'; +import { DataGrid, type UseIndexDataReturnType, INDEX_STATUS } from '@kbn/ml-data-grid'; import type { RuntimeMappings } from '@kbn/ml-runtime-field-utils'; import type { SimpleQuery } from '@kbn/ml-query-utils'; @@ -40,7 +40,7 @@ const runtimeMappings: RuntimeMappings = { const queryClient = new QueryClient(); describe('Transform: useIndexData()', () => { - test('dataView set triggers loading', async () => { + test('empty populatedFields does not trigger loading', async () => { const wrapper: FC> = ({ children }) => ( {children} @@ -49,15 +49,16 @@ describe('Transform: useIndexData()', () => { const { result, waitForNextUpdate } = renderHook( () => - useIndexData( - { + useIndexData({ + dataView: { id: 'the-id', getIndexPattern: () => 'the-index-pattern', fields: [], } as unknown as SearchItems['dataView'], query, - runtimeMappings - ), + combinedRuntimeMappings: runtimeMappings, + populatedFields: [], + }), { wrapper } ); @@ -66,60 +67,71 @@ describe('Transform: useIndexData()', () => { await waitForNextUpdate(); expect(IndexObj.errorMessage).toBe(''); - expect(IndexObj.status).toBe(1); + expect(IndexObj.status).toBe(INDEX_STATUS.UNUSED); expect(IndexObj.tableItems).toEqual([]); }); -}); -describe('Transform: with useIndexData()', () => { - test('Minimal initialization, no cross cluster search warning.', async () => { - // Arrange - const dataView = { - getIndexPattern: () => 'the-data-view-index-pattern', - fields: [] as any[], - } as SearchItems['dataView']; - - const Wrapper = () => { - const props = { - ...useIndexData(dataView, { match_all: {} }, runtimeMappings), - copyToClipboard: 'the-copy-to-clipboard-code', - copyToClipboardDescription: 'the-copy-to-clipboard-description', - dataTestSubj: 'the-data-test-subj', - title: 'the-index-preview-title', - toastNotifications: {} as CoreSetup['notifications']['toasts'], - }; - - return ; - }; - - render( + test('dataView set triggers loading', async () => { + const wrapper: FC> = ({ children }) => ( - - - + {children} ); - // Act - // Assert - await waitFor(() => { - expect(screen.queryByText('the-index-preview-title')).toBeInTheDocument(); - expect( - screen.queryByText('Cross-cluster search returned no fields data.') - ).not.toBeInTheDocument(); - }); + const { result, waitForNextUpdate } = renderHook( + () => + useIndexData({ + dataView: { + id: 'the-id', + getIndexPattern: () => 'the-index-pattern', + metaFields: [], + // minimal mock of DataView fields (array with getByName method) + fields: new (class DataViewFields extends Array<{ name: string }> { + getByName(id: string) { + return this.find((d) => d.name === id); + } + })( + { + name: 'the-populated-field', + }, + { + name: 'the-unpopulated-field', + } + ), + } as unknown as SearchItems['dataView'], + query, + combinedRuntimeMappings: runtimeMappings, + populatedFields: ['the-populated-field'], + }), + { wrapper } + ); + + const IndexObj: UseIndexDataReturnType = result.current; + + await waitForNextUpdate(); + + expect(IndexObj.errorMessage).toBe(''); + expect(IndexObj.status).toBe(INDEX_STATUS.LOADING); + expect(IndexObj.tableItems).toEqual([]); }); +}); - test('Cross-cluster search warning', async () => { +describe('Transform: with useIndexData()', () => { + test('Minimal initialization, no cross cluster search warning.', async () => { // Arrange const dataView = { - getIndexPattern: () => 'remote:the-index-pattern-title', + getIndexPattern: () => 'the-data-view-index-pattern', fields: [] as any[], } as SearchItems['dataView']; const Wrapper = () => { const props = { - ...useIndexData(dataView, { match_all: {} }, runtimeMappings), + ...useIndexData({ + dataView, + query: { match_all: {} }, + combinedRuntimeMappings: runtimeMappings, + populatedFields: ['the-populated-field'], + }), copyToClipboard: 'the-copy-to-clipboard-code', copyToClipboardDescription: 'the-copy-to-clipboard-description', dataTestSubj: 'the-data-test-subj', @@ -144,7 +156,7 @@ describe('Transform: with useIndexData()', () => { expect(screen.queryByText('the-index-preview-title')).toBeInTheDocument(); expect( screen.queryByText('Cross-cluster search returned no fields data.') - ).toBeInTheDocument(); + ).not.toBeInTheDocument(); }); }); }); diff --git a/x-pack/plugins/transform/public/app/hooks/use_index_data.ts b/x-pack/plugins/transform/public/app/hooks/use_index_data.ts index d93bb89eb927a..5732a0a747c93 100644 --- a/x-pack/plugins/transform/public/app/hooks/use_index_data.ts +++ b/x-pack/plugins/transform/public/app/hooks/use_index_data.ts @@ -43,13 +43,16 @@ import type { SearchItems } from './use_search_items'; import { useGetHistogramsForFields } from './use_get_histograms_for_fields'; import { useDataSearch } from './use_data_search'; -export const useIndexData = ( - dataView: SearchItems['dataView'], - query: TransformConfigQuery, - combinedRuntimeMappings?: StepDefineExposedState['runtimeMappings'], - timeRangeMs?: TimeRangeMs, - populatedFields?: string[] -): UseIndexDataReturnType => { +interface UseIndexDataOptions { + dataView: SearchItems['dataView']; + query: TransformConfigQuery; + populatedFields: string[]; + combinedRuntimeMappings?: StepDefineExposedState['runtimeMappings']; + timeRangeMs?: TimeRangeMs; +} + +export const useIndexData = (options: UseIndexDataOptions): UseIndexDataReturnType => { + const { dataView, query, populatedFields, combinedRuntimeMappings, timeRangeMs } = options; const { analytics } = useAppDependencies(); // Store the performance metric's start time using a ref @@ -59,11 +62,10 @@ export const useIndexData = ( const indexPattern = useMemo(() => dataView.getIndexPattern(), [dataView]); const toastNotifications = useToastNotifications(); - const baseFilterCriteria = buildBaseFilterCriteria( - dataView.timeFieldName, - timeRangeMs?.from, - timeRangeMs?.to, - query + const baseFilterCriteria = useMemo( + () => + buildBaseFilterCriteria(dataView.timeFieldName, timeRangeMs?.from, timeRangeMs?.to, query), + [dataView.timeFieldName, query, timeRangeMs] ); const defaultQuery = useMemo( @@ -71,85 +73,23 @@ export const useIndexData = ( [baseFilterCriteria, dataView, timeRangeMs] ); - const queryWithBaseFilterCriteria = { - bool: { - filter: baseFilterCriteria, - }, - }; - - // Fetch 500 random documents to determine populated fields. - // This is a workaround to avoid passing potentially thousands of unpopulated fields - // (for example, as part of filebeat/metricbeat/ECS based indices) - // to the data grid component which would significantly slow down the page. - const { - error: dataViewFieldsError, - data: dataViewFieldsData, - isError: dataViewFieldsIsError, - isLoading: dataViewFieldsIsLoading, - } = useDataSearch( - { - index: indexPattern, - body: { - fields: ['*'], - _source: false, - query: { - function_score: { - query: defaultQuery, - random_score: {}, - }, - }, - size: 500, + const queryWithBaseFilterCriteria = useMemo( + () => ({ + bool: { + filter: baseFilterCriteria, }, - }, - // Check whether fetching should be enabled - // If populatedFields are not provided, make own request to calculate - !Array.isArray(populatedFields) && - !(dataView.timeFieldName !== undefined && timeRangeMs === undefined) + }), + [baseFilterCriteria] ); - useEffect(() => { - if (dataViewFieldsIsLoading && !dataViewFieldsIsError) { - setErrorMessage(''); - setStatus(INDEX_STATUS.LOADING); - } else if (dataViewFieldsError !== null) { - setErrorMessage(getErrorMessage(dataViewFieldsError)); - setStatus(INDEX_STATUS.ERROR); - } else if ( - !dataViewFieldsIsLoading && - !dataViewFieldsIsError && - dataViewFieldsData !== undefined - ) { - const isCrossClusterSearch = indexPattern.includes(':'); - const isMissingFields = dataViewFieldsData.hits.hits.every( - (d) => typeof d.fields === 'undefined' - ); - - setCcsWarning(isCrossClusterSearch && isMissingFields); - setStatus(INDEX_STATUS.LOADED); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [dataViewFieldsData, dataViewFieldsError, dataViewFieldsIsError, dataViewFieldsIsLoading]); - const dataViewFields = useMemo(() => { - let allPopulatedFields = Array.isArray(populatedFields) ? populatedFields : []; - - if (populatedFields === undefined && dataViewFieldsData) { - // Get all field names for each returned doc and flatten it - // to a list of unique field names used across all docs. - const docs = dataViewFieldsData.hits.hits.map((d) => getProcessedFields(d.fields ?? {})); - allPopulatedFields = [...new Set(docs.map(Object.keys).flat(1))]; - } - + const allPopulatedFields = Array.isArray(populatedFields) ? populatedFields : []; const allDataViewFields = getFieldsFromKibanaDataView(dataView); return allPopulatedFields.filter((d) => allDataViewFields.includes(d)).sort(); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [dataViewFieldsData, populatedFields]); + }, [populatedFields]); const columns: EuiDataGridColumn[] = useMemo(() => { - if (typeof dataViewFields === 'undefined') { - return []; - } - let result: Array<{ id: string; schema: string | undefined }> = []; // Get the the runtime fields that are defined from API field and index patterns @@ -206,7 +146,9 @@ export const useIndexData = ( error: dataGridDataError, data: dataGridData, isError: dataGridDataIsError, - isLoading: dataGridDataIsLoading, + // React Query v4 has the weird behavior that `isLoading` is `true` on load + // when a query is disabled, that's why we need to use `isFetching` here. + isFetching: dataGridDataIsLoading, } = useDataSearch( { index: indexPattern, @@ -223,7 +165,7 @@ export const useIndexData = ( }, }, // Check whether fetching should be enabled - dataViewFields !== undefined + dataViewFields.length > 0 ); useEffect(() => { @@ -309,7 +251,7 @@ export const useIndexData = ( if ( dataGrid.status === INDEX_STATUS.LOADED && - dataViewFields !== undefined && + dataViewFields.length > 0 && Array.isArray(histogramsForFieldsData) && histogramsForFieldsData.length > 0 && loadIndexDataStartTime.current !== undefined @@ -325,8 +267,11 @@ export const useIndexData = ( }); } - return { - ...dataGrid, - renderCellValue, - }; + return useMemo( + () => ({ + ...dataGrid, + renderCellValue, + }), + [dataGrid, renderCellValue] + ); }; diff --git a/x-pack/plugins/transform/public/app/sections/create_transform/components/step_define/step_define_form.tsx b/x-pack/plugins/transform/public/app/sections/create_transform/components/step_define/step_define_form.tsx index 84ca627904a51..bcee389d1d91b 100644 --- a/x-pack/plugins/transform/public/app/sections/create_transform/components/step_define/step_define_form.tsx +++ b/x-pack/plugins/transform/public/app/sections/create_transform/components/step_define/step_define_form.tsx @@ -126,16 +126,23 @@ export const StepDefineForm: FC = React.memo((props) => { const { runtimeMappings } = stepDefineForm.runtimeMappingsEditor.state; const fieldStatsContext = useFieldStatsFlyoutContext(); + + const populatedFields = useMemo( + () => + isPopulatedFields(fieldStatsContext?.populatedFields) + ? [...fieldStatsContext.populatedFields] + : [], + [fieldStatsContext?.populatedFields] + ); + const indexPreviewProps = { - ...useIndexData( + ...useIndexData({ dataView, - transformConfigQuery, - runtimeMappings, + query: transformConfigQuery, + populatedFields, + combinedRuntimeMappings: runtimeMappings, timeRangeMs, - isPopulatedFields(fieldStatsContext?.populatedFields) - ? [...fieldStatsContext.populatedFields] - : [] - ), + }), dataTestSubj: 'transformIndexPreview', toastNotifications, };