Skip to content

Commit

Permalink
Better HTTP error messages (#133)
Browse files Browse the repository at this point in the history 
* Better error messages on failed HTTP requests

Signed-off-by: Joe Farro <joef@uber.com>

* Prettier reformatting

Signed-off-by: Joe Farro <joef@uber.com>

* Better error formatting

Signed-off-by: Joe Farro <joef@uber.com>

* Update README to refer to codecov.io

Signed-off-by: Joe Farro <joef@uber.com>

* Unit tests for better HTTP error messages

Signed-off-by: Joe Farro <joef@uber.com>

* Better error messages on failed HTTP requests

Signed-off-by: Joe Farro <joef@uber.com>

* Better error formatting

Signed-off-by: Joe Farro <joef@uber.com>

* Update README to refer to codecov.io

Signed-off-by: Joe Farro <joef@uber.com>

* Unit tests for better HTTP error messages

Signed-off-by: Joe Farro <joef@uber.com>

* Revert to master

Signed-off-by: Joe Farro <joef@uber.com>
  • Loading branch information
@tiffon
tiffon authored Dec 19, 2017
1 parent b5923ca commit b6c4d37
Show file tree
Hide file tree
Showing 13 changed files with 358 additions and 63 deletions.
4 changes: 2 additions & 2 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ See the [deployment guide](http://jaeger.readthedocs.io/en/latest/deployment/#ui
[doc]: http://jaeger.readthedocs.org/en/latest/
[ci-img]: https://travis-ci.org/jaegertracing/jaeger-ui.svg?branch=master
[ci]: https://travis-ci.org/jaegertracing/jaeger-ui
[cov-img]: https://coveralls.io/repos/jaegertracing/jaeger-ui/badge.svg?branch=master
[cov]: https://coveralls.io/github/jaegertracing/jaeger-ui?branch=master
[cov-img]: https://codecov.io/gh/jaegertracing/jaeger-ui/branch/master/graph/badge.svg
[cov]: https://codecov.io/gh/jaegertracing/jaeger-ui
[aio-docs]: http://jaeger.readthedocs.io/en/latest/getting_started/
[fossa-img]: https://app.fossa.io/api/projects/git%2Bgithub.com%2Fjaegertracing%2Fjaeger-ui.svg?type=shield
[fossa]: https://app.fossa.io/projects/git%2Bgithub.com%2Fjaegertracing%2Fjaeger-ui?ref=badge_shield
58 changes: 44 additions & 14 deletions src/api/jaeger.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,55 @@ import queryString from 'query-string';

import prefixUrl from '../utils/prefix-url';

// export for tests
export function getMessageFromError(errData, status) {
if (errData.code != null && errData.msg != null) {
if (errData.code === status) {
return errData.msg;
}
return `${errData.code} - ${errData.msg}`;
}
try {
return JSON.stringify(errData);
} catch (_) {
return String(errData);
}
}

function getJSON(url, query) {
return fetch(`${url}${query ? `?${queryString.stringify(query)}` : ''}`, {
credentials: 'include',
}).then(response => {
if (response.status >= 400) {
if (response.status === 404) {
throw new Error('Resource not found in the Jaeger Query Service.');
}

return response
.json()
.then(({ errors = [] }) => {
throw new Error(errors.length > 0 ? errors[0].msg : 'An unknown error occurred.');
})
.catch((/* err */) => {
throw new Error('Bad JSON returned from the Jaeger Query Service.');
});
if (response.status < 400) {
return response.json();
}
return response.json();
return response.text().then(bodyText => {
let data;
let bodyTextFmt;
let errorMessage;
try {
data = JSON.parse(bodyText);
bodyTextFmt = JSON.stringify(data, null, 2);
} catch (_) {
data = null;
bodyTextFmt = null;
}
if (data && Array.isArray(data.errors) && data.errors.length) {
errorMessage = data.errors.map(err => getMessageFromError(err, response.status)).join('; ');
} else {
errorMessage = bodyText || `${response.status} - ${response.statusText}`;
}
if (typeof errorMessage === 'string') {
errorMessage = errorMessage.trim();
}
const error = new Error(`HTTP Error: ${errorMessage}`);
error.httpStatus = response.status;
error.httpStatusText = response.statusText;
error.httpBody = bodyTextFmt || bodyText;
error.httpUrl = url;
error.httpQuery = typeof query === 'string' ? query : queryString.stringify(query);
throw error;
});
});
}

Expand Down
80 changes: 67 additions & 13 deletions src/api/jaeger.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import traceGenerator from '../demo/trace-generators';
import JaegerAPI from './jaeger';

const generatedTraces = traceGenerator.traces({ traces: 5 });
/* eslint-disable import/first */
jest.mock('isomorphic-fetch', () =>
jest.fn(() =>
Promise.resolve({
Expand All @@ -26,7 +23,12 @@ jest.mock('isomorphic-fetch', () =>
)
);

const fetchMock = require('isomorphic-fetch');
import fetchMock from 'isomorphic-fetch';

import traceGenerator from '../demo/trace-generators';
import JaegerAPI, { getMessageFromError } from './jaeger';

const generatedTraces = traceGenerator.traces({ traces: 5 });

it('fetchTrace() should fetch with the id', () => {
fetchMock.mockClear();
Expand All @@ -46,18 +48,70 @@ it('fetchTrace() should resolve the whole response', () => {
return JaegerAPI.fetchTrace('trace-id').then(resp => expect(resp.data).toBe(generatedTraces));
});

it('fetchTrace() should reject with a bad status code', () => {
it('fetchTrace() throws an error on a >= 400 status code', done => {
const status = 400;
const statusText = 'some-status';
const msg = 'some-message';
const errorData = { errors: [{ msg, code: status }] };

fetchMock.mockReturnValue(
Promise.resolve({
status: 400,
json: () => Promise.resolve({ data: null }),
status,
statusText,
text: () => Promise.resolve(JSON.stringify(errorData)),
})
);
JaegerAPI.fetchTrace('trace-id').catch(err => {
expect(err.message).toMatch(msg);
expect(err.httpStatus).toBe(status);
expect(err.httpStatusText).toBe(statusText);
done();
});
});

JaegerAPI.fetchTrace('trace-id').then(
() => new Error(),
err => {
expect(err instanceof Error).toBeTruthy();
}
it('fetchTrace() throws an useful error derived from a text payload', done => {
const status = 400;
const statusText = 'some-status';
const errorData = 'this is some error message';

fetchMock.mockReturnValue(
Promise.resolve({
status,
statusText,
text: () => Promise.resolve(errorData),
})
);
JaegerAPI.fetchTrace('trace-id').catch(err => {
expect(err.message).toMatch(errorData);
expect(err.httpStatus).toBe(status);
expect(err.httpStatusText).toBe(statusText);
done();
});
});

describe('getMessageFromError()', () => {
describe('{ code, msg } error data', () => {
const data = { code: 1, msg: 'some-message' };

it('ignores code if it is the same as `status` arg', () => {
expect(getMessageFromError(data, 1)).toBe(data.msg);
});

it('returns`$code - $msg` when code is novel', () => {
const rv = getMessageFromError(data, -1);
expect(rv).toBe(`${data.code} - ${data.msg}`);
});
});
describe('other data formats', () => {
it('stringifies the value, when possible', () => {
const data = ['abc'];
expect(getMessageFromError(data)).toBe(JSON.stringify(data));
});

it('returns the string, otherwise', () => {
const data = {};
data.data = data;
expect(getMessageFromError(data)).toBe(String(data));
});
});
});
12 changes: 4 additions & 8 deletions src/components/App/NotFound.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import React from 'react';
import { Link } from 'react-router-dom';

import ErrorMessage from '../common/ErrorMessage';
import prefixUrl from '../../utils/prefix-url';

type NotFoundProps = {
Expand All @@ -26,16 +27,11 @@ type NotFoundProps = {
export default function NotFound({ error }: NotFoundProps) {
return (
<section className="ui container">
<div className="ui center aligned basic segment">
<div className="ui basic segment">
<div className="ui center aligned basic segment">
<h1>{'404'}</h1>
<p>{"Looks like you tried to access something that doesn't exist."}</p>
<h1>Error</h1>
</div>
{error && (
<div className="ui red message">
<p>{String(error)}</p>
</div>
)}
{error && <ErrorMessage error={error} />}
<div className="ui center aligned basic segment">
<Link to={prefixUrl('/')}>{'Back home'}</Link>
</div>
Expand Down
39 changes: 25 additions & 14 deletions src/components/SearchTracePage/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import * as jaegerApiActions from '../../actions/jaeger-api';
import TraceSearchForm from './TraceSearchForm';
import TraceSearchResult from './TraceSearchResult';
import TraceResultsScatterPlot from './TraceResultsScatterPlot';
import ErrorMessage from '../common/ErrorMessage';
import * as orderBy from '../../model/order-by';
import { sortTraces, getTraceSummaries } from '../../model/search';
import { getPercentageOfDuration } from '../../utils/date';
Expand Down Expand Up @@ -78,7 +79,7 @@ export default class SearchTracePage extends Component {

render() {
const {
errorMessage,
errors,
isHomepage,
loadingServices,
loadingTraces,
Expand All @@ -104,11 +105,11 @@ export default class SearchTracePage extends Component {
</div>
<div className="twelve wide column padded">
{loadingTraces && <div className="ui active centered inline loader js-test-traces-loader" />}
{errorMessage &&
{errors &&
!loadingTraces && (
<div className="ui message red trace-search--error js-test-error-message">
There was an error querying for traces:<br />
{errorMessage}
<div className="ui message js-test-error-message">
<h2>There was an error querying for traces:</h2>
{errors.map(err => <ErrorMessage key={err.message} error={err} />)}
</div>
)}
{isHomepage &&
Expand All @@ -122,7 +123,7 @@ export default class SearchTracePage extends Component {
{!isHomepage &&
!hasTraceResults &&
!loadingTraces &&
!errorMessage && (
!errors && (
<div className="ui message trace-search--no-results js-test-no-results">
No trace results. Try another query.
</div>
Expand Down Expand Up @@ -205,7 +206,11 @@ SearchTracePage.propTypes = {
}),
fetchServiceOperations: PropTypes.func,
fetchServices: PropTypes.func,
errorMessage: PropTypes.string,
errors: PropTypes.arrayOf(
PropTypes.shape({
message: PropTypes.string,
})
),
};

const stateTraceXformer = getLastXformCacher(stateTrace => {
Expand Down Expand Up @@ -236,21 +241,27 @@ export function mapStateToProps(state) {
const isHomepage = !Object.keys(query).length;
const { traces, maxDuration, traceError, loadingTraces } = stateTraceXformer(state.trace);
const { loadingServices, services, serviceError } = stateServicesXformer(state.services);
const errorMessage = serviceError || traceError ? `${serviceError || ''} ${traceError || ''}` : '';
const errors = [];
if (traceError) {
errors.push(traceError);
}
if (serviceError) {
errors.push(serviceError);
}
const sortBy = traceResultsFiltersFormSelector(state, 'sortBy');
sortTraces(traces, sortBy);

return {
isHomepage,
sortTracesBy: sortBy,
traceResults: traces,
numberOfTraceResults: traces.length,
maxTraceDuration: maxDuration,
urlQueryParams: query,
services,
loadingTraces,
loadingServices,
errorMessage,
errors: errors.length ? errors : null,
maxTraceDuration: maxDuration,
numberOfTraceResults: traces.length,
sortTracesBy: sortBy,
traceResults: traces,
urlQueryParams: query,
};
}

Expand Down
4 changes: 2 additions & 2 deletions src/components/SearchTracePage/index.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ describe('<SearchTracePage>', () => {
});

it('shows an error message if there is an error message', () => {
wrapper.setProps({ errorMessage: 'big-error' });
wrapper.setProps({ errors: [{ message: 'big-error' }] });
expect(wrapper.find('.js-test-error-message').length).toBe(1);
});

Expand Down Expand Up @@ -160,7 +160,7 @@ describe('mapStateToProps()', () => {
],
loadingTraces: false,
loadingServices: false,
errorMessage: '',
errors: null,
});
});
});
22 changes: 18 additions & 4 deletions src/components/TracePage/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import SpanGraph from './SpanGraph';
import TracePageHeader from './TracePageHeader';
import TraceTimelineViewer from './TraceTimelineViewer';
import type { ViewRange, ViewRangeTimeUpdate } from './types';
import NotFound from '../App/NotFound';
import ErrorMessage from '../common/ErrorMessage';
import * as jaegerApiActions from '../../actions/jaeger-api';
import { getTraceName } from '../../model/trace-viewer';
import type { Trace } from '../../types';
Expand Down Expand Up @@ -102,7 +102,10 @@ export default class TracePage extends React.PureComponent<TracePageProps, Trace
},
};
this._headerElm = null;
this._scrollManager = new ScrollManager(props.trace, { scrollBy, scrollTo });
this._scrollManager = new ScrollManager(props.trace, {
scrollBy,
scrollTo,
});
}

componentDidMount() {
Expand Down Expand Up @@ -150,7 +153,10 @@ export default class TracePage extends React.PureComponent<TracePageProps, Trace
cancelScroll();
if (this._scrollManager) {
this._scrollManager.destroy();
this._scrollManager = new ScrollManager(undefined, { scrollBy, scrollTo });
this._scrollManager = new ScrollManager(undefined, {
scrollBy,
scrollTo,
});
}
}

Expand Down Expand Up @@ -231,7 +237,15 @@ export default class TracePage extends React.PureComponent<TracePageProps, Trace
}

if (trace instanceof Error) {
return <NotFound error={trace} />;
return (
<section className="ui container">
<div className="ui basic segment">
<div className="ui message">
<ErrorMessage error={trace} />
</div>
</div>
</section>
);
}

const { duration, processes, spans, startTime, traceID } = trace;
Expand Down
4 changes: 2 additions & 2 deletions src/components/TracePage/index.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { cancel as cancelScroll } from './scroll-page';
import SpanGraph from './SpanGraph';
import TracePageHeader from './TracePageHeader';
import TraceTimelineViewer from './TraceTimelineViewer';
import NotFound from '../App/NotFound';
import ErrorMessage from '../common/ErrorMessage';
import traceGenerator from '../../demo/trace-generators';
import transformTraceData from '../../model/transform-trace-data';

Expand Down Expand Up @@ -94,7 +94,7 @@ describe('<TracePage>', () => {

it('renders an error message when given an error', () => {
wrapper.setProps({ trace: new Error('some-error') });
expect(wrapper.find(NotFound).length).toBe(1);
expect(wrapper.find(ErrorMessage).length).toBe(1);
});

it('renders a loading indicator when loading', () => {
Expand Down
Loading

0 comments on commit b6c4d37

Please sign in to comment.