[Discover] Inline data fetching errors #152311

davismcphee · 2023-02-28T04:23:11Z

Summary

Note: If your team's review is requested on this PR related to notification service changes, it's likely because somewhere in your codebase NotificationsSetup was being passed to a function/component requiring NotificationsStart or vice versa. The interfaces were previously identical so it didn't matter, but an additional function was added to NotificationsStart in this PR that created type errors which needed to be resolved.

This PR updates Discover to use inline error messages for data fetching errors instead of toasts.

For most error messages, the title and full message will be displayed inline as well as a "Show details" button which opens the standard error dialog. For errors which have special handling and render custom content instead of just the error message, the custom content will be rendered in the callout body for the "No results" state, or rendered in a popover when clicking the "Show details" button for the banner-style error display.

No results with regular error message:

Banner-style with regular error message:

Banner-style with regular error message in mobile:

Standard error dialog:

No results with overridden content:

Banner-style with overridden content modal:

Resolves #149488.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
~~Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
~~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list~~
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-03-07T15:21:57Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

kpollich

Fleet change 🚀

ThomThomson

Service addition in dashboard LGTM! Code only review.

pgayvallet

LGTM for Core changes

kertal

👍 it's much better this way! quick first question, this PR doesn't change the saved search embeddable handling currently, this will be a follow up, right? (just tested an embeddable fetching in offline mode, there ain't any message displayed about this state)

yuliacech

Management team changes LGTM, quickly tested locally that Index Management works as expected

kc13greiner

LGTM!

kertal · 2023-03-09T15:54:17Z

src/plugins/data/public/search/search_interceptor/utils.ts

+  if (error.constructor.name === 'HttpFetchError' || error instanceof BfetchRequestError) {
+    const defaultMsg = i18n.translate('data.errors.fetchError', {
+      defaultMessage: 'Check your network connection and try again.',
+    });
+
+    return {
+      title: i18n.translate('data.search.httpErrorTitle', {
+        defaultMessage: 'Unable to connect to the Kibana server',
+      }),
+      body: error.message || defaultMsg,
+    };
+  }
+}


I'm just wondering how to get to the defaultMessage since it sounds more helpful than "Failed to fetch":

I was also curious about this. I think we should discuss it and decide if it might be more useful to always show the defaultMessage for these errors. Or maybe at least showing the defaultMessage + the error message.

kertal · 2023-03-09T16:04:43Z

src/plugins/data/public/search/search_interceptor/search_interceptor.ts

-        text: toMountPoint(e.message || defaultMsg, {
-          theme$: this.deps.theme.theme$,
-        }),
+        title: overrideDisplay.title,


It's unrelated to this PR, but just a thought, it would be interesting if when this is called (which is no longer the case in Discover 🥳 ) could lead to the storm of toast in certain situations

E.g. here:

kibana/src/plugins/visualizations/public/visualize_app/utils/get_visualization_instance.ts

Line 72 in cb0a805

data.search.showError(

I imagine calling data.search.showError too much probably is responsible for some of the toast storms. It might be a candidate to consider throttling too.

andreadelrio

Design changes LGTM 🥳

walterra

Transform changes LGTM.

…fix'

…when showing inline error banner

kibana-ci · 2023-03-20T20:56:24Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	445	446	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-notifications-browser`	23	27	+4
`data`	2573	2577	+4
total			+8

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dashboard`	386.7KB	386.7KB	+36.0B
`discover`	395.6KB	397.5KB	+1.9KB
`indexManagement`	525.5KB	525.5KB	-10.0B
total			+1.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	350.9KB	351.0KB	+112.0B
`data`	403.7KB	403.8KB	+88.0B
total			+200.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-notifications-browser`	35	39	+4
`data`	3269	3273	+4
total			+8

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💛 Build #114066 was flaky b60c1aa71c60676ab4ec889741c10370992509c3
💚 Build #113220 succeeded 42197c7bd68030defa6ac6c5831e8c8ea0c46d50
💛 Build #112388 was flaky 3ed0bfb612dc579fcb6e44025ef015c5452b7e6f
💚 Build #112191 succeeded 5a0ce83ad2c821c654db4b9ce980ff5bd4dce689
💔 Build #111911 failed 913c3e8252fcf32c3c148cdc249c6228438da794
💔 Build #111864 failed 777c1d52ac1fbd565d022136361a072147f9d683

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

cc @davismcphee

davismcphee added Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Feb 28, 2023

davismcphee self-assigned this Feb 28, 2023

davismcphee force-pushed the discover-inline-search-errors branch 5 times, most recently from 5a0ce83 to 3ed0bfb Compare March 7, 2023 14:07

davismcphee added release_note:enhancement v8.8.0 labels Mar 7, 2023

davismcphee marked this pull request as ready for review March 7, 2023 15:21

davismcphee requested review from a team as code owners March 7, 2023 15:21

kpollich approved these changes Mar 7, 2023

View reviewed changes

ThomThomson approved these changes Mar 7, 2023

View reviewed changes

pgayvallet approved these changes Mar 8, 2023

View reviewed changes

kertal reviewed Mar 8, 2023

View reviewed changes

yuliacech approved these changes Mar 8, 2023

View reviewed changes

kc13greiner self-requested a review March 8, 2023 18:09

kc13greiner approved these changes Mar 9, 2023

View reviewed changes

kertal reviewed Mar 9, 2023

View reviewed changes

andreadelrio approved these changes Mar 20, 2023

View reviewed changes

walterra approved these changes Mar 20, 2023

View reviewed changes

davismcphee and others added 19 commits March 20, 2023 14:56

Add initial support for inline error messages

df88056

Fixing broken tests

fa2356b

Fix notifications service mock type

dac576d

Remove discover.showErrorMessageAgain translations

2830795

Create and export getSearchErrorOverrideDisplay from search_interceptor

0043cb2

Remove data service from DiscoverNoResults

e962bfc

Add support for getSearchErrorOverrideDisplay to ErrorCallout

4be3503

Fix broken functional tests

a3bfa90

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

0b3bb70

…fix'

Fix notifications service mock type

fd73586

Fix functional tests

fb6c914

Update ErrorCallout popover

e415e87

Change Notifications to NotificationsStart in change_password.tsx

700bff8

Fix notifications service references

058a4c5

Clean up error popover and change callout title to h3

cddd5c1

Create ErrorCallout Jest tests

4a925d6

Fix storybook types

685a00e

Switch from popover to modal for displaying overridden error content …

b1fe4ce

…when showing inline error banner

Fix Jest tests

e74ef63

XavierM approved these changes Mar 20, 2023

View reviewed changes

davismcphee force-pushed the discover-inline-search-errors branch from b60c1aa to e74ef63 Compare March 20, 2023 18:58

davismcphee merged commit 26f0f52 into elastic:main Mar 20, 2023

kibanamachine added the backport:skip This commit does not require backporting label Mar 20, 2023

davismcphee deleted the discover-inline-search-errors branch March 20, 2023 23:48

kertal mentioned this pull request Apr 19, 2023

[Discover] Inline shard error warnings #155216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Inline data fetching errors #152311

[Discover] Inline data fetching errors #152311

davismcphee commented Feb 28, 2023 •

edited

Loading

elasticmachine commented Mar 7, 2023

kpollich left a comment

ThomThomson left a comment

pgayvallet left a comment

kertal left a comment

yuliacech left a comment

kc13greiner left a comment

kertal Mar 9, 2023

davismcphee Mar 10, 2023

kertal Mar 9, 2023 •

edited

Loading

davismcphee Mar 10, 2023

andreadelrio left a comment

walterra left a comment

kibana-ci commented Mar 20, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

[Discover] Inline data fetching errors #152311

[Discover] Inline data fetching errors #152311

Conversation

davismcphee commented Feb 28, 2023 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Mar 7, 2023

kpollich left a comment

Choose a reason for hiding this comment

ThomThomson left a comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

kertal left a comment

Choose a reason for hiding this comment

yuliacech left a comment

Choose a reason for hiding this comment

kc13greiner left a comment

Choose a reason for hiding this comment

kertal Mar 9, 2023

Choose a reason for hiding this comment

davismcphee Mar 10, 2023

Choose a reason for hiding this comment

kertal Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

davismcphee Mar 10, 2023

Choose a reason for hiding this comment

andreadelrio left a comment

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

kibana-ci commented Mar 20, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

davismcphee commented Feb 28, 2023 •

edited

Loading

kertal Mar 9, 2023 •

edited

Loading