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] Add internalState loading function preventing data view id change related race conditions #199419

Closed
wants to merge 12 commits into from
Closed

[Discover] Add internalState loading function preventing data view id change related race conditions #199419

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bf323b8
Attempt to fix a flaky test
kertal Oct 4, 2024
036538a
Add missing await when building data view list
kertal Oct 4, 2024
7d4e3da
Add a new isLoading internal state that is set to true when a ad hoc …
kertal Oct 4, 2024
d574963
Attempt to fix flakiness
kertal Oct 7, 2024
7726c7d
Add statContainer setIsLoading function
kertal Oct 7, 2024
4cfc4f3
Add statContainer setIsLoading function
kertal Oct 7, 2024
0cb4cf1
Add internalState loading function
kertal Nov 8, 2024
4f9d4ab
remove unrelated code changes
kertal Nov 8, 2024
7f1503e
Simplify code
kertal Nov 8, 2024
82ebd2e
Merge branch 'main' into discover-unskip-_lens_vis·ts
kertal Nov 11, 2024
fc4ddd2
Merge branch 'main' into discover-unskip-_lens_vis·ts
kertal Nov 12, 2024
3f77801
Address review feedback
kertal Nov 13, 2024
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
3 changes: 3 additions & 0 deletions src/plugins/discover/public/application/main/data_fetching/fetch_all.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ describe('test fetchAll', () => {
getAppState: () => ({}),
getInternalState: () => ({
dataView: undefined,
isLoading: false,
isDataViewLoading: false,
savedDataViews: [],
adHocDataViews: [],
Expand Down Expand Up @@ -262,6 +263,7 @@ describe('test fetchAll', () => {
getInternalState: () => ({
dataView: undefined,
isDataViewLoading: false,
isLoading: false,
savedDataViews: [],
adHocDataViews: [],
expandedDoc: undefined,
Expand Down Expand Up @@ -384,6 +386,7 @@ describe('test fetchAll', () => {
getInternalState: () => ({
dataView: undefined,
isDataViewLoading: false,
isLoading: false,
savedDataViews: [],
adHocDataViews: [],
expandedDoc: undefined,
Expand Down
48 changes: 32 additions & 16 deletions src/plugins/discover/public/application/main/discover_main_route.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { reportPerformanceMetricEvent } from '@kbn/ebt-tools';
import { withSuspense } from '@kbn/shared-ux-utility';
import { getInitialESQLQuery } from '@kbn/esql-utils';
import { ESQL_TYPE } from '@kbn/data-view-utils';
import { useInternalStateSelector } from './state_management/discover_internal_state_container';
import { useUrl } from './hooks/use_url';
import { useDiscoverStateContainer } from './hooks/use_discover_state_container';
import { MainHistoryLocationState } from '../../../common';
Expand Down Expand Up @@ -85,7 +86,6 @@ export function DiscoverMainRoute({
stateContainer,
});
const [error, setError] = useState<Error>();
const [loading, setLoading] = useState(true);
const [noDataState, setNoDataState] = useState({
hasESData: false,
hasUserDataView: false,
Expand Down Expand Up @@ -157,10 +157,10 @@ export function DiscoverMainRoute({
initialAppState,
}: { nextDataView?: DataView; initialAppState?: LoadParams['initialAppState'] } = {}) => {
const loadSavedSearchStartTime = window.performance.now();
setLoading(true);
stateContainer.actions.setIsLoading(true);
const skipNoData = await skipNoDataPage(nextDataView);
if (!skipNoData) {
setLoading(false);
stateContainer.actions.setIsLoading(false);
return;
}
try {
Expand All @@ -181,7 +181,7 @@ export function DiscoverMainRoute({

setBreadcrumbs({ services, titleBreadcrumbText: currentSavedSearch?.title ?? undefined });
}
setLoading(false);
stateContainer.actions.setIsLoading(false);
if (services.analytics) {
const loadSavedSearchDuration = window.performance.now() - loadSavedSearchStartTime;
reportPerformanceMetricEvent(services.analytics, {
Expand Down Expand Up @@ -231,7 +231,7 @@ export function DiscoverMainRoute({

useEffect(() => {
if (!isCustomizationServiceInitialized) return;
setLoading(true);
stateContainer.actions.setIsLoading(true);
setNoDataState({
hasESData: false,
hasUserDataView: false,
Expand Down Expand Up @@ -259,13 +259,13 @@ export function DiscoverMainRoute({
const onDataViewCreated = useCallback(
async (nextDataView: unknown) => {
if (nextDataView) {
setLoading(true);
stateContainer.actions.setIsLoading(true);
setNoDataState((state) => ({ ...state, showNoDataPage: false }));
setError(undefined);
await loadSavedSearch({ nextDataView: nextDataView as DataView });
}
},
[loadSavedSearch]
[loadSavedSearch, stateContainer]
);

const onESQLNavigationComplete = useCallback(async () => {
Expand Down Expand Up @@ -326,14 +326,8 @@ export function DiscoverMainRoute({
);
}

if (loading) {
return loadingIndicator;
}

return <DiscoverMainAppMemoized stateContainer={stateContainer} />;
}, [
loading,
loadingIndicator,
noDataDependencies,
onDataViewCreated,
onESQLNavigationComplete,
Expand All @@ -355,11 +349,11 @@ export function DiscoverMainRoute({
<DiscoverCustomizationProvider value={customizationService}>
<DiscoverMainProvider value={stateContainer}>
<rootProfileState.AppWrapper>
<DiscoverTopNavInline
<DiscoverMainLoading
mainContent={mainContent}
showNoDataPage={noDataState.showNoDataPage}
stateContainer={stateContainer}
hideNavMenuItems={loading || noDataState.showNoDataPage}
/>
{mainContent}
</rootProfileState.AppWrapper>
</DiscoverMainProvider>
</DiscoverCustomizationProvider>
Expand All @@ -368,6 +362,28 @@ export function DiscoverMainRoute({
// eslint-disable-next-line import/no-default-export
export default DiscoverMainRoute;

export function DiscoverMainLoading({
stateContainer,
showNoDataPage,
mainContent,
}: {
stateContainer: DiscoverStateContainer;
showNoDataPage: boolean;
mainContent: JSX.Element;
}) {
const loading = useInternalStateSelector((state) => state.isLoading);

return (
<>
<DiscoverTopNavInline
stateContainer={stateContainer}
hideNavMenuItems={showNoDataPage || loading}
/>
{loading && !showNoDataPage ? <LoadingIndicator /> : mainContent}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not we continue using loadingIndicator instead of <LoadingIndicator />?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this makes sense, it appears there were more changes, than I noticed since I started to fix this in another PR 👍

</>
);
}

function getLoadParamsForNewSearch(stateContainer: DiscoverStateContainer): {
nextDataView: LoadParams['dataView'];
initialAppState: LoadParams['initialAppState'];
Expand Down
7 changes: 7 additions & 0 deletions ...ns/discover/public/application/main/state_management/discover_internal_state_container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import type { UnifiedHistogramVisContext } from '@kbn/unified-histogram-plugin/p
export interface InternalState {
dataView: DataView | undefined;
isDataViewLoading: boolean;
isLoading: boolean;
savedDataViews: DataViewListItem[];
adHocDataViews: DataView[];
expandedDoc: DataTableRecord | undefined;
Expand All @@ -32,6 +33,7 @@ export interface InternalState {
export interface InternalStateTransitions {
setDataView: (state: InternalState) => (dataView: DataView) => InternalState;
setIsDataViewLoading: (state: InternalState) => (isLoading: boolean) => InternalState;
setIsLoading: (state: InternalState) => (isLoading: boolean) => InternalState;
setSavedDataViews: (state: InternalState) => (dataView: DataViewListItem[]) => InternalState;
setAdHocDataViews: (state: InternalState) => (dataViews: DataView[]) => InternalState;
appendAdHocDataViews: (
Expand Down Expand Up @@ -72,6 +74,7 @@ export function getInternalStateContainer() {
{
dataView: undefined,
isDataViewLoading: false,
isLoading: true,
adHocDataViews: [],
savedDataViews: [],
expandedDoc: undefined,
Expand All @@ -90,6 +93,10 @@ export function getInternalStateContainer() {
...prevState,
isDataViewLoading: loading,
}),
setIsLoading: (prevState: InternalState) => (isLoading: boolean) => ({
...prevState,
isLoading,
}),
setIsESQLToDataViewTransitionModalVisible:
(prevState: InternalState) => (isVisible: boolean) => ({
...prevState,
Expand Down
9 changes: 9 additions & 0 deletions src/plugins/discover/public/application/main/state_management/discover_state.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -956,6 +956,15 @@ describe('Test discover state actions', () => {
expect(setTime).toHaveBeenCalledWith({ from: 'now-15d', to: 'now-10d' });
expect(setRefreshInterval).toHaveBeenCalledWith({ pause: false, value: 1000 });
});

test('setIsLoading', async () => {
const { state } = await getState('/');
expect(state.internalState.getState().isLoading).toBe(true);
await state.actions.setIsLoading(false);
expect(state.internalState.getState().isLoading).toBe(false);
await state.actions.setIsLoading(true);
expect(state.internalState.getState().isLoading).toBe(true);
});
});

describe('Test discover state with embedded mode', () => {
Expand Down
17 changes: 15 additions & 2 deletions src/plugins/discover/public/application/main/state_management/discover_state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,11 @@ export interface DiscoverStateContainer {
* @param dataView
*/
setDataView: (dataView: DataView) => void;
/**
* Set Discover to loading state on the highest level, this cleans up all internal UI state
* @param value
*/
setIsLoading: (value: boolean) => void;
/**
* Undo changes made to the saved search, e.g. when the user triggers the "Reset search" button
*/
Expand Down Expand Up @@ -412,17 +417,24 @@ export function getDiscoverStateContainer({
}
};

const setIsLoading = (value: boolean) => {
internalStateContainer.transitions.setIsLoading(value);
};

const onDataViewEdited = async (editedDataView: DataView) => {
setIsLoading(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if isDataViewLoading should be toggled 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.

For me it appears we just use isDataViewLoading in

kibana/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

Line 169 in 8545b9c

useInternalStateSelector((state) => state.isDataViewLoading) && !isEsqlMode;

and

kibana/src/plugins/discover/public/application/main/components/layout/discover_layout.tsx

Lines 104 to 107 in 7369442

const [dataView, dataViewLoading] = useInternalStateSelector((state) => [
state.dataView!,
state.isDataViewLoading,
]);

We set it to true when a data view is being changes.

With the new isLoading prop those components should not be displayed. So I'd say it's not necessary ... one think I wonder, would it make sense to combing all those loading states into one prop? one that can be used to get information about general loading state, data view loading state, data loading state ... just load thinking

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was also wondering if we could use isDataViewLoading instead of introducing a new isLoading.
Editing a data view might be considered as "loading" its new state.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I splitted this work (again):
Hopefully near term solution: #199982
Experimental loading refactoring, moving to one loading state instead of isDataViewLoading and isLoading: #199956

const edited = editedDataView.id;
if (editedDataView.isPersisted()) {
// Clear the current data view from the cache and create a new instance
// of it, ensuring we have a new object reference to trigger a re-render
services.dataViews.clearInstanceCache(editedDataView.id);
services.dataViews.clearInstanceCache(edited);
setDataView(await services.dataViews.create(editedDataView.toSpec(), true));
} else {
await updateAdHocDataViewId();
}
loadDataViewList();
await loadDataViewList();
addLog('[getDiscoverStateContainer] onDataViewEdited triggers data fetching');
setIsLoading(false);
fetchData();
};

Expand Down Expand Up @@ -612,6 +624,7 @@ export function getDiscoverStateContainer({
onDataViewCreated,
onDataViewEdited,
onOpenSavedSearch,
setIsLoading,
transitionFromESQLToDataView,
transitionFromDataViewToESQL,
onUpdateQuery,
Expand Down
3 changes: 1 addition & 2 deletions test/functional/apps/discover/group3/_lens_vis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
return seriesType;
}

// FLAKY: https://github.com/elastic/kibana/issues/184600
describe.skip('discover lens vis', function () {
describe('discover lens vis', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I reproduced the issue on this branch too:

Screenshot 2024-11-12 at 14 54 53

Copy link
Contributor

@jughosta jughosta Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We might need to load data views right after a new one is created and before it gets replaces in the app state/URL:

kibana/src/plugins/discover/public/application/main/state_management/discover_state.ts

Line 334 in 8545b9c

});

Copy link
Member Author

Choose a reason for hiding this comment

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

there's some really dark magic here, that even when investigating this caused my IDE to freeze, so just before the last meeting of my day, I'm thinking that the cleanup of adhoc data views that were changed because of a spec change, should be triggered later on:

kibana/src/plugins/discover/public/application/main/state_management/discover_state.ts

Line 430 in 7f1503e

services.dataViews.clearInstanceCache(edited);

because then the previous data view would still be available, wouldn't do any harm, and be removed after the data fetching was triggered

Copy link
Member Author

Choose a reason for hiding this comment

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

clearly broken here, looking for a simpler solution, so far I couldn't this seems to resolve this issue locally, let's see what flaky test runner says: #199982

before(async () => {
await security.testUser.setRoles(['kibana_admin', 'test_logstash_reader']);
await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional');
Expand Down