[Discover] Refactor app state functionality to getDiscoverAppStateContainer #149389

kertal · 2023-01-24T10:17:04Z

Summary

Migrating more code changes from #140765.

extracting AppState related code of discover_state.ts to discover_app_state_container.ts
extracting functions of use_discover_state.ts to separate files adding tests
adding a utility function to conditionally enable console.logs in the code which make it much easier to debug complex changes of state, without that I would have gone mad when doing [Discover] Create enhanced central state container #140765. I think its a valuable add-on, we could also use it e.g. when debugging cloud deployments, where we usually can't add additional code to debug
- By entering window.ELASTIC_DISCOVER_LOGGER=true in the browser's console, logging messages by (addLog) are displayed in the console like this:
  It was also helpful to temporarily add it in the root plugin of Discover for functional testing.
- It was inspired by the Lens Redux logger window.ELASTIC_LENS_LOGGER=true

Testing

Nothing should change, nothing should break

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support

kertal · 2023-01-25T00:18:40Z

@elasticmachine merge upstream

kertal · 2023-01-25T06:10:09Z

@elasticmachine merge upstream

elasticmachine · 2023-01-27T10:34:36Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

…ctor-app-state

kertal · 2023-01-30T07:31:10Z

@elasticmachine merge upstream

kertal · 2023-01-31T09:04:20Z

@elasticmachine merge upstream

…e' into discover-state-refactor-app-state

…ctor-app-state

src/plugins/discover/public/utils/add_log.ts

kertal · 2023-02-01T15:20:43Z

@elasticmachine merge upstream

davismcphee

Left a few comments and suggestions, but overall lots of good refactoring here. Didn't get a chance to test locally but will continue my review tomorrow.

src/plugins/discover/public/utils/add_log.ts

src/plugins/discover/public/application/main/services/discover_app_state_container.ts

davismcphee · 2023-02-02T04:30:14Z

src/plugins/discover/public/application/main/services/discover_state.ts

+    );
+    return { fallback: !nextDataViewData.stateValFound, dataView: nextDataView };
+  };
+
  return {


At some point I think it would make sense to get rid of some of these methods that seem to just call the equivalent methods on appStateContainer since we expose appState directly anyway. For example, I'm not sure methods like resetInitialAppState and getPreviousAppState are necessary, and the duplication might just cause confusion. It might also solve the circular import issue between discover_app_state_container and discover_state.

yes, this should happen soon, nice cleanup potential for sure!

davismcphee · 2023-02-02T04:36:22Z

src/plugins/discover/public/application/main/services/discover_state.ts

      start();
      return stop;
    },
-    setAppState: (newPartial: AppState) => setState(appStateContainerModified, newPartial),
+    setAppState: (newPartial: AppState) => setState(appStateContainer, newPartial),


In what situations would we want to call setAppState instead of appStateContainer.update? They seem to do the same thing except appStateContainer.update also sets the previous state. Related to the comment above, but I feel like this could get confusing, and just calling appStateContainer.update would probably be preferred.

yes, another potential cleanup, I would target this when moving more functions away from the main state container

src/plugins/discover/public/application/main/services/discover_app_state_container.ts

Apply findings when chatting with Matt

kertal · 2023-02-07T07:01:06Z

@elasticmachine merge upstream

kertal · 2023-02-07T13:14:48Z

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

+    const prevState = stateContainer.appState.getPrevious();
+    if (isEqualState(prevState, nextState)) {
+      addLog('[appstate] subscribe update ignored due to no changes');
+      return;
+    }


FYI @mattkime this could help likely fix troubles with too much loading state triggers
FYI @davismcphee I've been adding this after you last review, it was part of the POC and would have been migrated next up, but it already makes sense

src/plugins/discover/public/application/main/services/discover_state.ts

davismcphee

Finished local testing and a final round of code review, and everything seems to be working from what I can tell! 🎉

I left a couple suggestions that should help cleanup some imports and remove a circular dependency, but I'm also fine if we make those changes in a followup PR if preferred.

Unfortunately I did notice a significant performance degradation when switching data views, although it's not clear to me why this is happening. It seems the time it takes to switch has gone from ~150-200ms to ~500-750ms and is fairly noticeable as a user. See the videos below for examples with logging.

Before:

fast.mov

After:

slow.mov

With that said, since this isn't going in before FF and we have the whole minor to figure it out, I'm fine with merging this as is and fixing the performance issue in a followup PR if you'd prefer, so I'll approve and leave it up to you. Overall there's lots of great changes here and it definitely helps clean up Discover's state management.

davismcphee · 2023-02-08T02:03:03Z

src/plugins/discover/public/application/main/services/discover_state.ts

@@ -165,7 +150,7 @@ export interface DiscoverStateContainer {
  };
 }

-const APP_STATE_URL_KEY = '_a';
+export const APP_STATE_URL_KEY = '_a';


I thought I left a comment on this in my last review, but I can't find it anywhere, so maybe I'm just imagining things 😅. It seems like this is only used in discover_app_state_container and nowhere else, so it might make sense to move it there instead of exporting from here.

sure thing! done

davismcphee · 2023-02-08T02:10:00Z

src/plugins/discover/public/application/main/services/discover_state.ts

 import { DiscoverServices } from '../../../build_services';
-import { handleSourceColumnState } from '../../../utils/state_helpers';
-import { cleanupUrlState } from '../utils/cleanup_url_state';
-import { getValidFilters } from '../../../utils/get_valid_filters';

 export interface AppStateUrl extends Omit<AppState, 'sort'> {


It looks like AppStateUrl also isn't used in discover_state, and it might make sense to move it to discover_app_state_container instead. If we moved AppStateUrl and APP_STATE_URL_KEY to discover_app_state_container, it would have no imports left from discover_state and would fix the circular dependency between discover_app_state_container and discover_state.

good point! thx! done!

kertal · 2023-02-08T07:15:25Z

With that said, since this isn't going in before FF and we have the whole minor to figure it out, I'm fine with merging this as is and fixing the performance issue in a followup PR if you'd prefer, so I'll approve and leave it up to you. Overall there's lots of great changes here and it definitely helps clean up Discover's state management.

Well having a whole minor of bad performance dreams 😄 ... let's have a closer look at it, my basic first testing doesn't give bad results, so I'm interested how exactly you did measure? thx

davismcphee · 2023-02-08T19:38:50Z

Well having a whole minor of bad performance dreams 😄 ... let's have a closer look at it, my basic first testing doesn't give bad results, so I'm interested how exactly you did measure? thx

Makes sense 😄. I tested by updating use_discover_state with console.time calls.

I updated onChangeDataView like this:

/**
 * Function triggered when user changes data view in the sidebar
 */
const onChangeDataView = useCallback(
  async (id: string) => {
    console.time('onChangeDataView');
    await changeDataView(id, { services, discoverState: stateContainer, setUrlTracking });
    setExpandedDoc(undefined);
  },
  [services, setExpandedDoc, setUrlTracking, stateContainer]
);

And updated the effect that watches the data view like this:

/**
 * Trigger data fetching on dataView or savedSearch changes
 */
useEffect(() => {
  if (dataView && initialFetchStatus === FetchStatus.LOADING) {
    console.timeEnd("onChangeDataView");
    refetch$.next(undefined);
  }
}, [initialFetchStatus, refetch$, dataView, savedSearch.id]);

In my testing it seems to be consistently slow across browsers (Chrome, Edge, Firefox, and Safari).

kertal · 2023-02-13T09:51:05Z

@elasticmachine merge upstream

…rformance

kertal · 2023-02-13T13:28:07Z

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

      stateContainer.dataState.reset();
+      stateContainer.actions.setDataView(nextDataView);


@davismcphee good performance catch! I've exchanged the order of resetting the data state and the setting of the next data view. and so the performance is like before, it was one of the changes I've suggested to ease the pain of #149294 , and it seems the re-rendering it triggered caused the performance issue (when you had just a single row, it didn't matter, but with many rows, et voila!, bad)

FYI @mattkime one of the changes that need to be reverted in #149294, but better once this is merged, and all testing is done

kibana-ci · 2023-02-13T14:20:26Z

💚 Build Succeeded

Buildkite Build
Commit: 3fd9295

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	432	435	+3

Async chunks

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

id	before	after	diff
`discover`	392.0KB	391.7KB	-250.0B

Page load bundle

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

id	before	after	diff
`discover`	28.1KB	28.1KB	-37.0B

Unknown metric groups

async chunk count

id	before	after	diff
`discover`	10	9	-1

ESLint disabled line counts

id	before	after	diff
`discover`	38	39	+1

References to deprecated APIs

id	before	after	diff
`discover`	43	41	-2

Total ESLint disabled count

id	before	after	diff
`discover`	39	40	+1

History

💛 Build #107505 was flaky cd9f8a1
💚 Build #106340 succeeded 523f4f5
💔 Build #106338 failed 9570d0f
💔 Build #106336 failed ce2c3dc
💚 Build #105955 succeeded acf7404
💚 Build #105420 succeeded bcfaf18

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

cc @kertal

davismcphee

Pulled and tested locally again, and no more performance issue 🙌 Thanks for looking into that, LGTM!

Refactor app state functionality to getDiscoverAppStateContainer

1cef169

kertal added Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jan 24, 2023

kertal self-assigned this Jan 24, 2023

Fix context tests

8864eaa

Merge branch 'main' into discover-state-refactor-app-state

0c5ffdc

kibanamachine and others added 5 commits January 25, 2023 01:10

Merge branch 'main' into discover-state-refactor-app-state

c0531ec

Refactor functions to utils

36c2c6f

Add unit tests

e4fc3ab

Improve unit tests

a94358d

Simplify logger

0caca3a

kertal added chore release_note:skip Skip the PR/issue when compiling release notes labels Jan 27, 2023

kertal marked this pull request as ready for review January 27, 2023 10:34

kertal requested a review from a team as a code owner January 27, 2023 10:34

kertal added 2 commits January 27, 2023 11:34

Merge branch 'main' into discover-state-refactor-app-state

677a767

Merge remote-tracking branch 'upstream/main' into discover-state-refa…

f7b38ca

…ctor-app-state

Merge branch 'main' into discover-state-refactor-app-state

5966072

kibanamachine and others added 4 commits January 31, 2023 04:04

Merge branch 'main' into discover-state-refactor-app-state

a6a5a92

Improve logging

c0530b2

Merge remote-tracking branch 'origin/discover-state-refactor-app-stat…

3eba48a

…e' into discover-state-refactor-app-state

Merge remote-tracking branch 'upstream/main' into discover-state-refa…

23b878d

…ctor-app-state

kertal commented Feb 1, 2023

View reviewed changes

src/plugins/discover/public/utils/add_log.ts Outdated Show resolved Hide resolved

Remove redundant line of code

8f71045

Merge branch 'main' into discover-state-refactor-app-state

d9d1358

davismcphee reviewed Feb 2, 2023

View reviewed changes

kertal added 3 commits February 2, 2023 23:19

Apply review suggestions

dc396ef

Apply findings when chatting with Matt

Fix test

9987ede

Cleanup

bcfaf18

Merge branch 'main' into discover-state-refactor-app-state

acf7404

kertal requested a review from davismcphee February 7, 2023 07:01

kertal commented Feb 7, 2023

View reviewed changes

src/plugins/discover/public/application/main/services/discover_state.ts Show resolved Hide resolved

davismcphee approved these changes Feb 8, 2023

View reviewed changes

kertal added 3 commits February 8, 2023 06:44

Address review feedback

ce2c3dc

Fix test

9570d0f

Fix another test

523f4f5

kibanamachine and others added 2 commits February 13, 2023 04:51

Merge branch 'main' into discover-state-refactor-app-state

cd9f8a1

Exchange order dataState reset and setting of data view for better pe…

3fd9295

…rformance

kertal commented Feb 13, 2023

View reviewed changes

davismcphee approved these changes Feb 14, 2023

View reviewed changes

kertal merged commit 288054d into elastic:main Feb 14, 2023

kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Refactor app state functionality to getDiscoverAppStateContainer #149389

[Discover] Refactor app state functionality to getDiscoverAppStateContainer #149389

kertal commented Jan 24, 2023 •

edited

Loading

kertal commented Jan 25, 2023

kertal commented Jan 25, 2023

elasticmachine commented Jan 27, 2023

kertal commented Jan 30, 2023

kertal commented Jan 31, 2023

kertal commented Feb 1, 2023

davismcphee left a comment

davismcphee Feb 2, 2023

kertal Feb 2, 2023

davismcphee Feb 2, 2023

kertal Feb 2, 2023

kertal commented Feb 7, 2023

kertal Feb 7, 2023

davismcphee left a comment

davismcphee Feb 8, 2023 •

edited

Loading

kertal Feb 8, 2023

davismcphee Feb 8, 2023

kertal Feb 8, 2023

kertal commented Feb 8, 2023

davismcphee commented Feb 8, 2023

kertal commented Feb 13, 2023

kertal Feb 13, 2023

kertal Feb 13, 2023

kibana-ci commented Feb 13, 2023

async chunk count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

davismcphee left a comment

		stateContainer.dataState.reset();
		stateContainer.actions.setDataView(nextDataView);

[Discover] Refactor app state functionality to getDiscoverAppStateContainer #149389

[Discover] Refactor app state functionality to getDiscoverAppStateContainer #149389

Conversation

kertal commented Jan 24, 2023 • edited Loading

Summary

Testing

Checklist

kertal commented Jan 25, 2023

kertal commented Jan 25, 2023

elasticmachine commented Jan 27, 2023

kertal commented Jan 30, 2023

kertal commented Jan 31, 2023

kertal commented Feb 1, 2023

davismcphee left a 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

kertal commented Feb 7, 2023

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Feb 8, 2023 • edited Loading

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

kertal commented Feb 8, 2023

davismcphee commented Feb 8, 2023

kertal commented Feb 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Feb 13, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

davismcphee left a comment

Choose a reason for hiding this comment

kertal commented Jan 24, 2023 •

edited

Loading

davismcphee Feb 8, 2023 •

edited

Loading