Better HTTP error messages (jaegertracing#133)

* Better error messages on failed HTTP requests Signed-off-by: Joe Farro <[email protected]> * Prettier reformatting Signed-off-by: Joe Farro <[email protected]> * Better error formatting Signed-off-by: Joe Farro <[email protected]> * Update README to refer to codecov.io Signed-off-by: Joe Farro <[email protected]> * Unit tests for better HTTP error messages Signed-off-by: Joe Farro <[email protected]> * Better error messages on failed HTTP requests Signed-off-by: Joe Farro <[email protected]> * Better error formatting Signed-off-by: Joe Farro <[email protected]> * Update README to refer to codecov.io Signed-off-by: Joe Farro <[email protected]> * Unit tests for better HTTP error messages Signed-off-by: Joe Farro <[email protected]> * Revert to master Signed-off-by: Joe Farro <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>
vvvprabhakar · Dec 19, 2017 · 6107218 · 6107218
1 parent 4497623
commit 6107218
Show file tree

Hide file tree

Showing 13 changed files with 358 additions and 63 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 (
     <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>

diff --git 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 {
         </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 &&
@@ -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>
@@ -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
@@ -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);
   });
 
@@ -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
@@ -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<TracePageProps, Trace
       },
     };
     this._headerElm = null;
-    this._scrollManager = new ScrollManager(props.trace, { scrollBy, scrollTo });
+    this._scrollManager = new ScrollManager(props.trace, {
+      scrollBy,
+      scrollTo,
+    });
   }
 
   componentDidMount() {
@@ -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,
+      });
     }
   }
 
@@ -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;

diff --git 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('<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', () => {