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

[APM] Surface http errors to users #42160

Merged
merged 4 commits into from
Jul 31, 2019
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 @@ -10,18 +10,24 @@ import React from 'react';
import { KibanaLink } from '../../shared/Links/KibanaLink';
import { SetupInstructionsLink } from '../../shared/Links/SetupInstructionsLink';
import { LoadingStatePrompt } from '../../shared/LoadingStatePrompt';
import { FETCH_STATUS } from '../../../hooks/useFetcher';
import { ErrorStatePrompt } from '../../shared/ErrorStatePrompt';

interface Props {
// any data submitted from APM agents found (not just in the given time range)
historicalDataFound: boolean;
isLoading: boolean;
status: FETCH_STATUS | undefined;
}

export function NoServicesMessage({ historicalDataFound, isLoading }: Props) {
if (isLoading) {
export function NoServicesMessage({ historicalDataFound, status }: Props) {
if (status === 'loading') {
return <LoadingStatePrompt />;
}

if (status === 'failure') {
return <ErrorStatePrompt />;
}

if (historicalDataFound) {
return (
<EuiEmptyPrompt
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,20 @@
import { shallow } from 'enzyme';
import React from 'react';
import { NoServicesMessage } from '../NoServicesMessage';
import { FETCH_STATUS } from '../../../../hooks/useFetcher';

describe('NoServicesMessage', () => {
it('should show only a "not found" message when historical data is found', () => {
const wrapper = shallow(
<NoServicesMessage isLoading={false} historicalDataFound={true} />
);
expect(wrapper).toMatchSnapshot();
});

it('should show a "no services installed" message, a link to the set up instructions page, a message about upgrading APM server, and a link to the upgrade assistant when NO historical data is found', () => {
const wrapper = shallow(
<NoServicesMessage isLoading={false} historicalDataFound={false} />
);
expect(wrapper).toMatchSnapshot();
Object.values(FETCH_STATUS).forEach(status => {
Copy link
Member

Choose a reason for hiding this comment

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

are these useful tests? I guess they don't hurt either, but it feels like it's just generating code paths and then generating snapshots of all the different code paths. The component itself seems to be simple (and will likely stay that way) to warrant snapshot tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I felt it was an okay-ish way to test the different code paths since the generated snapshots were rather small (I agree that changes in big snapshots are often overlooked). What do you suggest instead?

[true, false].forEach(historicalDataFound => {
it(`status: ${status} and historicalDataFound: ${historicalDataFound}`, () => {
const wrapper = shallow(
<NoServicesMessage
status={status}
historicalDataFound={historicalDataFound}
/>
);
expect(wrapper).toMatchSnapshot();
});
});
});
});

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export function ServiceOverview() {
noItemsMessage={
<NoServicesMessage
historicalDataFound={data.hasHistoricalData}
isLoading={status === 'loading'}
status={status}
/>
}
/>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* 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 React from 'react';
import { i18n } from '@kbn/i18n';
import { EuiEmptyPrompt } from '@elastic/eui';

export function ErrorStatePrompt() {
return (
<EuiEmptyPrompt
title={
<div>
{i18n.translate('xpack.apm.error.prompt.title', {
defaultMessage: `Sorry, an error occured :(`
})}
</div>
}
body={i18n.translate('xpack.apm.error.prompt.body', {
defaultMessage: `Please inspect your browser's developer console for details.`
})}
titleSize="s"
/>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,8 @@ interface TimeRange {
rangeTo: string;
}

function useUiFilters({
kuery,
environment,
transactionType
}: IUrlParams): UIFilters {
return useMemo(() => ({ kuery, environment, transactionType }), [
kuery,
environment,
transactionType
]);
function useUiFilters({ kuery, environment }: IUrlParams): UIFilters {
return useMemo(() => ({ kuery, environment }), [kuery, environment]);
}

const defaultRefresh = (time: TimeRange) => {};
Expand Down
41 changes: 34 additions & 7 deletions x-pack/legacy/plugins/apm/public/hooks/useFetcher.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { useContext, useEffect, useState, useMemo } from 'react';
import React, { useContext, useEffect, useState, useMemo } from 'react';
import { toastNotifications } from 'ui/notify';
import { idx } from '@kbn/elastic-idx/target';
import { i18n } from '@kbn/i18n';
import { LoadingIndicatorContext } from '../context/LoadingIndicatorContext';
import { useComponentId } from './useComponentId';
import { KFetchError } from '../../../../../../src/legacy/ui/public/kfetch/kfetch_error';

export enum FETCH_STATUS {
LOADING = 'loading',
Expand All @@ -16,7 +20,7 @@ export enum FETCH_STATUS {

export function useFetcher<Response>(
fn: () => Promise<Response> | undefined,
effectKey: any[],
fnDeps: any[],
options: { preservePreviousResponse?: boolean } = {}
) {
const { preservePreviousResponse = true } = options;
Expand All @@ -40,11 +44,11 @@ export function useFetcher<Response>(

dispatchStatus({ id, isLoading: true });

setResult({
data: preservePreviousResponse ? result.data : undefined, // preserve data from previous state while loading next state
setResult(prevResult => ({
data: preservePreviousResponse ? prevResult.data : undefined, // preserve data from previous state while loading next state
status: FETCH_STATUS.LOADING,
error: undefined
});
}));

try {
const data = await promise;
Expand All @@ -57,7 +61,30 @@ export function useFetcher<Response>(
});
}
} catch (e) {
const err = e as KFetchError;
if (!didCancel) {
toastNotifications.addWarning({
title: i18n.translate('xpack.apm.fetcher.error.title', {
defaultMessage: `Error while fetching resource`
}),
text: (
<div>
<h5>
{i18n.translate('xpack.apm.fetcher.error.status', {
defaultMessage: `Error`
})}
</h5>
{idx(err.res, r => r.statusText)} ({idx(err.res, r => r.status)}
)
<h5>
{i18n.translate('xpack.apm.fetcher.error.url', {
defaultMessage: `URL`
})}
</h5>
{idx(err.res, r => r.url)}
</div>
)
});
dispatchStatus({ id, isLoading: false });
setResult({
data: undefined,
Expand All @@ -80,15 +107,15 @@ export function useFetcher<Response>(
id,
preservePreviousResponse,
dispatchStatus,
...effectKey
...fnDeps
/* eslint-enable react-hooks/exhaustive-deps */
]);

return useMemo(
() => ({
...result,
refresh: () => {
// this will invalidate the effectKey and will result in a new request
// this will invalidate the deps to `useEffect` and will result in a new request
setCounter(count => count + 1);
}
}),
Expand Down