Skip to content

Commit

Permalink
[8.x] [ML] Transforms: Improve data grid memoization. (#195394) (#195975
Browse files Browse the repository at this point in the history
)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ML] Transforms: Improve data grid memoization.
(#195394)](#195394)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Walter
Rafelsberger","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-11T18:18:11Z","message":"[ML]
Transforms: Improve data grid memoization. (#195394)\n\n##
Summary\r\n\r\nPart of #178606 and #151664.\r\n\r\n- Removes some unused
code related to identifying populated index\r\nfields.\r\n- Changes
`useIndexData()` to accept just one config options arg instead\r\nof
individual args.\r\n- Improves data grid
memoziation.\r\n\r\nImprovements tested locally:\r\n\r\n####
`many_fields` dataset (no timestamp)\r\n\r\n- `main`: `~22s` and 10 data
grid rerenders until many_fields data set\r\nloaded. The transform
config dropdown are hardly usable and super slow,\r\neach edit causes 3
data grid rerenders.\r\n- This PR: `~4.5s` and 7 data grid rerenders
until many_fields data set\r\nloaded. The transform config dropdowns are
a bit slow but usable!\r\n\r\n#### `kibana_sample_data_logs` dataset
(whole dataset in the past to\r\ntest rerenders on load without
data)\r\n\r\n- `main`: 5 rerenders.\r\n- This PR: 3 rerenders\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] This was checked for breaking
API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"869ceec5ca8a1156d077bb2a888a91ef73e30511","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":[":ml","release_note:skip","Feature:Transforms","v9.0.0","v8.16.0","backport:version"],"title":"[ML]
Transforms: Improve data grid
memoization.","number":195394,"url":"https://github.com/elastic/kibana/pull/195394","mergeCommit":{"message":"[ML]
Transforms: Improve data grid memoization. (#195394)\n\n##
Summary\r\n\r\nPart of #178606 and #151664.\r\n\r\n- Removes some unused
code related to identifying populated index\r\nfields.\r\n- Changes
`useIndexData()` to accept just one config options arg instead\r\nof
individual args.\r\n- Improves data grid
memoziation.\r\n\r\nImprovements tested locally:\r\n\r\n####
`many_fields` dataset (no timestamp)\r\n\r\n- `main`: `~22s` and 10 data
grid rerenders until many_fields data set\r\nloaded. The transform
config dropdown are hardly usable and super slow,\r\neach edit causes 3
data grid rerenders.\r\n- This PR: `~4.5s` and 7 data grid rerenders
until many_fields data set\r\nloaded. The transform config dropdowns are
a bit slow but usable!\r\n\r\n#### `kibana_sample_data_logs` dataset
(whole dataset in the past to\r\ntest rerenders on load without
data)\r\n\r\n- `main`: 5 rerenders.\r\n- This PR: 3 rerenders\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] This was checked for breaking
API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"869ceec5ca8a1156d077bb2a888a91ef73e30511"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195394","number":195394,"mergeCommit":{"message":"[ML]
Transforms: Improve data grid memoization. (#195394)\n\n##
Summary\r\n\r\nPart of #178606 and #151664.\r\n\r\n- Removes some unused
code related to identifying populated index\r\nfields.\r\n- Changes
`useIndexData()` to accept just one config options arg instead\r\nof
individual args.\r\n- Improves data grid
memoziation.\r\n\r\nImprovements tested locally:\r\n\r\n####
`many_fields` dataset (no timestamp)\r\n\r\n- `main`: `~22s` and 10 data
grid rerenders until many_fields data set\r\nloaded. The transform
config dropdown are hardly usable and super slow,\r\neach edit causes 3
data grid rerenders.\r\n- This PR: `~4.5s` and 7 data grid rerenders
until many_fields data set\r\nloaded. The transform config dropdowns are
a bit slow but usable!\r\n\r\n#### `kibana_sample_data_logs` dataset
(whole dataset in the past to\r\ntest rerenders on load without
data)\r\n\r\n- `main`: 5 rerenders.\r\n- This PR: 3 rerenders\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [x] This was checked for breaking
API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"869ceec5ca8a1156d077bb2a888a91ef73e30511"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Walter Rafelsberger <[email protected]>
  • Loading branch information
kibanamachine and walterra authored Oct 11, 2024
1 parent bf56b4e commit c9b0d86
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 @@ -67,6 +67,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 c9b0d86

Please sign in to comment.