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

CONSOLE-3093: Add Error Boundaries around extension components #11607

Merged

Conversation

andrewballantyne
Copy link
Contributor

@andrewballantyne andrewballantyne commented May 30, 2022

Adds error boundaries around extension mounting so that if a runtime render error is thrown, the component from a plugin cannot crash our application.

Some test extensions: https://gist.github.com/andrewballantyne/fbbe6404d9bfdc09b172b55a04f4b2cd

What This Does For Users

There are many ways code can crash and sometimes these crashes can be more than a call not made or an item not updated. Sometimes these crashes take the whole app with it -- giving us the "white screen" situation.

React gives us something called Error Boundaries. These are special types of components that you can place anywhere in your React tree of components to only crash smaller parts of your app when things go awry. Even better, we can also put in fallback components to help more gracefully handle what happens when these crashes happen.

Now, add Dynamic Plugins into the mix, and all of a sudden we do not control all of our code running in OpenShift Console. Thus, what happens if a Dynamic Plugin brings in a component that crashes. We have a top-level Error Boundary, but can we do better? In comes this PR.

We wrap Error Boundaries around the usage of components brought in by plugins to help crash in smaller parts. Two types of error handling are done here.

  1. If the component is large portion of the screen -- like a route -- we want our standard error crashing catch (this is our standard "Oh no, something went wrong!" pages.
  2. If the component is a part of a larger piece, it could be a problem trying to render that whole page inline on a Dashboard Card (for instance) -- so we have a new inline component

See below for instances and what they'll look like in the UI. It is important to note this shouldn't ever happen in a live running production environment, but as we all know -- bugs happen.


Error states and what they look like in the UI

Inline error states during Dashboard Cards, activity, and inventory items:
Screen Shot 2022-06-21 at 10 23 51 AM

What the view details shows us:
Screen Shot 2022-06-17 at 11 15 02 AM

Other examples of inline:
Screen Shot 2022-06-17 at 11 03 09 AM

Add Storage is interesting -- I went with full screen error because most of the content we render is taking very little of the page:
Screen Shot 2022-06-17 at 11 07 56 AM

And naturally our normal error boundary (when say a route crashes):
Screen Shot 2022-06-17 at 11 02 22 AM

@openshift-ci openshift-ci bot added component/core Related to console core functionality component/dashboard Related to dashboard approved Indicates a PR has been approved by an approver from all required OWNERS files. component/olm Related to OLM component/shared Related to console-shared kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels May 30, 2022
@andrewballantyne
Copy link
Contributor Author

/assign @christianvogt @vojtechszocs @jhadvig

@jhadvig jhadvig changed the title Add Error Boundaries around extension components CONSOLE-3093: Add Error Boundaries around extension components Jun 1, 2022
Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

IMO, there should be some distinction between inline or page-level error boundary fallbacks. See comments. I also think it might make sense to keep the ErrorBoundaryFallback component alongside ErrorBoundary in console-shared since it's consumed by other packages.

@openshift-ci openshift-ci bot added component/monitoring Related to monitoring component/topology Related to topology labels Jun 15, 2022
Copy link
Contributor Author

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

@TheRealJon I've reworked the whole PR. Improved the error handling and rendering code by bringing it all into shared with new content for inline error boundaries.

I've broken it all down with readable commit names, so hopefully it's easier to consume. But this is effectively a new PR with the changes within'.

@andrewballantyne
Copy link
Contributor Author

/retest

@andrewballantyne
Copy link
Contributor Author

/retest

It's a connection error -- I'm hoping it's just a hiccup and not an infra issue

Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2022
@andrewballantyne
Copy link
Contributor Author

/retest

2 similar comments
@andrewballantyne
Copy link
Contributor Author

/retest

@andrewballantyne
Copy link
Contributor Author

/retest

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2022
@andrewballantyne
Copy link
Contributor Author

There was a merge that conflicted with the ErrorBoundary code -- needed to do a change fix an import path. Also rebased to try and cut down on anymore of those.

@andrewballantyne
Copy link
Contributor Author

/label tide/merge-method-squash

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jun 17, 2022
@andrewballantyne
Copy link
Contributor Author

/assign @yapei
Ya dan, to test this out there is a diff in my description that you can apply to the Dynamic Demo Plugin -- should be good to use this for some testing. What happens is I render a component and in the render thread throws an error to deliberately break the render thread and trigger the error boundary code.

/assign @RickJWagner
Rick, this is probably worth noting as someone may come for support if their dynamic plugins break / or have operator based dynamic plugins that break on them. It is intentional to show that something went wrong instead of nothing (at least in the inline cases). Let me know what you think.

/assign @opayne1
Olivia, not sure what we'd document here -- but may be worth noting for the Dynamic Plugin docs that we have error boundaries in place. Let me know if you want to talk more about this.

/cc @megan-hall
Megan, I'm tagging you because I took some liberties with new UX here. This is an error state, not a state I expect often, so let me know what you think of it. Mainly the biggest change here for you would be the inline errors on the Dashboard & User Preferences pages.

@andrewballantyne
Copy link
Contributor Author

/retest

@RickJWagner
Copy link

Hi @andrewballantyne ,
Thanks for calling this out.
I went to our case repository to see how CEE reacts when some customer complains about the error message "Oh no! Something went wrong". (It turns out Console and RHEL both currently use this messaging.)
CEE almost always gets some initial good information from the customer, and may follow up with requests for a must-gather.
In this case, I imagine most customers are going to provide CEE screenshots with the stack traces, these will point CEE to plugins immediately.
I think it's good, don't foresee a Support problem here.
Thanks for the good work.

@RickJWagner RickJWagner removed their assignment Jun 17, 2022
@opayne1
Copy link
Contributor

opayne1 commented Jun 17, 2022

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Jun 17, 2022
@andrewballantyne
Copy link
Contributor Author

andrewballantyne commented Jun 17, 2022

/label px-approved

On behalf of Rick.

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Jun 17, 2022
@megan-hall
Copy link

Thanks for tagging us Andrew! I think we can keep what you have in your PR for now (and make a small adjustment to the padding so the one on the details card is clearly not associated with the content below). I do think it is worth revisiting for 4.12 - perhaps we can consider creating an alert and think through some of the UX details a bit more. I can create a PD story for that so we don't lose track.

@andrewballantyne
Copy link
Contributor Author

andrewballantyne commented Jun 21, 2022

... I think we can keep what you have in your PR for now (and make a small adjustment to the padding so the one on the details card is clearly not associated with the content below)...

@megan-hall done. See new updated description screenshot for the whole page -- here is the closeup of the Details Card:
image

Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 21, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, TheRealJon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 21, 2022

@andrewballantyne: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@yapei
Copy link
Contributor

yapei commented Jun 22, 2022

changes are verified after applying diff

  • Dashboard Cards, activity, and inventory items will show inline errors, in modal we show detailed errors
  • also verified that Add storage & User preferences extension inline errors
    /label qe-approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dashboard Related to dashboard component/monitoring Related to monitoring component/olm Related to OLM component/shared Related to console-shared component/topology Related to topology docs-approved Signifies that Docs has signed off on this PR kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants