Add VisLayer error toasts when rendering vis #3649

ohltyler · 2023-03-22T19:17:38Z

Description

When rendering a visualization, we want to push a toast notification with error details if there was any failures fetching associated VisLayers. This PR adds that by aggregating any failures and using notifications service to add an error toast in fetchVisLayers().

We override the stack value of the error, such that when it is parsed and read in the notifications addError() fn, it is shown properly to be the aggregate details in string form.

It also makes message in VisLayerError a required field (instead of optional as before), which means expression functions that are generating these errors should populate a human-readable message here so it can be propagated and shown in the details modal.

Example screenshots:

Issues Resolved

Partially resolves #3580 but bulk will be covered in View Events PR #3415.

Check List

codecov-commenter · 2023-03-22T22:20:51Z

Codecov Report

Merging #3649 (2a8ac75) into feature/feature-anywhere (3a719c5) will decrease coverage by 0.05%.
The diff coverage is 80.55%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@                     Coverage Diff                      @@
##           feature/feature-anywhere    #3649      +/-   ##
============================================================
- Coverage                     66.49%   66.45%   -0.05%     
============================================================
  Files                          3243     3244       +1     
  Lines                         62400    62435      +35     
  Branches                       9630     9640      +10     
============================================================
- Hits                          41494    41489       -5     
- Misses                        18598    18632      +34     
- Partials                       2308     2314       +6

Flag	Coverage Δ
Linux	`66.45% <80.55%> (+0.01%)`	⬆️
Windows	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/plugins/vis_augmenter/public/index.ts	`0.00% <ø> (ø)`
...ter/public/saved_augment_vis/utils/test_helpers.ts	`100.00% <ø> (ø)`
...izations/public/embeddable/visualize_embeddable.ts	`0.61% <0.00%> (-0.02%)`	⬇️
src/plugins/visualizations/public/plugin.ts	`0.00% <0.00%> (ø)`
src/plugins/vis_augmenter/public/test_helpers.ts	`100.00% <100.00%> (ø)`
src/plugins/vis_augmenter/public/types.ts	`100.00% <100.00%> (ø)`
src/plugins/vis_augmenter/public/utils/utils.ts	`97.05% <100.00%> (+3.72%)`	⬆️
src/plugins/visualizations/public/services.ts	`100.00% <100.00%> (ø)`

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ohltyler · 2023-04-18T21:43:38Z

Confirmed with @opensearch-project/opensearch-ux on the finalized wording for the aggregate detailed error message.
Given that each error will contain a plugin resource type, plugin resource ID, and error message, we can aggregate and display it like the following:

<Plugin-resource-type-1>:
- ID: <id>
- Message: <error-message>

- ID: <id-2>
- Message: <error-message-2>

<Plugin-resource-type-2>:
...

src/plugins/vis_augmenter/public/test_helpers.ts

src/plugins/vis_augmenter/public/types.ts

src/plugins/vis_augmenter/public/test_helpers.ts

src/plugins/vis_augmenter/public/utils/utils.test.ts

src/plugins/vis_augmenter/public/utils/utils.ts

ashwin-pc · 2023-04-22T00:08:53Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

+          title: i18n.translate('visualizations.renderVisTitle', {
+            defaultMessage: `Error loading data on the ${this.vis.title} chart`,
+          }),
+          toastMessage: ' ',


why is the toast empty? Can you add a screenshot of the toast to the PR? it will be helpful to see what this looks like. Also you might want to add an id to the toast if you want to prevent he toast from being displayed again should the same expression run again before the toast is dismissed (But be careful to keep the ID specific to he vis since the id is global to the dashboard)

I am also a little confused why the toast is empty here. In this case there was no vis layer errors, why is there a default message still with "error loading data on the chart"

The reason for the toast message being empty here is because it is reserved for showing up in the modal details (the danger callout text). Added a screenshot of the toast to description.

I'll add an ID to the toast after rebasing and pulling in the upstream changes that include the ID as an option.

Note this is all happening inside an if (err !== undefined) block. This is only true when one or more VisLayers had an error. I'll add a comment in there too.

Update: after checking #3752 it doesn't include updating ErrorToastOptions which is needed for adding an ID to addError fn used for displaying error toasts. I will leave this empty for now and we can add later if that is decided to be added.

I've added the ID so it can be handled for addError scenarios. See commit 739ddaa

Confirmed that the toast only shows up once on multiple re-renders.

Is the ID here coming from the plugin resource ID?

Using the visualize_embeddable ID such that it's unique within the dashboard. This enforces that there will only be one toast per vis on a dashboard.

Did you do any testing what the toast would look like or have seen before if we have dozens of detector per the viz and they all have some error, will the toast be scrollable at some point?

I haven't individually tested that but yes, it will be scrollable. It is designed to hold very long text (error stack traces)

Sorry to reopen the thread, I'm just trying to understand whether our toast interface needs to be improved. The mocks make sense to me, where we have only a title in the toast. But why do we need to pass a whitespace character as toastMessage? It seems like we should be able to omit that property altogether, or else pass undefined, null, or at least an empty string. Do we need to open an issue to improve the addError method?

Signed-off-by: Tyler Ohlsen <[email protected]>

ashwin-pc

Just one change so that the context here isnt lost in future, but otherwise the change looks good :) Also do you know why the tests are failing?

src/plugins/vis_augmenter/public/types.ts

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler · 2023-04-26T00:08:44Z

Just one change so that the context here isnt lost in future, but otherwise the change looks good :) Also do you know why the tests are failing?

Fixed. Failure seemed to be some issue when switching to use toMatchInlineSnapsho() which was succeeding locally but failing in the runners. So reverted back to using toStrictEqual()

joshuarrrr

Cool. I had one question about what seems like an annoyance in the toasts interface and one suggestion for future refactoring, but looks good to me.

joshuarrrr · 2023-04-26T17:34:41Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

+          title: i18n.translate('visualizations.renderVisTitle', {
+            defaultMessage: `Error loading data on the ${this.vis.title} chart`,
+          }),
+          toastMessage: ' ',


Sorry to reopen the thread, I'm just trying to understand whether our toast interface needs to be improved. The mocks make sense to me, where we have only a title in the toast. But why do we need to pass a whitespace character as toastMessage? It seems like we should be able to omit that property altogether, or else pass undefined, null, or at least an empty string. Do we need to open an issue to improve the addError method?

joshuarrrr · 2023-04-26T17:50:03Z

src/plugins/vis_augmenter/public/utils/utils.ts

+      const matchingVisLayers = visLayersWithErrors.filter(
+        (visLayer) => visLayer.pluginResource.type === type
+      );
+      if (index !== 0) msgDetails += '\n\n\n';


style nit, no need to change: this is basically a join operation on an array of strings - we could refactor this message composition so that each of the forEach loops are just mapping operations that return arrays of strings and then join them together to form the final message rather than just concatenating everything together as we go.

ohltyler requested a review from a team as a code owner March 22, 2023 19:17

ohltyler added visualizations Issues and PRs related to visualizations unified visualization UX feature-anywhere v2.8.0 labels Mar 22, 2023

ohltyler marked this pull request as draft March 22, 2023 19:18

ohltyler mentioned this pull request Mar 22, 2023

[Feature Anywhere] Enable plugins to augment visualizations with additional data and context #2880

Closed

31 tasks

ohltyler force-pushed the add-error-toasts branch 2 times, most recently from d013852 to 242510f Compare March 22, 2023 20:51

ohltyler force-pushed the add-error-toasts branch 2 times, most recently from c739b3a to 246ba94 Compare April 19, 2023 22:19

ohltyler marked this pull request as ready for review April 21, 2023 22:33

ashwin-pc reviewed Apr 22, 2023

View reviewed changes

joshuarrrr assigned ashwin-pc Apr 22, 2023

ohltyler added 4 commits April 24, 2023 09:27

Add VisLayer error toasts when rendering vis

b3982af

Signed-off-by: Tyler Ohlsen <[email protected]>

Finalize formatting of err stack details

6b593f3

Signed-off-by: Tyler Ohlsen <[email protected]>

Add one test case

72c4d64

Signed-off-by: Tyler Ohlsen <[email protected]>

Make message of VisLayerError required

2a8ac75

Signed-off-by: Tyler Ohlsen <[email protected]>

ohltyler force-pushed the add-error-toasts branch from 2ae63b0 to 2a8ac75 Compare April 24, 2023 16:27

ohltyler requested review from ananzh, kavilla, seanneumann, AMoo-Miki, joshuarrrr, abbyhu2000, zengyan-amazon, kristenTian and zhongnansu as code owners April 24, 2023 16:27

ohltyler requested a review from manasvinibs as a code owner April 24, 2023 16:27

ohltyler added 2 commits April 24, 2023 10:40

Move test_helpers; clean up test; add comment

593b73a

Signed-off-by: Tyler Ohlsen <[email protected]>

Add id to error toast

739ddaa

Signed-off-by: Tyler Ohlsen <[email protected]>

amitgalitz approved these changes Apr 25, 2023

View reviewed changes

ashwin-pc reviewed Apr 25, 2023

View reviewed changes

src/plugins/vis_augmenter/public/types.ts Show resolved Hide resolved

ohltyler added 2 commits April 25, 2023 14:29

Add context to helper fns

485a8e0

Signed-off-by: Tyler Ohlsen <[email protected]>

Change back to toStrictEqual() for tests

7fef246

Signed-off-by: Tyler Ohlsen <[email protected]>

ashwin-pc approved these changes Apr 26, 2023

View reviewed changes

joshuarrrr approved these changes Apr 26, 2023

View reviewed changes

joshuarrrr merged commit 03b393f into opensearch-project:feature/feature-anywhere Apr 26, 2023

ohltyler mentioned this pull request Apr 26, 2023

[Feature Anywhere] General error handling of VisLayer errors #3580

Closed

ohltyler deleted the add-error-toasts branch May 2, 2023 17:02

kavilla added v2.9.0 and removed v2.8.0 labels Jun 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VisLayer error toasts when rendering vis #3649

Add VisLayer error toasts when rendering vis #3649

ohltyler commented Mar 22, 2023 •

edited

Loading

codecov-commenter commented Mar 22, 2023 •

edited

Loading

ohltyler commented Apr 18, 2023

ashwin-pc Apr 22, 2023

amitgalitz Apr 24, 2023

ohltyler Apr 24, 2023

ohltyler Apr 24, 2023

ohltyler Apr 24, 2023 •

edited

Loading

amitgalitz Apr 24, 2023

ohltyler Apr 24, 2023

amitgalitz Apr 25, 2023

ohltyler Apr 25, 2023

joshuarrrr Apr 26, 2023

ashwin-pc left a comment •

edited

Loading

ohltyler commented Apr 26, 2023

joshuarrrr left a comment

joshuarrrr Apr 26, 2023

joshuarrrr Apr 26, 2023

Add VisLayer error toasts when rendering vis #3649

Add VisLayer error toasts when rendering vis #3649

Conversation

ohltyler commented Mar 22, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Mar 22, 2023 • edited Loading

Codecov Report

ohltyler commented Apr 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohltyler Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc left a comment • edited Loading

Choose a reason for hiding this comment

ohltyler commented Apr 26, 2023

joshuarrrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohltyler commented Mar 22, 2023 •

edited

Loading

codecov-commenter commented Mar 22, 2023 •

edited

Loading

ohltyler Apr 24, 2023 •

edited

Loading

ashwin-pc left a comment •

edited

Loading