Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Embeddable] Avoid rerendering loop due to search context reload #203150

Merged
merged 17 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down
54 changes: 24 additions & 30 deletions x-pack/plugins/lens/public/react_embeddable/data_loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -142,8 +150,6 @@ export function loadEmbeddableData(

const currentState = getState();

const { searchSessionId, ...unifiedSearch } = unifiedSearch$.getValue();

const getExecutionContext = () => {
const parentContext = getParentContext(parentApi);
const lastState = getState();
Expand Down Expand Up @@ -198,7 +204,7 @@ export function loadEmbeddableData(

const searchContext = getMergedSearchContext(
currentState,
unifiedSearch,
getSearchContext(parentApi),
api.timeRange$,
parentApi,
services
Expand All @@ -216,7 +222,7 @@ export function loadEmbeddableData(
},
renderMode: getRenderMode(parentApi),
services,
searchSessionId,
searchSessionId: api.searchSessionId$.getValue(),
abortController: internalApi.expressionAbortController$.getValue(),
getExecutionContext,
logError: getLogError(getExecutionContext),
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export function LensRenderer({
filters,
timeRange,
disabledActions,
searchSessionId,
hidePanelTitles,
...props
}: LensRendererProps) {
Expand All @@ -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
Expand Down Expand Up @@ -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$,
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/lens/public/react_embeddable/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ export function useDatePicker({
(newDateRange: TimeRange) => {
setUrlState({ dateRange: newDateRange });
setParsedDateRange(parseDateRange(newDateRange));
updateSearchSessionId();
},
[setUrlState]
[setUrlState, updateSearchSessionId]
);

const onRefresh = useCallback(
Expand All @@ -62,12 +63,10 @@ export function useDatePicker({
if (autoRefreshEnabled) {
autoRefreshTick$.next(null);
} else {
updateSearchSessionId();
crespocarlos marked this conversation as resolved.
Show resolved Hide resolved
setDateRange(newDateRange);
}

setDateRange(newDateRange);
},
[autoRefreshEnabled, autoRefreshTick$, setDateRange, updateSearchSessionId]
[autoRefreshEnabled, autoRefreshTick$, setDateRange]
);

const setAutoRefresh = useCallback(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ export const useUnifiedSearch = () => {

const {
data: {
query: { filterManager: filterManagerService, queryString: queryStringService },
query: {
filterManager: filterManagerService,
queryString: queryStringService,
timefilter: timeFilterService,
},
},
telemetry,
} = services;
Expand All @@ -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(
Expand All @@ -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(() => {
Expand Down Expand Up @@ -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$()
Expand All @@ -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(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested following scenarios, LGTM!

Error in filter query Error in equation
image image

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.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you find any example for showing this message, or did you just keep the previous implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for now I've kept the same implementation. I did not have all the legacy knowledge of why the 2 errors check, so I've just fixed what I've seen not working for now.

}
);
}
}
});
}, [chartLoading, attributes]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Expand Down
Loading
Loading