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 to React BrowserRouter and Kibana provided History. #71941

Merged
merged 37 commits into from
Jul 31, 2020

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jul 15, 2020

Summary

Part of #72013

  • Migrate to React BrowserRouter and Kibana provided History including a fallback to redirect legacy hash based URLs.
  • Migrate breadcrumbs away from hash based URLs.
  • Make sure relative custom urls still work after migration.

Checklist

For maintainers

@walterra walterra added :ml Feature:Anomaly Detection ML anomaly detection refactoring v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Data Frame Analytics ML data frame analytics features v7.10.0 labels Jul 15, 2020
@walterra walterra self-assigned this Jul 15, 2020
@walterra walterra marked this pull request as ready for review July 16, 2020 08:11
@walterra walterra requested a review from a team as a code owner July 16, 2020 08:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Jul 16, 2020

Clicking the root Machine Learning breadcrumb at the top of the page causes the app to crash.
image

it looks like it's navigating away from ML and clearing our dependency cache.

@jgowdyelastic
Copy link
Member

The first link click once the ML app has loaded throws this error in the console.

image

e.g. once the overview page has loaded, navigate to Anomaly Detection

@walterra
Copy link
Contributor Author

@jgowdyelastic Did the following fixes:

  • The legacy hash url check is now using useEffect() to avoid the "pure function" warning (2c02364).
  • Breadcrumbs have been rewritten to no longer use hash based URLs (635e32e) and to not trigger full page refreshes (85fec20).
  • Fixed navigation on the job type selection page (1d9c8b4).

};

const navigateToPath = (path: string | undefined) =>
navigateToUrl(getUrlForApp('ml', { path: `${path}${getUrlParams()}` }));
Copy link
Member

@jgowdyelastic jgowdyelastic Jul 17, 2020

Choose a reason for hiding this comment

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

seeing as we use this function combination a lot, could this be wrapped in a provider function that just takes the path and supplied by useMlContext ?
it should also use PLUGIN_ID from kibana/x-pack/plugins/ml/common/constants/app.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, decided to do it in this PR, we now have useNavigateToPath() based on useMlContext(), available via contexts/kibana. Update in fad85ec.

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

@qn895
Copy link
Member

qn895 commented Jul 29, 2020

@qn895 Looks like the navigation issue with Index Management wasn't introduced in this PR, see issue #72854 and PR #72863. For the other navigation problem I created a separate issue since it wasn't introduced by this PR: #73261.

Thanks for creating those issues 👍

Changes LGTM

@walterra
Copy link
Contributor Author

@elasticmachine merge upstream

window.open(fullUrl, '_blank');
// If `url` is a relative path, we need to prefix the base path.
if (url.charAt(0) !== '/') {
url = `${basePath}${isKibanaUrl(urlConfig) ? '/app/' : '/'}${url}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we need to add the security app URLs to this check. Currently this is trying to open up e.g.

1:46
http://localhost:5601/pgy/security/hosts/ml-hosts?_g=()&query=(query:%27process.name%20:%20%22filebeat%22%27,language:kuery)&timerange=(global:(linkTo:!(timeline),timerange:(from:%272019-08-20T23:00:00.000Z%27,kind:absolute,to:%272019-08-21T22:59:59.999Z%27)),timeline:(linkTo:!(global),timerange:(from:%272019-08-20T23:00:00.000Z%27,kind:absolute,to:%272019-08-21T22:59:59.999Z%27)))

which is missing the app part before security.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a fix in 1e64ab3.

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 latest edits and LGTM

@walterra
Copy link
Contributor Author

@elasticmachine merge upstream

@walterra
Copy link
Contributor Author

@elasticmachine merge upstream

@walterra
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ml 1225 +1 1224

page load bundle size

id value diff baseline
ml 4.3MB +9.8KB 4.3MB

History

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

@walterra walterra merged commit 145f2ee into elastic:master Jul 31, 2020
@walterra walterra deleted the ml-migrate-router branch July 31, 2020 08:25
walterra added a commit to walterra/kibana that referenced this pull request Jul 31, 2020
…stic#71941)

- Migrate to React BrowserRouter and Kibana provided History including a fallback to redirect legacy hash based URLs.
- Migrate breadcrumbs away from hash based URLs.
- Make sure relative custom urls still work after migration.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 31, 2020
* master: (54 commits)
  [ML] Migrate to React BrowserRouter and Kibana provided History. (elastic#71941)
  [Discover] Improve  saveSearch functional test handling (elastic#73626)
  [Metrics UI] Fix all threshold alert conditions disappearing due to alert prefill (elastic#73708)
  [Metrics UI] Fix alert previews of ungrouped alerts (elastic#73735)
  [SIEM] Fixes "include building block button" to operate (elastic#73900)
  [Metrics UI] Fix alert management to open without refresh (elastic#73739)
  [Security Solution][Lists] - Tests cleanup and remove unnecessary import (elastic#73865)
  [Ingest Management] main branch uses epr-snapshot. Others production (elastic#73555)
  [Canvas][tech-debt] Fix SVG not shrinking vertically properly (elastic#73867)
  [Maps] upgrade turf (elastic#73816)
  [Security Solution][Telemetry] Concurrent telemetry requests (elastic#73558)
  [Security Solution][Exceptions] - Update how nested entries are displayed in exceptions viewer (elastic#73745)
  [Security Solution][Exceptions] Adds autocomplete workaround for .text fields (elastic#73761)
  [Metrics UI] Fix previewing of No Data results (elastic#73753)
  Closes elastic#72914 by hiding anomaly detection settings links when the ml plugin is disabled. (elastic#73638)
  [Ingest Manager] Fix config selection in enrollment flyout from config list page (elastic#73833)
  [DOCS] Fixes typo in Alerting actions (elastic#73756)
  [APM] fixes linking errors to ML and Discover (elastic#73758)
  Handle promise rejections when building artifacts (elastic#73831)
  [Security Solution][Detections] Change from sha1 to sha256 (elastic#73741)
  ...
walterra added a commit that referenced this pull request Jul 31, 2020
) (#73919)

- Migrate to React BrowserRouter and Kibana provided History including a fallback to redirect legacy hash based URLs.
- Migrate breadcrumbs away from hash based URLs.
- Make sure relative custom urls still work after migration.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 31, 2020
* master: (38 commits)
  [Discover] Context unskip date nanos functional tests (elastic#73781)
  [ML] Migrate to React BrowserRouter and Kibana provided History. (elastic#71941)
  [Discover] Improve  saveSearch functional test handling (elastic#73626)
  [Metrics UI] Fix all threshold alert conditions disappearing due to alert prefill (elastic#73708)
  [Metrics UI] Fix alert previews of ungrouped alerts (elastic#73735)
  [SIEM] Fixes "include building block button" to operate (elastic#73900)
  [Metrics UI] Fix alert management to open without refresh (elastic#73739)
  [Security Solution][Lists] - Tests cleanup and remove unnecessary import (elastic#73865)
  [Ingest Management] main branch uses epr-snapshot. Others production (elastic#73555)
  [Canvas][tech-debt] Fix SVG not shrinking vertically properly (elastic#73867)
  [Maps] upgrade turf (elastic#73816)
  [Security Solution][Telemetry] Concurrent telemetry requests (elastic#73558)
  [Security Solution][Exceptions] - Update how nested entries are displayed in exceptions viewer (elastic#73745)
  [Security Solution][Exceptions] Adds autocomplete workaround for .text fields (elastic#73761)
  [Metrics UI] Fix previewing of No Data results (elastic#73753)
  Closes elastic#72914 by hiding anomaly detection settings links when the ml plugin is disabled. (elastic#73638)
  [Ingest Manager] Fix config selection in enrollment flyout from config list page (elastic#73833)
  [DOCS] Fixes typo in Alerting actions (elastic#73756)
  [APM] fixes linking errors to ML and Discover (elastic#73758)
  Handle promise rejections when building artifacts (elastic#73831)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Anomaly Detection ML anomaly detection Feature:Data Frame Analytics ML data frame analytics features :ml refactoring release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants