[Embeddable] Avoid rerendering loop due to search context reload (#20…

…3150) ## Summary Fixes #202266 This PR fixes the underline rerendering issue at the `useSearchApi` hook level, so any embeddable component who uses this hook would benefit from the fix. Ideally the props passed to the Lens component should be memoized, but this assumption would break many integrations as the previous embeddable component did take care of filtering duplicates. To test this: * Go to `Observability > Alerts > Manage rules` and `Add Rule`, pick the `Custom threshold` option and verify the infinite reload does not happen Once fixed this, another problem bubbled up with the brushing use case: when brushing a chart the chart was always one time range step behind. The other bug was due to the `fetch$(api)` function propagating a stale `data` search context who the Lens embeddable was relying on. To solve this other problem the following changes have been applied: * read the `searchSessionId` from the `api` directly (used for `autoRefresh`) * make sure to test the `Refresh` feature with both relative and absolute time ranges * read the `search context` from the `parentApi` directly if implements the `unifiedSearch` API * to test this, brush and check that the final time range matches the correct time range **Note**: the fundamental issue for the latter is `fetch$` not emitting the up-to-date `data` when the parentApi search context updates. The retrieved `data` is stale and one step behind, so it is not reliable. cc @elastic/kibana-presentation As @ThomThomson noticed in his test failure investigation another issue was found in this PR due to the wrong handling of the searchSessionId within the Observability page (for more details see [his analysis](#203150 (comment))). @markov00 and @crespocarlos helped risolve this problem with some additional changes on the Observability side of things: this will lead to some extra searchSessionId to be created, which will be eventually solved by Observability team [shortly moving away from the `searchSessionId` mechanism](#203412) ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Marco Vettorello <[email protected]> Co-authored-by: Carlos Crespo <[email protected]>
elastic · Dec 11, 2024 · d4194ba · d4194ba
1 parent c1addc9
commit d4194ba
Show file tree

Hide file tree

Showing 10 changed files with 192 additions and 55 deletions.
diff --git a/packages/presentation/presentation_publishing/interfaces/fetch/publishes_unified_search.ts b/packages/presentation/presentation_publishing/interfaces/fetch/publishes_unified_search.ts
@@ -7,7 +7,15 @@
  * License v3.0 only", or the "Server Side Public License, v 1".
  */
 
-import { AggregateQuery, Filter, Query, TimeRange } from '@kbn/es-query';
+import {
+  AggregateQuery,
+  COMPARE_ALL_OPTIONS,
+  Filter,
+  Query,
+  TimeRange,
+  onlyDisabledFiltersChanged,
+} from '@kbn/es-query';
+import fastIsEqual from 'fast-deep-equal';
 import { useEffect, useMemo } from 'react';
 import { BehaviorSubject } from 'rxjs';
 import { PublishingSubject } from '../../publishing_subject';
@@ -113,15 +121,27 @@ export function useSearchApi({
   }, []);
 
   useEffect(() => {
-    searchApi.filters$.next(filters);
+    if (
+      !onlyDisabledFiltersChanged(searchApi.filters$.getValue(), filters, {
+        ...COMPARE_ALL_OPTIONS,
+        // do not compare $state to avoid refreshing when filter is pinned/unpinned (which does not impact results)
+        state: false,
+      })
+    ) {
+      searchApi.filters$.next(filters);
+    }
   }, [filters, searchApi.filters$]);
 
   useEffect(() => {
-    searchApi.query$.next(query);
+    if (!fastIsEqual(searchApi.query$.getValue(), query)) {
+      searchApi.query$.next(query);
+    }
   }, [query, searchApi.query$]);
 
   useEffect(() => {
-    searchApi.timeRange$.next(timeRange);
+    if (!fastIsEqual(searchApi.timeRange$.getValue(), timeRange)) {
+      searchApi.timeRange$.next(timeRange);
+    }
   }, [timeRange, searchApi.timeRange$]);
 
   return searchApi;

diff --git a/x-pack/plugins/lens/public/react_embeddable/data_loader.ts b/x-pack/plugins/lens/public/react_embeddable/data_loader.ts
@@ -6,8 +6,7 @@
  */
 
 import type { DefaultInspectorAdapters } from '@kbn/expressions-plugin/common';
-import { fetch$, type FetchContext } from '@kbn/presentation-publishing';
-import { apiPublishesSearchSession } from '@kbn/presentation-publishing/interfaces/fetch/publishes_search_session';
+import { apiPublishesUnifiedSearch, fetch$ } from '@kbn/presentation-publishing';
 import { type KibanaExecutionContext } from '@kbn/core/public';
 import {
   BehaviorSubject,
@@ -21,6 +20,7 @@ import {
   map,
 } from 'rxjs';
 import fastIsEqual from 'fast-deep-equal';
+import { pick } from 'lodash';
 import { getEditPath } from '../../common/constants';
 import type {
   GetStateType,
@@ -54,6 +54,24 @@ type ReloadReason =
   | 'viewMode'
   | 'searchContext';
 
+function getSearchContext(parentApi: unknown) {
+  const unifiedSearch$ = apiPublishesUnifiedSearch(parentApi)
+    ? pick(parentApi, 'filters$', 'query$', 'timeslice$', 'timeRange$')
+    : {
+        filters$: new BehaviorSubject(undefined),
+        query$: new BehaviorSubject(undefined),
+        timeslice$: new BehaviorSubject(undefined),
+        timeRange$: new BehaviorSubject(undefined),
+      };
+
+  return {
+    filters: unifiedSearch$.filters$.getValue(),
+    query: unifiedSearch$.query$.getValue(),
+    timeRange: unifiedSearch$.timeRange$.getValue(),
+    timeslice: unifiedSearch$.timeslice$?.getValue(),
+  };
+}
+
 /**
  * The function computes the expression used to render the panel and produces the necessary props
  * for the ExpressionWrapper component, binding any outer context to them.
@@ -112,16 +130,6 @@ export function loadEmbeddableData(
     }
   };
 
-  const unifiedSearch$ = new BehaviorSubject<
-    Pick<FetchContext, 'query' | 'filters' | 'timeRange' | 'timeslice' | 'searchSessionId'>
-  >({
-    query: undefined,
-    filters: undefined,
-    timeRange: undefined,
-    timeslice: undefined,
-    searchSessionId: undefined,
-  });
-
   async function reload(
     // make reload easier to debug
     sourceId: ReloadReason
@@ -142,8 +150,6 @@ export function loadEmbeddableData(
 
     const currentState = getState();
 
-    const { searchSessionId, ...unifiedSearch } = unifiedSearch$.getValue();
-
     const getExecutionContext = () => {
       const parentContext = getParentContext(parentApi);
       const lastState = getState();
@@ -198,7 +204,7 @@ export function loadEmbeddableData(
 
     const searchContext = getMergedSearchContext(
       currentState,
-      unifiedSearch,
+      getSearchContext(parentApi),
       api.timeRange$,
       parentApi,
       services
@@ -216,7 +222,7 @@ export function loadEmbeddableData(
         },
         renderMode: getRenderMode(parentApi),
         services,
-        searchSessionId,
+        searchSessionId: api.searchSessionId$.getValue(),
         abortController: internalApi.expressionAbortController$.getValue(),
         getExecutionContext,
         logError: getLogError(getExecutionContext),
@@ -259,20 +265,8 @@ export function loadEmbeddableData(
   }
 
   const mergedSubscriptions = merge(
-    // on data change from the parentApi, reload
-    fetch$(api).pipe(
-      tap((data) => {
-        const searchSessionId = apiPublishesSearchSession(parentApi) ? data.searchSessionId : '';
-        unifiedSearch$.next({
-          query: data.query,
-          filters: data.filters,
-          timeRange: data.timeRange,
-          timeslice: data.timeslice,
-          searchSessionId,
-        });
-      }),
-      map(() => 'searchContext' as ReloadReason)
-    ),
+    // on search context change, reload
+    fetch$(api).pipe(map(() => 'searchContext' as ReloadReason)),
     // On state change, reload
     // this is used to refresh the chart on inline editing
     // just make sure to avoid to rerender if there's no substantial change

diff --git a/x-pack/plugins/lens/public/react_embeddable/mocks/index.tsx b/x-pack/plugins/lens/public/react_embeddable/mocks/index.tsx
@@ -116,6 +116,7 @@ const LensApiMock: LensApi = {
   disabledActionIds: new BehaviorSubject<string[] | undefined>(undefined),
   setDisabledActionIds: jest.fn(),
   rendered$: new BehaviorSubject<boolean>(false),
+  searchSessionId$: new BehaviorSubject<string | undefined>(undefined),
 };
 
 const LensSerializedStateMock: LensSerializedState = createEmptyLensState(

diff --git a/x-pack/plugins/lens/public/react_embeddable/renderer/lens_custom_renderer_component.tsx b/x-pack/plugins/lens/public/react_embeddable/renderer/lens_custom_renderer_component.tsx
@@ -59,6 +59,7 @@ export function LensRenderer({
   filters,
   timeRange,
   disabledActions,
+  searchSessionId,
   hidePanelTitles,
   ...props
 }: LensRendererProps) {
@@ -72,6 +73,7 @@ export function LensRenderer({
   }, []);
   const disabledActionIds$ = useObservableVariable(disabledActions);
   const viewMode$ = useObservableVariable(viewMode);
+  const searchSessionId$ = useObservableVariable(searchSessionId);
   const hidePanelTitles$ = useObservableVariable(hidePanelTitles);
 
   // Lens API will be set once, but when set trigger a reflow to adopt the latest attributes
@@ -136,6 +138,7 @@ export function LensRenderer({
         ...props,
         // forward the unified search context
         ...searchApi,
+        searchSessionId$,
         disabledActionIds: disabledActionIds$,
         setDisabledActionIds: (ids: string[] | undefined) => disabledActionIds$.next(ids),
         viewMode: viewMode$,

diff --git a/x-pack/plugins/lens/public/react_embeddable/types.ts b/x-pack/plugins/lens/public/react_embeddable/types.ts
@@ -65,6 +65,7 @@ import type { AllowedPartitionOverrides } from '@kbn/expression-partition-vis-pl
 import type { AllowedXYOverrides } from '@kbn/expression-xy-plugin/common';
 import type { Action } from '@kbn/ui-actions-plugin/public';
 import { PresentationContainer } from '@kbn/presentation-containers';
+import { PublishesSearchSession } from '@kbn/presentation-publishing/interfaces/fetch/publishes_search_session';
 import type { LegacyMetricState } from '../../common';
 import type { LensDocument } from '../persistence';
 import type { LensInspector } from '../lens_inspector_service';
@@ -364,6 +365,8 @@ export type LensApi = Simplify<
     PublishesBlockingError &
     // This is used by dashboard/container to show filters/queries on the panel
     PublishesUnifiedSearch &
+    // Forward the search session id
+    PublishesSearchSession &
     // Let the container know the loading state
     PublishesDataLoading &
     // Let the container know when the rendering has completed rendering

diff --git a/...ins/observability_solution/infra/public/components/asset_details/hooks/use_date_picker.ts b/...ins/observability_solution/infra/public/components/asset_details/hooks/use_date_picker.ts
@@ -52,8 +52,9 @@ export function useDatePicker({
     (newDateRange: TimeRange) => {
       setUrlState({ dateRange: newDateRange });
       setParsedDateRange(parseDateRange(newDateRange));
+      updateSearchSessionId();
     },
-    [setUrlState]
+    [setUrlState, updateSearchSessionId]
   );
 
   const onRefresh = useCallback(
@@ -62,12 +63,10 @@ export function useDatePicker({
       if (autoRefreshEnabled) {
         autoRefreshTick$.next(null);
       } else {
-        updateSearchSessionId();
+        setDateRange(newDateRange);
       }
-
-      setDateRange(newDateRange);
     },
-    [autoRefreshEnabled, autoRefreshTick$, setDateRange, updateSearchSessionId]
+    [autoRefreshEnabled, autoRefreshTick$, setDateRange]
   );
 
   const setAutoRefresh = useCallback(

diff --git a/...ugins/observability_solution/infra/public/pages/metrics/hosts/hooks/use_unified_search.ts b/...ugins/observability_solution/infra/public/pages/metrics/hosts/hooks/use_unified_search.ts
@@ -53,7 +53,11 @@ export const useUnifiedSearch = () => {
 
   const {
     data: {
-      query: { filterManager: filterManagerService, queryString: queryStringService },
+      query: {
+        filterManager: filterManagerService,
+        queryString: queryStringService,
+        timefilter: timeFilterService,
+      },
     },
     telemetry,
   } = services;
@@ -68,29 +72,33 @@ export const useUnifiedSearch = () => {
   const onFiltersChange = useCallback(
     (filters: Filter[]) => {
       setSearch({ type: 'SET_FILTERS', filters });
+      updateSearchSessionId();
     },
-    [setSearch]
+    [setSearch, updateSearchSessionId]
   );
 
   const onPanelFiltersChange = useCallback(
     (panelFilters: Filter[]) => {
       setSearch({ type: 'SET_PANEL_FILTERS', panelFilters });
+      updateSearchSessionId();
     },
-    [setSearch]
+    [setSearch, updateSearchSessionId]
   );
 
   const onLimitChange = useCallback(
     (limit: number) => {
       setSearch({ type: 'SET_LIMIT', limit });
+      updateSearchSessionId();
     },
-    [setSearch]
+    [setSearch, updateSearchSessionId]
   );
 
   const onDateRangeChange = useCallback(
     (dateRange: StringDateRange) => {
       setSearch({ type: 'SET_DATE_RANGE', dateRange });
+      updateSearchSessionId();
     },
-    [setSearch]
+    [setSearch, updateSearchSessionId]
   );
 
   const onQueryChange = useCallback(
@@ -99,19 +107,19 @@ export const useUnifiedSearch = () => {
         setError(null);
         validateQuery(query);
         setSearch({ type: 'SET_QUERY', query });
+        updateSearchSessionId();
       } catch (err) {
         setError(err);
       }
     },
-    [validateQuery, setSearch]
+    [validateQuery, setSearch, updateSearchSessionId]
   );
 
   const onSubmit = useCallback(
     ({ dateRange }: { dateRange: TimeRange }) => {
       onDateRangeChange(dateRange);
-      updateSearchSessionId();
     },
-    [onDateRangeChange, updateSearchSessionId]
+    [onDateRangeChange]
   );
 
   const getDateRangeAsTimestamp = useCallback(() => {
@@ -168,6 +176,16 @@ export const useUnifiedSearch = () => {
         .subscribe()
     );
 
+    subscription.add(
+      timeFilterService.timefilter
+        .getTimeUpdate$()
+        .pipe(
+          map(() => timeFilterService.timefilter.getTime()),
+          tap((dateRange) => onDateRangeChange(dateRange))
+        )
+        .subscribe()
+    );
+
     subscription.add(
       queryStringService
         .getUpdates$()
@@ -181,7 +199,14 @@ export const useUnifiedSearch = () => {
     return () => {
       subscription.unsubscribe();
     };
-  }, [filterManagerService, queryStringService, onQueryChange, onFiltersChange]);
+  }, [
+    filterManagerService,
+    queryStringService,
+    onQueryChange,
+    onFiltersChange,
+    timeFilterService.timefilter,
+    onDateRangeChange,
+  ]);
 
   // Track telemetry event on query/filter/date changes
   useEffect(() => {

diff --git a/...ty_solution/observability/public/components/rule_condition_chart/rule_condition_chart.tsx b/...ty_solution/observability/public/components/rule_condition_chart/rule_condition_chart.tsx
@@ -138,19 +138,21 @@ export function RuleConditionChart({
       const errorDiv = document.querySelector('.lnsEmbeddedError');
       if (errorDiv) {
         const paragraphElements = errorDiv.querySelectorAll('p');
-        if (!paragraphElements || paragraphElements.length < 2) return;
+        if (!paragraphElements) return;
         paragraphElements[0].innerText = i18n.translate(
           'xpack.observability.ruleCondition.chart.error_equation.title',
           {
             defaultMessage: 'An error occurred while rendering the chart',
           }
         );
-        paragraphElements[1].innerText = i18n.translate(
-          'xpack.observability.ruleCondition.chart.error_equation.description',
-          {
-            defaultMessage: 'Check the rule equation.',
-          }
-        );
+        if (paragraphElements.length > 1) {
+          paragraphElements[1].innerText = i18n.translate(
+            'xpack.observability.ruleCondition.chart.error_equation.description',
+            {
+              defaultMessage: 'Check the rule equation.',
+            }
+          );
+        }
       }
     });
   }, [chartLoading, attributes]);

diff --git a/x-pack/test/observability_functional/apps/observability/index.ts b/x-pack/test/observability_functional/apps/observability/index.ts
@@ -17,6 +17,7 @@ export default function ({ loadTestFile }: FtrProviderContext) {
     loadTestFile(require.resolve('./pages/alerts/rule_stats'));
     loadTestFile(require.resolve('./pages/alerts/state_synchronization'));
     loadTestFile(require.resolve('./pages/alerts/table_storage'));
+    loadTestFile(require.resolve('./pages/alerts/custom_threshold_preview_chart'));
     loadTestFile(require.resolve('./pages/alerts/custom_threshold'));
     loadTestFile(require.resolve('./pages/cases/case_details'));
     loadTestFile(require.resolve('./pages/overview/alert_table'));