Better error message for failed xhr requests (#146340)

## Summary

Error messages should be as actionable as possible.

In this issue - https://github.com/elastic/kibana/issues/145810 - Kibana
is tested without a network connection and it returns a cryptic toast :
`Batch request failed with status 0`. A little research revealed that is
almost certainly a network connection problem. Lets state so directly so
that users can investigate network connection problems.

We should also discuss better text for other status codes. `Batch
request failed with status` probably isn't the most useful text. Whats a
batch request? Is it relevant to any action the user might take?

<img width="1059" alt="Screen Shot 2022-12-14 at 4 12 07 PM"
src="https://user-images.githubusercontent.com/216176/207726319-efb20350-03a1-4af9-8b6d-fdc34b472604.png">

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>
This commit is contained in:
Matthew Kime 2022-12-15 17:08:01 -06:00 committed by GitHub
parent 5998d29a2a
commit 5b421eb0db
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 63 additions and 39 deletions

View file

@ -0,0 +1,37 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/
import { i18n } from '@kbn/i18n';
/**
* Error thrown when xhr request fails
* @public
*/
export class BfetchRequestError extends Error {
/**
* constructor
* @param code - Xhr error code
*/
constructor(code: number) {
const message =
code === 0
? i18n.translate('bfetch.networkError', {
defaultMessage: 'Check your network connection and try again.',
})
: i18n.translate('bfetch.networkErrorWithStatus', {
defaultMessage: 'Check your network connection and try again. Code {code}',
values: { code },
});
super(message);
this.name = 'BfetchRequestError';
this.code = code;
}
code: number;
}

View file

@ -12,3 +12,4 @@ export type { ItemBufferParams, TimedItemBufferParams, BatchedFunctionParams } f
export { ItemBuffer, TimedItemBuffer, createBatchedFunction } from './buffer';
export type { ErrorLike, BatchRequestData, BatchResponseItem, BatchItemWrapper } from './batch';
export { DISABLE_BFETCH_COMPRESSION, DISABLE_BFETCH } from './constants';
export { BfetchRequestError } from './bfetch_error';

View file

@ -7,6 +7,7 @@
*/
import { ErrorLike } from '../batch';
import { BfetchRequestError } from '..';
export const normalizeError = <E extends ErrorLike = ErrorLike>(err: any): E => {
if (!err) {
@ -14,6 +15,11 @@ export const normalizeError = <E extends ErrorLike = ErrorLike>(err: any): E =>
message: 'Unknown error.',
} as E;
}
if (err instanceof BfetchRequestError) {
// ignoring so we can return the error as is
// @ts-expect-error
return err;
}
if (err instanceof Error) {
return { message: err.message } as E;
}

View file

@ -15,6 +15,7 @@ export { split } from './streaming';
export type { BatchedFunc } from './batching/types';
export { DISABLE_BFETCH } from '../common/constants';
export { BfetchRequestError } from '../common/bfetch_error';
export function plugin(initializerContext: PluginInitializerContext) {
return new BfetchPublicPlugin(initializerContext);

View file

@ -271,7 +271,7 @@ test('promise throws when request errors', async () => {
expect(spy).toHaveBeenCalledTimes(1);
expect(spy.mock.calls[0][0]).toBeInstanceOf(Error);
expect(spy.mock.calls[0][0].message).toMatchInlineSnapshot(
`"Batch request failed with status 400"`
`"Check your network connection and try again. Code 400"`
);
});
@ -300,7 +300,7 @@ test('stream observable errors when request errors', async () => {
expect(spy).toHaveBeenCalledTimes(1);
expect(spy.mock.calls[0][0]).toBeInstanceOf(Error);
expect(spy.mock.calls[0][0].message).toMatchInlineSnapshot(
`"Batch request failed with status 400"`
`"Check your network connection and try again. Code 400"`
);
});

View file

@ -152,7 +152,7 @@ test('errors observable if request returns with error', () => {
expect(error).toHaveBeenCalledTimes(1);
expect(error.mock.calls[0][0]).toBeInstanceOf(Error);
expect(error.mock.calls[0][0].message).toMatchInlineSnapshot(
`"Batch request failed with status 400"`
`"Check your network connection and try again. Code 400"`
);
});
@ -182,7 +182,7 @@ test('does not emit when gets error response', () => {
expect(error).toHaveBeenCalledTimes(1);
expect(error.mock.calls[0][0]).toBeInstanceOf(Error);
expect(error.mock.calls[0][0].message).toMatchInlineSnapshot(
`"Batch request failed with status 400"`
`"Check your network connection and try again. Code 400"`
);
});

View file

@ -7,6 +7,7 @@
*/
import { Observable, Subject } from 'rxjs';
import { BfetchRequestError } from '../../common';
/**
* Creates observable from streaming XMLHttpRequest, where each event
@ -61,7 +62,7 @@ export const fromStreamingXhr = (
if (signal) signal.removeEventListener('abort', onBatchAbort);
if (isErrorStatus()) {
subject.error(new Error(`Batch request failed with status ${xhr.status}`));
subject.error(new BfetchRequestError(xhr.status));
} else {
subject.complete();
}

View file

@ -1,27 +0,0 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/
import { EuiCodeBlock, EuiSpacer } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import React from 'react';
export function getHttpError(message: string) {
return (
<>
{i18n.translate('data.errors.fetchError', {
defaultMessage:
'Check your network and proxy configuration. If the problem persists, contact your network administrator.',
})}
<EuiSpacer size="s" />
<EuiSpacer size="s" />
<EuiCodeBlock data-test-subj="errMessage" isCopyable={true} paddingSize="s">
{message}
</EuiCodeBlock>
</>
);
}

View file

@ -11,5 +11,4 @@ export * from './painless_error';
export * from './timeout_error';
export * from './utils';
export * from './types';
export * from './http_error';
export * from './search_session_incomplete_warning';

View file

@ -31,6 +31,7 @@ import {
} from 'rxjs/operators';
import { PublicMethodsOf } from '@kbn/utility-types';
import type { HttpSetup, IHttpFetchError } from '@kbn/core-http-browser';
import { BfetchRequestError } from '@kbn/bfetch-plugin/public';
import {
ApplicationStart,
@ -60,7 +61,6 @@ import {
import { SearchUsageCollector } from '../collectors';
import {
EsError,
getHttpError,
isEsError,
isPainlessError,
PainlessError,
@ -195,8 +195,8 @@ 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) {
// In the case an application initiated abort, throw the existing AbortError.
} else 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)) {
@ -525,12 +525,18 @@ export class SearchInterceptor {
}),
text: toMountPoint(e.getErrorMessage(this.application), { theme$: this.deps.theme.theme$ }),
});
} else if (e.constructor.name === 'HttpFetchError') {
} else if (e.constructor.name === 'HttpFetchError' || e instanceof BfetchRequestError) {
const defaultMsg = i18n.translate('data.errors.fetchError', {
defaultMessage: 'Check your network connection and try again.',
});
this.deps.toasts.addDanger({
title: i18n.translate('data.search.httpErrorTitle', {
defaultMessage: 'Cannot retrieve your data',
defaultMessage: 'Unable to connect to the Kibana server',
}),
text: toMountPoint(e.message || defaultMsg, {
theme$: this.deps.theme.theme$,
}),
text: toMountPoint(getHttpError(e.message), { theme$: this.deps.theme.theme$ }),
});
} else {
this.deps.toasts.addError(e, {