From 61072187cc49988c8ff277c651d99819a911bff2 Mon Sep 17 00:00:00 2001 From: Joe Date: Tue, 19 Dec 2017 11:29:30 -0500 Subject: [PATCH] Better HTTP error messages (#133) * Better error messages on failed HTTP requests Signed-off-by: Joe Farro * Prettier reformatting Signed-off-by: Joe Farro * Better error formatting Signed-off-by: Joe Farro * Update README to refer to codecov.io Signed-off-by: Joe Farro * Unit tests for better HTTP error messages Signed-off-by: Joe Farro * Better error messages on failed HTTP requests Signed-off-by: Joe Farro * Better error formatting Signed-off-by: Joe Farro * Update README to refer to codecov.io Signed-off-by: Joe Farro * Unit tests for better HTTP error messages Signed-off-by: Joe Farro * Revert to master Signed-off-by: Joe Farro Signed-off-by: vvvprabhakar --- README.md | 4 +- src/api/jaeger.js | 58 ++++++++++---- src/api/jaeger.test.js | 80 ++++++++++++++++---- src/components/App/NotFound.js | 12 +-- src/components/SearchTracePage/index.js | 39 ++++++---- src/components/SearchTracePage/index.test.js | 4 +- src/components/TracePage/index.js | 22 +++++- src/components/TracePage/index.test.js | 4 +- src/components/common/ErrorMessage.css | 42 ++++++++++ src/components/common/ErrorMessage.js | 72 ++++++++++++++++++ src/components/common/ErrorMessage.test.js | 70 +++++++++++++++++ src/reducers/services.js | 12 ++- src/reducers/trace.js | 2 +- 13 files changed, 358 insertions(+), 63 deletions(-) create mode 100644 src/components/common/ErrorMessage.css create mode 100644 src/components/common/ErrorMessage.js create mode 100644 src/components/common/ErrorMessage.test.js diff --git a/README.md b/README.md index 098e662c61..64fbe822d8 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/src/api/jaeger.js b/src/api/jaeger.js index 94b1d2b1c0..abff80afac 100644 --- a/src/api/jaeger.js +++ b/src/api/jaeger.js @@ -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; + }); }); } diff --git a/src/api/jaeger.test.js b/src/api/jaeger.test.js index 1fd7a091bd..037774d6a6 100644 --- a/src/api/jaeger.test.js +++ b/src/api/jaeger.test.js @@ -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({ @@ -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(); @@ -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)); + }); + }); }); diff --git a/src/components/App/NotFound.js b/src/components/App/NotFound.js index 94f205ee12..342340b6d6 100644 --- a/src/components/App/NotFound.js +++ b/src/components/App/NotFound.js @@ -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 = { @@ -26,16 +27,11 @@ type NotFoundProps = { export default function NotFound({ error }: NotFoundProps) { return (
-
+
-

{'404'}

-

{"Looks like you tried to access something that doesn't exist."}

+

Error

- {error && ( -
-

{String(error)}

-
- )} + {error && }
{'Back home'}
diff --git a/src/components/SearchTracePage/index.js b/src/components/SearchTracePage/index.js index e46f4793f3..285601203f 100644 --- a/src/components/SearchTracePage/index.js +++ b/src/components/SearchTracePage/index.js @@ -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'; @@ -78,7 +79,7 @@ export default class SearchTracePage extends Component { render() { const { - errorMessage, + errors, isHomepage, loadingServices, loadingTraces, @@ -104,11 +105,11 @@ export default class SearchTracePage extends Component {
{loadingTraces &&
} - {errorMessage && + {errors && !loadingTraces && ( -
- There was an error querying for traces:
- {errorMessage} +
+

There was an error querying for traces:

+ {errors.map(err => )}
)} {isHomepage && @@ -122,7 +123,7 @@ export default class SearchTracePage extends Component { {!isHomepage && !hasTraceResults && !loadingTraces && - !errorMessage && ( + !errors && (
No trace results. Try another query.
@@ -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 => { @@ -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, }; } diff --git a/src/components/SearchTracePage/index.test.js b/src/components/SearchTracePage/index.test.js index dd46bcfbb3..8741469dbe 100644 --- a/src/components/SearchTracePage/index.test.js +++ b/src/components/SearchTracePage/index.test.js @@ -97,7 +97,7 @@ describe('', () => { }); 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); }); @@ -160,7 +160,7 @@ describe('mapStateToProps()', () => { ], loadingTraces: false, loadingServices: false, - errorMessage: '', + errors: null, }); }); }); diff --git a/src/components/TracePage/index.js b/src/components/TracePage/index.js index a1657f8c49..1c3de20f82 100644 --- a/src/components/TracePage/index.js +++ b/src/components/TracePage/index.js @@ -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'; @@ -102,7 +102,10 @@ export default class TracePage extends React.PureComponent; + return ( +
+
+
+ +
+
+
+ ); } const { duration, processes, spans, startTime, traceID } = trace; diff --git a/src/components/TracePage/index.test.js b/src/components/TracePage/index.test.js index 5236756949..4152344bbb 100644 --- a/src/components/TracePage/index.test.js +++ b/src/components/TracePage/index.test.js @@ -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'; @@ -94,7 +94,7 @@ describe('', () => { 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', () => { diff --git a/src/components/common/ErrorMessage.css b/src/components/common/ErrorMessage.css new file mode 100644 index 0000000000..931626c1c8 --- /dev/null +++ b/src/components/common/ErrorMessage.css @@ -0,0 +1,42 @@ +/* +Copyright (c) 2017 Uber Technologies, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +.ErrorMessage { + margin-top: 2.5em; + position: relative; + word-wrap: break-word; + word-break: break-word; +} + +.ErrorMessage--msg { + color: #c00; +} + +.ErrorMessage--details { + overflow: auto; +} + +.ErrorMessage--attr { + padding-bottom: 0.5em; + padding-right: 1em; + vertical-align: top; + white-space: nowrap; +} + +.ErrorMessage--value { + font-family: monospace; + white-space: pre; +} diff --git a/src/components/common/ErrorMessage.js b/src/components/common/ErrorMessage.js new file mode 100644 index 0000000000..31ed19ee43 --- /dev/null +++ b/src/components/common/ErrorMessage.js @@ -0,0 +1,72 @@ +// @flow + +// Copyright (c) 2017 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import React from 'react'; + +import './ErrorMessage.css'; + +type ErrorMessageProps = { + error: + | string + | { + message: string, + httpStatus?: any, + httpStatusText?: string, + httpUrl?: string, + httpQuery?: string, + httpBody?: string, + }, +}; + +function ErrorAttr({ name, value }: { name: string, value: any }) { + return ( + + {name} + {value} + + ); +} + +export default function ErrorMessage({ error }: ErrorMessageProps) { + if (!error) { + return null; + } + if (typeof error === 'string') { + return ( +
+

{error}

+
+ ); + } + const { message, httpStatus, httpStatusText, httpUrl, httpQuery, httpBody } = error; + const bodyExcerpt = httpBody && httpBody.length > 1024 ? `${httpBody.slice(0, 1021).trim()}...` : httpBody; + return ( +
+

{message}

+
+ + + {httpStatus && } + {httpStatusText && } + {httpUrl && } + {httpQuery && } + {bodyExcerpt && } + +
+
+
+ ); +} diff --git a/src/components/common/ErrorMessage.test.js b/src/components/common/ErrorMessage.test.js new file mode 100644 index 0000000000..0b96e1da81 --- /dev/null +++ b/src/components/common/ErrorMessage.test.js @@ -0,0 +1,70 @@ +// Copyright (c) 2017 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import React from 'react'; +import { shallow, mount } from 'enzyme'; + +import ErrorMessage from './ErrorMessage'; + +describe('', () => { + let wrapper; + let error; + + beforeEach(() => { + error = 'some-error'; + wrapper = shallow(); + }); + + it('is ok when not passed an error', () => { + wrapper.setProps({ error: null }); + expect(wrapper).toBeDefined(); + }); + + it('renders a message when passed a string', () => { + expect(wrapper.text()).toMatch(error); + }); + + describe('rendering more complex errors', () => { + it('renders the error message', () => { + error = new Error('another-error'); + wrapper.setProps({ error }); + expect(wrapper.text()).toMatch(error.message); + }); + + it('renders HTTP related data from the error', () => { + error = { + message: 'some-http-ish-message', + httpStatus: 'value-httpStatus', + httpStatusText: 'value-httpStatusText', + httpUrl: 'value-httpUrl', + httpQuery: 'value-httpQuery', + httpBody: 'value-httpBody', + }; + wrapper.setProps({ error }); + Object.keys(error).forEach(key => { + if (key === 'message') { + return; + } + const errorAttr = wrapper.find(`ErrorAttr[value="${error[key]}"]`); + expect(errorAttr.length).toBe(1); + }); + }); + }); + + it('is fine when mounted', () => { + error = { message: 'le-error', httpStatus: 'some-status' }; + wrapper = mount(); + expect(wrapper).toBeDefined(); + }); +}); diff --git a/src/reducers/services.js b/src/reducers/services.js index a1e60d150c..5c3a9efd3b 100644 --- a/src/reducers/services.js +++ b/src/reducers/services.js @@ -36,11 +36,14 @@ function fetchServicesDone(state, { payload }) { } function fetchServicesErred(state, { payload: error }) { - return { ...state, error: error.message, loading: false, services: [] }; + return { ...state, error, loading: false, services: [] }; } function fetchOpsStarted(state, { meta: { serviceName } }) { - const operationsForService = { ...state.operationsForService, [serviceName]: [] }; + const operationsForService = { + ...state.operationsForService, + [serviceName]: [], + }; return { ...state, operationsForService }; } @@ -49,7 +52,10 @@ function fetchOpsDone(state, { meta, payload }) { if (Array.isArray(operations)) { operations.sort(baseStringComparator); } - const operationsForService = { ...state.operationsForService, [meta.serviceName]: operations || [] }; + const operationsForService = { + ...state.operationsForService, + [meta.serviceName]: operations || [], + }; return { ...state, operationsForService }; } diff --git a/src/reducers/trace.js b/src/reducers/trace.js index f6ae7515f4..a918797c28 100644 --- a/src/reducers/trace.js +++ b/src/reducers/trace.js @@ -51,7 +51,7 @@ function searchDone(state, { payload }) { } function searchErred(state, action) { - const error = action.payload.message; + const error = action.payload; return { ...state, error, loading: false, traces: [] }; }