[ML] AIOps: Fix race condition where stale url state would reset search bar. #154885
Conversation
walterra
added
bug
|
Pinging @elastic/ml-ui (:ml) |
| @@ -45,9 +45,6 @@ export const useData = ( | |||
| } = useAiopsAppContext(); | |||
|
|
|||
| const [lastRefresh, setLastRefresh] = useState(0); | |||
There was a problem hiding this comment.
I reckon we should get rid of this local state and use useRefresh and useTimeRangeUpdates hooks instead.
There was a problem hiding this comment.
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.
| }; | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [lastRefresh, searchQuery]); |
There was a problem hiding this comment.
and afterwards fieldStatsRequest should be triggered based on explicit refresh or time range updates.
| const timefilter = useTimefilter({ | ||
| timeRangeSelector: selectedDataView?.timeFieldName !== undefined, | ||
| autoRefreshSelector: true, | ||
| }); | ||
|
|
||
| const fieldStatsRequest: DocumentStatsSearchStrategyParams | undefined = useMemo(() => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @walterra |
Summary
Fixes an issue there the global state
_gand app state_awould get out of sync and overwrite each other. For example, a click on Refresh in the date picker (global state) could reset the search bar (app state) to empty.The issue was that in
x-pack/packages/ml/url_state/src/url_state.tsxthesearchStringcould become a stale value insetUrlState. This PR fixes it by using the approach already used inusePageUrlState: ThesearchStringis passed on to be stored viauseRefso that thesetUrlStatesetter can always access the most recent value.The exact sequence of actions to trigger the bug is a bit cumbersome to find, it doesn't always happen. Functional tests have been extended to cover the case reliably, actually it was these functional tests which surfaced the bug.
use_data.tshas been refactored to rely less on callbacks and useuseMemoinstead.Checklist