[Discover] Drop the separate total hits request in ES|QL mode. Use th…

…e number of fetched records instead. (#178711) - Addresses a part of #177156 ## Summary This PR changes how the total hits counter is reported in Discover for ES|QL mode. The previous logic could be misleading as the received documents count sometimes was not the same as the shown total hits counter. Often it's because for ES|QL queries without sorting, separate requests might return different result. Before: Number of received docs in Discover was considered only as "partial" result for total hits. The final value was coming from UnifiedHistogram: - if chart is visible and histogram => total hits are based on the result of the histogram fetch request - if chart is visible and it's not a histogram but based on discover fetched records => total hits are calculated from the number of records - if chart is hidden => total hits are based on the results of the separate request for getting the count only After: Number of received docs in Discover is considered as "complete" result for total hits. Changes for UnifiedHistogram: - if chart is visible and histogram => logic stays but the histogram total hits count is ignored on Discover side - if chart is visible and it's not a histogram but based on discover fetched records => logic stays but the histogram total hits count is ignored on Discover side - if chart is hidden => this request is removed completely. It's a proposal. Happy to discuss. --------- Co-authored-by: Stratoula Kalafateli <[email protected]>
elastic · Mar 25, 2024 · 6cb6c90 · 6cb6c90
1 parent f8a6324
commit 6cb6c90
Show file tree

Hide file tree

Showing 5 changed files with 52 additions and 108 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
@@ -35,6 +35,7 @@ import type { DiscoverStateContainer } from '../../services/discover_state';
 import { addLog } from '../../../../utils/add_log';
 import { useInternalStateSelector } from '../../services/discover_internal_state_container';
 import type { DiscoverAppState } from '../../services/discover_app_state_container';
+import { RecordRawType } from '../../services/discover_data_state_container';
 
 export interface UseDiscoverHistogramProps {
   stateContainer: DiscoverStateContainer;
@@ -148,14 +149,19 @@ export const useDiscoverHistogram = ({
   useEffect(() => {
     const subscription = createTotalHitsObservable(unifiedHistogram?.state$)?.subscribe(
       ({ status, result }) => {
+        const { recordRawType, result: totalHitsResult } = savedSearchData$.totalHits$.getValue();
+
+        if (recordRawType === RecordRawType.PLAIN) {
+          // ignore histogram's total hits updates for text-based records as Discover manages them during docs fetching
+          return;
+        }
+
         if (result instanceof Error) {
           // Set totalHits$ to an error state
           setTotalHitsError(result);
           return;
         }
 
-        const { recordRawType, result: totalHitsResult } = savedSearchData$.totalHits$.getValue();
-
         if (
           (status === UnifiedHistogramFetchStatus.loading ||
             status === UnifiedHistogramFetchStatus.uninitialized) &&

diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts
@@ -71,7 +71,7 @@ export function fetchAll(
     const query = getAppState().query;
     const prevQuery = dataSubjects.documents$.getValue().query;
     const recordRawType = getRawRecordType(query);
-    const useTextbased = recordRawType === RecordRawType.PLAIN;
+    const useTextBased = recordRawType === RecordRawType.PLAIN;
     if (reset) {
       sendResetMsg(dataSubjects, initialFetchStatus, recordRawType);
     }
@@ -92,18 +92,18 @@ export function fetchAll(
     // histogram will send `loading` for totalHits$
 
     // Start fetching all required requests
-    const response =
-      useTextbased && query
-        ? fetchTextBased(
-            query,
-            dataView,
-            data,
-            services.expressions,
-            inspectorAdapters,
-            abortController.signal
-          )
-        : fetchDocuments(searchSource, fetchDeps);
-    const fetchType = useTextbased && query ? 'fetchTextBased' : 'fetchDocuments';
+    const shouldFetchTextBased = useTextBased && !!query;
+    const response = shouldFetchTextBased
+      ? fetchTextBased(
+          query,
+          dataView,
+          data,
+          services.expressions,
+          inspectorAdapters,
+          abortController.signal
+        )
+      : fetchDocuments(searchSource, fetchDeps);
+    const fetchType = shouldFetchTextBased ? 'fetchTextBased' : 'fetchDocuments';
     const startTime = window.performance.now();
     // Handle results of the individual queries and forward the results to the corresponding dataSubjects
     response
@@ -117,16 +117,24 @@ export function fetchAll(
           });
         }
 
-        const currentTotalHits = dataSubjects.totalHits$.getValue();
-        // If the total hits (or chart) query is still loading, emit a partial
-        // hit count that's at least our retrieved document count
-        if (currentTotalHits.fetchStatus === FetchStatus.LOADING && !currentTotalHits.result) {
-          // trigger `partial` only for the first request (if no total hits value yet)
+        if (shouldFetchTextBased) {
           dataSubjects.totalHits$.next({
-            fetchStatus: FetchStatus.PARTIAL,
+            fetchStatus: FetchStatus.COMPLETE,
             result: records.length,
             recordRawType,
           });
+        } else {
+          const currentTotalHits = dataSubjects.totalHits$.getValue();
+          // If the total hits (or chart) query is still loading, emit a partial
+          // hit count that's at least our retrieved document count
+          if (currentTotalHits.fetchStatus === FetchStatus.LOADING && !currentTotalHits.result) {
+            // trigger `partial` only for the first request (if no total hits value yet)
+            dataSubjects.totalHits$.next({
+              fetchStatus: FetchStatus.PARTIAL,
+              result: records.length,
+              recordRawType,
+            });
+          }
         }
         /**
          * The partial state for text based query languages is necessary in case the query has changed
@@ -136,7 +144,7 @@ export function fetchAll(
          * So it takes too long, a bad user experience, also a potential flakniess in tests
          */
         const fetchStatus =
-          useTextbased && (!prevQuery || !isEqual(query, prevQuery))
+          useTextBased && (!prevQuery || !isEqual(query, prevQuery))
             ? FetchStatus.PARTIAL
             : FetchStatus.COMPLETE;
 

diff --git a/src/plugins/unified_histogram/kibana.jsonc b/src/plugins/unified_histogram/kibana.jsonc
@@ -12,7 +12,6 @@
       "dataViews",
       "embeddable",
       "inspector",
-      "expressions",
       "visualizations"
     ]
   }

diff --git a/src/plugins/unified_histogram/public/chart/hooks/use_total_hits.test.ts b/src/plugins/unified_histogram/public/chart/hooks/use_total_hits.test.ts
@@ -119,7 +119,7 @@ describe('useTotalHits', () => {
     });
   });
 
-  it('should fetch total hits if isPlainRecord is true', async () => {
+  it('should not fetch total hits if isPlainRecord is true', async () => {
     const onTotalHitsChange = jest.fn();
     const deps = {
       ...getDeps(),
@@ -130,11 +130,7 @@ describe('useTotalHits', () => {
     const { rerender } = renderHook(() => useTotalHits(deps));
     refetch$.next({ type: 'refetch' });
     rerender();
-    expect(onTotalHitsChange).toBeCalledTimes(1);
-    await waitFor(() => {
-      expect(deps.services.expressions.run).toBeCalledTimes(1);
-      expect(onTotalHitsChange).toBeCalledWith(UnifiedHistogramFetchStatus.complete, 3);
-    });
+    expect(onTotalHitsChange).not.toHaveBeenCalled();
   });
 
   it('should not fetch total hits if chartVisible is true', async () => {

diff --git a/src/plugins/unified_histogram/public/chart/hooks/use_total_hits.ts b/src/plugins/unified_histogram/public/chart/hooks/use_total_hits.ts
@@ -6,14 +6,12 @@
  * Side Public License, v 1.
  */
 
-import { textBasedQueryStateToAstWithValidation } from '@kbn/data-plugin/common';
 import { isRunningResponse } from '@kbn/data-plugin/public';
 import { DataView, DataViewType } from '@kbn/data-views-plugin/public';
 import type { AggregateQuery, Filter, Query, TimeRange } from '@kbn/es-query';
-import { Datatable, isExpressionValueError } from '@kbn/expressions-plugin/common';
 import { i18n } from '@kbn/i18n';
 import { MutableRefObject, useEffect, useRef } from 'react';
-import { catchError, filter, lastValueFrom, map, Observable, of, pluck } from 'rxjs';
+import { catchError, filter, lastValueFrom, map, Observable, of } from 'rxjs';
 import {
   UnifiedHistogramFetchStatus,
   UnifiedHistogramHitsContext,
@@ -96,6 +94,10 @@ const fetchTotalHits = async ({
   onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, result?: number | Error) => void;
   isPlainRecord?: boolean;
 }) => {
+  if (isPlainRecord) {
+    // skip, it will be handled by Discover code
+    return;
+  }
   abortController.current?.abort();
   abortController.current = undefined;
 
@@ -109,24 +111,15 @@ const fetchTotalHits = async ({
 
   abortController.current = newAbortController;
 
-  const response = isPlainRecord
-    ? await fetchTotalHitsTextBased({
-        services,
-        abortController: newAbortController,
-        dataView,
-        request,
-        query,
-        timeRange,
-      })
-    : await fetchTotalHitsSearchSource({
-        services,
-        abortController: newAbortController,
-        dataView,
-        request,
-        filters,
-        query,
-        timeRange,
-      });
+  const response = await fetchTotalHitsSearchSource({
+    services,
+    abortController: newAbortController,
+    dataView,
+    request,
+    filters,
+    query,
+    timeRange,
+  });
 
   if (!response) {
     return;
@@ -218,61 +211,3 @@ const fetchTotalHitsSearchSource = async ({
 
   return { resultStatus, result };
 };
-
-const fetchTotalHitsTextBased = async ({
-  services: { expressions },
-  abortController,
-  dataView,
-  request,
-  query,
-  timeRange,
-}: {
-  services: UnifiedHistogramServices;
-  abortController: AbortController;
-  dataView: DataView;
-  request: UnifiedHistogramRequestContext | undefined;
-  query: Query | AggregateQuery;
-  timeRange: TimeRange;
-}) => {
-  const ast = await textBasedQueryStateToAstWithValidation({
-    query,
-    time: timeRange,
-    dataView,
-  });
-
-  if (abortController.signal.aborted) {
-    return undefined;
-  }
-
-  if (!ast) {
-    return {
-      resultStatus: UnifiedHistogramFetchStatus.error,
-      result: new Error('Invalid text based query'),
-    };
-  }
-
-  const result = await lastValueFrom(
-    expressions
-      .run<null, Datatable>(ast, null, {
-        inspectorAdapters: { requests: request?.adapter },
-        searchSessionId: request?.searchSessionId,
-        executionContext: {
-          description: 'fetch total hits',
-        },
-      })
-      .pipe(pluck('result'))
-  );
-
-  if (abortController.signal.aborted) {
-    return undefined;
-  }
-
-  if (isExpressionValueError(result)) {
-    return {
-      resultStatus: UnifiedHistogramFetchStatus.error,
-      result: new Error(result.error.message),
-    };
-  }
-
-  return { resultStatus: UnifiedHistogramFetchStatus.complete, result: result.rows.length };
-};