From 8c265bde68e05c9083440392d02e7e19f14f3ee3 Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Wed, 11 Dec 2024 16:03:36 +0100 Subject: [PATCH] [Embeddable] Avoid rerendering loop due to search context reload (#203150) ## 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](https://github.com/elastic/kibana/pull/203150#issuecomment-2524080129)). @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](https://github.com/elastic/kibana/issues/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 Co-authored-by: Carlos Crespo (cherry picked from commit d4194ba5eb5a72960dad00cf956d9ea6e31219e0) --- .../fetch/publishes_unified_search.ts | 28 +++++- .../public/react_embeddable/data_loader.ts | 54 +++++------ .../public/react_embeddable/mocks/index.tsx | 1 + .../lens_custom_renderer_component.tsx | 3 + .../lens/public/react_embeddable/types.ts | 3 + .../asset_details/hooks/use_date_picker.ts | 9 +- .../metrics/hosts/hooks/use_unified_search.ts | 43 +++++++-- .../rule_condition_chart.tsx | 16 ++-- .../apps/observability/index.ts | 1 + .../alerts/custom_threshold_preview_chart.ts | 89 +++++++++++++++++++ 10 files changed, 192 insertions(+), 55 deletions(-) create mode 100644 x-pack/test/observability_functional/apps/observability/pages/alerts/custom_threshold_preview_chart.ts diff --git a/packages/presentation/presentation_publishing/interfaces/fetch/publishes_unified_search.ts b/packages/presentation/presentation_publishing/interfaces/fetch/publishes_unified_search.ts index 332a47e039e01..7f455bef8e3b9 100644 --- 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'; @@ -112,15 +120,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 index 64b7ca4501b08..ac69b79b42230 100644 --- 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 - >({ - 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 index ddcd5e6089592..6b1a21c17620a 100644 --- 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(undefined), setDisabledActionIds: jest.fn(), rendered$: new BehaviorSubject(false), + searchSessionId$: new BehaviorSubject(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 index eab54a7fa9f9d..70d59fc7486b2 100644 --- 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 index c860c543570c1..1a4bf45b11f17 100644 --- 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/x-pack/plugins/observability_solution/infra/public/components/asset_details/hooks/use_date_picker.ts b/x-pack/plugins/observability_solution/infra/public/components/asset_details/hooks/use_date_picker.ts index e6b69c1ddca8f..4cb20bfc4f3d5 100644 --- a/x-pack/plugins/observability_solution/infra/public/components/asset_details/hooks/use_date_picker.ts +++ b/x-pack/plugins/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/x-pack/plugins/observability_solution/infra/public/pages/metrics/hosts/hooks/use_unified_search.ts b/x-pack/plugins/observability_solution/infra/public/pages/metrics/hosts/hooks/use_unified_search.ts index 6feefd399a829..291f95554e89c 100644 --- a/x-pack/plugins/observability_solution/infra/public/pages/metrics/hosts/hooks/use_unified_search.ts +++ b/x-pack/plugins/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/x-pack/plugins/observability_solution/observability/public/components/rule_condition_chart/rule_condition_chart.tsx b/x-pack/plugins/observability_solution/observability/public/components/rule_condition_chart/rule_condition_chart.tsx index 2a9fa2c295274..02dc7bc1f51d9 100644 --- a/x-pack/plugins/observability_solution/observability/public/components/rule_condition_chart/rule_condition_chart.tsx +++ b/x-pack/plugins/observability_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 index f061fe68649d5..dce84cee32dfa 100644 --- 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')); diff --git a/x-pack/test/observability_functional/apps/observability/pages/alerts/custom_threshold_preview_chart.ts b/x-pack/test/observability_functional/apps/observability/pages/alerts/custom_threshold_preview_chart.ts new file mode 100644 index 0000000000000..6bc0564c711c1 --- /dev/null +++ b/x-pack/test/observability_functional/apps/observability/pages/alerts/custom_threshold_preview_chart.ts @@ -0,0 +1,89 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import expect from 'expect'; +import { FtrProviderContext } from '../../../../ftr_provider_context'; + +export default ({ getService, getPageObject }: FtrProviderContext) => { + const common = getPageObject('common'); + const esArchiver = getService('esArchiver'); + const testSubjects = getService('testSubjects'); + const kibanaServer = getService('kibanaServer'); + const supertest = getService('supertest'); + const find = getService('find'); + const logger = getService('log'); + const retry = getService('retry'); + + describe('Custom threshold preview chart', () => { + const observability = getService('observability'); + const DATA_VIEW_1 = 'metricbeat-*'; + const DATA_VIEW_1_ID = 'data-view-id_1'; + const DATA_VIEW_1_NAME = 'test-data-view-name_1'; + + before(async () => { + await esArchiver.load('x-pack/test/functional/es_archives/infra/metrics_and_logs'); + await observability.alerts.common.createDataView({ + supertest, + name: DATA_VIEW_1_NAME, + id: DATA_VIEW_1_ID, + title: DATA_VIEW_1, + logger, + }); + await observability.alerts.common.navigateToRulesPage(); + }); + + after(async () => { + await esArchiver.unload('x-pack/test/functional/es_archives/infra/metrics_and_logs'); + // This also deletes the created data views + await kibanaServer.savedObjects.cleanStandardList(); + }); + + it('does render the empty chart only once at bootstrap', async () => { + await observability.alerts.rulesPage.clickCreateRuleButton(); + await observability.alerts.rulesPage.clickOnObservabilityCategory(); + await observability.alerts.rulesPage.clickOnCustomThresholdRule(); + await common.sleep(1000); + expect(await find.existsByCssSelector('[data-rendering-count="2"]')).toBe(true); + }); + + it('does render the correct error message', async () => { + await testSubjects.setValue('ruleNameInput', 'test custom threshold rule'); + + await testSubjects.click('customEquation'); + const customEquationField = await find.byCssSelector( + '[data-test-subj="thresholdRuleCustomEquationEditorFieldText"]' + ); + await customEquationField.click(); + // set an invalid equation + await customEquationField.type('A + '); + + await testSubjects.click('o11yClosablePopoverTitleButton'); + + await testSubjects.existOrFail('embeddable-lens-failure'); + const el = await find.byCssSelector('[data-test-subj="embeddable-lens-failure"] p'); + const textContent = await el.getVisibleText(); + expect(textContent).toBe('An error occurred while rendering the chart'); + }); + + it('does render the chart after fixing the error', async () => { + await testSubjects.click('customEquation'); + const customEquationField = await find.byCssSelector( + '[data-test-subj="thresholdRuleCustomEquationEditorFieldText"]' + ); + await customEquationField.click(); + // fix the equation + await customEquationField.type('A'); + await testSubjects.click('o11yClosablePopoverTitleButton'); + + // check no error is visible + await testSubjects.missingOrFail('embeddable-lens-failure'); + await retry.waitFor('Chart rendered correctly', () => { + return find.existsByCssSelector('[data-render-complete="true"]'); + }); + }); + }); +};