[ML] Transforms: Improves transform list reloading behavior. #164296

walterra · 2023-08-21T12:37:05Z

Summary

Fixes #164303 (Fix transforms in serverless projects).
Fixes #163145 (Improve transformlist reloading behavior).
Part of #153288 (Migrate snapshot tests using enzyme to use @testing-library/react).

Before:

After:

If the transform list fails to load, it does no longer show a non-refreshable full page error. Instead the error is shown in an inline callout and the refresh button is still present.
AuthorizationProvider is gone and has been replaced by a custom hook useTransformCapabilities. All client side code no longer relies on privileges being present but makes use of capabilities (like canGetTransform etc.). The custom route to fetch privileges and capabilities is also gone, instead capabilities are retrieved from Kibana's own application.capabilities.transform which we register server side.
Refactors all remote data fetching to use react-query. This gets rid of the custom code to refresh the transform list using observables, instead all CRUD actions now make use of react-query's useMutation and trigger a cache invalidation of the transform list data to initiate a refetch. The useApi hook is gone, instead we now have specific hooks for data fetching that wrap useQuery (useGetTransform, useGetTransformStats etc.) and the existing hooks for actions have been refactored to use useMutation (useStartTransforms, useStopTransforms etc.).
Toasts for "success" messages have been removed.
All tests that made use of toMatchSnapshot have been refactored away from enzyme to react-testing-lib and no longer rely on snapshots, instead we make basic assertions on the rendered components.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

kibana-ci · 2023-08-23T07:42:29Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 34998eacda209eed30e1a078b376358bcdb0707f

Failed CI Steps

FTR Configs #32

Test Failures

[job] [logs] FTR Configs #32 / machine learning basic license - permissions for user with full ML access (ft_ml_poweruser) "after all" hook for "should not allow access to the Stack Management ML page"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`transform`	365	371	+6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`transform`	404.7KB	401.5KB	-3.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`transform`	17.8KB	18.4KB	+690.0B

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`transform`	3	4	+1

ESLint disabled line counts

id	before	after	diff
`transform`	31	29	-2

Total ESLint disabled count

id	before	after	diff
`transform`	34	33	-1

History

💔 Build #152578 failed 3adfd4aacaf0f5fdbd1d7ab423536f37915faa06
💔 Build #152562 failed c1dbc1e646aaba2c084b2587ab67956b6cb46698
💛 Build #152473 was flaky 9272144b1a76a95d1ecd8feae32d93cdcca0a247
💛 Build #152407 was flaky 270b8a214670ab2469d4abc1414dc0a7bc60d94b
💔 Build #152341 failed f8efbfb03a21fc2c074777b4e80b3d8b444c9cc7

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @walterra

darnautov · 2023-08-31T08:42:56Z

x-pack/plugins/transform/public/app/components/capabilities_wrapper.tsx

+  <EuiFlexGroup justifyContent="spaceAround">
+    <EuiFlexItem grow={false}>
+      <EuiPageContent verticalPosition="center" horizontalPosition="center" color="danger">


I think you can use EuiPageTemplate.EmptyPrompt instead

Good catch, must have missed it when I merged in your PR that fixed those, updated in 9b02239.

darnautov · 2023-08-31T08:43:54Z

x-pack/plugins/transform/public/app/components/toast_notification_text.tsx

-      <EuiButtonEmpty onClick={openModal}>
+      {!inline && (
+        <>
+          <pre>{previewText}</pre>{' '}


Suggested change

<pre>{previewText}</pre>{' '}

<pre>{previewText}</pre>

Updated in a47947c.

darnautov · 2023-08-31T09:10:22Z

x-pack/plugins/transform/public/app/hooks/use_delete_transform.tsx

+  const { canDeleteIndex: userCanDeleteIndex } = useTransformCapabilities();
+
+  const userCanDeleteDataView =
+    (capabilities.savedObjectsManagement && capabilities.savedObjectsManagement.delete === true) ||


Suggested change

(capabilities.savedObjectsManagement && capabilities.savedObjectsManagement.delete === true) ||

(capabilities.savedObjectsManagement?.delete === true) ||

Updated in 4efc4bd.

darnautov · 2023-08-31T09:10:43Z

x-pack/plugins/transform/public/app/hooks/use_delete_transform.tsx

+
+  const userCanDeleteDataView =
+    (capabilities.savedObjectsManagement && capabilities.savedObjectsManagement.delete === true) ||
+    (capabilities.indexPatterns && capabilities.indexPatterns.save === true);


Suggested change

(capabilities.indexPatterns && capabilities.indexPatterns.save === true);

capabilities.indexPatterns?.save === true;

Updated in 4efc4bd.

darnautov · 2023-08-31T09:14:23Z

x-pack/plugins/transform/public/app/hooks/use_get_data_view_titles.ts

+
+  return useQuery<string[], IHttpFetchError>(
+    [TRANSFORM_REACT_QUERY_KEYS.GET_DATA_VIEW_TITLES],
+    () => data.dataViews.getTitles()


Suggested change

() => data.dataViews.getTitles()

data.dataViews.getTitles

This would give a TS error because the useQuery callback would pass on incompatible arguments.

darnautov · 2023-08-31T09:26:06Z

x-pack/plugins/transform/public/app/hooks/use_index_data.ts

+  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'
+      );

-        if (!isEsSearchResponse(resp)) {
-          setErrorMessage(getErrorMessage(resp));
-          setStatus(INDEX_STATUS.ERROR);
-          return;
-        }
-        const docs = resp.hits.hits.map((d) => getProcessedFields(d.fields ?? {}));
-        isMissingFields = resp.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]);


The entire effect looks very complex. Essentially it derives error message and status.
Can we get rid of the state and use memo instead? e.g.

const errorMessage = useMemo(() => {...}, [dataViewFieldsIsLoading, dataViewFieldsIsError]);

darnautov · 2023-08-31T09:26:34Z

x-pack/plugins/transform/public/app/hooks/use_index_data.ts

+  );
+
+  useEffect(() => {
+    if (dataGridDataIsLoading && !dataGridDataIsError) {


same as above

darnautov · 2023-08-31T09:28:44Z

x-pack/plugins/transform/public/app/hooks/use_transform_config_data.ts

+    isLoading,
+  } = useGetTransformsPreview(previewRequest, validationStatus.isValid);
+
+  useEffect(() => {


I reckon we should avoid such massive effects. It seems like it can be broken down in independent variables

darnautov · 2023-08-31T09:29:17Z

x-pack/plugins/transform/public/app/sections/clone_transform/clone_transform_section.tsx


-    const transformConfigs = await api.getTransform(transformId);
-    if (isHttpFetchError(transformConfigs)) {
+  useEffect(() => {


same about this effect

walterra · 2023-08-31T10:55:10Z

@darnautov I agree those useEffects are quite complex. The problem is the transform plugin itself doesn't own the callbacks like setErrorMessage/setStatus etc., they are retrieved from the ml-data-grid package which is shared with the ml plugin. The original complexity comes from that data grid component being shared from the ml plugin directly in times when packages were not available. I tried to avoid touching that data grid package in this PR because it would have made refactoring the ml plugin necessary too, so instead I opted for satisfying it's current API. It's definitely something we should try to simplify in follow ups.

kibana-ci · 2023-09-01T19:21:38Z

💔 Build Failed

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`transform`	379	383	+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`transform`	405.8KB	396.5KB	-9.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`transform`	17.8KB	18.0KB	+193.0B

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`transform`	3	4	+1

References to deprecated APIs

id	before	after	diff
`transform`	62	33	-29

Total ESLint disabled count

id	before	after	diff
`transform`	34	35	+1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @walterra

…164296) ## Summary - If the transform list fails to load, it does no longer show a non-refreshable full page error. Instead the error is shown in an inline callout and the refresh button is still present. - `AuthorizationProvider` is gone and has been replaced by a custom hook `useTransformCapabilities`. All client side code no longer relies on `privileges` being present but makes use of `capabilities` (like `canGetTransform` etc.). The custom route to fetch privileges and capabilities is also gone, instead capabilities are retrieved from Kibana's own `application.capabilities.transform` which we register server side. - Refactors all remote data fetching to use `react-query`. This gets rid of the custom code to refresh the transform list using observables, instead all CRUD actions now make use of `react-query`'s `useMutation` and trigger a cache invalidation of the transform list data to initiate a refetch. The `useApi` hook is gone, instead we now have specific hooks for data fetching that wrap `useQuery` (`useGetTransform`, `useGetTransformStats` etc.) and the existing hooks for actions have been refactored to use `useMutation` (`useStartTransforms`, `useStopTransforms` etc.). - Toasts for "success" messages have been removed. - All tests that made use of `toMatchSnapshot` have been refactored away from `enzyme` to `react-testing-lib` and no longer rely on snapshots, instead we make basic assertions on the rendered components. ### 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 - [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)

walterra self-assigned this Aug 21, 2023

walterra force-pushed the ml-transform-fix-transform-list-reload branch from f8efbfb to 270b8a2 Compare August 22, 2023 15:01

walterra added the release_note:enhancement label Aug 23, 2023

walterra force-pushed the ml-transform-fix-transform-list-reload branch 2 times, most recently from 8022e8f to 8f76e78 Compare August 24, 2023 09:08

walterra added 24 commits August 24, 2023 13:22

migrate transform list to use react-query

3919a8b

refactor inline useQuery to custom hook. cleanup useApi.

ec2bd1c

migrate wizard to use react-query

04ae3c0

fix i18n

2daa660

migrate cloning to use react-query

c23dc68

remove getTransformStats from useApi

a8ee0ea

migrate update to use react-query

a2978b2

migrate preview to use react-query

1f55030

migrate reauthorize to use react-query

7d5107b

migrate reset to use react-query

e7d1c0e

migrate stop to use react-query

9c47fe6

migrate schedule now to use react-query

cb904c0

migrate more to react-query

1526d62

migrate rest of useApi to react-query

8cade10

remove unnecessary type guards

0614d00

remove unnecessary type guards

5f04ab7

cleanup

c4b4d53

cleanup

be3e0fc

refactor IndexService into custom hooks using react-query

3080150

fix tests

d302856

remove success toasts

5fbaec2

fix i18n

b986a42

refactor AuthorizationProvider into useAuthorization hook.

32c12eb

fix react-query caching

27233be

walterra added 5 commits August 31, 2023 09:01

Merge branch 'main' into ml-transform-fix-transform-list-reload

d7db6af

add null checks

f25ea75

cleanup types

c6b72a6

fix passing on populated fields

ee46ca2

refactor useCreateTransform for consistency

1fa7a2e

darnautov reviewed Aug 31, 2023

View reviewed changes

walterra added 5 commits August 31, 2023 12:25

move fetching transform nodes to its own hook

493f52d

refactor EuiPageContent_Deprecated

9b02239

revert space after preview text

a47947c

tweak capabilities checks

4efc4bd

fix i18n

0a26daf

walterra added 4 commits August 31, 2023 13:16

fix handling options default

279e526

Merge branch 'main' into ml-transform-fix-transform-list-reload

534ac1f

Merge branch 'main' into ml-transform-fix-transform-list-reload

28aeedd

fix populated fields cleanup

f0e555f

walterra removed the ci:build-serverless-image label Sep 1, 2023

walterra added 5 commits September 1, 2023 11:08

Merge branch 'main' into ml-transform-fix-transform-list-reload

0a73faa

Merge branch 'main' into ml-transform-fix-transform-list-reload

6144781

Merge branch 'main' into ml-transform-fix-transform-list-reload

2091a57

Merge branch 'main' into ml-transform-fix-transform-list-reload

d10bdb9

Merge branch 'main' into ml-transform-fix-transform-list-reload

ecedbad

walterra enabled auto-merge (squash) September 1, 2023 18:31

walterra merged commit 0b705eb into elastic:main Sep 1, 2023

kibanamachine added the backport:skip This commit does not require backporting label Sep 1, 2023

walterra deleted the ml-transform-fix-transform-list-reload branch September 18, 2023 08:04

szabosteve changed the title ~~[ML] Transforms: Improve transform list reloading behavior.~~ [ML] Transforms: Improves transform list reloading behavior. Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Transforms: Improves transform list reloading behavior. #164296

[ML] Transforms: Improves transform list reloading behavior. #164296

walterra commented Aug 21, 2023 •

edited

Loading

kibana-ci commented Aug 23, 2023

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

darnautov Aug 31, 2023

walterra Aug 31, 2023

darnautov Aug 31, 2023

walterra Aug 31, 2023

darnautov Aug 31, 2023

walterra Aug 31, 2023

darnautov Aug 31, 2023

walterra Aug 31, 2023

darnautov Aug 31, 2023

walterra Aug 31, 2023

darnautov Aug 31, 2023

darnautov Aug 31, 2023

darnautov Aug 31, 2023

darnautov Aug 31, 2023

walterra commented Aug 31, 2023

kibana-ci commented Sep 1, 2023 •

edited

Loading

ESLint disabled in files

References to deprecated APIs

Total ESLint disabled count

	(capabilities.savedObjectsManagement && capabilities.savedObjectsManagement.delete === true) \|\|
	(capabilities.savedObjectsManagement?.delete === true) \|\|

	(capabilities.indexPatterns && capabilities.indexPatterns.save === true);
	capabilities.indexPatterns?.save === true;

[ML] Transforms: Improves transform list reloading behavior. #164296

[ML] Transforms: Improves transform list reloading behavior. #164296

Conversation

walterra commented Aug 21, 2023 • edited Loading

Summary

Checklist

kibana-ci commented Aug 23, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterra commented Aug 31, 2023

kibana-ci commented Sep 1, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled in files

References to deprecated APIs

Total ESLint disabled count

History

walterra commented Aug 21, 2023 •

edited

Loading

kibana-ci commented Sep 1, 2023 •

edited

Loading