Skip to content

[Lens] Leverage original http request error #79831

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 13, 2020
Merged

[Lens] Leverage original http request error #79831

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
926fbae
leverage original http request error
flash1293 Oct 7, 2020
a0b1958
default to standard error message
flash1293 Oct 7, 2020
20710c8
fix documentation
flash1293 Oct 7, 2020
9073897
Merge remote-tracking branch 'upstream/master' into lens/fix-empty-error
flash1293 Oct 12, 2020
b7d33a7
use root error
flash1293 Oct 12, 2020
521e932
fix i18n
flash1293 Oct 12, 2020
a8cf08f
fix key
flash1293 Oct 12, 2020
0b2aaa4
Merge remote-tracking branch 'upstream/master' into lens/fix-empty-error
flash1293 Oct 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...ssions/public/kibana-plugin-plugins-expressions-public.expressionrendererror.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ export interface ExpressionRenderError extends Error

| Property | Type | Description |
| --- | --- | --- |
| [original](./kibana-plugin-plugins-expressions-public.expressionrendererror.original.md) | <code>Error</code> | |
| [type](./kibana-plugin-plugins-expressions-public.expressionrendererror.type.md) | <code>string</code> | |

11 changes: 11 additions & 0 deletions ...blic/kibana-plugin-plugins-expressions-public.expressionrendererror.original.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-plugins-expressions-public](./kibana-plugin-plugins-expressions-public.md) &gt; [ExpressionRenderError](./kibana-plugin-plugins-expressions-public.expressionrendererror.md) &gt; [original](./kibana-plugin-plugins-expressions-public.expressionrendererror.original.md)

## ExpressionRenderError.original property

<b>Signature:</b>

```typescript
original?: Error;
```
2 changes: 1 addition & 1 deletion ...public/kibana-plugin-plugins-expressions-public.reactexpressionrendererprops.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ export interface ReactExpressionRendererProps extends IExpressionLoaderParams
| [onEvent](./kibana-plugin-plugins-expressions-public.reactexpressionrendererprops.onevent.md) | <code>(event: ExpressionRendererEvent) =&gt; void</code> | |
| [padding](./kibana-plugin-plugins-expressions-public.reactexpressionrendererprops.padding.md) | <code>'xs' &#124; 's' &#124; 'm' &#124; 'l' &#124; 'xl'</code> | |
| [reload$](./kibana-plugin-plugins-expressions-public.reactexpressionrendererprops.reload_.md) | <code>Observable&lt;unknown&gt;</code> | An observable which can be used to re-run the expression without destroying the component |
| [renderError](./kibana-plugin-plugins-expressions-public.reactexpressionrendererprops.rendererror.md) | <code>(error?: string &#124; null) =&gt; React.ReactElement &#124; React.ReactElement[]</code> | |
| [renderError](./kibana-plugin-plugins-expressions-public.reactexpressionrendererprops.rendererror.md) | <code>(message?: string &#124; null, error?: ExpressionRenderError &#124; null) =&gt; React.ReactElement &#124; React.ReactElement[]</code> | |

2 changes: 1 addition & 1 deletion ...a-plugin-plugins-expressions-public.reactexpressionrendererprops.rendererror.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
<b>Signature:</b>

```typescript
renderError?: (error?: string | null) => React.ReactElement | React.ReactElement[];
renderError?: (message?: string | null, error?: ExpressionRenderError | null) => React.ReactElement | React.ReactElement[];
```
11 changes: 9 additions & 2 deletions src/plugins/expressions/common/util/create_error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ import { ExpressionValueError } from '../../common';

type ErrorLike = Partial<Pick<Error, 'name' | 'message' | 'stack'>>;

export const createError = (err: string | Error | ErrorLike): ExpressionValueError => ({
export const createError = (
err: string | Error | ErrorLike | ExpressionValueError['error']
): ExpressionValueError => ({
type: 'error',
error: {
stack:
Expand All @@ -32,6 +34,11 @@ export const createError = (err: string | Error | ErrorLike): ExpressionValueErr
: undefined,
message: typeof err === 'string' ? err : String(err.message),
name: typeof err === 'object' ? err.name || 'Error' : 'Error',
original: err instanceof Error ? err : undefined,
original:
err instanceof Error
? err
: typeof err === 'object' && 'original' in err && err.original instanceof Error
Copy link
Contributor

@dej611 dej611 Oct 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think you could leverage some knowledge from TS here and just do

...
: err.original instanceof Error ? err.original : undefined

Copy link
Contributor Author

@flash1293 flash1293 Oct 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This code is necessary because err could be a string based on the type, so we have to do some generic checks first, otherwise typescript will complain about accessing original on a union type that might not have the property

? err.original
: undefined,
},
});
4 changes: 3 additions & 1 deletion src/plugins/expressions/public/public.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,8 @@ export class ExpressionRendererRegistry implements IRegistry<ExpressionRenderer>
//
// @public (undocumented)
export interface ExpressionRenderError extends Error {
// (undocumented)
original?: Error;
// (undocumented)
type?: string;
}
Expand Down Expand Up @@ -1073,7 +1075,7 @@ export interface ReactExpressionRendererProps extends IExpressionLoaderParams {
padding?: 'xs' | 's' | 'm' | 'l' | 'xl';
reload$?: Observable<unknown>;
// (undocumented)
renderError?: (error?: string | null) => React.ReactElement | React.ReactElement[];
renderError?: (message?: string | null, error?: ExpressionRenderError | null) => React.ReactElement | React.ReactElement[];
}

// Warning: (ae-missing-release-tag) "ReactExpressionRendererType" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
Expand Down
10 changes: 8 additions & 2 deletions src/plugins/expressions/public/react_expression_renderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ export interface ReactExpressionRendererProps extends IExpressionLoaderParams {
className?: string;
dataAttrs?: string[];
expression: string | ExpressionAstExpression;
renderError?: (error?: string | null) => React.ReactElement | React.ReactElement[];
renderError?: (
message?: string | null,
error?: ExpressionRenderError | null
) => React.ReactElement | React.ReactElement[];
padding?: 'xs' | 's' | 'm' | 'l' | 'xl';
onEvent?: (event: ExpressionRendererEvent) => void;
/**
Expand Down Expand Up @@ -186,7 +189,10 @@ export const ReactExpressionRenderer = ({
<div {...dataAttrs} className={classes}>
{state.isEmpty && <EuiLoadingChart mono size="l" />}
{state.isLoading && <EuiProgress size="xs" color="accent" position="absolute" />}
{!state.isLoading && state.error && renderError && renderError(state.error.message)}
{!state.isLoading &&
state.error &&
renderError &&
renderError(state.error.message, state.error)}
<div
className="expExpressionRenderer__expression"
style={expressionStyles}
Expand Down
1 change: 1 addition & 0 deletions src/plugins/expressions/public/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export interface IExpressionLoaderParams {

export interface ExpressionRenderError extends Error {
type?: string;
original?: Error;
}

export type RenderErrorHandlerFnType = (
Expand Down
9 changes: 6 additions & 3 deletions ...plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { CoreStart, CoreSetup } from 'kibana/public';
import { ExecutionContextSearch } from 'src/plugins/expressions';
import {
ExpressionRendererEvent,
ExpressionRenderError,
ReactExpressionRendererType,
} from '../../../../../../../src/plugins/expressions/public';
import { Action } from '../state_management';
Expand All @@ -40,6 +41,7 @@ import {
} from '../../../../../../../src/plugins/data/public';
import { WorkspacePanelWrapper } from './workspace_panel_wrapper';
import { DropIllustration } from '../../../assets/drop_illustration';
import { getOriginalRequestErrorMessage } from '../../error_helper';

export interface WorkspacePanelProps {
activeVisualizationId: string | null;
Expand Down Expand Up @@ -342,7 +344,8 @@ export const InnerVisualizationWrapper = ({
searchContext={context}
reload$={autoRefreshFetch$}
onEvent={onEvent}
renderError={(errorMessage?: string | null) => {
renderError={(errorMessage?: string | null, error?: ExpressionRenderError | null) => {
const visibleErrorMessage = getOriginalRequestErrorMessage(error) || errorMessage;
return (
<EuiFlexGroup style={{ maxWidth: '100%' }} direction="column" alignItems="center">
<EuiFlexItem>
Expand All @@ -354,7 +357,7 @@ export const InnerVisualizationWrapper = ({
defaultMessage="An error occurred when loading data."
/>
</EuiFlexItem>
{errorMessage ? (
{visibleErrorMessage ? (
<EuiFlexItem className="eui-textBreakAll" grow={false}>
<EuiButtonEmpty
onClick={() => {
Expand All @@ -369,7 +372,7 @@ export const InnerVisualizationWrapper = ({
})}
</EuiButtonEmpty>

{localState.expandError ? errorMessage : null}
{localState.expandError ? visibleErrorMessage : null}
</EuiFlexItem>
) : null}
</EuiFlexGroup>
Expand Down
16 changes: 15 additions & 1 deletion x-pack/plugins/lens/public/editor_frame_service/embeddable/expression_wrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
ReactExpressionRendererType,
} from 'src/plugins/expressions/public';
import { ExecutionContextSearch } from 'src/plugins/expressions';
import { getOriginalRequestErrorMessage } from '../error_helper';

export interface ExpressionWrapperProps {
ExpressionRenderer: ReactExpressionRendererType;
Expand Down Expand Up @@ -50,7 +51,20 @@ export function ExpressionWrapper({
padding="m"
expression={expression}
searchContext={searchContext}
renderError={(error) => <div data-test-subj="expression-renderer-error">{error}</div>}
renderError={(errorMessage, error) => (
<div data-test-subj="expression-renderer-error">
<EuiFlexGroup direction="column" alignItems="center" justifyContent="center">
<EuiFlexItem>
<EuiIcon type="alert" color="danger" />
</EuiFlexItem>
<EuiFlexItem>
<EuiText size="s">
{getOriginalRequestErrorMessage(error) || errorMessage}
</EuiText>
</EuiFlexItem>
</EuiFlexGroup>
</div>
)}
onEvent={handleEvent}
/>
</div>
Expand Down
36 changes: 36 additions & 0 deletions x-pack/plugins/lens/public/editor_frame_service/error_helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { i18n } from '@kbn/i18n';

import { ExpressionRenderError } from 'src/plugins/expressions/public';

interface RequestError extends Error {
body?: { attributes?: { error: { caused_by: { type: string; reason: string } } } };
}

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

export function getOriginalRequestErrorMessage(error?: ExpressionRenderError | null) {
return (
error &&
'original' in error &&
error.original &&
isRequestError(error.original) &&
i18n.translate('xpack.lens.editorFrame.expressionFailureMessage', {
defaultMessage: 'Request error: {causedByType}, {causedByReason}',
values: {
causedByType: error.original.body?.attributes?.error?.caused_by.type,
causedByReason: error.original.body?.attributes?.error?.caused_by.reason,
Copy link
Contributor

@wylieconlon wylieconlon Oct 8, 2020

Choose a reason for hiding this comment

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

The logic you're using here looks incomplete, so I tried to break it. There appear to be different error responses for different types of failures, and I think you've only tested permission failures. I don't feel comfortable with the sort of one-off error response handling that you're doing here unless we can validate it against some kind of reference error handling implementation. Maybe the ES Clients team can take a look at this?

For example, sometimes there is a caused_by.caused_by response from Elasticsearch when Elasticsearch fails to validate the request itself, such as when you use a terms agg on a text field. For example, try running this request in the dev tools:

GET kibana_sample_data_logs/_search
{
  "size": 0,
  "aggs": {
    "t": {
      "terms": {
        "field": "host"
      }
    }
  }
}

Another example of a response that you're missing a case for is when the script is failing. This can happen fairly often with an example like metricbeat:

GET metricbeat-*/_search
{
  "size": 0,
  "aggs": {
    "d": {
      "max": {
        "script": """doc['system.network.in.bytes'].value"""
      }
    }
  }
}

Copy link
Contributor Author

@flash1293 flash1293 Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I used the same logic as this piece of code from core:

kibana/src/core/public/notifications/toasts/error_toast.tsx

Line 71 in 24d29a3

if (isRequestError(error)) {

I totally see your point, but I don’t think we have a “handleAllCommonEsErrors” helper (yet). What do you think about adding nice handling for the cases you mention above (great findings btw), and if none of these apply falling back to the default behavior of unknown errors (which is showing the message of the thrown exception and also what’s happening on master right now) - at least for 7.10. We should continue to investigate to find a holistic solution and ideally roll it out everywhere (Discover, Visualize etc - all of them produce really bad error messages depending on the case)

I’m also more than happy to loop in someone knowledgeable on the topic to improve the PR beyond what’s mentioned above (do you know who would be a good fit?), my only ask is to not increase scope too much for an improvement of the current state of 7.10 (that’s also why I didn’t touch core code itself to refactor out a generic helper, it felt like opening a can of worms too big for the scope of the intended fix).

},
})
);
}