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

[Search] Error notification alignment #77788

Merged
merged 38 commits into from
Sep 28, 2020

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Sep 17, 2020

Summary

I've attached response files that can be used to mock each error type

This PR introduces explicit error types into the search service. The search service can now throw the following error types:

  • AbortError if the request was canceled by the application or by calling cancelPending
  • SearchTimeoutError if the request has timed out on the client or on the server.
  • PainlessError if there's an painless script error inside the response
  • Or if the error is unidentified, it throws the error as is.

It also introduces a showError function which handles showing toast messages for the errors thrown. This allows most applications to so something like this, without having to implement logic per specific error type (Dashboard for example doesn't use the default error handling and shows labels on individual panels instead).

data.search.search(...)
   .catchError((e: Error) => {
       data.search.showError(e);
   }

This logic is applied to discover, visualize and dashboard. In discover, it allowed deleting the old code handling of a PainlessError.

This PR also fixes #77579 by fixing how timeout subscriptions are removed.

How it looks

In Discover and Visualize

image
image
image

In dashboard

Most errors are shown inside individual panels, but timeout errors still show as a notification (once per session).

image

Dev Docs

This PR introduces explicit error types to the data.search service:

  • AbortError if the request was canceled by the application or by calling cancelPending
  • SearchTimeoutError if the request has timed out on the client or on the server.
  • PainlessError if there's an painless script error inside the response
  • Or if the error is unidentified, it throws the error as is.

The new showError function can be used with these errors, to show customized toast messages. Applications may choose to handle errors differently, however the SearchTimeoutError error notification will be shown regardless.

data.search.search(...)
   .catchError((e: Error) => {
       data.search.showError(e);
   }

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@lizozom lizozom added Feature:Search Querying infrastructure in Kibana Team:AppArch release_note:skip Skip the PR/issue when compiling release notes labels Sep 21, 2020
@lizozom lizozom marked this pull request as ready for review September 21, 2020 11:30
@lizozom lizozom requested a review from a team September 21, 2020 11:30
@lizozom lizozom requested review from a team as code owners September 21, 2020 11:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lizozom lizozom changed the title [Search] Error alignment [Search] Error notification alignment Sep 21, 2020
@botelastic botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Sep 21, 2020
@lizozom
Copy link
Contributor Author

lizozom commented Sep 21, 2020

@kibana-design changes are only deletion of a react component and it's css

@lizozom lizozom requested a review from kertal September 24, 2020 10:20
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Kibana App owned code LGTM 👍. like I mentioned, I will create an issue to improve the error message in Discover once this is merged.

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.

I tested basic "failed to fetch" error scenario in different apps by blocking requests. Seems good.
Didn't cover "upgrade to basic" scenario. (Didn't want to block, I need to dig into using those responses you've shared)

Thanks for addressing feedback suggestions.
Regarding react components as part of errors, I share @lukeelmers's point on it: #77788 (comment)

src/plugins/data/public/search/types.ts Outdated Show resolved Hide resolved
@lizozom
Copy link
Contributor Author

lizozom commented Sep 24, 2020

@lukeelmers @Dosant @lukasolson I agree that having the notification code in the interceptor is not ideal and we can revisit this when unifying the search service and search interceptor code.

@lizozom lizozom linked an issue Sep 24, 2020 that may be closed by this pull request
@lizozom lizozom requested review from a team as code owners September 24, 2020 19:55
@gchaps
Copy link
Contributor

gchaps commented Sep 24, 2020

There's a bit of repetition in the "Timed out" message. How about combining the title and the first sentence:

Title: Your query has timed out

Description: With our free Basic tier, your queries never time out.
Upgrade

@lizozom
Copy link
Contributor Author

lizozom commented Sep 25, 2020

@elasticmachine merge upstream

@lizozom
Copy link
Contributor Author

lizozom commented Sep 28, 2020

@elasticmachine merge upstream

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

SIEM/Endpoint changes LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id value diff baseline
data 570 +3 567
discover 240 -11 251
total -8

async chunks size

id value diff baseline
discover 430.9KB -7.5KB 438.5KB
visualize 271.2KB -180.0B 271.4KB
total -7.7KB

page load bundle size

id value diff baseline
data 1.3MB +8.9KB 1.3MB
dataEnhanced 34.1KB -429.0B 34.5KB
expressions 204.2KB +44.0B 204.1KB
total +8.5KB

History

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

@lizozom lizozom merged commit 689e1e3 into elastic:master Sep 28, 2020
lizozom added a commit to lizozom/kibana that referenced this pull request Sep 28, 2020
* OSS error alignemnt

* Adjust error messages in xpack

* Add getErrorMessage

* Use showError in vizualize
Add original error to expression exception

* Cleanup

* ts, doc and i18n fixes

* Fix jest tests

* Fix functional test

* functional test

* ts

* Update functional tests

* Add unit tests to interceptor and timeout error

* expose toasts test function

* doc

* typos

* review 1

* Code review

* doc

* doc fix

* visualization type fix

* fix jest

* Fix xpack functional test

* fix xpack test

* code review

* delete debubg flag

* Update texts by @gchaps

* docs and ts

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 28, 2020
* master:
  Fix APM lodash imports (elastic#78438)
  Add deprecated message to tile_map and region_map visualizations. (elastic#77683)
  Fix Lens smokescreen flaky tests (elastic#78566)
  updated discover with alt text (elastic#77660)
  Fix types (elastic#78619)
  Update tutorial-visualizing.asciidoc (elastic#76977)
  Update tutorial-discovering.asciidoc (elastic#76976)
  [Search] Error notification alignment (elastic#77788)
  Update tutorial-define-index.asciidoc (elastic#76975)
  [Lens] Fieldless operations (elastic#78080)
  [Usage Collection] [schema] Explicit "array" definition (elastic#78141)
  Update tutorial-define-index.asciidoc (elastic#76973)
  Fix --no-basepath references in doc (elastic#78570)
  Move StubIndexPattern to data plugin and convert to TS. (elastic#78518)
  Index pattern class - remove unused methods (elastic#78538)
  [Security Solution] [ALL] Eliminates all console.error and console.warn from Jest output (elastic#78523)
  [Actions] avoids setting a default dedupKey on PagerDuty (elastic#77773)
  First stab at developer-focussed saved objects docs (elastic#71430)
  remove unnecessary config validations (elastic#78527)
lizozom added a commit that referenced this pull request Sep 29, 2020
* OSS error alignemnt

* Adjust error messages in xpack

* Add getErrorMessage

* Use showError in vizualize
Add original error to expression exception

* Cleanup

* ts, doc and i18n fixes

* Fix jest tests

* Fix functional test

* functional test

* ts

* Update functional tests

* Add unit tests to interceptor and timeout error

* expose toasts test function

* doc

* typos

* review 1

* Code review

* doc

* doc fix

* visualization type fix

* fix jest

* Fix xpack functional test

* fix xpack test

* code review

* delete debubg flag

* Update texts by @gchaps

* docs and ts

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Search Querying infrastructure in Kibana release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random "query timed out" toasts while using Kibana Adjust search timeout messaging
10 participants