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

[Discover] Refactor totalHits$ loading state handling to omit race conditions #196114

Merged
merged 14 commits into from
Oct 22, 2024
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export const useDiscoverHistogram = ({
}: UseDiscoverHistogramProps) => {
const services = useDiscoverServices();
const savedSearchData$ = stateContainer.dataState.data$;
const savedSearchHits$ = savedSearchData$.totalHits$;
kertal marked this conversation as resolved.
Show resolved Hide resolved
const savedSearchState = useSavedSearch();
const isEsqlMode = useIsEsqlMode();

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

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

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

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

if (
(status === UnifiedHistogramFetchStatus.loading ||
Expand All @@ -184,11 +182,15 @@ 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 the saved search
kertal marked this conversation as resolved.
Show resolved Hide resolved
if (fetchStatus !== FetchStatus.LOADING) {
savedSearchHits$.next({
Copy link
Contributor

@jughosta jughosta Oct 15, 2024

Choose a reason for hiding this comment

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

I think there was a good reason why the histogram was sending the loading status all this time. But I don't remember all the cases about that. Do you mind if we wait for @davismcphee review?

Copy link
Member Author

@kertal kertal Oct 15, 2024

Choose a reason for hiding this comment

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

yes, let's wait, from what I've seen, it doesn't hurt keeping it, but on the other hand, I thought it's redundant

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, as long as we're setting the loading state every time infetchAll, I think this should be ok.

fetchStatus,
result,
});
}

if (status !== UnifiedHistogramFetchStatus.complete || typeof result !== 'number') {
return;
Expand All @@ -205,7 +207,7 @@ export const useDiscoverHistogram = ({
}, [
isEsqlMode,
savedSearchData$.main$,
savedSearchData$.totalHits$,
savedSearchHits$,
setTotalHitsError,
stateContainer.appState,
unifiedHistogram?.state$,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ export function fetchAll(
sendLoadingMsg(dataSubjects.totalHits$, {
result: dataSubjects.totalHits$.getValue().result,
});
} else {
sendLoadingMsg(dataSubjects.totalHits$);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should keep the previous result too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about that, because I think if I send a loading, the previous result is not longer valid? So if we rely on a previous result somewhere we might change this

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also not sure it's needed, but we used to do it when setting the loading state in use_discover_histogtam too, so probably best to keep passing result for now. It would get rid of this conditional too (and the comment above doesn't apply anymore anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

You're both right, let's simplify the code ... done

}

// Start fetching all required requests
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
Copy link
Member Author

Choose a reason for hiding this comment

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

uncommenting the code can be very helpful to unterstand data fetching observables race conditions, I think it's worth keeping it here

Copy link
Member Author

Choose a reason for hiding this comment

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

This could be helpful to debug at least a part of the staff you mention in #165192 (comment) @jughosta


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