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

Improve dashboard loading error handling #66372

Conversation

kertal
Copy link
Member

@kertal kertal commented May 13, 2020

Summary

An invalid dashboard with another error reason than InvalidJSONProperty led to an empty screen in Kibana. This PR takes care of this case, so the user is redirected to the dashboards list view and an error message is displayed:

Bildschirmfoto 2020-05-14 um 12 38 00

Before this PR, the following screen was displayed
image

Here's an invalid dashboard that can be used for testing:

invalid_dashboard.ndjson.zip

@@ -230,7 +222,9 @@ export function initDashboardApp(app, deps) {
);
return new Promise(() => {});
} else {
throw error;
// E.g. a corrupt dashboard was detected (e.g. with invalid JSON properties)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just wondering if there are any implications of eliminating throwing an error here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The error we were discussing was throw this way, and that's why we got an empty dashboard. By changing this, the user is redirected to the listing page, and an error message is displayed. Of course we've to check for a case that requires an error thrown this way, but I do think this is legacy

@kertal kertal self-assigned this May 13, 2020
@kertal kertal added the Feature:Dashboard Dashboard related features label May 13, 2020
@majagrubic
Copy link
Contributor

Jenkins, test this

…-13-dashboard-show-error-message-when-invalid-saved-object-was-loaded
…-13-dashboard-show-error-message-when-invalid-saved-object-was-loaded
@kertal kertal marked this pull request as ready for review May 14, 2020 10:58
@kertal kertal requested a review from a team May 14, 2020 10:58
@kertal kertal added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label May 14, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in Firefox and works, LGTM 👍

@kertal kertal merged commit f0d1dce into elastic:master May 15, 2020
kertal added a commit to kertal/kibana that referenced this pull request May 15, 2020
* Improve dashboard loading error handling, prevent a white screen without error msg

* Remove redirectWhenMissing, since it's not needed anymore
kertal added a commit to kertal/kibana that referenced this pull request May 15, 2020
* Improve dashboard loading error handling, prevent a white screen without error msg

* Remove redirectWhenMissing, since it's not needed anymore
# Conflicts:
#	src/plugins/dashboard/public/application/legacy_app.js
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 15, 2020
…ent/add-support-in-url-for-hidden-toggle

* 'master' of github.com:elastic/kibana: (34 commits)
  [SIEM][CASE] Fix bug when connector is deleted. (elastic#65876)
  [SIEM][CASE] Improve layout (elastic#66232)
  [Index Management] Support Hidden Indices (elastic#66422)
  Add Login Selector functional tests. (elastic#65705)
  Lens drilldowns (elastic#65675)
  [ML] Custom template for apiDoc markdown (elastic#66567)
  Don't bootstrap core type emits (elastic#66377)
  [Dashboard] Improve loading error handling (elastic#66372)
  [APM] Minor style fixes for the node strokes (elastic#66574)
  [Ingest Manager] Fix create data source from integration (elastic#66626)
  [Metrics UI] Fix default metric alert interval for new conditions (elastic#66610)
  [Metrics UI] Fix alignment and allow clearing metric value (elastic#66589)
  Don't return package name for non-package data streams (elastic#66606)
  [Ingest Manager] Consolidate routing and add breadcrumbs to all pages (elastic#66475)
  [Docs/Reporting] Have the docs about granular timeout match Cloud docs (elastic#66267)
  Don't automatically add license header to code inside plugins dir. (elastic#66601)
  [APM] Don't trigger map layout if no elements (elastic#66625)
  [Logs UI] Validate ML job setup time ranges (elastic#66426)
  Fix pagination bugs in CCR and Remote Clusters (elastic#65931)
  Add cloud icon for supported settings and embed single-sourced getting started (elastic#65610)
  ...

# Conflicts:
#	x-pack/plugins/index_management/public/application/sections/home/index_list/index_table/index_table.js
#	x-pack/plugins/index_management/server/lib/fetch_indices.ts
kertal added a commit that referenced this pull request May 15, 2020
* Improve dashboard loading error handling, prevent a white screen without error msg

* Remove redirectWhenMissing, since it's not needed anymore
kertal added a commit that referenced this pull request May 15, 2020
* Improve dashboard loading error handling, prevent a white screen without error msg

* Remove redirectWhenMissing, since it's not needed anymore
@kertal kertal deleted the kertal-pr-2020-05-13-dashboard-show-error-message-when-invalid-saved-object-was-loaded branch July 1, 2020 10:08
@giraffeCjl
Copy link

Try to open the Dashboard with Firefox. I have a large amount of data, and I have visualized 8 tables. This method works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants