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

kertal · 2024-10-14T11:33:21Z

Summary

A simpler approach than #194904, solving the root cause of the flakiness. We had a race condition based on observable messaging of our data state, that led to the following screenshot:

It was happening because the loading state of totalHits$ was set by the histogram in use_discover_histogram.ts.
By setting the loading state in the fetchAll function, which is executed before the hook, the loading state is set on a higher level. This can no longer lead to a state that the overall data fetching is set to loading, while the histogram is still set to complete while already getting new properties like a new data view id, but not having access to a refreshed list of data views. Overall this seems to be the cleaner approach.

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

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed

…al-hits

kertal · 2024-10-14T11:33:29Z

/ci

kibanamachine · 2024-10-14T14:25:18Z

Flaky Test Runner Stats

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

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

see run history

kertal · 2024-10-14T14:40:22Z

/ci

kibanamachine · 2024-10-14T15:56:41Z

Flaky Test Runner Stats

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

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

see run history

kertal · 2024-10-15T03:54:53Z

/ci

elasticmachine · 2024-10-15T06:42:21Z

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

kertal · 2024-10-15T06:49:14Z

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

+  // dataSubjects.main$.subscribe((msg) => addLog('dataSubjects.main$', msg));
+  // dataSubjects.documents$.subscribe((msg) => addLog('dataSubjects.documents$', msg));
+  // dataSubjects.totalHits$.subscribe((msg) => addLog('dataSubjects.totalHits$', msg););
+  // Add window.ELASTIC_DISCOVER_LOGGER = 'debug' to see messages in console


uncommenting the code can be very helpful to unterstand data fetching observables race conditions, I think it's worth keeping it here

This could be helpful to debug at least a part of the staff you mention in #165192 (comment) @jughosta

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

jughosta · 2024-10-15T07:27:26Z

src/plugins/discover/public/application/main/data_fetching/fetch_all.ts

@@ -98,6 +98,8 @@ export function fetchAll(
      sendLoadingMsg(dataSubjects.totalHits$, {
        result: dataSubjects.totalHits$.getValue().result,
      });
+    } else {
+      sendLoadingMsg(dataSubjects.totalHits$);


Maybe we should keep the previous result too?

I'm not sure about that, because I think if I send a loading, the previous result is not longer valid? So if we rely on a previous result somewhere we might change this

I'm also not sure it's needed, but we used to do it when setting the loading state in use_discover_histogtam too, so probably best to keep passing result for now. It would get rid of this conditional too (and the comment above doesn't apply anymore anyway).

You're both right, let's simplify the code ... done

jughosta · 2024-10-15T07:32:47Z

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

+
+        // Do not sync the loading state since it's already handled by the saved search
+        if (fetchStatus !== FetchStatus.LOADING) {
+          savedSearchHits$.next({


I think there was a good reason why the histogram was sending the loading status all this time. But I don't remember all the cases about that. Do you mind if we wait for @davismcphee review?

yes, let's wait, from what I've seen, it doesn't hurt keeping it, but on the other hand, I thought it's redundant

As far as I can tell, as long as we're setting the loading state every time infetchAll, I think this should be ok.

…/use_discover_histogram.ts Co-authored-by: Julia Rechkunova <[email protected]>

kibanamachine · 2024-10-15T08:54:31Z

Flaky Test Runner Stats

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

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

see run history

elasticmachine · 2024-10-18T08:07:23Z

💚 Build Succeeded

Buildkite Build
Commit: 4b6480f

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`	824.9KB	824.9KB	+2.0B

History

💛 Build #242950 was flaky 62b1fdd
💚 Build #242566 succeeded b50e996
💛 Build #242431 was flaky 931aa13
💚 Build #242412 succeeded 8b4ae35
💔 Build #242218 failed b36d6a1
💔 Build #242104 failed b777be0

cc @kertal

davismcphee

Code changes look good, and it worked well when testing in both data view and ES|QL mode! Just one thing around passing result I think we should update before merging.

davismcphee · 2024-10-19T03:46:13Z

src/plugins/discover/public/application/main/data_fetching/fetch_all.ts

@@ -98,6 +98,8 @@ export function fetchAll(
      sendLoadingMsg(dataSubjects.totalHits$, {
        result: dataSubjects.totalHits$.getValue().result,
      });
+    } else {
+      sendLoadingMsg(dataSubjects.totalHits$);


I'm also not sure it's needed, but we used to do it when setting the loading state in use_discover_histogtam too, so probably best to keep passing result for now. It would get rid of this conditional too (and the comment above doesn't apply anymore anyway).

davismcphee · 2024-10-19T03:57:45Z

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

+
+        // Do not sync the loading state since it's already handled by the saved search
+        if (fetchStatus !== FetchStatus.LOADING) {
+          savedSearchHits$.next({


As far as I can tell, as long as we're setting the loading state every time infetchAll, I think this should be ok.

kertal · 2024-10-21T08:03:55Z

@elasticmachine merge upstream

davismcphee

LGTM, thanks for fixing this!

kibanamachine · 2024-10-22T06:06:56Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11454245367

…nditions (elastic#196114) Fix loading state management in `use_discover_histogram.ts` Moving the loading state for `totalHits$` to the `fetchAll` function, which is executed before the hook. This ensures that the loading state is set at a higher level, preventing a situation where the overall data fetching is in a `loading` state, but the histogram is marked as `complete` while receiving new properties (like a new data view ID) without access to refreshed data views. (cherry picked from commit 722a913)

kibanamachine · 2024-10-22T06:11:35Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ace conditions (#196114) (#197166) # Backport This will backport the following commits from `main` to `8.x`: - [[Discover] Refactor totalHits$ loading state handling to omit race conditions (#196114)](#196114)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Matthias Wilhelm <[email protected]>

kertal added 4 commits October 14, 2024 12:20

Refactor discover totalHits code

3f08047

Cleanup code

ce1ab00

Fix loading state sync

5f6b405

Merge remote-tracking branch 'upstream/main' into discover-change-tot…

b777be0

…al-hits

Unskip test

b36d6a1

Fix Jest test

8b4ae35

Merge branch 'main' into discover-change-total-hits

9795323

kertal self-assigned this Oct 15, 2024

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. release_note:skip Skip the PR/issue when compiling release notes labels Oct 15, 2024

kertal marked this pull request as ready for review October 15, 2024 06:42

kertal requested a review from a team as a code owner October 15, 2024 06:42

kertal commented Oct 15, 2024

View reviewed changes

kertal changed the title ~~Discover change total hits~~ [Discover] Refactor totalHits$ loading state handling to omit race conditions Oct 15, 2024

kertal mentioned this pull request Oct 15, 2024

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

Closed

1 task

jughosta reviewed Oct 15, 2024

View reviewed changes

Update src/plugins/discover/public/application/main/components/layout…

931aa13

…/use_discover_histogram.ts Co-authored-by: Julia Rechkunova <[email protected]>

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

kertal added 3 commits October 15, 2024 13:55

Refactor code

b50e996

Merge branch 'main' into discover-change-total-hits

62b1fdd

Merge branch 'main' into discover-change-total-hits

4b6480f

davismcphee reviewed Oct 19, 2024

View reviewed changes

Simplify sending of loading message

2eef183

kertal requested review from jughosta and davismcphee October 21, 2024 06:29

Merge branch 'main' into discover-change-total-hits

be3ae5f

davismcphee approved these changes Oct 21, 2024

View reviewed changes

Merge branch 'main' into discover-change-total-hits

91c587f

kertal enabled auto-merge (squash) October 22, 2024 04:26

kertal merged commit 722a913 into elastic:main Oct 22, 2024
24 checks passed

kibanamachine added the v9.0.0 label Oct 22, 2024

kibanamachine mentioned this pull request Oct 22, 2024

[8.x] [Discover] Refactor totalHits$ loading state handling to omit race conditions (#196114) #197166

Merged

kertal deleted the discover-change-total-hits branch October 28, 2024 15:01

kertal mentioned this pull request Nov 8, 2024

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

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kertal commented Oct 14, 2024 •

edited by kibanamachine

Loading

kertal commented Oct 14, 2024

kibanamachine commented Oct 14, 2024

kertal commented Oct 14, 2024

kibanamachine commented Oct 14, 2024

kertal commented Oct 15, 2024

elasticmachine commented Oct 15, 2024

kertal Oct 15, 2024

kertal Oct 15, 2024

jughosta Oct 15, 2024

kertal Oct 15, 2024

davismcphee Oct 19, 2024

kertal Oct 21, 2024

jughosta Oct 15, 2024 •

edited

Loading

kertal Oct 15, 2024 •

edited

Loading

davismcphee Oct 19, 2024

kibanamachine commented Oct 15, 2024

elasticmachine commented Oct 18, 2024

davismcphee left a comment

davismcphee Oct 19, 2024

davismcphee Oct 19, 2024

kertal commented Oct 21, 2024

davismcphee left a comment

kibanamachine commented Oct 22, 2024

kibanamachine commented Oct 22, 2024

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

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

Conversation

kertal commented Oct 14, 2024 • edited by kibanamachine Loading

Summary

Checklist

kertal commented Oct 14, 2024

kibanamachine commented Oct 14, 2024

Flaky Test Runner Stats

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

kertal commented Oct 14, 2024

kibanamachine commented Oct 14, 2024

Flaky Test Runner Stats

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

kertal commented Oct 15, 2024

elasticmachine commented Oct 15, 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

jughosta Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

kertal Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Oct 15, 2024

Flaky Test Runner Stats

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

elasticmachine commented Oct 18, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

History

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

kertal commented Oct 21, 2024

davismcphee left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 22, 2024

kibanamachine commented Oct 22, 2024

💚 All backports created successfully

Questions ?

kertal commented Oct 14, 2024 •

edited by kibanamachine

Loading

jughosta Oct 15, 2024 •

edited

Loading

kertal Oct 15, 2024 •

edited

Loading