[ML] Adds missing error handling to annotations request. (elastic#32384)

- This adds more thorough error handling to retrieving annotations from the server to avoid breaking Anomaly Explorer when there's a problem retrieving annotations. The UI will silently fail and hide the annotations table if for whatever reasons annotations cannot be retrieved for the selected jobs (this could be permissions or setup related if e.g. the annotations feature index/aliases are not present). This is now the same behavior like in Single Metric Viewer. The browser console will still output an error message. This fixes an error scenario where Anomaly Explorer would fail to load if a user upgraded to 6.6 because the backend task wasn't run yet to create the annotation index/aliases. - Disables annotations feature in Single Metric Viewer if annotations fail to load. - Use componentDidUpdate and an update check instead of deprecated componentWillUpdate for updating the annotations table in jobs management.
walterra · Mar 5, 2019 · 11a8da1 · 11a8da1
1 parent a226c86
commit 11a8da1
Show file tree

Hide file tree

Showing 7 changed files with 117 additions and 66 deletions.
diff --git a/x-pack/plugins/ml/public/explorer/explorer_controller.js b/x-pack/plugins/ml/public/explorer/explorer_controller.js
@@ -953,7 +953,7 @@ module.controller('MlExplorerController', function (
     }
   }
 
-  async function loadAnnotationsTableData() {
+  function loadAnnotationsTableData() {
     $scope.annotationsData = [];
 
     const cellData = $scope.cellData;
@@ -962,30 +962,35 @@ module.controller('MlExplorerController', function (
     const timeRange = getSelectionTimeRange(cellData);
 
     if (mlAnnotationsEnabled) {
-      const resp = await ml.annotations.getAnnotations({
+      ml.annotations.getAnnotations({
         jobIds,
         earliestMs: timeRange.earliestMs,
         latestMs: timeRange.latestMs,
         maxAnnotations: ANNOTATIONS_TABLE_DEFAULT_QUERY_SIZE
-      });
+      }).then((resp) => {
+        $scope.$evalAsync(() => {
+          const annotationsData = [];
+          jobIds.forEach((jobId) => {
+            const jobAnnotations = resp.annotations[jobId];
+            if (jobAnnotations !== undefined) {
+              annotationsData.push(...jobAnnotations);
+            }
+          });
 
-      $scope.$evalAsync(() => {
-        const annotationsData = [];
-        jobIds.forEach((jobId) => {
-          const jobAnnotations = resp.annotations[jobId];
-          if (jobAnnotations !== undefined) {
-            annotationsData.push(...jobAnnotations);
-          }
+          $scope.annotationsData = annotationsData
+            .sort((a, b) => {
+              return a.timestamp - b.timestamp;
+            })
+            .map((d, i) => {
+              d.key = String.fromCharCode(65 + i);
+              return d;
+            });
         });
-
-        $scope.annotationsData = annotationsData
-          .sort((a, b) => {
-            return a.timestamp - b.timestamp;
-          })
-          .map((d, i) => {
-            d.key = String.fromCharCode(65 + i);
-            return d;
-          });
+      }).catch((resp) => {
+        console.log('Error loading list of annotations for jobs list:', resp);
+        // Silently fail and just return an empty array for annotations to not break the UI.
+        $scope.annotationsData = [];
+        $scope.$applyAsync();
       });
     }
   }

diff --git a/x-pack/plugins/ml/public/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js b/x-pack/plugins/ml/public/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js
@@ -19,8 +19,6 @@ import _ from 'lodash';
 import d3 from 'd3';
 import moment from 'moment';
 
-import chrome from 'ui/chrome';
-
 import {
   getSeverityWithLow,
   getMultiBucketImpactLabel,
@@ -56,8 +54,6 @@ import {
   unhighlightFocusChartAnnotation
 } from './timeseries_chart_annotations';
 
-const mlAnnotationsEnabled = chrome.getInjected('mlAnnotationsEnabled', false);
-
 const focusZoomPanelHeight = 25;
 const focusChartHeight = 310;
 const focusHeight = focusZoomPanelHeight + focusChartHeight;
@@ -91,6 +87,7 @@ function getSvgHeight() {
 
 export class TimeseriesChart extends React.Component {
   static propTypes = {
+    annotationsEnabled: PropTypes.bool,
     indexAnnotation: PropTypes.func,
     autoZoomDuration: PropTypes.number,
     contextAggregationInterval: PropTypes.object,
@@ -215,6 +212,7 @@ export class TimeseriesChart extends React.Component {
 
   componentDidMount() {
     const {
+      annotationsEnabled,
       svgWidth
     } = this.props;
 
@@ -247,7 +245,7 @@ export class TimeseriesChart extends React.Component {
     this.fieldFormat = undefined;
 
     // Annotations Brush
-    if (mlAnnotationsEnabled) {
+    if (annotationsEnabled) {
       this.annotateBrush = getAnnotationBrush.call(this);
     }
 
@@ -297,6 +295,7 @@ export class TimeseriesChart extends React.Component {
 
   renderChart() {
     const {
+      annotationsEnabled,
       contextChartData,
       contextForecastData,
       detectorIndex,
@@ -398,7 +397,7 @@ export class TimeseriesChart extends React.Component {
       .attr('transform', 'translate(' + margin.left + ',' + (focusHeight + margin.top + chartSpacing) + ')');
 
     // Mask to hide annotations overflow
-    if (mlAnnotationsEnabled) {
+    if (annotationsEnabled) {
       const annotationsMask = svg
         .append('defs')
         .append('mask')
@@ -474,6 +473,7 @@ export class TimeseriesChart extends React.Component {
     // as we want to re-render the paths and points when the zoom area changes.
 
     const {
+      annotationsEnabled,
       contextForecastData
     } = this.props;
 
@@ -490,7 +490,7 @@ export class TimeseriesChart extends React.Component {
     this.createZoomInfoElements(zoomGroup, fcsWidth);
 
     // Create the elements for annotations
-    if (mlAnnotationsEnabled) {
+    if (annotationsEnabled) {
       const annotateBrush = this.annotateBrush.bind(this);
 
       fcsGroup.append('g')
@@ -572,6 +572,7 @@ export class TimeseriesChart extends React.Component {
 
   renderFocusChart() {
     const {
+      annotationsEnabled,
       focusAggregationInterval,
       focusAnnotationData,
       focusChartData,
@@ -659,7 +660,7 @@ export class TimeseriesChart extends React.Component {
 
       // if annotations are present, we extend yMax to avoid overlap
       // between annotation labels, chart lines and anomalies.
-      if (mlAnnotationsEnabled && focusAnnotationData && focusAnnotationData.length > 0) {
+      if (annotationsEnabled && focusAnnotationData && focusAnnotationData.length > 0) {
         const levels = getAnnotationLevels(focusAnnotationData);
         const maxLevel = d3.max(Object.keys(levels).map(key => levels[key]));
         // TODO needs revisting to be a more robust normalization
@@ -695,7 +696,7 @@ export class TimeseriesChart extends React.Component {
         .classed('hidden', !showModelBounds);
     }
 
-    if (mlAnnotationsEnabled) {
+    if (annotationsEnabled) {
       renderAnnotations(
         focusChart,
         focusAnnotationData,
@@ -709,7 +710,7 @@ export class TimeseriesChart extends React.Component {
 
       // disable brushing (creation of annotations) when annotations aren't shown
       focusChart.select('.mlAnnotationBrush')
-        .style('pointer-events', (showAnnotations) ? 'all' : 'none');
+        .style('display', (showAnnotations) ? null : 'none');
     }
 
     focusChart.select('.values-line')
@@ -1285,6 +1286,7 @@ export class TimeseriesChart extends React.Component {
 
   showFocusChartTooltip(marker, circle) {
     const {
+      annotationsEnabled,
       modelPlotEnabled
     } = this.props;
 
@@ -1350,7 +1352,7 @@ export class TimeseriesChart extends React.Component {
       contents += `<br/><hr/>Scheduled events:<br/>${marker.scheduledEvents.map(mlEscape).join('<br/>')}`;
     }
 
-    if (mlAnnotationsEnabled && _.has(marker, 'annotation')) {
+    if (annotationsEnabled && _.has(marker, 'annotation')) {
       contents = mlEscape(marker.annotation);
       contents += `<br />${moment(marker.timestamp).format('MMMM Do YYYY, HH:mm')}`;
 

diff --git a/...ns/ml/public/timeseriesexplorer/components/timeseries_chart/timeseries_chart_directive.js b/...ns/ml/public/timeseriesexplorer/components/timeseries_chart/timeseries_chart_directive.js
@@ -27,9 +27,6 @@ const module = uiModules.get('apps/ml');
 
 import { ml } from 'plugins/ml/services/ml_api_service';
 
-import chrome from 'ui/chrome';
-const mlAnnotationsEnabled = chrome.getInjected('mlAnnotationsEnabled', false);
-
 module.directive('mlTimeseriesChart', function ($timeout) {
 
   function link(scope, element) {
@@ -45,6 +42,7 @@ module.directive('mlTimeseriesChart', function ($timeout) {
       svgWidth = Math.max(angular.element('.results-container').width(), 0);
 
       const props = {
+        annotationsEnabled: scope.annotationsEnabled,
         indexAnnotation: ml.annotations.indexAnnotation,
         autoZoomDuration: scope.autoZoomDuration,
         contextAggregationInterval: scope.contextAggregationInterval,
@@ -93,7 +91,8 @@ module.directive('mlTimeseriesChart', function ($timeout) {
     scope.$watchCollection('focusForecastData', renderFocusChart);
     scope.$watchCollection('focusChartData', renderFocusChart);
     scope.$watchGroup(['showModelBounds', 'showForecast'], renderFocusChart);
-    if (mlAnnotationsEnabled) {
+    scope.$watch('annotationsEnabled', renderReactComponent);
+    if (scope.annotationsEnabled) {
       scope.$watchCollection('focusAnnotationData', renderFocusChart);
       scope.$watch('showAnnotations', renderFocusChart);
     }
@@ -118,6 +117,7 @@ module.directive('mlTimeseriesChart', function ($timeout) {
 
   return {
     scope: {
+      annotationsEnabled: '=',
       selectedJob: '=',
       detectorIndex: '=',
       modelPlotEnabled: '=',

diff --git a/x-pack/plugins/ml/public/timeseriesexplorer/timeseriesexplorer.html b/x-pack/plugins/ml/public/timeseriesexplorer/timeseriesexplorer.html
@@ -108,6 +108,7 @@
       <div class="ml-timeseries-chart">
 
         <ml-timeseries-chart style="width: 1200px; height: 400px;"
+          annotations-enabled="showAnnotationsCheckbox"
           selected-job="selectedJob"
           detector-index="detectorId"
           model-plot-enabled="modelPlotEnabled"

diff --git a/x-pack/plugins/ml/public/timeseriesexplorer/timeseriesexplorer_controller.js b/x-pack/plugins/ml/public/timeseriesexplorer/timeseriesexplorer_controller.js
@@ -56,7 +56,7 @@ import {
 
 
 import chrome from 'ui/chrome';
-const mlAnnotationsEnabled = chrome.getInjected('mlAnnotationsEnabled', false);
+let mlAnnotationsEnabled = chrome.getInjected('mlAnnotationsEnabled', false);
 
 uiRoutes
   .when('/timeseriesexplorer/?', {
@@ -392,6 +392,12 @@ module.controller('MlTimeSeriesExplorerController', function (
           console.log('Time series explorer focus chart data set:', $scope.focusChartData);
 
           $scope.loading = false;
+
+          // If the annotations failed to load and the feature flag is set to `false`,
+          // make sure the checkbox toggle gets hidden.
+          if (mlAnnotationsEnabled === false) {
+            $scope.showAnnotationsCheckbox = false;
+          }
         });
       }
     }
@@ -466,19 +472,24 @@ module.controller('MlTimeSeriesExplorerController', function (
         latestMs: searchBounds.max.valueOf(),
         maxAnnotations: ANNOTATIONS_TABLE_DEFAULT_QUERY_SIZE
       }).then((resp) => {
-        refreshFocusData.focusAnnotationData = resp.annotations[$scope.selectedJob.job_id]
-          .sort((a, b) => {
-            return a.timestamp - b.timestamp;
-          })
-          .map((d, i) => {
-            d.key = String.fromCharCode(65 + i);
-            return d;
-          });
+        refreshFocusData.focusAnnotationData = [];
+
+        if (Array.isArray(resp.annotations[$scope.selectedJob.job_id])) {
+          refreshFocusData.focusAnnotationData = resp.annotations[$scope.selectedJob.job_id]
+            .sort((a, b) => {
+              return a.timestamp - b.timestamp;
+            })
+            .map((d, i) => {
+              d.key = String.fromCharCode(65 + i);
+              return d;
+            });
+        }
 
         finish();
       }).catch(() => {
-        // silent fail
+        // silently fail and disable annotations feature if loading annotations fails.
         refreshFocusData.focusAnnotationData = [];
+        mlAnnotationsEnabled = false;
         finish();
       });
     } else {

diff --git a/x-pack/plugins/ml/server/models/annotation_service/annotation.test.ts b/x-pack/plugins/ml/server/models/annotation_service/annotation.test.ts
@@ -16,6 +16,8 @@ import { annotationServiceProvider } from './index';
 
 const acknowledgedResponseMock = { acknowledged: true };
 
+const jobIdMock = 'jobIdMock';
+
 describe('annotation_service', () => {
   let callWithRequestSpy: jest.Mock;
 
@@ -56,8 +58,6 @@ describe('annotation_service', () => {
     it('should get annotations for specific job', async done => {
       const { getAnnotations } = annotationServiceProvider(callWithRequestSpy);
 
-      const jobIdMock = 'jobIdMock';
-
       const indexAnnotationArgsMock: IndexAnnotationArgs = {
         jobIds: [jobIdMock],
         earliestMs: 1454804100000,
@@ -74,13 +74,37 @@ describe('annotation_service', () => {
       expect(isAnnotations(response.annotations[jobIdMock])).toBeTruthy();
       done();
     });
+
+    it('should throw and catch an error', async () => {
+      const mockEsError = {
+        statusCode: 404,
+        error: 'Not Found',
+        message: 'mock error message',
+      };
+
+      const callWithRequestSpyError = jest.fn(() => {
+        return Promise.resolve(mockEsError);
+      });
+
+      const { getAnnotations } = annotationServiceProvider(callWithRequestSpyError);
+
+      const indexAnnotationArgsMock: IndexAnnotationArgs = {
+        jobIds: [jobIdMock],
+        earliestMs: 1454804100000,
+        latestMs: 1455233399999,
+        maxAnnotations: 500,
+      };
+
+      await expect(getAnnotations(indexAnnotationArgsMock)).rejects.toEqual(
+        Error(`Annotations couldn't be retrieved from Elasticsearch.`)
+      );
+    });
   });
 
   describe('indexAnnotation()', () => {
     it('should index annotation', async done => {
       const { indexAnnotation } = annotationServiceProvider(callWithRequestSpy);
 
-      const jobIdMock = 'jobIdMock';
       const annotationMock: Annotation = {
         annotation: 'Annotation text',
         job_id: jobIdMock,
@@ -108,7 +132,6 @@ describe('annotation_service', () => {
     it('should remove ._id and .key before updating annotation', async done => {
       const { indexAnnotation } = annotationServiceProvider(callWithRequestSpy);
 
-      const jobIdMock = 'jobIdMock';
       const annotationMock: Annotation = {
         _id: 'mockId',
         annotation: 'Updated annotation text',
@@ -140,8 +163,6 @@ describe('annotation_service', () => {
     it('should update annotation text and the username for modified_username', async done => {
       const { getAnnotations, indexAnnotation } = annotationServiceProvider(callWithRequestSpy);
 
-      const jobIdMock = 'jobIdMock';
-
       const indexAnnotationArgsMock: IndexAnnotationArgs = {
         jobIds: [jobIdMock],
         earliestMs: 1454804100000,