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

[Lens] Leverage original http request error #79831

Merged
merged 8 commits into from
Oct 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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> | |

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;
```
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> | |

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[];
```
7 changes: 6 additions & 1 deletion src/plugins/expressions/common/util/create_error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ export const createError = (err: string | ErrorLike): ExpressionValueError => ({
: undefined,
message: typeof err === 'string' ? err : String(err.message),
name: typeof err === 'object' ? err.name || 'Error' : 'Error',
original: err instanceof Error ? (err as SerializedError) : undefined,
original:
err instanceof Error
? err
: typeof err === 'object' && 'original' in err && err.original instanceof Error
Copy link
Contributor

@dej611 dej611 Oct 7, 2020

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

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
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,8 @@ export class ExpressionRendererRegistry implements IRegistry<ExpressionRenderer>
//
// @public (undocumented)
export interface ExpressionRenderError extends Error {
// (undocumented)
original?: Error;
// (undocumented)
type?: string;
}
Expand Down Expand Up @@ -1095,7 +1097,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
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
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
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
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
52 changes: 52 additions & 0 deletions x-pack/plugins/lens/public/editor_frame_service/error_helper.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* 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 ElasticsearchErrorClause {
type: string;
reason: string;
caused_by?: ElasticsearchErrorClause;
}

interface RequestError extends Error {
body?: { attributes?: { error: ElasticsearchErrorClause } };
}

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

function getNestedErrorClause({
type,
reason,
caused_by: causedBy,
}: ElasticsearchErrorClause): { type: string; reason: string } {
if (causedBy) {
return getNestedErrorClause(causedBy);
}
return { type, reason };
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be skipping all the intermediate types and reasons? This recursion makes sense, but are there cases where it's losing important information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the point - for the known cases the root exception is the relevant one and I made this experience very often in Java in general (lot's of re-thrown exceptions, but the actual cause is in the very last one). I guess when we solve this cleanly there should be an option to expand the error to show all the details. I will open an issue for this.

}

export function getOriginalRequestErrorMessage(error?: ExpressionRenderError | null) {
if (error && 'original' in error && error.original && isRequestError(error.original)) {
const rootError = getNestedErrorClause(error.original.body!.attributes!.error);
if (rootError.reason && rootError.type) {
return i18n.translate('xpack.lens.editorFrame.expressionFailureMessage', {
defaultMessage: 'Request error: {type}, {reason}',
values: {
reason: rootError.reason,
type: rootError.type,
},
});
}
}
}