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

Switch to core application service #63443

Merged
merged 221 commits into from
May 13, 2020

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Apr 14, 2020

Fixes #61304
Fixes #49060

This PR switches the Kibana app apps (Home, Lens, Visualize, Discover, Dashboard, Dev tools) from the legacy app renderer to the core application service.

Did you get here because your team got pinged and you are wondering what this is about?

This is not a rebase gone bad as it happens from time to time.

A lot of teams ended up in this PR because of changed links to the home application - all links change from app/kibana#/home/... to app/home#/.... If this is the case for your code area, then the only thing to do is to make sure these links still end up at the right place (if your app is already in the new platform, there should be no page load when going there).

How to test this PR

This is a big PR that makes changes in a lot of different places. This means it's also not easy to test. This is a (non-exhaustive) list of things which can be used as starters, but be creative :)

  • Switching from app to app should work without page reload (for the Kibana app apps and some solution apps) for all navigations (the "Visualize" button in Discover, the "Edit panel" in the context menu on dashboards, the "Add new" on dashboards, ...)
  • Switching from Kibana app app to management sections now causes a full page reload (it didn't previously). This is also the case for navigations to management in case of missing index patterns, saved searches and so on
  • The title of the page should stay consistent with before this change
  • All old URLs should still redirect correctly to their new counterparts
  • The deprecated dashboard_only_user role should still work
  • The defaultAppId config key should still work
  • The defaultRoute ui setting should still work
  • As a lot of stuff is lazy loaded now when an app is actually accessed, it's necessary to make sure all apps load when the page is refreshed when viewing them, as well as when the user navigates to the app from another app
  • Short urls should still work (even when pointing to the legacy URLs)
  • URL state should still be synced correctly in all cases.

In general, it helps to start two instances of Kibana (this PR and master) in parallel to see how they differentiate.

Notable changes

URL changes

This PR changes the URLs of some apps:

  • home: app/kibana#/home -> app/home#/
  • discover: app/kibana#/discover -> app/discover#/
  • visualize: app/kibana#/visualize -> app/visualize#/
  • dashboard: app/kibana#/dashboards -> app/dashboards#/list
  • dev_tools: app/kibana#/dev_tools -> app/dev_tools#/
  • lens: app/kibana#/lens -> app/lens#/

For all apps, sub paths change by having the app name removed as part of the hash. The only exception doing other changes as well is the dashboards app, like discussed here: #49060

This PR does not change from hash history to browser history to keep the change manageable.

Old URLs should be forwarded automatically. The definitions for this are located in the plugins itself (like here: https://github.com/elastic/kibana/pull/63443/files#diff-673a132a1b2e5bef49aaefa30ee13899R76)

The majority of touched files is due to this change (functional test adjustments and crosslinking)

Page reload when going to management

As the management app is still located in the legacy platform, going from Dashboard/Discover/Visualize/Home to Management now causes a page reload.

Home logo

The home logo now uses the navigateToApp service to navigate to the home app because the old way of just using a native anchor tag would cause a full page reload.

Scoped history in-app navigation

When triggering navigations within the same app (just changing the hash part of the url) using the scoped history object provided by core, the hash-change event is not triggered, causing angular routing, the kbn url tracker and state sync utilities to not pick up this change correctly.

This PR refactors the kbn url tracker to work with browser history instead of hash history objectsplugin.ts
https://github.com/elastic/kibana/pull/63443/files#diff-6ddbb6d7fc0fa22c821b2091a1d154ea

For the state sync utilities this change would be more difficult and for angular it's downright impossible to use the scoped history object, so
https://github.com/elastic/kibana/pull/63443/files#diff-82fbb8f394a64ab0b3d09519e72ea36fR150 makes sure that in apps requiring this behavior, a navigation change triggered by the global scoped history object is always triggering a hashchange event as well which is notifying the hash history objects and angular routing.

This behavior is actively used in two places: Clicking the nav link of the currently active app and using drilldowns on dashboards.

Telemetry access right probing

The telemetry plugin is requesting a saved object on app change which might not be available. If this is the case, the saved object request will fail. This becomes a problem when it is batched together with a saved object request of the app currently mounting, because that request will fail as well. This PR switches to the bulkGet api instead of the regular get api to avoid the telemetry request getting batched with other requests.

https://github.com/elastic/kibana/pull/63443/files#diff-422a281bf153a09aa4a2ab8b77df1867

Security behavior change

In the legacy kibana app, users would get redirected to home if their access rights wouldn't match the requested app. Now, as they are split out into regular new platform applications, the request fails already at the server side and just shows a technical 404 page: https://github.com/elastic/kibana/pull/63443/files#diff-3da7b4ce0de91b903189fde128a83a79R425

Alias vis type interface change

When navigating to a visualization editor from the new vis modal, we can't just assign a new location because it would trigger a page load. Because of this, the vis alias type is now not featuring a aliasUrl, but an aliasApp and an aliasPath, to make it possible to trigger navigation using navigateToApp
https://github.com/elastic/kibana/pull/63443/files#diff-9eb2b74fb351fd8d9b9729cc8ac8c553

Edit panel navigation change

The edit panel action currently directly sets window.location.href. This causes a page load when the path is not the same as in the current url. To avoid this problem, this PR makes use of the editPath and editApp embeddable inputs to trigger a navigation without page reload instead.
https://github.com/elastic/kibana/pull/63443/files#diff-dbb0b091a47e51c3022284d8e2ad4d70

"Back to dashboard" navigation change

To navigate back to the dashboard after editing an embeddable, lens and visualize used to mess with the dashboard url directly plucked from the navlink. This doesn't work anymore in the new platform. This PR moves this responsibility to the dashboard plugin. It exposes an API which takes the user back to the last edited dashboard while adding the embeddable to add to the URL.

This also allows us to get rid of the duplicated helpers for this.

Dashboard mode partial migration

Dashboard mode is slated to be removed with 8.0, but for this cutover it was necessary to migrate the server part to the new platform - otherwise it can't control the functionality and hide other navlinks. The server part of dashboard mode was changed to redirect to the NP app if the user has the dashboard only user role. The app restricts the UI as expected, migrates the path if necessary and then loads the actual dashboard app which handles the rest of the user experience.

New dashboard mode plugin: https://github.com/elastic/kibana/pull/63443/files#diff-ee10a7f4fda8f42acda3547c487ec3ab

Getting rid of redirect message helper

The redirect message helper was used to load the home page and show an error toast. It was only used in Graph to redirect away if the licensing level is not sufficient. As this navigation now happens without a page load, Graph can simply show the toast and then navigate to the home app using core services.

Temporary workaround for "not found" messages in management

The thing that could be removed for Graph now doesn't work anymore for the banners shown in management when the user gets redirected because there are no index patterns or a saved object couldn't be found when trying to access it in Visualize.

This PR implements a workaround for this by adding the message to the URL before redirecting. The legacy management app picks up the URL parameter and shows the banner

https://github.com/elastic/kibana/pull/63443/files#diff-024022209a9bd71018503285c51df1ca
https://github.com/elastic/kibana/pull/63443/files#diff-ceac0240dab178033ddad287293d7f11R62

This can get removed once management is also rendered in the new platform.

Temporary workaround for passing legacy settings to input control vis

The input control vis needs access to the legacy yml settings kibana.autocompleteTerminateAfter and kibana.autocompleteTimeout. These are currently only exposed on the server side and can't use the exposeToBrowser property to bring them to the client. To unblock this PR, I put a workaround in place which introduces a new API which exposes these settings. When the input control vis is used, it fetches these settings once and then operates on the cache. Once these settings are properly migrated (e.g. to the data plugin), this can be cleaned up by using exposeToBrowser and client side plugin contracts instead.

https://github.com/elastic/kibana/pull/63443/files#diff-d7eb42f5ac5ddbbc41af5644c3138ba8R67
https://github.com/elastic/kibana/pull/63443/files#diff-42b6b53a2fe8aa6213120f242562b4ecR34

Setting doctitle when mounting an app

When switching between apps, the page reload often took care to correctly reset the title. This doesn't work anymore because a lot of app switches are now happening on the same page. All apps should take care of setting the title to the correct value when a navigation happens.

Using navigateToApp for home tiles to avoid page load

The tiles on the home page link to a lot of apps. To make sure navigateToApp is used when possible, this PR introduced a helper that checks the URL and uses navigateToApp when possible.

https://github.com/elastic/kibana/pull/63443/files#diff-0a07343036159004454ec36157f72dd9

defaultAppId behavior

There is a deprecated yml setting defaultAppId which specifies the app kibana navigates to when the requested url is not matching anything else. To keep this behavior as close as possible, the helper implemented in https://github.com/elastic/kibana/pull/63443/files#diff-643a25a80e351c5303b4866205fd915b will take the current path, try to migrate it and navigate to the respective place.

Remove discover link from context error message

When the context page can't load its anchor document, it will show a message to the user explaining the problem offering a link to go back to "the discover app". This link is fetched directly from the nav link which doesn't work anymore in the new platform. As context and discover are now the same app anyway and the back button has the same functionality, this navigation didn't seem relevant enough to implement a lot of code to fetch the last visited url. This PR simplifies the message and tells the user to just go back which seems sufficient for this rare edge case.

https://github.com/elastic/kibana/pull/63443/files#diff-a534ce79cd4b61e4ed069c191664b8b7R47
https://github.com/elastic/kibana/pull/63443/files#diff-4c23724f1b54556ac8f97bfb5bdf3e17L89

Create app wrapper styles for each app separately

In the current state on master, apps rely on the styling put in place by the local application service to be styled correctly. To avoid a dependency to legacy styling in each app, this PR creates separate stylesheets for the individual cases (e.g. https://github.com/elastic/kibana/pull/63443/files#diff-b3f4a09b020ab577358b8d3db8c0bc58R111 )

Migrating short URLs

Existing short URLs could still point to legacy URLs. Those would get forwarded fine, but cause a double page load - once for the legacy platform just to redirect to the new platform directly after. Unfortunately migrating the URLs directly to the new URL is not trivial because this information is not available during migration (as it happens before other plugins have the chance to register their forwarded URLs). To avoid making the short url migration directly aware of all existing URL schemes, this PR migrates all short urls pointing to the kibana app by moving them to the url_migrate app without changing the hash. This app will do the redirect normally happening in the legacy platform completelty in the new platform. https://github.com/elastic/kibana/pull/63443/files#diff-b9927d073469e0dfc479a59eac402cef
Also this redirect app will become handy once the legacy platform is turned off completely: #61308

sulemanof and others added 30 commits April 2, 2020 11:49
# Conflicts:
#	src/legacy/core_plugins/kibana/index.js
#	src/legacy/core_plugins/kibana/public/kibana.js
#	src/legacy/core_plugins/kibana/public/visualize/_index.scss
#	src/legacy/core_plugins/kibana/public/visualize/legacy_imports.ts
#	src/plugins/dashboard/public/index.ts
#	src/plugins/vis_default_editor/public/default_editor.tsx
#	src/plugins/vis_default_editor/public/default_editor_controller.tsx
#	src/plugins/visualize/public/kibana_services.ts
#	src/plugins/visualize/public/plugin.ts
Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

These are only SIEM changes and they've already been approved so 👍

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I retested (in Chrome, sorry), and the issues I raised previously have been fixed. I'd recommend (if you haven't already) to definitely check in IE to make sure the layouts still work.

@ThomThomson
Copy link
Contributor

Added a quick change which prevents a load screen when switching back to canvas from lens and visualize. This makes the issues in Canvas much more apparent, but also aligns better with the goals of this PR

Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

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

Overall changes look good to me!
A lot of things were discussed/fixed offline.
Was mostly focused on testing transitions through Discover/Visualize/Dashboard/Home/Graph/Canvas/Management in Chrome browser.
Great work @flash1293 ! And a great step for Kibana 🎉

P.S.: Didn't look through >300 files, but quickly look at some points. Left the last nit, but feel free to forget it 😆

@@ -33,7 +33,7 @@ export default function({ getService, getPageObjects }: FtrProviderContext) {

describe('Print PDF button', () => {
it('is not available if new', async () => {
await PageObjects.common.navigateToUrl('visualize', 'new');
await PageObjects.common.navigateToUrl('visualize', 'new', { useActualUrl: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would this work PageObjects.common.navigateToActualUrl('visualize', 'new') ?

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM!

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293 flash1293 merged commit b7e87c2 into elastic:master May 13, 2020
flash1293 added a commit to flash1293/kibana that referenced this pull request May 13, 2020
@sulemanof sulemanof mentioned this pull request May 13, 2020
7 tasks
v1v added a commit to v1v/kibana that referenced this pull request May 13, 2020
* upstream/master: (223 commits)
  [Ingest] Support root level yaml variables in agent stream template (elastic#66120)
  [Snapshot Restore] Fix error when deleting snapshots behind reverse proxy (elastic#66147)
  [Lens] fix empty state for pie (elastic#66206)
  [APM] Improve e2e tests (elastic#66373)
  [ML] Data Frame Analytics: Fix steps to be named phases. (elastic#65855)
  [Discover] Encode context link filter part (elastic#63831)
  [APM] Scope APM alert creation to environment (elastic#65681)
  Move Kibana Usage collectors outside the telemetry plugin (elastic#65663)
  [ML] Data Frame Analytics: Fix confusion matrix data grid width. (elastic#65818)
  Switch to core application service (elastic#63443)
  Removes use of prefer_v2_templates (elastic#66316)
  [Maps] handle case where fit to bounds does not match any documents (elastic#66307)
  log error instead of throw (elastic#66254)
  [plugin-helpers] remove outdated postinstall task (elastic#66324)
  Spaces - migrate default space and enter space view to KP (elastic#66098)
  [APM] Change plugin id for `apm_oss` to `apmOss` (elastic#66164)
  [Maps] convert map_selectors to TS (elastic#65905)
  [uptime/usage-collector] add missing await (elastic#66079)
  [Ingest] Add additional attributes to the Datasources Saved Object (elastic#66127)
  [Endpoint]EMT-339: add new policy response schema (elastic#66264)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame Feature:NP Migration release_note:enhancement Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet