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.

Sign up for GitHub

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

[Discover] [ES|QL] Prevent redundant requests when loading Discover sessions and toggling chart visibility #206699

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+196 −431
Open

[Discover] [ES|QL] Prevent redundant requests when loading Discover sessions and toggling chart visibility #206699

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eab1c4c
Skip initial currentSuggestionContext emission to avoid refetching on…
davismcphee Jan 15, 2025
b01b79c
Move table to getUnifiedHistogramPropsForEsql to avoid additional cur…
davismcphee Jan 15, 2025
a0d9b5b
Remove Unified Histogram auto fetching, and avoid auto fetching on mo…
davismcphee Jan 15, 2025
a6a6c63
Merge branch 'main' into fix-esql-fetches
davismcphee Jan 17, 2025
728ef56
Merge branch 'main' into fix-esql-fetches
kertal Jan 17, 2025
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
24 changes: 1 addition & 23 deletions ...s/shared/discover/public/application/main/components/layout/discover_histogram_layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,13 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import React, { useCallback, useMemo } from 'react';
import React, { useCallback } from 'react';
import { UnifiedHistogramContainer } from '@kbn/unified-histogram-plugin/public';
import { css } from '@emotion/react';
import useObservable from 'react-use/lib/useObservable';
import { ESQL_TABLE_TYPE } from '@kbn/data-plugin/common';
import type { Datatable } from '@kbn/expressions-plugin/common';
import { useDiscoverHistogram } from './use_discover_histogram';
import { type DiscoverMainContentProps, DiscoverMainContent } from './discover_main_content';
import { useAppStateSelector } from '../../state_management/discover_app_state_container';
import { FetchStatus } from '../../../types';
import { useIsEsqlMode } from '../../hooks/use_is_esql_mode';

export interface DiscoverHistogramLayoutProps extends DiscoverMainContentProps {
Expand Down Expand Up @@ -44,7 +41,6 @@ export const DiscoverHistogramLayout = ({
hideChart,
});

const datatable = useObservable(dataState.data$.documents$);
const renderCustomChartToggleActions = useCallback(
() =>
React.isValidElement(panelsToggle)
Expand All @@ -53,23 +49,6 @@ export const DiscoverHistogramLayout = ({
[panelsToggle]
);

const table: Datatable | undefined = useMemo(() => {
if (
isEsqlMode &&
datatable &&
[FetchStatus.PARTIAL, FetchStatus.COMPLETE].includes(datatable.fetchStatus)
) {
return {
type: 'datatable' as 'datatable',
rows: datatable.result!.map((r) => r.raw),
columns: datatable.esqlQueryColumns || [],
meta: {
type: ESQL_TABLE_TYPE,
},
};
}
}, [datatable, isEsqlMode]);

// Initialized when the first search has been requested or
// when in ES|QL mode since search sessions are not supported
if (!searchSessionId && !isEsqlMode) {
Expand All @@ -81,7 +60,6 @@ export const DiscoverHistogramLayout = ({
{...unifiedHistogramProps}
searchSessionId={searchSessionId}
requestAdapter={dataState.inspectorAdapters.requests}
table={table}
container={container}
css={histogramLayoutCss}
renderCustomChartToggleActions={renderCustomChartToggleActions}
Expand Down
31 changes: 4 additions & 27 deletions ...shared/discover/public/application/main/components/layout/use_discover_histogram.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ describe('useDiscoverHistogram', () => {
const { hook } = await renderUseDiscoverHistogram();
const params = hook.result.current.getCreationOptions();
expect(params?.localStorageKeyPrefix).toBe('discover');
expect(params?.disableAutoFetching).toBe(true);
expect(Object.keys(params?.initialState ?? {})).toEqual([
'chartHidden',
'timeInterval',
Expand Down Expand Up @@ -398,8 +397,8 @@ describe('useDiscoverHistogram', () => {
});
});

describe('refetching', () => {
it('should call refetch when savedSearchFetch$ is triggered', async () => {
describe('fetching', () => {
it('should call fetch when savedSearchFetch$ is triggered', async () => {
const savedSearchFetch$ = new Subject<void>();
const stateContainer = getStateContainer();
stateContainer.dataState.fetchChart$ = savedSearchFetch$;
Expand All @@ -408,33 +407,11 @@ describe('useDiscoverHistogram', () => {
act(() => {
hook.result.current.ref(api);
});
expect(api.refetch).toHaveBeenCalled();
expect(api.fetch).toHaveBeenCalled();
act(() => {
savedSearchFetch$.next();
});
expect(api.refetch).toHaveBeenCalledTimes(2);
});

it('should skip the next refetch when hideChart changes from true to false', async () => {
const savedSearchFetch$ = new Subject<void>();
const stateContainer = getStateContainer();
stateContainer.dataState.fetchChart$ = savedSearchFetch$;
const { hook, initialProps } = await renderUseDiscoverHistogram({ stateContainer });
const api = createMockUnifiedHistogramApi();
act(() => {
hook.result.current.ref(api);
});
expect(api.refetch).toHaveBeenCalled();
act(() => {
hook.rerender({ ...initialProps, hideChart: true });
});
act(() => {
hook.rerender({ ...initialProps, hideChart: false });
});
act(() => {
savedSearchFetch$.next();
});
expect(api.refetch).toHaveBeenCalledTimes(1);
expect(api.fetch).toHaveBeenCalledTimes(2);
});
});

Expand Down
59 changes: 30 additions & 29 deletions ...ugins/shared/discover/public/application/main/components/layout/use_discover_histogram.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
UnifiedHistogramVisContext,
} from '@kbn/unified-histogram-plugin/public';
import { isEqual } from 'lodash';
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
import { useCallback, useEffect, useMemo, useState } from 'react';
import {
debounceTime,
distinctUntilChanged,
Expand All @@ -28,13 +28,15 @@ import {
merge,
Observable,
pairwise,
skip,
startWith,
} from 'rxjs';
import useObservable from 'react-use/lib/useObservable';
import type { RequestAdapter } from '@kbn/inspector-plugin/common';
import type { DatatableColumn } from '@kbn/expressions-plugin/common';
import type { Datatable, DatatableColumn } from '@kbn/expressions-plugin/common';
import type { SavedSearch } from '@kbn/saved-search-plugin/common';
import { Filter } from '@kbn/es-query';
import { Filter, isOfAggregateQueryType } from '@kbn/es-query';
import { ESQL_TABLE_TYPE } from '@kbn/data-plugin/common';
import { useDiscoverCustomization } from '../../../../customizations';
import { useDiscoverServices } from '../../../../hooks/use_discover_services';
import { FetchStatus } from '../../../types';
Expand Down Expand Up @@ -240,6 +242,7 @@ export const useDiscoverHistogram = ({
dataView: esqlDataView,
query: esqlQuery,
columns: esqlColumns,
table,
} = useObservable(esqlFetchComplete$, initialEsqlProps);

useEffect(() => {
Expand All @@ -249,9 +252,7 @@ export const useDiscoverHistogram = ({
}

const fetchStart = stateContainer.dataState.fetchChart$.subscribe(() => {
if (!skipRefetch.current) {
setIsSuggestionLoading(true);
}
setIsSuggestionLoading(true);
});
const fetchComplete = esqlFetchComplete$.subscribe(() => {
setIsSuggestionLoading(false);
Expand All @@ -267,18 +268,6 @@ export const useDiscoverHistogram = ({
* Data fetching
*/

const skipRefetch = useRef<boolean>();
Copy link
Member

Choose a reason for hiding this comment

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

it's great that we can remove this logic here! 🥳

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! I've always disliked this workaround and have been wanting to remove it for a while, and now I finally had a good reason 😁


// Skip refetching when showing the chart since Lens will
// automatically fetch when the chart is shown
useEffect(() => {
if (skipRefetch.current === undefined) {
skipRefetch.current = false;
} else {
skipRefetch.current = !hideChart;
}
}, [hideChart]);

// Handle unified histogram refetching
useEffect(() => {
if (!unifiedHistogram) {
Expand All @@ -304,18 +293,14 @@ export const useDiscoverHistogram = ({
}

const subscription = fetchChart$.subscribe((source) => {
if (!skipRefetch.current) {
if (source === 'discover') addLog('Unified Histogram - Discover refetch');
if (source === 'lens') addLog('Unified Histogram - Lens suggestion refetch');
unifiedHistogram.refetch();
}

skipRefetch.current = false;
if (source === 'discover') addLog('Unified Histogram - Discover refetch');
if (source === 'lens') addLog('Unified Histogram - Lens suggestion refetch');
unifiedHistogram.fetch();
});

// triggering the initial request for total hits hook
if (!isEsqlMode && !skipRefetch.current) {
unifiedHistogram.refetch();
// triggering the initial chart request
if (!isEsqlMode) {
unifiedHistogram.fetch();
}

return () => {
Expand Down Expand Up @@ -397,6 +382,7 @@ export const useDiscoverHistogram = ({
timeRange: timeRangeMemoized,
relativeTimeRange,
columns: isEsqlMode ? esqlColumns : undefined,
table: isEsqlMode ? table : undefined,
onFilter: histogramCustomization?.onFilter,
onBrushEnd: histogramCustomization?.onBrushEnd,
withDefaultActions: histogramCustomization?.withDefaultActions,
Expand Down Expand Up @@ -495,7 +481,10 @@ const createTotalHitsObservable = (state$?: Observable<UnifiedHistogramState>) =
const createCurrentSuggestionObservable = (state$: Observable<UnifiedHistogramState>) => {
return state$.pipe(
map((state) => state.currentSuggestionContext),
distinctUntilChanged(isEqual)
distinctUntilChanged(isEqual),
// Skip the first emission since it's the
// initial state and doesn't need a refetch
skip(1)
);
};

Expand All @@ -507,11 +496,23 @@ function getUnifiedHistogramPropsForEsql({
savedSearch: SavedSearch;
}) {
const columns = documentsValue?.esqlQueryColumns || EMPTY_ESQL_COLUMNS;
const query = savedSearch.searchSource.getField('query');
const isEsqlMode = isOfAggregateQueryType(query);
const table: Datatable | undefined =
isEsqlMode && documentsValue?.result
? {
type: 'datatable',
rows: documentsValue.result.map((r) => r.raw),
columns,
meta: { type: ESQL_TABLE_TYPE },
}
: undefined;

const nextProps = {
dataView: savedSearch.searchSource.getField('index')!,
query: savedSearch.searchSource.getField('query'),
columns,
table,
};

addLog('[UnifiedHistogram] delayed next props for ES|QL', nextProps);
Expand Down
38 changes: 8 additions & 30 deletions src/platform/plugins/shared/unified_histogram/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,15 @@ It manages its own state and data fetching, and can easily be dropped into pages

```tsx
// Import the container component
import {
UnifiedHistogramContainer,
} from '@kbn/unified-histogram-plugin/public';
import { UnifiedHistogramContainer } from '@kbn/unified-histogram-plugin/public';

// Import modules required for your application
import {
useServices,
useResizeRef,
useRequestParams,
MyLayout,
MyButton,
} from './my-modules';
import { useServices, useResizeRef, useRequestParams, MyLayout, MyButton } from './my-modules';

const services = useServices();
const resizeRef = useResizeRef();
const {
dataView,
query,
filters,
timeRange,
relativeTimeRange,
searchSessionId,
requestAdapter,
} = useRequestParams();
const { dataView, query, filters, timeRange, relativeTimeRange, searchSessionId, requestAdapter } =
useRequestParams();

return (
<UnifiedHistogramContainer
Expand Down Expand Up @@ -71,7 +56,7 @@ import {
useCallbacks,
useRequestParams,
useStateParams,
useManualRefetch,
useFetch,
MyLayout,
MyButton,
} from './my-modules';
Expand All @@ -97,20 +82,13 @@ const getCreationOptions = useCallback(() => ({
// Optionally provide a local storage key prefix to save parts of the state,
// such as the chart hidden state and top panel height, to local storage
localStorageKeyPrefix: 'myApp',
// By default Unified Histogram will automatically refetch based on certain
// state changes, such as chart hidden and request params, but this can be
// disabled in favour of manual fetching if preferred. Note that an initial
// request is always triggered when first initialized, and when the chart
// changes from hidden to visible, Lens will automatically trigger a refetch
// regardless of what this property is set to
disableAutoFetching: true,
// Customize the initial state in order to override the defaults
initialState: { chartHidden, breakdownField },
}), [...]);

// Manually refetch if disableAutoFetching is true
useManualRefetch(() => {
unifiedHistogram?.refetch();
// Trigger a fetch, must be called on init to render the chart
useFetch(() => {
unifiedHistogram?.fetch();
});

// Update the Unified Histogram state when our state params change
Expand Down
4 changes: 3 additions & 1 deletion src/platform/plugins/shared/unified_histogram/public/chart/chart.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import type { ReactWrapper } from 'enzyme';
import { unifiedHistogramServicesMock } from '../__mocks__/services';
import { getLensVisMock } from '../__mocks__/lens_vis';
import { searchSourceInstanceMock } from '@kbn/data-plugin/common/search/search_source/mocks';
import { of } from 'rxjs';
import { Subject, of } from 'rxjs';
import { dataViewWithTimefieldMock } from '../__mocks__/data_view_with_timefield';
import { dataViewMock } from '../__mocks__/data_view';
import { BreakdownFieldSelector } from './breakdown_field_selector';
Expand Down Expand Up @@ -135,13 +135,15 @@ async function mountComponent({
withDefaultActions: undefined,
isChartAvailable: checkChartAvailability({ chart, dataView, isPlainRecord }),
renderCustomChartToggleActions: customToggle ? () => customToggle : undefined,
input$: new Subject(),
};

let instance: ReactWrapper = {} as ReactWrapper;
await act(async () => {
instance = mountWithIntl(<Chart {...props} />);
// wait for initial async loading to complete
await new Promise((r) => setTimeout(r, 0));
props.input$?.next({ type: 'fetch' });
instance.update();
});
return instance;
Expand Down
Loading