Skip to content

Commit

Permalink
Remove Unified Histogram auto fetching, and avoid auto fetching on mo…
Browse files Browse the repository at this point in the history
…unt, reducing redundant requests
  • Loading branch information
davismcphee committed Jan 15, 2025
1 parent b01b79c commit a0d9b5b
Show file tree
Hide file tree
Showing 21 changed files with 174 additions and 406 deletions.
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
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 Down Expand Up @@ -252,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 @@ -270,18 +268,6 @@ export const useDiscoverHistogram = ({
* Data fetching
*/

const skipRefetch = useRef<boolean>();

// 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 @@ -307,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
38 changes: 8 additions & 30 deletions src/platform/plugins/shared/unified_histogram/README.md
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import { useTotalHits } from './hooks/use_total_hits';
import { useChartStyles } from './hooks/use_chart_styles';
import { useChartActions } from './hooks/use_chart_actions';
import { ChartConfigPanel } from './chart_config_panel';
import { useRefetch } from './hooks/use_refetch';
import { useFetch } from './hooks/use_fetch';
import { useEditVisualization } from './hooks/use_edit_visualization';
import { LensVisService } from '../services/lens_vis_service';
import type { UseRequestParamsResult } from '../hooks/use_request_params';
Expand All @@ -64,7 +64,6 @@ export interface ChartProps {
breakdown?: UnifiedHistogramBreakdownContext;
renderCustomChartToggleActions?: () => ReactElement | undefined;
appendHistogram?: ReactElement;
disableAutoFetching?: boolean;
disableTriggers?: LensEmbeddableInput['disableTriggers'];
disabledActions?: LensEmbeddableInput['disabledActions'];
input$?: UnifiedHistogramInput$;
Expand Down Expand Up @@ -99,7 +98,6 @@ export function Chart({
isPlainRecord,
renderCustomChartToggleActions,
appendHistogram,
disableAutoFetching,
disableTriggers,
disabledActions,
input$: originalInput$,
Expand Down Expand Up @@ -140,20 +138,9 @@ export function Chart({

const { filters, query, getTimeRange, updateTimeRange, relativeTimeRange } = requestParams;

const refetch$ = useRefetch({
dataView,
request,
hits,
chart,
chartVisible,
breakdown,
filters,
query,
relativeTimeRange,
currentSuggestion,
disableAutoFetching,
const fetch$ = useFetch({
input$,
beforeRefetch: updateTimeRange,
beforeFetch: updateTimeRange,
});

useTotalHits({
Expand All @@ -165,7 +152,7 @@ export function Chart({
filters,
query,
getTimeRange,
refetch$,
fetch$,
onTotalHitsChange,
isPlainRecord,
});
Expand Down Expand Up @@ -364,7 +351,7 @@ export function Chart({
hits={hits}
chart={chart}
getTimeRange={getTimeRange}
refetch$={refetch$}
fetch$={fetch$}
visContext={visContext}
isPlainRecord={isPlainRecord}
disableTriggers={disableTriggers}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ async function mountComponent(isPlainRecord = false, hasLensSuggestions = false)
};

const timefilterUpdateHandler = jest.fn();
const refetch$: UnifiedHistogramInput$ = new Subject();
const fetch$: UnifiedHistogramInput$ = new Subject();
const props = {
services: unifiedHistogramServicesMock,
request: {
Expand All @@ -73,17 +73,20 @@ async function mountComponent(isPlainRecord = false, hasLensSuggestions = false)
from: '2020-05-14T11:05:13.590',
to: '2020-05-14T11:20:13.590',
}),
refetch$,
fetch$,
visContext: (await getMockLensAttributes())!,
onTotalHitsChange: jest.fn(),
onChartLoad: jest.fn(),
withDefaultActions: undefined,
};

return {
props,
component: mountWithIntl(<Histogram {...props} />),
};
const component = mountWithIntl(<Histogram {...props} />);

act(() => {
props.fetch$?.next({ type: 'fetch' });
});

return { props, component: component.update() };
}

describe('Histogram', () => {
Expand All @@ -92,7 +95,7 @@ describe('Histogram', () => {
expect(component.find('[data-test-subj="unifiedHistogramChart"]').exists()).toBe(true);
});

it('should only update lens.EmbeddableComponent props when refetch$ is triggered', async () => {
it('should only update lens.EmbeddableComponent props when fetch$ is triggered', async () => {
const { component, props } = await mountComponent();
const embeddable = unifiedHistogramServicesMock.lens.EmbeddableComponent;
expect(component.find(embeddable).exists()).toBe(true);
Expand All @@ -108,7 +111,7 @@ describe('Histogram', () => {
lensProps = component.find(embeddable).props();
expect(lensProps).toMatchObject(expect.objectContaining(originalProps));
await act(async () => {
props.refetch$.next({ type: 'refetch' });
props.fetch$.next({ type: 'fetch' });
});
component.update();
lensProps = component.find(embeddable).props();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export interface HistogramProps {
isPlainRecord?: boolean;
hasLensSuggestions: boolean;
getTimeRange: () => TimeRange;
refetch$: Observable<UnifiedHistogramInputMessage>;
fetch$: Observable<UnifiedHistogramInputMessage>;
visContext: UnifiedHistogramVisContext;
disableTriggers?: LensEmbeddableInput['disableTriggers'];
disabledActions?: LensEmbeddableInput['disabledActions'];
Expand Down Expand Up @@ -92,7 +92,7 @@ export function Histogram({
isPlainRecord,
hasLensSuggestions,
getTimeRange,
refetch$,
fetch$,
visContext,
disableTriggers,
disabledActions,
Expand Down Expand Up @@ -161,10 +161,10 @@ export function Histogram({
}
);

const { lensProps, requestData } = useLensProps({
const lensPropsContext = useLensProps({
request,
getTimeRange,
refetch$,
fetch$,
visContext,
onLoad,
});
Expand Down Expand Up @@ -201,6 +201,13 @@ export function Histogram({
}
`;

// Avoid rendering until a fetch has been triggered
if (!lensPropsContext) {
return null;
}

const { lensProps, requestData } = lensPropsContext;

return (
<>
<div
Expand Down
Loading

0 comments on commit a0d9b5b

Please sign in to comment.