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

[8.x] [Discover] Refactor totalHits$ loading state handling to omit race conditions (#196114) #197166

Merged
merged 1 commit into from
Oct 22, 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 @@ -61,7 +61,7 @@ export const useDiscoverHistogram = ({
hideChart,
}: UseDiscoverHistogramProps) => {
const services = useDiscoverServices();
const savedSearchData$ = stateContainer.dataState.data$;
const { main$, documents$, totalHits$ } = stateContainer.dataState.data$;
const savedSearchState = useSavedSearch();
const isEsqlMode = useIsEsqlMode();

Expand Down Expand Up @@ -153,10 +153,7 @@ export const useDiscoverHistogram = ({
* Total hits
*/

const setTotalHitsError = useMemo(
() => sendErrorTo(savedSearchData$.totalHits$),
[savedSearchData$.totalHits$]
);
const setTotalHitsError = useMemo(() => sendErrorTo(totalHits$), [totalHits$]);

useEffect(() => {
const subscription = createTotalHitsObservable(unifiedHistogram?.state$)?.subscribe(
Expand All @@ -172,7 +169,7 @@ export const useDiscoverHistogram = ({
return;
}

const { result: totalHitsResult } = savedSearchData$.totalHits$.getValue();
const { result: totalHitsResult } = totalHits$.getValue();

if (
(status === UnifiedHistogramFetchStatus.loading ||
Expand All @@ -184,18 +181,22 @@ export const useDiscoverHistogram = ({
return;
}

// Sync the totalHits$ observable with the unified histogram state
savedSearchData$.totalHits$.next({
fetchStatus: status.toString() as FetchStatus,
result,
});
const fetchStatus = status.toString() as FetchStatus;

// Do not sync the loading state since it's already handled by fetchAll
if (fetchStatus !== FetchStatus.LOADING) {
totalHits$.next({
fetchStatus,
result,
});
}

if (status !== UnifiedHistogramFetchStatus.complete || typeof result !== 'number') {
return;
}

// Check the hits count to set a partial or no results state
checkHitCount(savedSearchData$.main$, result);
checkHitCount(main$, result);
}
);

Expand All @@ -204,8 +205,8 @@ export const useDiscoverHistogram = ({
};
}, [
isEsqlMode,
savedSearchData$.main$,
savedSearchData$.totalHits$,
main$,
totalHits$,
setTotalHitsError,
stateContainer.appState,
unifiedHistogram?.state$,
Expand Down Expand Up @@ -234,7 +235,7 @@ export const useDiscoverHistogram = ({

const [initialEsqlProps] = useState(() =>
getUnifiedHistogramPropsForEsql({
documentsValue: savedSearchData$.documents$.getValue(),
documentsValue: documents$.getValue(),
savedSearch: stateContainer.savedSearchState.getState(),
})
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,9 @@ export function fetchAll(
// Mark all subjects as loading
sendLoadingMsg(dataSubjects.main$);
sendLoadingMsg(dataSubjects.documents$, { query });

// histogram for data view mode will send `loading` for totalHits$
if (isEsqlQuery) {
sendLoadingMsg(dataSubjects.totalHits$, {
result: dataSubjects.totalHits$.getValue().result,
});
}
sendLoadingMsg(dataSubjects.totalHits$, {
result: dataSubjects.totalHits$.getValue().result,
});

// Start fetching all required requests
const response = isEsqlQuery
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ describe('test getDataStateContainer', () => {
expect(
stateContainer.searchSessionManager.getCurrentSearchSessionId as jest.Mock
).toHaveBeenCalled();

unsubscribe();
done();
}
Expand All @@ -169,21 +168,24 @@ describe('test getDataStateContainer', () => {
});

it('should update app state from default profile state', async () => {
mockFetchDocuments.mockResolvedValue({ records: [] });
const stateContainer = getDiscoverStateMock({ isTimeBased: true });
const dataState = stateContainer.dataState;
const dataUnsub = dataState.subscribe();
const appUnsub = stateContainer.appState.initAndSync();
discoverServiceMock.profilesManager.resolveDataSourceProfile({});
await discoverServiceMock.profilesManager.resolveDataSourceProfile({});
stateContainer.actions.setDataView(dataViewMock);
stateContainer.internalState.transitions.setResetDefaultProfileState({
columns: true,
rowHeight: true,
});

dataState.data$.totalHits$.next({
fetchStatus: FetchStatus.COMPLETE,
result: 0,
});
dataState.refetch$.next(undefined);

await waitFor(() => {
expect(dataState.data$.main$.value.fetchStatus).toBe(FetchStatus.COMPLETE);
});
Expand All @@ -202,7 +204,7 @@ describe('test getDataStateContainer', () => {
const dataState = stateContainer.dataState;
const dataUnsub = dataState.subscribe();
const appUnsub = stateContainer.appState.initAndSync();
discoverServiceMock.profilesManager.resolveDataSourceProfile({});
await discoverServiceMock.profilesManager.resolveDataSourceProfile({});
stateContainer.actions.setDataView(dataViewMock);
stateContainer.internalState.transitions.setResetDefaultProfileState({
columns: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,12 @@ export function getDataStateContainer({
documents$: new BehaviorSubject<DataDocumentsMsg>(initialState),
totalHits$: new BehaviorSubject<DataTotalHitsMsg>(initialState),
};
// This is debugging code, helping you to understand which messages are sent to the data observables
// Adding a debugger in the functions can be helpful to understand what triggers a message
// dataSubjects.main$.subscribe((msg) => addLog('dataSubjects.main$', msg));
// dataSubjects.documents$.subscribe((msg) => addLog('dataSubjects.documents$', msg));
// dataSubjects.totalHits$.subscribe((msg) => addLog('dataSubjects.totalHits$', msg););
// Add window.ELASTIC_DISCOVER_LOGGER = 'debug' to see messages in console

let autoRefreshDone: AutoRefreshDoneFn | undefined | null = null;
/**
Expand Down
3 changes: 1 addition & 2 deletions test/functional/apps/discover/group3/_lens_vis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
return seriesType;
}

// Failing: See https://github.com/elastic/kibana/issues/184600
describe.skip('discover lens vis', function () {
describe('discover lens vis', function () {
before(async () => {
await security.testUser.setRoles(['kibana_admin', 'test_logstash_reader']);
await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional');
Expand Down
Loading