[search source] return rawResponse on search failure (#168389)

Closes #167099 #### Problem `/bsearch` and `/search` APIs only return `error` key from elasticsearch error response. This is problematic because Inspector needs `rawResponse` to populate "Clusters and shards" While working on this issue, I discovered another problem with how error responses are added to inspector requestResponder. The `Error` instance is added as `json` key. This is a little awkward since the response tab just stringifies the contents of `json`, thus stringifing the Error object instead of just the error body returned from API. This PR address this problem by setting `json` to either `attributes` or `{ message }`. #### Solution PR updates `/bsearch` and `/search` APIs to return `{ attributes: { error: ErrorCause, rawResponse }}` for failed responses. Solution avoided changing KbnServerError and reportServerError since these methods are used extensivly throughout Kibana (see #167544 (comment) for more details). Instead, KbnSearchError and reportSearchError are created to report search error messages. #### Test 1) install web logs sample data set 2) open discover 3) add filter ``` { "error_query": { "indices": [ { "error_type": "exception", "message": "local shard failure message 123", "name": "kibana_sample_data_logs", "shard_ids": [ 0 ] } ] } } ``` 4) Open inspector. Verify "Clusters and shards" tab is visible and populated. Verify "Response" tab shows "error" and "rawResponse" keys. <img width="500" alt="Screenshot 2023-10-09 at 9 29 16 AM" src="https://github.com/elastic/kibana/assets/373691/461b0eb0-0502-4d48-a487-68025ef24d35"> <img width="500" alt="Screenshot 2023-10-09 at 9 29 06 AM" src="https://github.com/elastic/kibana/assets/373691/9aff41eb-f771-48e3-a66d-1447689c2c6a"> --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Gloria Hornero <[email protected]>
elastic · Oct 23, 2023 · adf3b8b · adf3b8b
1 parent 3587c20
commit adf3b8b
Show file tree

Hide file tree

Showing 34 changed files with 333 additions and 242 deletions.
diff --git a/src/plugins/data/common/search/expressions/eql.ts b/src/plugins/data/common/search/expressions/eql.ts
@@ -166,7 +166,7 @@ export const getEqlFn = ({
           body: response.rawResponse,
         };
       } catch (e) {
-        request.error({ json: e });
+        request.error({ json: 'attributes' in e ? e.attributes : { message: e.message } });
         throw e;
       }
     },

diff --git a/src/plugins/data/common/search/expressions/esdsl.ts b/src/plugins/data/common/search/expressions/esdsl.ts
@@ -188,7 +188,7 @@ export const getEsdslFn = ({
           body: rawResponse,
         };
       } catch (e) {
-        request.error({ json: e });
+        request.error({ json: 'attributes' in e ? e.attributes : { message: e.message } });
         throw e;
       }
     },

diff --git a/src/plugins/data/common/search/expressions/esql.ts b/src/plugins/data/common/search/expressions/esql.ts
@@ -227,7 +227,9 @@ export const getEsqlFn = ({ getStartDependencies }: EsqlFnArguments) => {
                   .ok({ json: rawResponse });
               },
               error(error) {
-                logInspectorRequest().error({ json: error });
+                logInspectorRequest().error({
+                  json: 'attributes' in error ? error.attributes : { message: error.message },
+                });
               },
             })
           );

diff --git a/src/plugins/data/common/search/expressions/essql.ts b/src/plugins/data/common/search/expressions/essql.ts
@@ -248,7 +248,9 @@ export const getEssqlFn = ({ getStartDependencies }: EssqlFnArguments) => {
                   .ok({ json: rawResponse });
               },
               error(error) {
-                logInspectorRequest().error({ json: error });
+                logInspectorRequest().error({
+                  json: 'attributes' in error ? error.attributes : { message: error.message },
+                });
               },
             })
           );

diff --git a/src/plugins/data/common/search/search_source/search_source.ts b/src/plugins/data/common/search/search_source/search_source.ts
@@ -458,7 +458,9 @@ export class SearchSource {
     const last$ = s$
       .pipe(
         catchError((e) => {
-          requestResponder?.error({ json: e });
+          requestResponder?.error({
+            json: 'attributes' in e ? e.attributes : { message: e.message },
+          });
           return EMPTY;
         }),
         last(undefined, null),

diff --git a/src/plugins/data/public/index.ts b/src/plugins/data/public/index.ts
@@ -167,7 +167,6 @@ export type {
   SerializedSearchSourceFields,
   // errors
   IEsError,
-  Reason,
   WaitUntilNextSessionCompletesOptions,
 } from './search';
 

diff --git a/src/plugins/data/public/search/errors/es_error.test.tsx b/src/plugins/data/public/search/errors/es_error.test.tsx
@@ -7,6 +7,7 @@
  */
 
 import { EsError } from './es_error';
+import { IEsError } from './types';
 
 describe('EsError', () => {
   it('contains the same body as the wrapped error', () => {
@@ -19,7 +20,7 @@ describe('EsError', () => {
           reason: 'top-level reason',
         },
       },
-    } as any;
+    } as IEsError;
     const esError = new EsError(error);
 
     expect(typeof esError.attributes).toEqual('object');
@@ -33,20 +34,22 @@ describe('EsError', () => {
         'x_content_parse_exception: [x_content_parse_exception] Reason: [1:78] [date_histogram] failed to parse field [calendar_interval]',
       statusCode: 400,
       attributes: {
-        root_cause: [
-          {
-            type: 'x_content_parse_exception',
-            reason: '[1:78] [date_histogram] failed to parse field [calendar_interval]',
+        error: {
+          root_cause: [
+            {
+              type: 'x_content_parse_exception',
+              reason: '[1:78] [date_histogram] failed to parse field [calendar_interval]',
+            },
+          ],
+          type: 'x_content_parse_exception',
+          reason: '[1:78] [date_histogram] failed to parse field [calendar_interval]',
+          caused_by: {
+            type: 'illegal_argument_exception',
+            reason: 'The supplied interval [2q] could not be parsed as a calendar interval.',
           },
-        ],
-        type: 'x_content_parse_exception',
-        reason: '[1:78] [date_histogram] failed to parse field [calendar_interval]',
-        caused_by: {
-          type: 'illegal_argument_exception',
-          reason: 'The supplied interval [2q] could not be parsed as a calendar interval.',
         },
       },
-    } as any;
+    } as IEsError;
     const esError = new EsError(error);
     expect(esError.message).toEqual(
       'EsError: The supplied interval [2q] could not be parsed as a calendar interval.'

diff --git a/src/plugins/data/public/search/errors/es_error.tsx b/src/plugins/data/public/search/errors/es_error.tsx
@@ -8,8 +8,8 @@
 
 import React from 'react';
 import { EuiCodeBlock, EuiSpacer } from '@elastic/eui';
-import { ApplicationStart } from '@kbn/core/public';
 import { i18n } from '@kbn/i18n';
+import { ApplicationStart } from '@kbn/core/public';
 import { KbnError } from '@kbn/kibana-utils-plugin/common';
 import { IEsError } from './types';
 import { getRootCause } from './utils';
@@ -20,26 +20,28 @@ export class EsError extends KbnError {
   constructor(protected readonly err: IEsError) {
     super(
       `EsError: ${
-        getRootCause(err)?.reason ||
+        getRootCause(err?.attributes?.error)?.reason ||
         i18n.translate('data.esError.unknownRootCause', { defaultMessage: 'unknown' })
       }`
     );
     this.attributes = err.attributes;
   }
 
   public getErrorMessage(application: ApplicationStart) {
-    const rootCause = getRootCause(this.err)?.reason;
-    const topLevelCause = this.attributes?.reason;
+    if (!this.attributes?.error) {
+      return null;
+    }
+
+    const rootCause = getRootCause(this.attributes.error)?.reason;
+    const topLevelCause = this.attributes.error.reason;
     const cause = rootCause ?? topLevelCause;
 
     return (
       <>
         <EuiSpacer size="s" />
-        {cause ? (
-          <EuiCodeBlock data-test-subj="errMessage" isCopyable={true} paddingSize="s">
-            {cause}
-          </EuiCodeBlock>
-        ) : null}
+        <EuiCodeBlock data-test-subj="errMessage" isCopyable={true} paddingSize="s">
+          {cause}
+        </EuiCodeBlock>
       </>
     );
   }

diff --git a/src/plugins/data/public/search/errors/painless_error.test.tsx b/src/plugins/data/public/search/errors/painless_error.test.tsx
@@ -23,11 +23,13 @@ describe('PainlessError', () => {
     const e = new PainlessError({
       statusCode: 400,
       message: 'search_phase_execution_exception',
-      attributes: searchPhaseException.error,
+      attributes: {
+        error: searchPhaseException.error,
+      },
     });
     const component = mount(e.getErrorMessage(startMock.application));
 
-    const failedShards = e.attributes?.failed_shards![0];
+    const failedShards = searchPhaseException.error.failed_shards![0];
 
     const stackTraceElem = findTestSubject(component, 'painlessStackTrace').getDOMNode();
     const stackTrace = failedShards!.reason.script_stack!.splice(-2).join('\n');

diff --git a/src/plugins/data/public/search/errors/painless_error.tsx b/src/plugins/data/public/search/errors/painless_error.tsx
@@ -31,7 +31,7 @@ export class PainlessError extends EsError {
       });
     }
 
-    const rootCause = getRootCause(this.err);
+    const rootCause = getRootCause(this.err.attributes?.error);
     const scriptFromStackTrace = rootCause?.script_stack
       ? rootCause?.script_stack?.slice(-2).join('\n')
       : undefined;
@@ -78,7 +78,7 @@ export class PainlessError extends EsError {
 export function isPainlessError(err: Error | IEsError) {
   if (!isEsError(err)) return false;
 
-  const rootCause = getRootCause(err as IEsError);
+  const rootCause = getRootCause((err as IEsError).attributes?.error);
   if (!rootCause) return false;
 
   const { lang } = rootCause;

diff --git a/src/plugins/data/public/search/errors/types.ts b/src/plugins/data/public/search/errors/types.ts
@@ -5,39 +5,13 @@
  * in compliance with, at your election, the Elastic License 2.0 or the Server
  * Side Public License, v 1.
  */
-import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
-import { KibanaServerError } from '@kbn/kibana-utils-plugin/common';
-
-export interface FailedShard {
-  shard: number;
-  index: string;
-  node: string;
-  reason: Reason;
-}
 
-export interface Reason {
-  type: string;
-  reason?: string;
-  script_stack?: string[];
-  position?: {
-    offset: number;
-    start: number;
-    end: number;
-  };
-  lang?: estypes.ScriptLanguage;
-  script?: string;
-  caused_by?: {
-    type: string;
-    reason: string;
-  };
-}
+import { estypes } from '@elastic/elasticsearch';
+import { KibanaServerError } from '@kbn/kibana-utils-plugin/common';
 
 interface IEsErrorAttributes {
-  type: string;
-  reason: string;
-  root_cause?: Reason[];
-  failed_shards?: FailedShard[];
-  caused_by?: IEsErrorAttributes;
+  rawResponse?: estypes.SearchResponseBody;
+  error?: estypes.ErrorCause;
 }
 
 export type IEsError = KibanaServerError<IEsErrorAttributes>;

diff --git a/src/plugins/data/public/search/errors/utils.ts b/src/plugins/data/public/search/errors/utils.ts
@@ -5,26 +5,21 @@
  * in compliance with, at your election, the Elastic License 2.0 or the Server
  * Side Public License, v 1.
  */
-import type { ErrorCause } from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
-import { KibanaServerError } from '@kbn/kibana-utils-plugin/common';
-import type { FailedShard, Reason } from './types';
 
-export function getFailedShards(err: KibanaServerError<any>): FailedShard | undefined {
-  const errorInfo = err.attributes;
-  const failedShards = errorInfo?.failed_shards || errorInfo?.caused_by?.failed_shards;
-  return failedShards ? failedShards[0] : undefined;
+import { estypes } from '@elastic/elasticsearch';
+
+function getFailedShardCause(error: estypes.ErrorCause): estypes.ErrorCause | undefined {
+  const failedShards = error.failed_shards || error.caused_by?.failed_shards;
+  return failedShards ? failedShards[0]?.reason : undefined;
 }
 
-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 };
+function getNestedCause(error: estypes.ErrorCause): estypes.ErrorCause {
+  return error.caused_by ? getNestedCause(error.caused_by) : error;
 }
 
-export function getRootCause(err: KibanaServerError) {
-  // Give shard failures priority, then try to get the error navigating nested objects
-  return getFailedShards(err)?.reason || getNestedCause(err);
+export function getRootCause(error?: estypes.ErrorCause): estypes.ErrorCause | undefined {
+  return error
+    ? // Give shard failures priority, then try to get the error navigating nested objects
+      getFailedShardCause(error) || getNestedCause(error)
+    : undefined;
 }
diff --git a/src/plugins/data/public/search/search_interceptor/search_interceptor.test.ts b/src/plugins/data/public/search/search_interceptor/search_interceptor.test.ts
@@ -22,6 +22,7 @@ import * as resourceNotFoundException from '../../../common/search/test_data/res
 import { BehaviorSubject } from 'rxjs';
 import { dataPluginMock } from '../../mocks';
 import { UI_SETTINGS } from '../../../common';
+import type { IEsError } from '../errors';
 
 jest.mock('./utils', () => {
   const originalModule = jest.requireActual('./utils');
@@ -151,7 +152,9 @@ describe('SearchInterceptor', () => {
         new PainlessError({
           statusCode: 400,
           message: 'search_phase_execution_exception',
-          attributes: searchPhaseException.error,
+          attributes: {
+            error: searchPhaseException.error,
+          },
         })
       );
       expect(mockCoreSetup.notifications.toasts.addDanger).toBeCalledTimes(1);
@@ -1452,10 +1455,12 @@ describe('SearchInterceptor', () => {
       });
 
       test('Should throw Painless error on server error with OSS format', async () => {
-        const mockResponse: any = {
+        const mockResponse: IEsError = {
           statusCode: 400,
           message: 'search_phase_execution_exception',
-          attributes: searchPhaseException.error,
+          attributes: {
+            error: searchPhaseException.error,
+          },
         };
         fetchMock.mockRejectedValueOnce(mockResponse);
         const mockRequest: IEsSearchRequest = {
@@ -1466,10 +1471,12 @@ describe('SearchInterceptor', () => {
       });
 
       test('Should throw ES error on ES server error', async () => {
-        const mockResponse: any = {
+        const mockResponse: IEsError = {
           statusCode: 400,
           message: 'resource_not_found_exception',
-          attributes: resourceNotFoundException.error,
+          attributes: {
+            error: resourceNotFoundException.error,
+          },
         };
         fetchMock.mockRejectedValueOnce(mockResponse);
         const mockRequest: IEsSearchRequest = {

diff --git a/src/plugins/data/public/search/search_interceptor/search_interceptor.ts b/src/plugins/data/public/search/search_interceptor/search_interceptor.ts
@@ -194,18 +194,18 @@ export class SearchInterceptor {
       // The timeout error is shown any time a request times out, or once per session, if the request is part of a session.
       this.showTimeoutError(err, options?.sessionId);
       return err;
-    } else if (e instanceof AbortError || e instanceof BfetchRequestError) {
+    }
+
+    if (e instanceof AbortError || e instanceof BfetchRequestError) {
       // In the case an application initiated abort, throw the existing AbortError, same with BfetchRequestErrors
       return e;
-    } else if (isEsError(e)) {
-      if (isPainlessError(e)) {
-        return new PainlessError(e, options?.indexPattern);
-      } else {
-        return new EsError(e);
-      }
-    } else {
-      return e instanceof Error ? e : new Error(e.message);
     }
+
+    if (isEsError(e)) {
+      return isPainlessError(e) ? new PainlessError(e, options?.indexPattern) : new EsError(e);
+    }
+
+    return e instanceof Error ? e : new Error(e.message);
   }
 
   private getSerializableOptions(options?: ISearchOptions) {