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] Migrate machine learning URLs to BrowserRouter format for APM, Security, and Infra #78209

Merged
merged 26 commits into from
Sep 30, 2020

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Sep 22, 2020

Summary

This PR replaces machine learning URLs from the previous hash-based paths to the new paths as part of the migration to BrowserRouter #72013. Updates were made to APM, Infra, and Security solutions plugins.

Changes include:

  • We expose the mlUrlGenerator on ML plugin’s contract. Then consumers would declare that they need ml as a dependency and would access mlUrlGenerator from deps.mlUrlGenerator passed in start or setup hooks.
  • Updating the Anomaly Detection job list to accept list of jobIds in the URL. This is because some plugins are passing in array of jobIds, and this is not currently supported.
  • Fall back to go to basePath/app/ml if ml.urlGenerator is not available to generate the urls for some reasons.

Checklist

Delete any items that are not applicable to this PR.

…place-hash

# Conflicts:
#	x-pack/plugins/apm/public/components/shared/Links/MachineLearningLinks/MLJobLink.test.tsx
#	x-pack/plugins/apm/public/components/shared/Links/MachineLearningLinks/MLLink.test.tsx
#	x-pack/plugins/apm/public/components/shared/Links/MachineLearningLinks/MLLink.tsx
#	x-pack/plugins/apm/public/components/shared/Links/MachineLearningLinks/useTimeSeriesExplorerHref.test.ts
#	x-pack/plugins/apm/public/components/shared/Links/MachineLearningLinks/useTimeSeriesExplorerHref.ts
#	x-pack/plugins/ml/public/plugin.ts
@qn895 qn895 added :ml refactoring v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 22, 2020
@qn895 qn895 self-assigned this Sep 22, 2020
@qn895 qn895 marked this pull request as ready for review September 22, 2020 23:47
@qn895 qn895 requested review from a team as code owners September 22, 2020 23:47
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

@qn895 these changes look good to me! With the additional state added by this hook we're getting some warnings from jest, but I've got a commit that fixes those here. Up to you whether to include those changes here or not, I can always follow up afterward.

@@ -113,6 +114,12 @@ export const createStartServicesMock = (): StartServices => {
},
security,
storage,
ml: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not critical, but: it would be nice if ml provided a mock for this contract!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Ryland! Updated here 1c390c1

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id value diff baseline
ml 1186 +1 1185

async chunks size

id value diff baseline
apm 4.1MB +935.0B 4.1MB
infra 4.3MB +8.0B 4.3MB
ml 10.5MB -16.7KB 10.5MB
securitySolution 10.3MB +801.0B 10.3MB
total -15.0KB

page load bundle size

id value diff baseline
apm 44.3KB +229.0B 44.1KB
ml 58.5KB +18.3KB 40.3KB
securitySolution 581.2KB +229.0B 581.0KB
total +18.7KB

History

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

@qn895 qn895 merged commit 2344dcf into elastic:master Sep 30, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 1, 2020
* master: (36 commits)
  [I18n] fix eui tokens (elastic#78951)
  Changed the color of the confirm button in trusted app deletion dialog. (elastic#78768)
  Make the actual Vislib import async (elastic#78949)
  Fix ML conditionals links Cypress tests (elastic#78568)
  [Drilldowns][Docs] Communicate the visualization types that support drilldowns (elastic#78761)
  [UX] Improve page-load axis (elastic#78392)
  [SECURITY SOLUTIONS] Map embeddable working with index patterns selection (elastic#78610)
  Data plugin README (elastic#78750)
  [TSVB] Request validation error: [panels.0.series.0.metrics.0.percentiles.1.value] (elastic#79009)
  fixing api test (elastic#78964)
  [Task names in TaskManager] Rename "telemetry" to "usage" (elastic#78129)
  [Loggers] Rename "telemetry" to "usage" (elastic#78130)
  [Usage Collection] [schema] `ui_metric` (elastic#78827)
  [Actions][Jira] Set parent issue for Sub-task issue type (elastic#78772)
  [Discover] Unskip doc link functional test (elastic#78600)
  [ML] Functional tests - stabilize calendar edit tests (elastic#78950)
  [UX] Improve page responsive  (elastic#78759)
  [QA][Code Coverage] Team Assignment Docs Update (elastic#78890)
  [ML] Migrate machine learning URLs to BrowserRouter format for APM, Security, and Infra  (elastic#78209)
  [ts] enable "resolveJsonModule" and disable existing failures (elastic#78855)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 2, 2020
@kibanamachine
Copy link
Contributor

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

qn895 added a commit to qn895/kibana that referenced this pull request Oct 5, 2020
…ecurity, and Infra (elastic#78209)

* [ML] Adds ability to pass multiple jobIds to job management url

* [ML][APM] Update links to jobs management page for MLLink and LegacyJobsCallout

* [ML][APM] Update useTimeSeriesExplorerHref

* [ML][APM] Update tests

* [ML][APM] Move test from useTimeSeriesExplorerHref to MLJobLink.test.tsx

* [ML][Infra] Update ML links in infra to non-hash paths

* [ML] Move MlUrlGenerator registration outside of licensing block for security solution

* [ML][Security] Update ml links in security

* [ML][APM] Update test snapshots

* [ML][APM] Update snapshots

* [ML][Security solution] Update tests

* [ML] Update MLLink to include globalState

* [ML] Update useTimeSeriesExplorerHref

* [ML] Update apm and security_solution to use useMlHref hook

* [ML] Update APM to use useUrlParams hook, update security solution hook

* [ML] Update tests, fix duplicate imports

* [ML] Update imports, remove ml exports to shared cause it's not needed

[ML] Add import

* [ML] Update snapshot

* [ML] Fix warnings for jobs_table.test.tsx

Co-authored-by: Elastic Machine <[email protected]>
# Conflicts:
#	x-pack/plugins/apm/public/utils/testHelpers.tsx
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 5, 2020
qn895 added a commit that referenced this pull request Oct 5, 2020
@qn895 qn895 deleted the ml-exernal-url-replace-hash branch October 7, 2020 15:23
@smith smith self-assigned this Oct 19, 2020
@smith
Copy link
Contributor

smith commented Oct 21, 2020

Tested in Safari, Chrome, and Firefox:

@smith smith added the apm:test-plan-done Pull request that was successfully tested during the test plan label Oct 21, 2020
@smith smith removed their assignment Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:test-plan-done Pull request that was successfully tested during the test plan :ml refactoring release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants