Skip to content

[ML] Data Frame Analytics: add analytics ID to url when using selector flyout #128222

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

Merged
alvarezmelissa87 merged 5 commits into from
Mar 24, 2022
Merged

[ML] Data Frame Analytics: add analytics ID to url when using selector flyout #128222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e52e463
fix model selection for exploration page
alvarezmelissa87 Mar 22, 2022
6845b03
add selected analyticsId to url
alvarezmelissa87 Mar 22, 2022
6325e2f
linting fix
alvarezmelissa87 Mar 22, 2022
ba88ada
ensure selected id can be deselected
alvarezmelissa87 Mar 22, 2022
1e70565
fix linting error
alvarezmelissa87 Mar 22, 2022
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
17 changes: 17 additions & 0 deletions ...k/plugins/ml/public/application/data_frame_analytics/pages/analytics_exploration/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { useMlKibana, useMlApiContext } from '../../../contexts/kibana';
import { MlPageHeader } from '../../../components/page_header';
import { AnalyticsIdSelector, AnalyticsSelectorIds } from '../components/analytics_selector';
import { AnalyticsEmptyPrompt } from '../analytics_management/components/empty_prompt';
import { useUrlState } from '../../../util/url_state';

export const Page: FC<{
jobId: string;
Expand All @@ -37,6 +38,8 @@ export const Page: FC<{
const jobIdToUse = jobId ?? analyticsId?.job_id;
const analysisTypeToUse = analysisType || analyticsId?.analysis_type;

const [, setGlobalState] = useUrlState('_g');

const checkJobsExist = async () => {
try {
const { count } = await getDataFrameAnalytics(undefined, undefined, 0);
Expand All @@ -51,6 +54,20 @@ export const Page: FC<{
checkJobsExist();
}, []);

useEffect(
function updateUrl() {
if (analyticsId !== undefined) {
setGlobalState({
ml: {
...(analyticsId.analysis_type ? { analysisType: analyticsId.analysis_type } : {}),
Comment thread
peteharverson marked this conversation as resolved.
Outdated
...(analyticsId.job_id ? { jobId: analyticsId.job_id } : {}),
},
});
}
},
[analyticsId?.job_id, analyticsId?.model_id]
);
Comment on lines 57 to 69
Copy link
Copy Markdown
Contributor

@darnautov darnautov Mar 22, 2022

Choose a reason for hiding this comment

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

shall we store it in the _a state instead? the _g is intended to be used as a global Kibana URL state

Copy link
Copy Markdown
Contributor

@darnautov darnautov Mar 22, 2022

Choose a reason for hiding this comment

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

I reckon we don't need an intermediate analyticsId state here. The value should be provided directly from the URL and updated as well

Copy link
Copy Markdown
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Mar 22, 2022

Choose a reason for hiding this comment

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

In this case the value wouldn't be in the url - we're adding it into the url when the user selects the analytics id from the selection flyout. Moving it to _a would require some refactoring so I think it's outside the scope of this for now and better suited as an improvements or maintenance task.

Copy link
Copy Markdown
Contributor

@peteharverson peteharverson Mar 22, 2022

Choose a reason for hiding this comment

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

The _g parameter was originally intended for any settings which might want to be preserved across pages. This is why we put the job ID for the anomaly detection pages in _g, so that on switching from the Anomaly Explorer to the Single Metric Viewer for example, the selected job would be retained. So using _g would make sense here too, so that in theory the job would be retained if the user switched from the Results view to the Map view. Whether using _g and _a in the long term still makes sense is another matter entirely, but here makes total sense to use _g for analytics job ID.


const getEmptyState = () => {
if (jobsExist === false) {
return <AnalyticsEmptyPrompt />;
Expand Down
8 changes: 5 additions & 3 deletions ...cation/data_frame_analytics/pages/components/analytics_selector/analytics_id_selector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,13 @@ export function AnalyticsIdSelector({ setAnalyticsId, jobsOnly = false }: Props)

const selectionValue = {
selectable: (item: TableItem) => {
const selectedId = selected?.job_id ?? selected?.model_id;
const isDFA = isDataFrameAnalyticsConfigs(item);
const itemId = isDFA ? item.id : item.model_id;
const isBuiltInModel = isDFA ? false : item.tags.includes(BUILT_IN_MODEL_TAG);
return (selected === undefined || selectedId === itemId) && !isBuiltInModel;
return (
(selected === undefined || selected?.job_id === itemId || selected?.model_id === itemId) &&
!isBuiltInModel
);
},
onSelectionChange: (selectedItem: TableItem[]) => {
const item = selectedItem[0];
Expand All @@ -208,7 +210,7 @@ export function AnalyticsIdSelector({ setAnalyticsId, jobsOnly = false }: Props)

setSelected({
model_id: isDFA ? undefined : item.model_id,
job_id: isDFA ? item.id : undefined,
job_id: isDFA ? item.id : item.metadata?.analytics_config.id,
analysis_type: analysisType,
});
},
Expand Down
16 changes: 15 additions & 1 deletion x-pack/plugins/ml/public/application/data_frame_analytics/pages/job_map/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { AnalyticsIdSelector, AnalyticsSelectorIds } from '../components/analyti
import { AnalyticsEmptyPrompt } from '../analytics_management/components/empty_prompt';

export const Page: FC = () => {
const [globalState] = useUrlState('_g');
const [globalState, setGlobalState] = useUrlState('_g');
const [isLoading, setIsLoading] = useState(false);
const [jobsExist, setJobsExist] = useState(true);
const { refresh } = useRefreshAnalyticsList({ isLoading: setIsLoading });
Expand Down Expand Up @@ -51,6 +51,20 @@ export const Page: FC = () => {
checkJobsExist();
}, []);

useEffect(
function updateUrl() {
if (analyticsId !== undefined) {
setGlobalState({
ml: {
...(analyticsId.job_id && !analyticsId.model_id ? { jobId: analyticsId.job_id } : {}),
...(analyticsId.model_id ? { modelId: analyticsId.model_id } : {}),
},
});
}
},
[analyticsId?.job_id, analyticsId?.model_id]
);

const getEmptyState = () => {
if (jobsExist === false) {
return <AnalyticsEmptyPrompt />;
Expand Down