[Vega] Better error explanation for EsErrors and inspector now showing error responses #112634

dej611 · 2021-09-21T07:20:47Z

Summary

This PR addresses two main bugs in Vega:

When showing an EsError it now produces a better explanation of what it gone wrong

The inspector panel will show the response tab even when the something has gone wrong

Note: part of the fix here comes from the Lens error formatter. It would be nice to port most of the Lens error helpers logic into the data/search plugin in order to provide better error feedback for all Kibana.

Checklist

Delete any items that are not applicable to this PR.

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

elasticmachine · 2021-09-21T10:03:40Z

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

alexwizp · 2021-09-21T12:27:23Z

src/plugins/data/public/search/errors/es_error.tsx

@@ -17,7 +17,7 @@ export class EsError extends KbnError {
  readonly attributes: IEsError['attributes'];

  constructor(protected readonly err: IEsError) {
-    super('EsError');
+    super(`EsError: ${getRootCause(err)?.reason || err.attributes?.reason || 'unknown'}`);


just interesting is || err.attributes?.reason needed? I see that case is handled in getRootCause -> getNestedCause.

alexwizp · 2021-09-21T13:04:40Z

src/plugins/vis_types/vega/public/data_model/search_api.ts

+                    requestResponders[requestId]
+                  );
+                  // now throw it again as the Vega parser will catch it and carry on with its flow
+                  return throwError(err);


nit: what if we continue to inspect search results as a side effects without modifying the main stream. I don't insist but I think the following code is more semantically right

tap( (data) => this.inspectSearchResult(data, requestResponders[requestId]), (err) => this.inspectSearchResult( { rawResponse: 'err' in err ? err.err : undefined, }, requestResponders[requestId] ) )

alexwizp · 2021-09-21T13:07:09Z

src/plugins/vis_types/vega/public/data_model/search_api.ts

+                  // catch the error and log it into the inspector
+                  this.inspectSearchResult(
+                    {
+                      rawResponse: 'err' in err ? err.err : undefined,


nit: what if we replace it to err?.err

alexwizp · 2021-09-21T13:10:39Z

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

@@ -15,6 +15,15 @@ export function getFailedShards(err: KibanaServerError<any>): FailedShard | unde
  return failedShards ? failedShards[0] : undefined;
 }

+function getNestedCause(err: KibanaServerError<any>): Reason {


nit: you call getNestedCause from getRootCause. In parent method I see KibanaServerError instead of KibanaServerError<any>. I think we can remove <any>

Yes, thought about that.
But removing the any will set the generic T to unknown with types errors that need to be addressed. I think this is a major problem with EsErrors that should be addressed in another PR by providing better types.

alexwizp

LGTM! Thank you

stratoula

I love this PR! LGTM, works like a charm :D

jloleysens

@dej611 this is a great improvement that will definitely help a lot of users!

I have one suggestion about avoiding the use of any that I'd like to get your thoughts on. Let me know what you think!

jloleysens · 2021-09-22T08:38:48Z

src/plugins/data/public/search/errors/es_error.tsx

@@ -17,7 +17,7 @@ export class EsError extends KbnError {
  readonly attributes: IEsError['attributes'];

  constructor(protected readonly err: IEsError) {
-    super('EsError');
+    super(`EsError: ${getRootCause(err)?.reason || 'unknown'}`);


Since this is user-facing, we should consider translation:

i18n.translate('...', { defaultMessage: 'unknown' })

jloleysens · 2021-09-22T10:06:14Z

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

+function getNestedCause(err: KibanaServerError<any>): Reason {
+  const { type, reason, caused_by: causedBy } = err.attributes || err;


We should avoid introducing additional anys. Here is a possible alternative:

import type { ErrorCause } from '@elastic/elasticsearch/api/types'; ... function getNestedCause(err: KibanaServerError | ErrorCause): Reason { const attr = ((err as KibanaServerError).attributes || err) as ErrorCause; const { type, reason, caused_by: causedBy } = attr; if (causedBy) { return getNestedCause(causedBy); } return { type, reason }; }

We could improve KibanaServerError by defaulting the generic to ErrorCause. I'll open up an issue so that we can discuss that change separately. We could also add a new isKibanaServerError predicate to avoid using as as an improvement.

Makes sense. There's some more type work to do, in Lens we did inspect quite a bit the EsError shape (for our use cases) but the type error source is not precise itself (I've discussed with @delvedor before about mapping all possible EsError in types but it's a big task to tackle right now).

It's definitely possible to patch this now and open a discussion about error types improvement later on.

jloleysens

Thanks for addressing my feedback @dej611 !

Did not test this locally but code changes look good to me!

dej611 · 2021-09-22T13:07:57Z

@elasticmachine merge upstream

dej611 · 2021-09-22T15:31:23Z

Let's give it another round of tests.

dej611 · 2021-09-22T15:31:35Z

jenkins, test this

kibanamachine · 2021-09-22T17:37:45Z

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id	before	after	diff
`data`	492.3KB	492.5KB	+243.0B
`visTypeVega`	35.8KB	35.9KB	+69.0B
total			+312.0B

History

💛 Build #154953 was flaky cc9b5e9
💔 Build #154933 failed 00099e6
💔 Build #154900 failed ddc38c3
💚 Build #154565 succeeded 53789e0
💚 Build #154552 succeeded 3f5c751

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

…g error responses (elastic#112634) * 🐛 Fix EsError not showing info * 🐛 Record error response in the inspector * 🏷️ Fix type issue * 👌 Integrate feedback * 👌 Integrated latest feedback * 👌 i18n unknwon message * 👌 Integrate feedback * 🐛 Fix syntax error * 🏷️ Fix type issue Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-09-23T08:57:53Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…g error responses (#112634) (#112932) * 🐛 Fix EsError not showing info * 🐛 Record error response in the inspector * 🏷️ Fix type issue * 👌 Integrate feedback * 👌 Integrated latest feedback * 👌 i18n unknwon message * 👌 Integrate feedback * 🐛 Fix syntax error * 🏷️ Fix type issue Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

dej611 added 2 commits September 20, 2021 19:19

🐛 Fix EsError not showing info

7ff21ed

🐛 Record error response in the inspector

39ca687

dej611 added release_note:fix Feature:Vega Vega visualizations Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 21, 2021

🏷️ Fix type issue

f87d095

dej611 marked this pull request as ready for review September 21, 2021 10:03

dej611 requested review from a team as code owners September 21, 2021 10:03

stratoula requested a review from alexwizp September 21, 2021 10:31

alexwizp reviewed Sep 21, 2021

View reviewed changes

👌 Integrate feedback

3f5c751

alexwizp reviewed Sep 21, 2021

View reviewed changes

👌 Integrated latest feedback

53789e0

alexwizp approved these changes Sep 21, 2021

View reviewed changes

stratoula approved these changes Sep 22, 2021

View reviewed changes

jloleysens reviewed Sep 22, 2021

View reviewed changes

dej611 added 2 commits September 22, 2021 12:23

👌 i18n unknwon message

da0bcd1

👌 Integrate feedback

ddc38c3

jloleysens approved these changes Sep 22, 2021

View reviewed changes

dej611 and others added 2 commits September 22, 2021 14:09

🐛 Fix syntax error

00099e6

🏷️ Fix type issue

2f43082

Merge branch 'master' into fix/106478

cc9b5e9

dej611 merged commit 9dcacf7 into elastic:master Sep 23, 2021

kibanamachine mentioned this pull request Sep 23, 2021

[7.x] [Vega] Better error explanation for EsErrors and inspector now showing error responses (#112634) #112932

Merged

dokmic mentioned this pull request Sep 15, 2022

[Search] Allow user to see the root cause of any error (if available) #100291

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vega] Better error explanation for EsErrors and inspector now showing error responses #112634

[Vega] Better error explanation for EsErrors and inspector now showing error responses #112634

dej611 commented Sep 21, 2021

elasticmachine commented Sep 21, 2021

alexwizp Sep 21, 2021

alexwizp Sep 21, 2021 •

edited

Loading

alexwizp Sep 21, 2021

alexwizp Sep 21, 2021 •

edited

Loading

dej611 Sep 21, 2021

alexwizp left a comment

stratoula left a comment

jloleysens left a comment

jloleysens Sep 22, 2021

jloleysens Sep 22, 2021

dej611 Sep 22, 2021

jloleysens left a comment

dej611 commented Sep 22, 2021

dej611 commented Sep 22, 2021

dej611 commented Sep 22, 2021

kibanamachine commented Sep 22, 2021

kibanamachine commented Sep 23, 2021

		function getNestedCause(err: KibanaServerError<any>): Reason {
		const { type, reason, caused_by: causedBy } = err.attributes \|\| err;

[Vega] Better error explanation for EsErrors and inspector now showing error responses #112634

[Vega] Better error explanation for EsErrors and inspector now showing error responses #112634

Conversation

dej611 commented Sep 21, 2021

Summary

Checklist

elasticmachine commented Sep 21, 2021

alexwizp Sep 21, 2021

Choose a reason for hiding this comment

alexwizp Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

alexwizp Sep 21, 2021

Choose a reason for hiding this comment

alexwizp Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

dej611 Sep 21, 2021

Choose a reason for hiding this comment

alexwizp left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens Sep 22, 2021

Choose a reason for hiding this comment

jloleysens Sep 22, 2021

Choose a reason for hiding this comment

dej611 Sep 22, 2021

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

dej611 commented Sep 22, 2021

dej611 commented Sep 22, 2021

dej611 commented Sep 22, 2021

kibanamachine commented Sep 22, 2021

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

kibanamachine commented Sep 23, 2021

💚 Backport successful

alexwizp Sep 21, 2021 •

edited

Loading

alexwizp Sep 21, 2021 •

edited

Loading