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.

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] Data Frame Analytics: add analytics ID to url when using selector flyout #128222

Merged

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Mar 22, 2022

Summary

When using the DFA job selector flyout - ensure url is updated with the selected analytics ID so that the url is shareable.
This PR also fixes the model id selection for the explorer.

Checklist

Delete any items that are not applicable to this PR.

  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

@alvarezmelissa87 alvarezmelissa87 added :ml release_note:skip Skip the PR/issue when compiling release notes Feature:Data Frame Analytics ML data frame analytics features v8.2.0 labels Mar 22, 2022
@alvarezmelissa87 alvarezmelissa87 self-assigned this Mar 22, 2022
@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner March 22, 2022 00:52
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Comment on lines +57 to +69
useEffect(
function updateUrl() {
if (analyticsId !== undefined) {
setGlobalState({
ml: {
...(analyticsId.analysis_type ? { analysisType: analyticsId.analysis_type } : {}),
...(analyticsId.job_id ? { jobId: analyticsId.job_id } : {}),
},
});
}
},
[analyticsId?.job_id, analyticsId?.model_id]
);
Copy link
Contributor

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
Contributor

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
Contributor Author

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
Contributor

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.

@qn895
Copy link
Member

qn895 commented Mar 22, 2022

Tested and functionally LGTM 🎉

@alvarezmelissa87
Copy link
Contributor Author

This has been updated and is ready for another look when you get a chance 🙏 cc @peteharverson, @darnautov

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-dfa-selected-id-in-url branch from 2c2bd35 to 1e70565 Compare March 24, 2022 16:44
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.3MB 3.3MB +447.0B

History

  • 💚 Build #32800 succeeded 2c2bd35fbc21510cfedfa112d0d1379c6681feac
  • 💔 Build #32766 failed 7216254129e9ad62dfbc6fe184e7d54117948481
  • 💚 Build #32480 succeeded c11c4b7d0fa88aed8388cb75ed5ead429c09ec47
  • 💔 Build #32466 failed 031de3fb3bbfd68abe19d55fd595d34f9b9f2426

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @alvarezmelissa87

@alvarezmelissa87 alvarezmelissa87 merged commit d7a3335 into elastic:main Mar 24, 2022
@alvarezmelissa87 alvarezmelissa87 deleted the ml-dfa-selected-id-in-url branch March 24, 2022 18:26
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 28, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 128222 or prevent reminders by adding the backport:skip label.

@peteharverson peteharverson added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Data Frame Analytics ML data frame analytics features :ml release_note:skip Skip the PR/issue when compiling release notes v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants