From 3f08047d9c5b98956ae1ac5560b8305780235dcf Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Mon, 14 Oct 2024 12:01:51 +0200 Subject: [PATCH 1/8] Refactor discover totalHits code --- .../layout/use_discover_histogram.ts | 43 ++++++++++++++----- .../main/data_fetching/fetch_all.ts | 2 + .../discover_data_state_container.ts | 6 +++ 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts index 1ebeb8d7d1b7f..f73b2d98ea7c7 100644 --- a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts +++ b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts @@ -62,6 +62,7 @@ export const useDiscoverHistogram = ({ }: UseDiscoverHistogramProps) => { const services = useDiscoverServices(); const savedSearchData$ = stateContainer.dataState.data$; + const savedSearchHits$ = savedSearchData$.totalHits$; const savedSearchState = useSavedSearch(); const isEsqlMode = useIsEsqlMode(); @@ -153,10 +154,7 @@ export const useDiscoverHistogram = ({ * Total hits */ - const setTotalHitsError = useMemo( - () => sendErrorTo(savedSearchData$.totalHits$), - [savedSearchData$.totalHits$] - ); + const setTotalHitsError = useMemo(() => sendErrorTo(savedSearchHits$), [savedSearchHits$]); useEffect(() => { const subscription = createTotalHitsObservable(unifiedHistogram?.state$)?.subscribe( @@ -172,7 +170,7 @@ export const useDiscoverHistogram = ({ return; } - const { result: totalHitsResult } = savedSearchData$.totalHits$.getValue(); + const { result: totalHitsResult } = savedSearchHits$.getValue(); if ( (status === UnifiedHistogramFetchStatus.loading || @@ -184,11 +182,34 @@ export const useDiscoverHistogram = ({ return; } - // Sync the totalHits$ observable with the unified histogram state - savedSearchData$.totalHits$.next({ - fetchStatus: status.toString() as FetchStatus, - result, - }); + const fetchStatus = status.toString() as FetchStatus; + if ( + fetchStatus === FetchStatus.COMPLETE && + savedSearchHits$.getValue().fetchStatus === FetchStatus.COMPLETE && + result !== savedSearchHits$.getValue().result + ) { + // 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 } + ); + savedSearchHits$.next({ + fetchStatus: FetchStatus.LOADING, + }); + } + + const isSavedSearchLoading = + savedSearchHits$.getValue().fetchStatus === FetchStatus.LOADING; + const isUnifiedHistogramLoading = status === UnifiedHistogramFetchStatus.loading; + + // Sync the totalHits$ observable with the unified histogram state unless both are set to loading + if (isSavedSearchLoading !== isUnifiedHistogramLoading) { + savedSearchHits$.next({ + fetchStatus, + result, + }); + } if (status !== UnifiedHistogramFetchStatus.complete || typeof result !== 'number') { return; @@ -205,7 +226,7 @@ export const useDiscoverHistogram = ({ }, [ isEsqlMode, savedSearchData$.main$, - savedSearchData$.totalHits$, + savedSearchHits$, setTotalHitsError, stateContainer.appState, unifiedHistogram?.state$, diff --git a/src/plugins/discover/public/application/main/data_fetching/fetch_all.ts b/src/plugins/discover/public/application/main/data_fetching/fetch_all.ts index 660dff3bdb4ff..63479a5a884a5 100644 --- a/src/plugins/discover/public/application/main/data_fetching/fetch_all.ts +++ b/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$); } // Start fetching all required requests diff --git a/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts b/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts index ce7f820aa3cdd..4d01c2b72d327 100644 --- a/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts +++ b/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts @@ -185,6 +185,12 @@ export function getDataStateContainer({ documents$: new BehaviorSubject(initialState), totalHits$: new BehaviorSubject(initialState), }; + // This is debugging code, helping you to understand which messages are sent to the data observables + // Adding a debugger in the functions can be helpful to understand what triggers a message + // 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 let autoRefreshDone: AutoRefreshDoneFn | undefined | null = null; /** From ce1ab0008ca3d7391c1d54d23ad320a2860fc22f Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Mon, 14 Oct 2024 12:19:58 +0200 Subject: [PATCH 2/8] Cleanup code --- .../components/layout/use_discover_histogram.ts | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts index f73b2d98ea7c7..b76180c488bca 100644 --- a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts +++ b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts @@ -183,22 +183,6 @@ export const useDiscoverHistogram = ({ } const fetchStatus = status.toString() as FetchStatus; - if ( - fetchStatus === FetchStatus.COMPLETE && - savedSearchHits$.getValue().fetchStatus === FetchStatus.COMPLETE && - result !== savedSearchHits$.getValue().result - ) { - // 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 } - ); - savedSearchHits$.next({ - fetchStatus: FetchStatus.LOADING, - }); - } - const isSavedSearchLoading = savedSearchHits$.getValue().fetchStatus === FetchStatus.LOADING; const isUnifiedHistogramLoading = status === UnifiedHistogramFetchStatus.loading; From 5f6b405fc90c1238b28d0c591ecc5114a30e9b35 Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Mon, 14 Oct 2024 12:37:38 +0200 Subject: [PATCH 3/8] Fix loading state sync --- .../main/components/layout/use_discover_histogram.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts index b76180c488bca..1d7b30e238c75 100644 --- a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts +++ b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts @@ -183,12 +183,9 @@ export const useDiscoverHistogram = ({ } const fetchStatus = status.toString() as FetchStatus; - const isSavedSearchLoading = - savedSearchHits$.getValue().fetchStatus === FetchStatus.LOADING; - const isUnifiedHistogramLoading = status === UnifiedHistogramFetchStatus.loading; - // Sync the totalHits$ observable with the unified histogram state unless both are set to loading - if (isSavedSearchLoading !== isUnifiedHistogramLoading) { + // Do not sync the loading state since it's already handled by the saved search + if (fetchStatus !== FetchStatus.LOADING) { savedSearchHits$.next({ fetchStatus, result, From b36d6a14e9f4e4e73435b6e92d60b4e2002f7794 Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Mon, 14 Oct 2024 16:37:32 +0200 Subject: [PATCH 4/8] Unskip test --- test/functional/apps/discover/group3/_lens_vis.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/functional/apps/discover/group3/_lens_vis.ts b/test/functional/apps/discover/group3/_lens_vis.ts index 1bd6f8099fd22..321486a40238e 100644 --- a/test/functional/apps/discover/group3/_lens_vis.ts +++ b/test/functional/apps/discover/group3/_lens_vis.ts @@ -110,8 +110,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { return seriesType; } - // Failing: See https://github.com/elastic/kibana/issues/184600 - describe.skip('discover lens vis', function () { + describe('discover lens vis', function () { before(async () => { await security.testUser.setRoles(['kibana_admin', 'test_logstash_reader']); await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional'); From 8b4ae3535d4ce434426909a142d98d59b30b18b3 Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Tue, 15 Oct 2024 05:54:12 +0200 Subject: [PATCH 5/8] Fix Jest test --- .../discover_data_state_container.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/plugins/discover/public/application/main/state_management/discover_data_state_container.test.ts b/src/plugins/discover/public/application/main/state_management/discover_data_state_container.test.ts index 226a48dc1aeca..1bbb16ab3c9dd 100644 --- a/src/plugins/discover/public/application/main/state_management/discover_data_state_container.test.ts +++ b/src/plugins/discover/public/application/main/state_management/discover_data_state_container.test.ts @@ -159,7 +159,6 @@ describe('test getDataStateContainer', () => { expect( stateContainer.searchSessionManager.getCurrentSearchSessionId as jest.Mock ).toHaveBeenCalled(); - unsubscribe(); done(); } @@ -169,21 +168,24 @@ describe('test getDataStateContainer', () => { }); it('should update app state from default profile state', async () => { + mockFetchDocuments.mockResolvedValue({ records: [] }); const stateContainer = getDiscoverStateMock({ isTimeBased: true }); const dataState = stateContainer.dataState; const dataUnsub = dataState.subscribe(); const appUnsub = stateContainer.appState.initAndSync(); - discoverServiceMock.profilesManager.resolveDataSourceProfile({}); + await discoverServiceMock.profilesManager.resolveDataSourceProfile({}); stateContainer.actions.setDataView(dataViewMock); stateContainer.internalState.transitions.setResetDefaultProfileState({ columns: true, rowHeight: true, }); + dataState.data$.totalHits$.next({ fetchStatus: FetchStatus.COMPLETE, result: 0, }); dataState.refetch$.next(undefined); + await waitFor(() => { expect(dataState.data$.main$.value.fetchStatus).toBe(FetchStatus.COMPLETE); }); @@ -202,7 +204,7 @@ describe('test getDataStateContainer', () => { const dataState = stateContainer.dataState; const dataUnsub = dataState.subscribe(); const appUnsub = stateContainer.appState.initAndSync(); - discoverServiceMock.profilesManager.resolveDataSourceProfile({}); + await discoverServiceMock.profilesManager.resolveDataSourceProfile({}); stateContainer.actions.setDataView(dataViewMock); stateContainer.internalState.transitions.setResetDefaultProfileState({ columns: false, From 931aa1398f60df9fa5d22c2f3f05a672226c905f Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Tue, 15 Oct 2024 09:47:54 +0200 Subject: [PATCH 6/8] Update src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts Co-authored-by: Julia Rechkunova --- .../main/components/layout/use_discover_histogram.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts index 1d7b30e238c75..f6ac292933096 100644 --- a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts +++ b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts @@ -184,7 +184,7 @@ export const useDiscoverHistogram = ({ const fetchStatus = status.toString() as FetchStatus; - // Do not sync the loading state since it's already handled by the saved search + // Do not sync the loading state since it's already handled by fetchAll if (fetchStatus !== FetchStatus.LOADING) { savedSearchHits$.next({ fetchStatus, From b50e996237f6c0822ee069f4c8e0d582c3c06a44 Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Tue, 15 Oct 2024 13:55:52 +0200 Subject: [PATCH 7/8] Refactor code --- .../components/layout/use_discover_histogram.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts index f6ac292933096..66b5be4d02ab7 100644 --- a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts +++ b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts @@ -61,8 +61,7 @@ export const useDiscoverHistogram = ({ hideChart, }: UseDiscoverHistogramProps) => { const services = useDiscoverServices(); - const savedSearchData$ = stateContainer.dataState.data$; - const savedSearchHits$ = savedSearchData$.totalHits$; + const { main$, documents$, totalHits$ } = stateContainer.dataState.data$; const savedSearchState = useSavedSearch(); const isEsqlMode = useIsEsqlMode(); @@ -154,7 +153,7 @@ export const useDiscoverHistogram = ({ * Total hits */ - const setTotalHitsError = useMemo(() => sendErrorTo(savedSearchHits$), [savedSearchHits$]); + const setTotalHitsError = useMemo(() => sendErrorTo(totalHits$), [totalHits$]); useEffect(() => { const subscription = createTotalHitsObservable(unifiedHistogram?.state$)?.subscribe( @@ -170,7 +169,7 @@ export const useDiscoverHistogram = ({ return; } - const { result: totalHitsResult } = savedSearchHits$.getValue(); + const { result: totalHitsResult } = totalHits$.getValue(); if ( (status === UnifiedHistogramFetchStatus.loading || @@ -186,7 +185,7 @@ export const useDiscoverHistogram = ({ // Do not sync the loading state since it's already handled by fetchAll if (fetchStatus !== FetchStatus.LOADING) { - savedSearchHits$.next({ + totalHits$.next({ fetchStatus, result, }); @@ -197,7 +196,7 @@ export const useDiscoverHistogram = ({ } // Check the hits count to set a partial or no results state - checkHitCount(savedSearchData$.main$, result); + checkHitCount(main$, result); } ); @@ -206,8 +205,8 @@ export const useDiscoverHistogram = ({ }; }, [ isEsqlMode, - savedSearchData$.main$, - savedSearchHits$, + main$, + totalHits$, setTotalHitsError, stateContainer.appState, unifiedHistogram?.state$, @@ -236,7 +235,7 @@ export const useDiscoverHistogram = ({ const [initialEsqlProps] = useState(() => getUnifiedHistogramPropsForEsql({ - documentsValue: savedSearchData$.documents$.getValue(), + documentsValue: documents$.getValue(), savedSearch: stateContainer.savedSearchState.getState(), }) ); From 2eef183433f2cc76bc4d241735f9521c63f1cc7e Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Mon, 21 Oct 2024 08:28:07 +0200 Subject: [PATCH 8/8] Simplify sending of loading message --- .../application/main/data_fetching/fetch_all.ts | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/plugins/discover/public/application/main/data_fetching/fetch_all.ts b/src/plugins/discover/public/application/main/data_fetching/fetch_all.ts index 63479a5a884a5..3b54e6f8ce083 100644 --- a/src/plugins/discover/public/application/main/data_fetching/fetch_all.ts +++ b/src/plugins/discover/public/application/main/data_fetching/fetch_all.ts @@ -92,15 +92,9 @@ export function fetchAll( // Mark all subjects as loading sendLoadingMsg(dataSubjects.main$); sendLoadingMsg(dataSubjects.documents$, { query }); - - // histogram for data view mode will send `loading` for totalHits$ - if (isEsqlQuery) { - sendLoadingMsg(dataSubjects.totalHits$, { - result: dataSubjects.totalHits$.getValue().result, - }); - } else { - sendLoadingMsg(dataSubjects.totalHits$); - } + sendLoadingMsg(dataSubjects.totalHits$, { + result: dataSubjects.totalHits$.getValue().result, + }); // Start fetching all required requests const response = isEsqlQuery