[Discover] Add internalState loading function preventing data view id change related race conditions #199419

kertal · 2024-11-08T07:22:17Z

Summary

There's an occasional race condition, when an adhoc data view is changed, its id is also regenerated, technically we create a new data replacing the previous one. Therefore we guarantee each ad hoc data view id is uniqe when its spec changed. Otherwise we can't prevent duplicates. Lens, being used by the UnifiedHistogram, occasionally didn't know about the new id, having a list of data views with just the old id, and this led to the following error:

This is caused by a race condition I assumed it would be fixed in #196114. But it started to fail again. So this PR makes should make sure, it can't fail again.

It's introducing a new variable isLoading on Discover's internal state, replacing the isLoading on discover_main_route. While the previous implementation was just available in discover_main_route, now other parts of Discover main can set Discover to it's loading state on the main route level. This cleans up any local UI state, so Discover is loaded in a fresh state which is beneficial for complex operation like the described id change of an underlying data structure like the data view.

Ideally we should aim to fix the race condition, but this area of code should be focused to be refacored soon, and having a way to switch Discover to its intial loading state can for sure be useful for other complex changes, that's why I considered to move forward here

Fixes #184600

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…data view changes

- this deserves to be a first level function - we can exchange underlying implementation if needed

kertal · 2024-11-08T07:22:31Z

/ci

kibanamachine · 2024-11-08T09:15:47Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7353

[✅] test/functional/apps/discover/group3/config.ts: 25/25 tests passed.

see run history

elasticmachine · 2024-11-11T09:04:49Z

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

jughosta · 2024-11-12T13:47:12Z

src/plugins/discover/public/application/main/discover_main_route.tsx

+        stateContainer={stateContainer}
+        hideNavMenuItems={showNoDataPage || loading}
+      />
+      {loading && !showNoDataPage ? <LoadingIndicator /> : mainContent}


Should not we continue using loadingIndicator instead of <LoadingIndicator />?

yes, this makes sense, it appears there were more changes, than I noticed since I started to fix this in another PR 👍

jughosta · 2024-11-12T13:48:26Z

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

  const onDataViewEdited = async (editedDataView: DataView) => {
+    setIsLoading(true);


I wonder if isDataViewLoading should be toggled too.

For me it appears we just use isDataViewLoading in

kibana/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

Line 169 in 8545b9c

useInternalStateSelector((state) => state.isDataViewLoading) && !isEsqlMode;

and

kibana/src/plugins/discover/public/application/main/components/layout/discover_layout.tsx

Lines 104 to 107 in 7369442

const [dataView, dataViewLoading] = useInternalStateSelector((state) => [

state.dataView!,

state.isDataViewLoading,

]);

We set it to true when a data view is being changes.

With the new isLoading prop those components should not be displayed. So I'd say it's not necessary ... one think I wonder, would it make sense to combing all those loading states into one prop? one that can be used to get information about general loading state, data view loading state, data loading state ... just load thinking

Yeah, I was also wondering if we could use isDataViewLoading instead of introducing a new isLoading.
Editing a data view might be considered as "loading" its new state.

So I splitted this work (again):
Hopefully near term solution: #199982
Experimental loading refactoring, moving to one loading state instead of isDataViewLoading and isLoading: #199956

jughosta · 2024-11-12T13:55:50Z

test/functional/apps/discover/group3/_lens_vis.ts

@@ -110,8 +110,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
    return seriesType;
  }

-  // FLAKY: https://github.com/elastic/kibana/issues/184600
-  describe.skip('discover lens vis', function () {
+  describe('discover lens vis', function () {


I reproduced the issue on this branch too:

We might need to load data views right after a new one is created and before it gets replaces in the app state/URL:

kibana/src/plugins/discover/public/application/main/state_management/discover_state.ts

Line 334 in 8545b9c

});

there's some really dark magic here, that even when investigating this caused my IDE to freeze, so just before the last meeting of my day, I'm thinking that the cleanup of adhoc data views that were changed because of a spec change, should be triggered later on:

kibana/src/plugins/discover/public/application/main/state_management/discover_state.ts

Line 430 in 7f1503e

services.dataViews.clearInstanceCache(edited);

because then the previous data view would still be available, wouldn't do any harm, and be removed after the data fetching was triggered

clearly broken here, looking for a simpler solution, so far I couldn't this seems to resolve this issue locally, let's see what flaky test runner says: #199982

elasticmachine · 2024-11-12T17:22:33Z

💚 Build Succeeded

Buildkite Build
Commit: fc4ddd2

Metrics [docs]

Async chunks

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

id	before	after	diff
`discover`	806.2KB	806.6KB	+434.0B

History

💚 Build #249858 succeeded 82ebd2e
💛 Build #249450 was flaky 0cb4cf1

cc @kertal

elasticmachine · 2024-11-13T07:57:45Z

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

Click to trigger kibana-pull-request for this PR!
Click to trigger kibana-deploy-project-from-pr for this PR!

kertal · 2024-11-13T15:59:08Z

Closing this for a different approach

kertal added 7 commits October 4, 2024 09:21

Attempt to fix a flaky test

bf323b8

Add missing await when building data view list

036538a

Add a new isLoading internal state that is set to true when a ad hoc …

7d4e3da

…data view changes

Attempt to fix flakiness

d574963

Add statContainer setIsLoading function

7726c7d

- this deserves to be a first level function - we can exchange underlying implementation if needed

Add statContainer setIsLoading function

4cfc4f3

- this deserves to be a first level function - we can exchange underlying implementation if needed

Add internalState loading function

0cb4cf1

kertal self-assigned this Nov 8, 2024

kertal added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Nov 8, 2024

kertal added 2 commits November 8, 2024 16:19

remove unrelated code changes

4f9d4ab

Simplify code

7f1503e

kertal changed the title ~~[Discover] Add internalState loading function~~ [Discover] Add internalState loading function preventing data view id change related race conditions Nov 8, 2024

kertal added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Nov 8, 2024

Merge branch 'main' into discover-unskip-_lens_vis·ts

82ebd2e

kertal marked this pull request as ready for review November 11, 2024 09:04

kertal requested a review from a team as a code owner November 11, 2024 09:04

kertal mentioned this pull request Nov 11, 2024

[Discover] Make use of stateContainer action onDataViewEdited #199588

Merged

jughosta reviewed Nov 12, 2024

View reviewed changes

Merge branch 'main' into discover-unskip-_lens_vis·ts

fc4ddd2

kertal marked this pull request as draft November 13, 2024 07:57

Address review feedback

3f77801

kertal closed this Nov 13, 2024

kertal deleted the discover-unskip-_lens_vis·ts branch November 13, 2024 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Add internalState loading function preventing data view id change related race conditions #199419

[Discover] Add internalState loading function preventing data view id change related race conditions #199419

kertal commented Nov 8, 2024 •

edited

Loading

kertal commented Nov 8, 2024

kibanamachine commented Nov 8, 2024

elasticmachine commented Nov 11, 2024

jughosta Nov 12, 2024

kertal Nov 12, 2024

jughosta Nov 12, 2024

kertal Nov 12, 2024

jughosta Nov 12, 2024

kertal Nov 13, 2024

jughosta Nov 12, 2024

jughosta Nov 12, 2024 •

edited

Loading

kertal Nov 12, 2024

kertal Nov 13, 2024

elasticmachine commented Nov 12, 2024

elasticmachine commented Nov 13, 2024

kertal commented Nov 13, 2024

		const onDataViewEdited = async (editedDataView: DataView) => {
		setIsLoading(true);

	const [dataView, dataViewLoading] = useInternalStateSelector((state) => [
	state.dataView!,
	state.isDataViewLoading,
	]);

[Discover] Add internalState loading function preventing data view id change related race conditions #199419

[Discover] Add internalState loading function preventing data view id change related race conditions #199419

Conversation

kertal commented Nov 8, 2024 • edited Loading

Summary

Checklist

kertal commented Nov 8, 2024

kibanamachine commented Nov 8, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7353

elasticmachine commented Nov 11, 2024

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

jughosta Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 12, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

History

elasticmachine commented Nov 13, 2024

kertal commented Nov 13, 2024

kertal commented Nov 8, 2024 •

edited

Loading

jughosta Nov 12, 2024 •

edited

Loading