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

[Discover] Add caused_by.type and caused_by.reason to error toast modal #70404

Conversation

kertal
Copy link
Member

@kertal kertal commented Jul 1, 2020

Summary

Improves the current error modal toast with additional information about what caused a Elasticsearch request to fail. Seems this info was provided in older 7.x versions differently and was part of the error message, but no longer is.

Before this PR:

Now the it's clear what caused the error, an unknown timezone:
Bildschirmfoto 2020-07-01 um 10 22 23

Related to: #70352

How to test

Set the timezone of Kibana to MST in Advanced settings, that should trigger this error.

@kertal kertal self-assigned this Jul 1, 2020
@kertal kertal added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Discover Discover Application Team:AppArch labels Jul 1, 2020
@timroes
Copy link
Contributor

timroes commented Jul 1, 2020

I removed the "Fix note", since I think it's not the fix to the mentioned issue, but just related to it. We should actually check which time zones are allowed by ES and only allow users to select those in Kibana. This PR nevertheless is of course useful and should go in, would just not close the issue with it.

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM!

@kertal kertal marked this pull request as ready for review July 1, 2020 18:33
@kertal kertal requested a review from a team as a code owner July 1, 2020 18:33
@elasticmachine
Copy link
Contributor

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

Comment on lines 40 to 39
error: Error;
error: RequestError;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would:
1/ keep Error in ErrorToastProps as RequestError does not reflect the real type (is can be any kind of error)
2/ add a typeguard for RequestError: isRequestError = (e: Error): e is RequestError => error.body?.attributes?.error?.caused_by !== undefined

Use it in the block

if (isRequestError(error)) {
    text += `${error.body.attributes.error.caused_by.type}\n`;
    text += `${error.body.attributes.error.caused_by.reason}\n\n`;
  }

Copy link
Member Author

Choose a reason for hiding this comment

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

thx! adapted the code in this way

Copy link
Member Author

Choose a reason for hiding this comment

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

dear @pgayvallet , have some TypeScript errors here to resolve

const isRequestError = (e: Error): e is RequestError => e.body?.attributes?.error?.caused_by !== undefined;

Error message: Property 'body' does not exist on type 'Error'.

How can I resolve this? many thx

Copy link
Contributor

Choose a reason for hiding this comment

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

(e: Error) -> (e: any) should do the trick

Copy link
Member Author

@kertal kertal Jul 14, 2020

Choose a reason for hiding this comment

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

thx, to omit any, I've implemented it the following way:

const isRequestError = (e: Error | RequestError): e is RequestError => {
   if ('body' in e) {
     return e.body?.attributes?.error?.caused_by !== undefined;
   }
   return false;
 };

@kertal kertal requested a review from pgayvallet July 2, 2020 12:55
@kertal
Copy link
Member Author

kertal commented Jul 2, 2020

Jenkins, test this

@kertal
Copy link
Member Author

kertal commented Jul 2, 2020

@elasticmachine merge upstream

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Code LGTM

@kertal kertal added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels Jul 2, 2020
@kertal
Copy link
Member Author

kertal commented Jul 6, 2020

@elasticmachine merge upstream

@kertal
Copy link
Member Author

kertal commented Jul 8, 2020

@elasticmachine merge upstream

@kertal
Copy link
Member Author

kertal commented Jul 13, 2020

@elasticmachine merge upstream

@kertal
Copy link
Member Author

kertal commented Jul 13, 2020

@elasticmachine merge upstream

…h-add-context' of github.com:kertal/kibana into kertal-pr-2020-07-01-discover-improve-error-message-with-add-context
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@kertal kertal merged commit 24d29a3 into elastic:master Jul 14, 2020
kertal added a commit to kertal/kibana that referenced this pull request Jul 14, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 14, 2020
* master: (314 commits)
  [APM] Use status_code field to calculate error rate (elastic#71109)
  [Observability] Change appLink passing the date range (elastic#71259)
  [Security] Add Timeline improvements (elastic#71506)
  adjust vislib bar opacity (elastic#71421)
  Fix ScopedHistory mock and adapt usages (elastic#71404)
  [Security Solution] Add hook for reading/writing resolver query params (elastic#70809)
  [APM] Bug fixes from ML integration testing (elastic#71564)
  [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404)
  [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275)
  [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321)
  Change signal.rule.risk score mapping from keyword to float (elastic#71126)
  Added help text where needed on connectors and alert actions UI (elastic#69601)
  [SIEM][Detections] Value Lists Management Modal (elastic#67068)
  [test] Skips test preventing promotion of ES snapshot elastic#71582
  [test] Skips test preventing promotion of ES snapshot elastic#71555
  [ILM] Fix alignment of the timing field (elastic#71273)
  [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540)
  initial telemetry setup (elastic#69330)
  [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027)
  Search across spaces (elastic#67644)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 14, 2020
…t-apps-page-titles

* 'master' of github.com:elastic/kibana: (88 commits)
  [ML] Functional tests - disable DFA creation and cloning tests
  [APM] Use status_code field to calculate error rate (elastic#71109)
  [Observability] Change appLink passing the date range (elastic#71259)
  [Security] Add Timeline improvements (elastic#71506)
  adjust vislib bar opacity (elastic#71421)
  Fix ScopedHistory mock and adapt usages (elastic#71404)
  [Security Solution] Add hook for reading/writing resolver query params (elastic#70809)
  [APM] Bug fixes from ML integration testing (elastic#71564)
  [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404)
  [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275)
  [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321)
  Change signal.rule.risk score mapping from keyword to float (elastic#71126)
  Added help text where needed on connectors and alert actions UI (elastic#69601)
  [SIEM][Detections] Value Lists Management Modal (elastic#67068)
  [test] Skips test preventing promotion of ES snapshot elastic#71582
  [test] Skips test preventing promotion of ES snapshot elastic#71555
  [ILM] Fix alignment of the timing field (elastic#71273)
  [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540)
  initial telemetry setup (elastic#69330)
  [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027)
  ...

# Conflicts:
#	x-pack/plugins/index_management/public/application/index.tsx
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 14, 2020
* master: (72 commits)
  [test] Skips test preventing promotion of ES snapshot elastic#71612
  [Logs UI] Remove UUID from Alert Instances (elastic#71340)
  [Metrics UI] Remove UUID from Alert Instance IDs (elastic#71335)
  [ML] Functional tests - disable DFA creation and cloning tests
  [APM] Use status_code field to calculate error rate (elastic#71109)
  [Observability] Change appLink passing the date range (elastic#71259)
  [Security] Add Timeline improvements (elastic#71506)
  adjust vislib bar opacity (elastic#71421)
  Fix ScopedHistory mock and adapt usages (elastic#71404)
  [Security Solution] Add hook for reading/writing resolver query params (elastic#70809)
  [APM] Bug fixes from ML integration testing (elastic#71564)
  [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404)
  [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275)
  [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321)
  Change signal.rule.risk score mapping from keyword to float (elastic#71126)
  Added help text where needed on connectors and alert actions UI (elastic#69601)
  [SIEM][Detections] Value Lists Management Modal (elastic#67068)
  [test] Skips test preventing promotion of ES snapshot elastic#71582
  [test] Skips test preventing promotion of ES snapshot elastic#71555
  [ILM] Fix alignment of the timing field (elastic#71273)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants