Fix flaky histogram when an adhoc data view is being edited #194904

kertal · 2024-10-04T07:23:06Z

Summary

This fixes the following problem: 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:

It's a race condition caused by the facts that lens is treating data views a bit differently, having it's own variant of data view handling. I couldn't figure out, what causes this race condition. I therefore tried another approach. 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.

Overall this approach did the job removing the flakiness on one level, and introduced flakiness on another level (That high likely was already there). CI failed not having the right total hits number. This happend because totalHits$ occionally submits 2 complete states with different numbers (one the previous number, one the actual number), the UI is detecting state changes, to omit unnecessary updates, and therefore, since complete -> complete, no UI was updated.

Overall this PR is a bit like duct tape. but duct tape that's fixing things. If accepted, there should be a follow up issue investigating the histogram state changes a bit closer.

[✅] test/functional/apps/dashboard/group3/config.ts: 25/25 tests passed. kibana-flaky-test-suite-runner#7105

Fixes #184600

Checklist

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

kibanamachine · 2024-10-04T09:12:07Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7082

[❌] test/functional/apps/discover/group3/config.ts: 22/50 tests passed.

see run history

kibanamachine · 2024-10-04T09:13:50Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7083

[❌] test/functional/apps/discover/group3/config.ts: 23/50 tests passed.

see run history

…data view changes

kibanamachine · 2024-10-04T18:52:08Z

Flaky Test Runner Stats

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

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

see run history

kertal · 2024-10-07T10:11:14Z

src/plugins/discover/public/application/main/components/top_nav/discover_topnav.tsx

-    },
-    [dataViews, stateContainer.actions, stateContainer.dataState]
-  );
-
  const updateSavedQueryId = (newSavedQueryId: string | undefined) => {


We were already having the same code in our state container, time use stateContainer.actions.onDataViewEdited instead of this code

kibanamachine · 2024-10-07T12:04:16Z

Flaky Test Runner Stats

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

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

see run history

kibana-ci · 2024-10-07T15:09:56Z

💔 Build Failed

Failed CI Steps

Quick Checks

History

💔 Build #239550 failed 7d4e3da

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

cc @kertal

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

kertal · 2024-10-07T19:36:10Z

src/plugins/discover/public/application/main/components/top_nav/discover_topnav.tsx

-      stateContainer.dataState.fetch();
-    },
-    [dataViews, stateContainer.actions, stateContainer.dataState]
-  );


seems we had this code already in our state container without using it

kertal · 2024-10-07T19:38:19Z

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

      </DiscoverMainProvider>
    </DiscoverCustomizationProvider>
  );
 }
 // eslint-disable-next-line import/no-default-export
 export default DiscoverMainRoute;

+export function DiscoverMainLoading({


since the loading state now is in the stateContainer, we need to have the state container in context so it's provided, therefore this change. This could be seen as first part of refactoring this file, which could use some love

kertal · 2024-10-07T19:39:38Z

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

      setDataView(await services.dataViews.create(editedDataView.toSpec(), true));
    } else {
      await updateAdHocDataViewId();
    }
-    loadDataViewList();
+    await loadDataViewList();


I first thought, ha! got you flaky test monster, a missing await, however it didn't help, but it should be changed anyway

elasticmachine · 2024-10-07T21:02:05Z

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

kibanamachine · 2024-10-07T21:30:54Z

Flaky Test Runner Stats

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

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

see run history

kertal · 2024-10-10T15:35:29Z

src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts

+        ) {
+          // this is a workaround to make sure the new total hits value is displayed
+          // a different value without a loading state in between would lead to be ignored by useDataState
+          addLog(
+            '[UnifiedHistogram] send loading to totalHits$ to make sure the new value is displayed',
+            { status, result }
+          );
+          savedSearchData$.totalHits$.next({
+            fetchStatus: FetchStatus.LOADING,
+          });


@dej611 this needs an issue as follow up to investigate

elasticmachine · 2024-10-14T10:40:32Z

🤖 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-10-14T10:47:35Z

/ci

elasticmachine · 2024-10-14T10:57:56Z

💔 Build Failed

Buildkite Build
Commit: 6b45e1f

Failed CI Steps

Build Kibana Distribution and Plugins

History

💔 Build #241924 failed 92eb2e4
💚 Build #241410 succeeded 39264c8
💔 Build #241241 failed 6d48b5c
💔 Build #240974 failed c0a6422
💚 Build #240520 succeeded 7ea856d

cc @kertal

kertal · 2024-10-15T06:56:01Z

keeping this open, because i think having a general loading state in the state container could be useful, even if it's not needed now to fix the issue because of #196114

Attempt to fix a flaky test

bf323b8

kertal self-assigned this Oct 4, 2024

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

kertal commented Oct 7, 2024

View reviewed changes

Add statContainer setIsLoading function

7726c7d

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

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

Add statContainer setIsLoading function

4cfc4f3

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

kertal commented Oct 7, 2024

View reviewed changes

kertal marked this pull request as ready for review October 7, 2024 21:02

kertal requested a review from a team as a code owner October 7, 2024 21:02

kertal changed the title ~~Attempt to fix a flaky test~~ Fix flaky histogram when adhoc data view are being edited Oct 7, 2024

Merge branch 'main' into discover-flaky-histogram-chart

68a2478

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

Merge branch 'main' into discover-flaky-histogram-chart

89fdf5b

kertal changed the title ~~Fix flaky histogram when adhoc data view are being edited~~ Fix flaky histogram when an adhoc data view is being edited Oct 8, 2024

kertal added 2 commits October 9, 2024 07:45

Merge branch 'main' into discover-flaky-histogram-chart

7ea856d

Merge branch 'main' into discover-flaky-histogram-chart

c0a6422

Merge branch 'main' into discover-flaky-histogram-chart

6d48b5c

kertal commented Oct 10, 2024

View reviewed changes

kertal added 5 commits October 11, 2024 05:57

Merge branch 'main' into discover-flaky-histogram-chart

39264c8

Merge branch 'main' into discover-flaky-histogram-chart

92eb2e4

Fix merge

3ceb8d6

Refactor discover totalHits code

b04350b

Update use_discover_histogram.ts

6b45e1f

kertal marked this pull request as draft October 14, 2024 10:40

dej611 mentioned this pull request Oct 14, 2024

[Lens] Embeddable react refactor #186642

Merged

74 tasks

kertal mentioned this pull request Oct 15, 2024

[Discover] Refactor totalHits$ loading state handling to omit race conditions #196114

Merged

2 tasks

kertal closed this Nov 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flaky histogram when an adhoc data view is being edited #194904

Fix flaky histogram when an adhoc data view is being edited #194904

kertal commented Oct 4, 2024 •

edited

Loading

kibanamachine commented Oct 4, 2024

kibanamachine commented Oct 4, 2024

kibanamachine commented Oct 4, 2024

kertal Oct 7, 2024

kibanamachine commented Oct 7, 2024

kibana-ci commented Oct 7, 2024 •

edited

Loading

kertal Oct 7, 2024

kertal Oct 7, 2024 •

edited

Loading

kertal Oct 7, 2024

elasticmachine commented Oct 7, 2024

kibanamachine commented Oct 7, 2024

kertal Oct 10, 2024

elasticmachine commented Oct 14, 2024

kertal commented Oct 14, 2024

elasticmachine commented Oct 14, 2024 •

edited

Loading

kertal commented Oct 15, 2024

Fix flaky histogram when an adhoc data view is being edited #194904

Fix flaky histogram when an adhoc data view is being edited #194904

Conversation

kertal commented Oct 4, 2024 • edited Loading

Summary

Checklist

kibanamachine commented Oct 4, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7082

kibanamachine commented Oct 4, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7083

kibanamachine commented Oct 4, 2024

Flaky Test Runner Stats

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

kertal Oct 7, 2024

Choose a reason for hiding this comment

kibanamachine commented Oct 7, 2024

Flaky Test Runner Stats

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

kibana-ci commented Oct 7, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

History

kertal Oct 7, 2024

Choose a reason for hiding this comment

kertal Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

kertal Oct 7, 2024

Choose a reason for hiding this comment

elasticmachine commented Oct 7, 2024

kibanamachine commented Oct 7, 2024

Flaky Test Runner Stats

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

kertal Oct 10, 2024

Choose a reason for hiding this comment

elasticmachine commented Oct 14, 2024

kertal commented Oct 14, 2024

elasticmachine commented Oct 14, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

History

kertal commented Oct 15, 2024

kertal commented Oct 4, 2024 •

edited

Loading

kibana-ci commented Oct 7, 2024 •

edited

Loading

kertal Oct 7, 2024 •

edited

Loading

elasticmachine commented Oct 14, 2024 •

edited

Loading