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

[ML] AIOps: Fix race condition where stale url state would reset search bar. #154885

Merged
merged 8 commits into from
Apr 18, 2023
Merged

[ML] AIOps: Fix race condition where stale url state would reset search bar. #154885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9595fcd
wip fix functional tests
walterra Apr 12, 2023
c2f2d79
fix url state. fix functional tests
walterra Apr 12, 2023
33e3358
Merge branch 'main' into ml-fix-url-state
walterra Apr 13, 2023
5e4f9a5
fix callback dependencies
walterra Apr 13, 2023
907db31
wip fixing jest tests
walterra Apr 13, 2023
222c28b
Merge branch 'main' into ml-fix-url-state
walterra Apr 14, 2023
625dd4f
fix url state
walterra Apr 14, 2023
0c30ee2
Merge branch 'main' into ml-fix-url-state
walterra Apr 17, 2023
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
118 changes: 87 additions & 31 deletions x-pack/packages/ml/url_state/src/url_state.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,18 @@
* 2.0.
*/

import React, { FC } from 'react';
import React, { useEffect, type FC } from 'react';
import { render, act } from '@testing-library/react';
import { parseUrlState, useUrlState, UrlStateProvider } from './url_state';
import { MemoryRouter } from 'react-router-dom';

const mockHistoryPush = jest.fn();
import { parseUrlState, useUrlState, UrlStateProvider } from './url_state';

jest.mock('react-router-dom', () => ({
useHistory: () => ({
push: mockHistoryPush,
}),
useLocation: () => ({
search:
"?_a=(mlExplorerFilter:(),mlExplorerSwimlane:(viewByFieldName:action),query:(query_string:(analyze_wildcard:!t,query:'*')))&_g=(ml:(jobIds:!(dec-2)),refreshInterval:(display:Off,pause:!f,value:0),time:(from:'2019-01-01T00:03:40.000Z',mode:absolute,to:'2019-08-30T11:55:07.000Z'))&savedSearchId=571aaf70-4c88-11e8-b3d7-01146121b73d",
}),
}));
const mockHistoryInitialState =
"?_a=(mlExplorerFilter:(),mlExplorerSwimlane:(viewByFieldName:action),query:(query_string:(analyze_wildcard:!t,query:'*')))&_g=(ml:(jobIds:!(dec-2)),refreshInterval:(display:Off,pause:!f,value:0),time:(from:'2019-01-01T00:03:40.000Z',mode:absolute,to:'2019-08-30T11:55:07.000Z'))&savedSearchId=571aaf70-4c88-11e8-b3d7-01146121b73d";

describe('getUrlState', () => {
test('properly decode url with _g and _a', () => {
expect(
parseUrlState(
"?_a=(mlExplorerFilter:(),mlExplorerSwimlane:(viewByFieldName:action),query:(query_string:(analyze_wildcard:!t,query:'*')))&_g=(ml:(jobIds:!(dec-2)),refreshInterval:(display:Off,pause:!t,value:0),time:(from:'2019-01-01T00:03:40.000Z',mode:absolute,to:'2019-08-30T11:55:07.000Z'))&savedSearchId=571aaf70-4c88-11e8-b3d7-01146121b73d"
)
).toEqual({
expect(parseUrlState(mockHistoryInitialState)).toEqual({
_a: {
mlExplorerFilter: {},
mlExplorerSwimlane: {
Expand All @@ -46,7 +35,7 @@ describe('getUrlState', () => {
},
refreshInterval: {
display: 'Off',
pause: true,
pause: false,
value: 0,
},
time: {
Expand All @@ -61,29 +50,96 @@ describe('getUrlState', () => {
});

describe('useUrlState', () => {
beforeEach(() => {
mockHistoryPush.mockClear();
it('pushes a properly encoded search string to history', () => {
const TestComponent: FC = () => {
darnautov marked this conversation as resolved.
Show resolved Hide resolved
const [appState, setAppState] = useUrlState('_a');

useEffect(() => {
setAppState(parseUrlState(mockHistoryInitialState)._a);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

return (
<>
<button onClick={() => setAppState({ query: 'my-query' })}>ButtonText</button>
<div data-test-subj="appState">{JSON.stringify(appState?.query)}</div>
</>
);
};

const { getByText, getByTestId } = render(
<MemoryRouter>
<UrlStateProvider>
<TestComponent />
</UrlStateProvider>
</MemoryRouter>
);

expect(getByTestId('appState').innerHTML).toBe(
'{"query_string":{"analyze_wildcard":true,"query":"*"}}'
);

act(() => {
getByText('ButtonText').click();
});

expect(getByTestId('appState').innerHTML).toBe('"my-query"');
});

test('pushes a properly encoded search string to history', () => {
it('updates both _g and _a state successfully', () => {
const TestComponent: FC = () => {
const [, setUrlState] = useUrlState('_a');
return <button onClick={() => setUrlState({ query: {} })}>ButtonText</button>;
const [globalState, setGlobalState] = useUrlState('_g');
const [appState, setAppState] = useUrlState('_a');

useEffect(() => {
setGlobalState({ time: 'initial time' });
setAppState({ query: 'initial query' });
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

return (
<>
<button onClick={() => setGlobalState({ time: 'now-15m' })}>GlobalStateButton1</button>
<button onClick={() => setGlobalState({ time: 'now-5y' })}>GlobalStateButton2</button>
<button onClick={() => setAppState({ query: 'the updated query' })}>
AppStateButton
</button>
<div data-test-subj="globalState">{globalState?.time}</div>
<div data-test-subj="appState">{appState?.query}</div>
</>
);
};

const { getByText } = render(
<UrlStateProvider>
<TestComponent />
</UrlStateProvider>
const { getByText, getByTestId } = render(
<MemoryRouter>
<UrlStateProvider>
<TestComponent />
</UrlStateProvider>
</MemoryRouter>
);

expect(getByTestId('globalState').innerHTML).toBe('initial time');
expect(getByTestId('appState').innerHTML).toBe('initial query');

act(() => {
getByText('ButtonText').click();
getByText('GlobalStateButton1').click();
});

expect(mockHistoryPush).toHaveBeenCalledWith({
search:
'_a=%28mlExplorerFilter%3A%28%29%2CmlExplorerSwimlane%3A%28viewByFieldName%3Aaction%29%2Cquery%3A%28%29%29&_g=%28ml%3A%28jobIds%3A%21%28dec-2%29%29%2CrefreshInterval%3A%28display%3AOff%2Cpause%3A%21f%2Cvalue%3A0%29%2Ctime%3A%28from%3A%272019-01-01T00%3A03%3A40.000Z%27%2Cmode%3Aabsolute%2Cto%3A%272019-08-30T11%3A55%3A07.000Z%27%29%29&savedSearchId=571aaf70-4c88-11e8-b3d7-01146121b73d',
expect(getByTestId('globalState').innerHTML).toBe('now-15m');
expect(getByTestId('appState').innerHTML).toBe('initial query');

act(() => {
getByText('AppStateButton').click();
});

expect(getByTestId('globalState').innerHTML).toBe('now-15m');
expect(getByTestId('appState').innerHTML).toBe('the updated query');

act(() => {
getByText('GlobalStateButton2').click();
});

expect(getByTestId('globalState').innerHTML).toBe('now-5y');
expect(getByTestId('appState').innerHTML).toBe('the updated query');
});
});
15 changes: 13 additions & 2 deletions x-pack/packages/ml/url_state/src/url_state.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,21 @@ export const UrlStateProvider: FC = ({ children }) => {
const history = useHistory();
const { search: searchString } = useLocation();

const searchStringRef = useRef<string>(searchString);

useEffect(() => {
searchStringRef.current = searchString;
}, [searchString]);

const setUrlState: SetUrlState = useCallback(
(
accessor: Accessor,
attribute: string | Dictionary<any>,
value?: any,
replaceState?: boolean
) => {
const prevSearchString = searchString;
const prevSearchString = searchStringRef.current;

const urlState = parseUrlState(prevSearchString);
const parsedQueryString = parse(prevSearchString, { sort: false });

Expand Down Expand Up @@ -142,6 +149,10 @@ export const UrlStateProvider: FC = ({ children }) => {

if (oldLocationSearchString !== newLocationSearchString) {
const newSearchString = stringify(parsedQueryString, { sort: false });
// Another `setUrlState` call could happen before the updated
// `searchString` gets propagated via `useLocation` therefore
// we update the ref right away too.
searchStringRef.current = newSearchString;
if (replaceState) {
history.replace({ search: newSearchString });
} else {
Expand All @@ -154,7 +165,7 @@ export const UrlStateProvider: FC = ({ children }) => {
}
},
// eslint-disable-next-line react-hooks/exhaustive-deps
[searchString]
[]
);

return <Provider value={{ searchString, setUrlState }}>{children}</Provider>;
Expand Down
52 changes: 21 additions & 31 deletions x-pack/plugins/aiops/public/hooks/use_data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ export const useData = (
} = useAiopsAppContext();

const [lastRefresh, setLastRefresh] = useState(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I reckon we should get rid of this local state and use useRefresh and useTimeRangeUpdates hooks instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since lastRefresh is also passed on to use_document_count_stats.ts it will result in quite a rewrite, that's a bit out of scope for this PR, I'd like to focus on just fixing the bug here.

const [fieldStatsRequest, setFieldStatsRequest] = useState<
DocumentStatsSearchStrategyParams | undefined
>();

/** Prepare required params to pass to search strategy **/
const { searchQueryLanguage, searchString, searchQuery } = useMemo(() => {
Expand Down Expand Up @@ -91,12 +88,30 @@ export const useData = (
]);

const _timeBuckets = useTimeBuckets();

const timefilter = useTimefilter({
timeRangeSelector: selectedDataView?.timeFieldName !== undefined,
autoRefreshSelector: true,
});

const fieldStatsRequest: DocumentStatsSearchStrategyParams | undefined = useMemo(() => {
Copy link
Member

Choose a reason for hiding this comment

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

not related to this line...

In my current WIP PR for embedding log categorisation in Discover I've had to add a hack to stop this file from calling the various set functions in filterManager because they update the URL and cause Discover problems.
It makes me wonder if this could be considered an unexpected side effect and whether this file should be updating the URL at all?

I don't have a good solution, apart from perhaps creating a different hook which monitors the aiopsListState and updates the URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, we'll have to consider this also for Explain Log Rate Spike once it gets used in other places. I added a note to the meta issue here: #146065

const timefilterActiveBounds = timefilter.getActiveBounds();
if (timefilterActiveBounds !== undefined) {
_timeBuckets.setInterval('auto');
_timeBuckets.setBounds(timefilterActiveBounds);
_timeBuckets.setBarTarget(barTarget);
return {
earliest: timefilterActiveBounds.min?.valueOf(),
latest: timefilterActiveBounds.max?.valueOf(),
intervalMs: _timeBuckets.getInterval()?.asMilliseconds(),
index: selectedDataView.getIndexPattern(),
searchQuery,
timeFieldName: selectedDataView.timeFieldName,
runtimeFieldMap: selectedDataView.getRuntimeMappings(),
};
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [lastRefresh, searchQuery]);
Copy link
Contributor

Choose a reason for hiding this comment

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

and afterwards fieldStatsRequest should be triggered based on explicit refresh or time range updates.


const overallStatsRequest = useMemo(() => {
return fieldStatsRequest
? {
Expand Down Expand Up @@ -125,25 +140,6 @@ export const useData = (
lastRefresh
);

function updateFieldStatsRequest() {
const timefilterActiveBounds = timefilter.getActiveBounds();
if (timefilterActiveBounds !== undefined) {
_timeBuckets.setInterval('auto');
_timeBuckets.setBounds(timefilterActiveBounds);
_timeBuckets.setBarTarget(barTarget);
setFieldStatsRequest({
earliest: timefilterActiveBounds.min?.valueOf(),
latest: timefilterActiveBounds.max?.valueOf(),
intervalMs: _timeBuckets.getInterval()?.asMilliseconds(),
index: selectedDataView.getIndexPattern(),
searchQuery,
timeFieldName: selectedDataView.timeFieldName,
runtimeFieldMap: selectedDataView.getRuntimeMappings(),
});
setLastRefresh(Date.now());
}
}

useEffect(() => {
const timefilterUpdateSubscription = merge(
timefilter.getAutoRefreshFetch$(),
Expand All @@ -156,13 +152,13 @@ export const useData = (
refreshInterval: timefilter.getRefreshInterval(),
});
}
updateFieldStatsRequest();
setLastRefresh(Date.now());
});

// This listens just for an initial update of the timefilter to be switched on.
const timefilterEnabledSubscription = timefilter.getEnabledUpdated$().subscribe(() => {
if (fieldStatsRequest === undefined) {
updateFieldStatsRequest();
setLastRefresh(Date.now());
}
});

Expand All @@ -173,12 +169,6 @@ export const useData = (
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

// Ensure request is updated when search changes
useEffect(() => {
updateFieldStatsRequest();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [searchString, JSON.stringify(searchQuery)]);

return {
documentStats,
timefilter,
Expand Down
50 changes: 34 additions & 16 deletions x-pack/test/functional/apps/aiops/explain_log_rate_spikes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { orderBy } from 'lodash';
import expect from '@kbn/expect';

import type { FtrProviderContext } from '../../ftr_provider_context';
import type { TestData } from './types';
import { isTestDataExpectedWithSampleProbability, type TestData } from './types';
import { explainLogRateSpikesTestData } from './test_data';

export default function ({ getPageObject, getService }: FtrProviderContext) {
Expand Down Expand Up @@ -43,9 +43,21 @@ export default function ({ getPageObject, getService }: FtrProviderContext) {
await aiops.explainLogRateSpikesPage.assertTimeRangeSelectorSectionExists();

await ml.testExecution.logTestStep(`${testData.suiteTitle} loads data for full time range`);
if (testData.query) {
await aiops.explainLogRateSpikesPage.setQueryInput(testData.query);
}
await aiops.explainLogRateSpikesPage.clickUseFullDataButton(
testData.expected.totalDocCountFormatted
);

if (isTestDataExpectedWithSampleProbability(testData.expected)) {
await aiops.explainLogRateSpikesPage.assertSamplingProbability(
testData.expected.sampleProbabilityFormatted
);
} else {
await aiops.explainLogRateSpikesPage.assertSamplingProbabilityMissing();
}

await headerPage.waitUntilLoadingHasFinished();

await ml.testExecution.logTestStep(
Expand Down Expand Up @@ -147,21 +159,24 @@ export default function ({ getPageObject, getService }: FtrProviderContext) {

await aiops.explainLogRateSpikesAnalysisGroupsTable.assertSpikeAnalysisTableExists();

const analysisGroupsTable =
await aiops.explainLogRateSpikesAnalysisGroupsTable.parseAnalysisTable();

expect(orderBy(analysisGroupsTable, 'group')).to.be.eql(
orderBy(testData.expected.analysisGroupsTable, 'group')
);
if (!isTestDataExpectedWithSampleProbability(testData.expected)) {
const analysisGroupsTable =
await aiops.explainLogRateSpikesAnalysisGroupsTable.parseAnalysisTable();
expect(orderBy(analysisGroupsTable, 'group')).to.be.eql(
orderBy(testData.expected.analysisGroupsTable, 'group')
);
}

await ml.testExecution.logTestStep('expand table row');
await aiops.explainLogRateSpikesAnalysisGroupsTable.assertExpandRowButtonExists();
await aiops.explainLogRateSpikesAnalysisGroupsTable.expandRow();

const analysisTable = await aiops.explainLogRateSpikesAnalysisTable.parseAnalysisTable();
expect(orderBy(analysisTable, ['fieldName', 'fieldValue'])).to.be.eql(
orderBy(testData.expected.analysisTable, ['fieldName', 'fieldValue'])
);
if (!isTestDataExpectedWithSampleProbability(testData.expected)) {
const analysisTable = await aiops.explainLogRateSpikesAnalysisTable.parseAnalysisTable();
expect(orderBy(analysisTable, ['fieldName', 'fieldValue'])).to.be.eql(
orderBy(testData.expected.analysisTable, ['fieldName', 'fieldValue'])
);
}

// Assert the field selector that allows to costumize grouping
await aiops.explainLogRateSpikesPage.assertFieldFilterPopoverButtonExists(false);
Expand All @@ -182,11 +197,14 @@ export default function ({ getPageObject, getService }: FtrProviderContext) {

if (testData.fieldSelectorApplyAvailable) {
await aiops.explainLogRateSpikesPage.clickFieldFilterApplyButton();
const filteredAnalysisGroupsTable =
await aiops.explainLogRateSpikesAnalysisGroupsTable.parseAnalysisTable();
expect(orderBy(filteredAnalysisGroupsTable, 'group')).to.be.eql(
orderBy(testData.expected.filteredAnalysisGroupsTable, 'group')
);

if (!isTestDataExpectedWithSampleProbability(testData.expected)) {
const filteredAnalysisGroupsTable =
await aiops.explainLogRateSpikesAnalysisGroupsTable.parseAnalysisTable();
expect(orderBy(filteredAnalysisGroupsTable, 'group')).to.be.eql(
orderBy(testData.expected.filteredAnalysisGroupsTable, 'group')
);
}
}
});
}
Expand Down
Loading