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

[Time to Visualize] Embeddable Error Handling Without ReplacePanel #82201

Merged

Conversation

ThomThomson
Copy link
Contributor

Summary

Fixes #81399.

This PR uses the output$ Observable as described in #81399 (comment) to notify the embeddable panel when a fatal error has occurred.

This PR does not use replacePanel or implement a factory for the error embeddable because of the potential for it to allow the user to save a dashboard with an errorEmbeddable in place - which means that it would not react if the saved object was subsequently restored.

This implementation exists purely in the embeddable panel. The implementation for isErrorEmbeddable has also been subtly changed so it returns true for embeddables that have previously had a fatalError

How to Test

  1. Create a new Lens Visualization and add it to the library
  2. Add the lens vis to a dashboard and save the dashboard
  3. Use the saved objects management section to delete the saved object associated with the lens visualization.
  4. Reload the dashboard. You should see a 404 error.

For maintainers

@ThomThomson ThomThomson added Feature:Embedding Embedding content via iFrame release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0 v7.11.0 Project:TimeToVisualize apm:test-plan-7.10.0 labels Oct 30, 2020
@ThomThomson ThomThomson marked this pull request as ready for review November 2, 2020 14:28
@ThomThomson ThomThomson requested a review from a team November 2, 2020 14:28
@ThomThomson ThomThomson requested a review from a team as a code owner November 2, 2020 14:28
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

});
},
(error) => {
this.props.embeddable.destroy();
Copy link
Contributor

Choose a reason for hiding this comment

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

The approach LGTM,

One weirdness I noticed:

  1. Load a "broken" dashboard in view move. See Lens embeddable with error
  2. Go to edit mode and try to delete it. There is no option to "DELETE" an embeddable. O_O.
    I think it is important that it still possible to delete an embeddable in this scenario.

Probably we shouldn't call destroy in this scenario, because embeddable still needs to receive update from parent (viewMode in this case) that will be used during context menu rendering?

Probably we could rely on destroy being called when this component unmounts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a really good catch. I avoided destroying the embeddable, and added clauses in the isCompatible methods of a few problematic actions like Clone, add to library, and unlink from library. Now you should be able to properly remove or replace errorEmbeddables.

this.props.embeddable.destroy();
if (this.embeddableRoot.current) {
const errorEmbeddable = new ErrorEmbeddable(error, { id: this.props.embeddable.id });
errorEmbeddable.render(this.embeddableRoot.current);
Copy link
Contributor

Choose a reason for hiding this comment

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

This embeddable is created but destroy is never called on it. I wonder if this is causing memory leaks or if there is nothing to leak for this simple embeddable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that the ErrorEmbeddable is simple enough not to cause any memory leaks, but it's still best practice to destroy it, so I added it to the state, and destroyed it during the unmount

…beddables. Added tests. Embeddable panel does not destroy embeddable on error and destroys error embeddable on unmount.
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

LGTM,
I retested the edge case with removing broken panel

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine kibanamachine requested a review from a team as a code owner November 4, 2020 15:02
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
dashboard 198.0KB 198.1KB +109.0B
lens 1020.4KB 1020.6KB +176.0B
total +285.0B

page load bundle size

id before after diff
dashboard 311.5KB 312.0KB +510.0B
embeddable 219.6KB 220.1KB +465.0B
total +975.0B

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 Chrome, Lens changes LGTM - if loading a visualization throws an error, it's shown in the panel.

Side note: I first tried to test the error case by turning off the internet connection, then adding a panel to a dashboard. The saved object client request fails, but to show the error message it actually requests an async chunk (which doesn't work as well of course).

This results in the same behavior as before (empty panel is shown) and this error in the console:
Screenshot 2020-11-04 at 18 06 39

Seems like a good fallback to me.

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

@ThomThomson ThomThomson merged commit 7c66880 into elastic:master Nov 5, 2020
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Nov 5, 2020
…lastic#82201)

Fixed embeddable error handling so that fatal errors are caught and displayed with an errorEmbeddable no matter when they occur.
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Nov 5, 2020
…lastic#82201)

Fixed embeddable error handling so that fatal errors are caught and displayed with an errorEmbeddable no matter when they occur.
# Conflicts:
#	docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddable.md
#	docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.iembeddable.md
#	src/plugins/dashboard/public/application/actions/library_notification_action.tsx
#	src/plugins/embeddable/public/public.api.md
#	x-pack/plugins/lens/public/lens_attribute_service.ts
ThomThomson added a commit that referenced this pull request Nov 5, 2020
…82201) (#82742)

Fixed embeddable error handling so that fatal errors are caught and displayed with an errorEmbeddable no matter when they occur.
ThomThomson added a commit that referenced this pull request Nov 5, 2020
…anel (#82201) (#82744)

Fixed embeddable error handling so that fatal errors are caught and displayed with an errorEmbeddable no matter when they occur.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame Project:TimeToVisualize release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.10.1 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Time to Visualize] Embeddable Error Handling
6 participants