Skip to content

Commit

Permalink
[ML] Transforms: Improve data grid memoization. (#195394)
Browse files Browse the repository at this point in the history
## 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)
  • Loading branch information
walterra authored Oct 11, 2024
1 parent d0bdbdd commit 869ceec
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 173 deletions.
4 changes: 3 additions & 1 deletion x-pack/packages/ml/data_grid/components/data_grid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,9 @@ export const DataGrid: FC<Props> = memo(

const wrapperEl = useRef<HTMLDivElement>(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 (
<div data-test-subj={`${dataTestSubj} empty`}>
{isWithHeader(props) && <DataGridTitle title={props.title} />}
Expand Down
81 changes: 50 additions & 31 deletions x-pack/packages/ml/data_grid/hooks/use_data_grid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
]
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
102 changes: 57 additions & 45 deletions x-pack/plugins/transform/public/app/hooks/use_index_data.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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<PropsWithChildren<unknown>> = ({ children }) => (
<QueryClientProvider client={queryClient}>
<IntlProvider locale="en">{children}</IntlProvider>
Expand All @@ -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 }
);

Expand All @@ -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: <DataGrid /> 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 <DataGrid {...props} />;
};

render(
test('dataView set triggers loading', async () => {
const wrapper: FC<PropsWithChildren<unknown>> = ({ children }) => (
<QueryClientProvider client={queryClient}>
<IntlProvider locale="en">
<Wrapper />
</IntlProvider>
<IntlProvider locale="en">{children}</IntlProvider>
</QueryClientProvider>
);

// 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: <DataGrid /> 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',
Expand All @@ -144,7 +156,7 @@ describe('Transform: <DataGrid /> with useIndexData()', () => {
expect(screen.queryByText('the-index-preview-title')).toBeInTheDocument();
expect(
screen.queryByText('Cross-cluster search returned no fields data.')
).toBeInTheDocument();
).not.toBeInTheDocument();
});
});
});
Loading

0 comments on commit 869ceec

Please sign in to comment.