-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ML] Anomaly Explorer: Fixes handling of job group IDs when opening from dashboard panels #203224
Conversation
|
||
// Creates index pattern in the format expected by the kuery bar/kuery autocomplete provider | ||
// Field objects required fields: name, type, aggregatable, searchable | ||
export function getIndexPattern(influencers: ExplorerJob[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from x-pack/plugins/ml/public/application/explorer/reducers/explorer_reducer/get_index_pattern.ts
|
||
const refresh = useRefresh(); | ||
const lastRefresh = refresh?.lastRefresh ?? 0; | ||
|
||
// We cannot simply infer bounds from the globalState's `time` attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it’s no longer used, as I couldn’t get globalState.time.mode
to an invalid
state.
); | ||
|
||
useEffect(() => { | ||
if (!loadExplorerDataConfig || loadExplorerDataConfig?.selectedCells === undefined) return; | ||
// TODO: Find other way to set loading state as it causes unnecessary re-renders - handle it in anomaly_explorer_common_state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fixed when we decouple ExplorerState
into individual services for handling, e.g., chartsData
, influencers
, etc.
Pinging @elastic/ml-ui (:ml) |
@@ -143,3 +149,89 @@ describe('useUrlState', () => { | |||
expect(getByTestId('appState').innerHTML).toBe('the updated query'); | |||
}); | |||
}); | |||
|
|||
describe('usePageUrlState', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the motivation of using components in tests? did yo try rendering the hook itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I just aligned with the existing tests, but rendering the test component seems redundant, updated in: #7df5acb
anomalyExplorerCommonStateService.selectedJobs$, | ||
anomalyExplorerCommonStateService.selectedJobs | ||
); | ||
|
||
const selectedGroups = useObservable( | ||
anomalyExplorerCommonStateService.selectedGroups$, | ||
anomalyExplorerCommonStateService.selectedGroups | ||
); | ||
|
||
const mergedGroupsAndJobsIds = useMemo( | ||
() => getMergedGroupsAndJobsIds(selectedGroups, selectedJobs), | ||
[selectedGroups, selectedJobs] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I noticed it's been used in several places, maybe it's worth creating a hook useJobSelection
that return this data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Done in: #e854c74
@@ -56,7 +58,7 @@ export function getExplorerDefaultState(): ExplorerState { | |||
indexPattern: getDefaultIndexPattern(), | |||
influencers: {}, | |||
isAndOperator: false, | |||
loading: true, | |||
loading: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ is it a delbarate change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we use the loading
prop to determine whether to show no jobs selected
view: https://github.com/elastic/kibana/pull/203224/files#diff-ed860f8ee6aaa77969a5132f71414c4da278732ec436bd254ecfe4a4a625c89bL471
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
cc @rbrtj |
Starting backport for target branches: 8.x |
…ups & Explorer Refactor (elastic#203224) ## Summary Fixes issues around handling groupIds in Anomaly Explorer: [elastic#198824](elastic#198824) * `UrlStateService` is now more generic, allowing for the creation of `useGlobalUrlState` and `usePageUrlState` hooks based on it. * New `useGlobalUrlState` hook: Responsible for managing global state with proper typing. * `id_badges.js` converted to typescript -> `id_badges.tsx` * The use of `maps` in the `id_badges`, `job_selector` and `job_selector_flyout` components has been removed. Now, selected group and job ids are passed directly. * Instead of manually setting the global state in `job_selector`, an `onSelectionChange` callback is now expected. In the future, the `Timeseries explorer` and `Anomaly Explorer` are expected to use a (maybe) shared context to track selected jobs easily, eliminating the need to manually update the global state for the `Timeseries explorer`. * The `AnomalyExplorerCommonStateService` now listens for URL changes to track properties derived from the URL. This replaces the `useUrlState` hook, which lacked proper typing, and serves as the single source of truth for selected jobs and groups. * The `ExplorerDashboardService`, previously a hybrid of a "service" and redux-like action handler, has been removed. The state is now managed within `state_manager` component. In the future, data like `chartsData` and `influencers` should be decoupled from `ExplorerState` and moved to individual services. 1. Fix for all issues mentioned in elastic#198824 as visible in the video: - The initial group selection now results in the correct badge being displayed. - Creating an embeddable with a selected group properly propagates the selected group instead of individual job ids. - Navigating to the Anomaly Explorer from Dashboards now correctly uses the selected group instead of individual job ids. https://github.com/user-attachments/assets/82c641e0-d930-42eb-8627-fad4bdc417f6 --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit ea85f05)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…ed Groups & Explorer Refactor (#203224) (#204361) # Backport This will backport the following commits from `main` to `8.x`: - [[ML] Anomaly Explorer: Enhancements for Embeddables with Selected Groups & Explorer Refactor (#203224)](#203224) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Robert Jaszczurek","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-16T10:50:38Z","message":"[ML] Anomaly Explorer: Enhancements for Embeddables with Selected Groups & Explorer Refactor (#203224)\n\n## Summary\n\nFixes issues around handling groupIds in Anomaly Explorer:\n[#198824](https://github.com/elastic/kibana/issues/198824)\n\n* `UrlStateService` is now more generic, allowing for the creation of\n`useGlobalUrlState` and `usePageUrlState` hooks based on it.\n* New `useGlobalUrlState` hook: Responsible for managing global state\nwith proper typing.\n* `id_badges.js` converted to typescript -> `id_badges.tsx` \n* The use of `maps` in the `id_badges`, `job_selector` and\n`job_selector_flyout` components has been removed. Now, selected group\nand job ids are passed directly.\n* Instead of manually setting the global state in `job_selector`, an\n`onSelectionChange` callback is now expected. In the future, the\n`Timeseries explorer` and `Anomaly Explorer` are expected to use a\n(maybe) shared context to track selected jobs easily, eliminating the\nneed to manually update the global state for the `Timeseries explorer`.\n* The `AnomalyExplorerCommonStateService` now listens for URL changes to\ntrack properties derived from the URL. This replaces the `useUrlState`\nhook, which lacked proper typing, and serves as the single source of\ntruth for selected jobs and groups.\n* The `ExplorerDashboardService`, previously a hybrid of a \"service\" and\nredux-like action handler, has been removed. The state is now managed\nwithin `state_manager` component. In the future, data like `chartsData`\nand `influencers` should be decoupled from `ExplorerState` and moved to\nindividual services.\n\n1. Fix for all issues mentioned in #198824 as visible in the video:\n- The initial group selection now results in the correct badge being\ndisplayed.\n- Creating an embeddable with a selected group properly propagates the\nselected group instead of individual job ids.\n- Navigating to the Anomaly Explorer from Dashboards now correctly uses\nthe selected group instead of individual job ids.\n\n\nhttps://github.com/user-attachments/assets/82c641e0-d930-42eb-8627-fad4bdc417f6\n\n---------\n\nCo-authored-by: kibanamachine <[email protected]>","sha":"ea85f0551cea68b6aec8f1382fb859633c159e00","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix",":ml","Feature:Anomaly Detection","v9.0.0","Team:ML","backport:version","v8.18.0"],"title":"[ML] Anomaly Explorer: Enhancements for Embeddables with Selected Groups & Explorer Refactor","number":203224,"url":"https://github.com/elastic/kibana/pull/203224","mergeCommit":{"message":"[ML] Anomaly Explorer: Enhancements for Embeddables with Selected Groups & Explorer Refactor (#203224)\n\n## Summary\n\nFixes issues around handling groupIds in Anomaly Explorer:\n[#198824](https://github.com/elastic/kibana/issues/198824)\n\n* `UrlStateService` is now more generic, allowing for the creation of\n`useGlobalUrlState` and `usePageUrlState` hooks based on it.\n* New `useGlobalUrlState` hook: Responsible for managing global state\nwith proper typing.\n* `id_badges.js` converted to typescript -> `id_badges.tsx` \n* The use of `maps` in the `id_badges`, `job_selector` and\n`job_selector_flyout` components has been removed. Now, selected group\nand job ids are passed directly.\n* Instead of manually setting the global state in `job_selector`, an\n`onSelectionChange` callback is now expected. In the future, the\n`Timeseries explorer` and `Anomaly Explorer` are expected to use a\n(maybe) shared context to track selected jobs easily, eliminating the\nneed to manually update the global state for the `Timeseries explorer`.\n* The `AnomalyExplorerCommonStateService` now listens for URL changes to\ntrack properties derived from the URL. This replaces the `useUrlState`\nhook, which lacked proper typing, and serves as the single source of\ntruth for selected jobs and groups.\n* The `ExplorerDashboardService`, previously a hybrid of a \"service\" and\nredux-like action handler, has been removed. The state is now managed\nwithin `state_manager` component. In the future, data like `chartsData`\nand `influencers` should be decoupled from `ExplorerState` and moved to\nindividual services.\n\n1. Fix for all issues mentioned in #198824 as visible in the video:\n- The initial group selection now results in the correct badge being\ndisplayed.\n- Creating an embeddable with a selected group properly propagates the\nselected group instead of individual job ids.\n- Navigating to the Anomaly Explorer from Dashboards now correctly uses\nthe selected group instead of individual job ids.\n\n\nhttps://github.com/user-attachments/assets/82c641e0-d930-42eb-8627-fad4bdc417f6\n\n---------\n\nCo-authored-by: kibanamachine <[email protected]>","sha":"ea85f0551cea68b6aec8f1382fb859633c159e00"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/203224","number":203224,"mergeCommit":{"message":"[ML] Anomaly Explorer: Enhancements for Embeddables with Selected Groups & Explorer Refactor (#203224)\n\n## Summary\n\nFixes issues around handling groupIds in Anomaly Explorer:\n[#198824](https://github.com/elastic/kibana/issues/198824)\n\n* `UrlStateService` is now more generic, allowing for the creation of\n`useGlobalUrlState` and `usePageUrlState` hooks based on it.\n* New `useGlobalUrlState` hook: Responsible for managing global state\nwith proper typing.\n* `id_badges.js` converted to typescript -> `id_badges.tsx` \n* The use of `maps` in the `id_badges`, `job_selector` and\n`job_selector_flyout` components has been removed. Now, selected group\nand job ids are passed directly.\n* Instead of manually setting the global state in `job_selector`, an\n`onSelectionChange` callback is now expected. In the future, the\n`Timeseries explorer` and `Anomaly Explorer` are expected to use a\n(maybe) shared context to track selected jobs easily, eliminating the\nneed to manually update the global state for the `Timeseries explorer`.\n* The `AnomalyExplorerCommonStateService` now listens for URL changes to\ntrack properties derived from the URL. This replaces the `useUrlState`\nhook, which lacked proper typing, and serves as the single source of\ntruth for selected jobs and groups.\n* The `ExplorerDashboardService`, previously a hybrid of a \"service\" and\nredux-like action handler, has been removed. The state is now managed\nwithin `state_manager` component. In the future, data like `chartsData`\nand `influencers` should be decoupled from `ExplorerState` and moved to\nindividual services.\n\n1. Fix for all issues mentioned in #198824 as visible in the video:\n- The initial group selection now results in the correct badge being\ndisplayed.\n- Creating an embeddable with a selected group properly propagates the\nselected group instead of individual job ids.\n- Navigating to the Anomaly Explorer from Dashboards now correctly uses\nthe selected group instead of individual job ids.\n\n\nhttps://github.com/user-attachments/assets/82c641e0-d930-42eb-8627-fad4bdc417f6\n\n---------\n\nCo-authored-by: kibanamachine <[email protected]>","sha":"ea85f0551cea68b6aec8f1382fb859633c159e00"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Robert Jaszczurek <[email protected]>
…ups & Explorer Refactor (elastic#203224) ## Summary Fixes issues around handling groupIds in Anomaly Explorer: [elastic#198824](elastic#198824) * `UrlStateService` is now more generic, allowing for the creation of `useGlobalUrlState` and `usePageUrlState` hooks based on it. * New `useGlobalUrlState` hook: Responsible for managing global state with proper typing. * `id_badges.js` converted to typescript -> `id_badges.tsx` * The use of `maps` in the `id_badges`, `job_selector` and `job_selector_flyout` components has been removed. Now, selected group and job ids are passed directly. * Instead of manually setting the global state in `job_selector`, an `onSelectionChange` callback is now expected. In the future, the `Timeseries explorer` and `Anomaly Explorer` are expected to use a (maybe) shared context to track selected jobs easily, eliminating the need to manually update the global state for the `Timeseries explorer`. * The `AnomalyExplorerCommonStateService` now listens for URL changes to track properties derived from the URL. This replaces the `useUrlState` hook, which lacked proper typing, and serves as the single source of truth for selected jobs and groups. * The `ExplorerDashboardService`, previously a hybrid of a "service" and redux-like action handler, has been removed. The state is now managed within `state_manager` component. In the future, data like `chartsData` and `influencers` should be decoupled from `ExplorerState` and moved to individual services. 1. Fix for all issues mentioned in elastic#198824 as visible in the video: - The initial group selection now results in the correct badge being displayed. - Creating an embeddable with a selected group properly propagates the selected group instead of individual job ids. - Navigating to the Anomaly Explorer from Dashboards now correctly uses the selected group instead of individual job ids. https://github.com/user-attachments/assets/82c641e0-d930-42eb-8627-fad4bdc417f6 --------- Co-authored-by: kibanamachine <[email protected]>
Summary
Fixes issues around handling job group IDs in Anomaly Explorer: #198824.
Also does the following refactors:
UrlStateService
is now more generic, allowing for the creation ofuseGlobalUrlState
andusePageUrlState
hooks based on it.useGlobalUrlState
hook: Responsible for managing global state with proper typing.id_badges.js
converted to typescript ->id_badges.tsx
maps
in theid_badges
,job_selector
andjob_selector_flyout
components has been removed. Now, selected group and job ids are passed directly.job_selector
, anonSelectionChange
callback is now expected. In the future, theTimeseries explorer
andAnomaly Explorer
are expected to use a (maybe) shared context to track selected jobs easily, eliminating the need to manually update the global state for theTimeseries explorer
.AnomalyExplorerCommonStateService
now listens for URL changes to track properties derived from the URL. This replaces theuseUrlState
hook, which lacked proper typing, and serves as the single source of truth for selected jobs and groups.ExplorerDashboardService
, previously a hybrid of a "service" and redux-like action handler, has been removed. The state is now managed withinstate_manager
component. In the future, data likechartsData
andinfluencers
should be decoupled fromExplorerState
and moved to individual services.Screen.Recording.2024-12-09.at.13.57.25.mov