From 2a6d5e01b351bb909029479b7368fe95a4d4abb0 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Wed, 13 Mar 2019 09:30:19 +0100 Subject: [PATCH] [7.0] [ML] Adds missing error handling to annotations request. (#32384) (#32489) - 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. --- .../annotations_table/annotations_table.js | 7 ++- .../ml/public/explorer/explorer_utils.js | 58 +++++++++++-------- .../timeseries_chart/timeseries_chart.js | 26 +++++---- .../timeseries_chart_directive.js | 8 +-- .../timeseriesexplorer.html | 1 + .../timeseriesexplorer_controller.js | 31 ++++++---- .../annotation_service/annotation.test.ts | 33 +++++++++-- .../models/annotation_service/annotation.ts | 43 +++++++++----- 8 files changed, 133 insertions(+), 74 deletions(-) diff --git a/x-pack/plugins/ml/public/components/annotations/annotations_table/annotations_table.js b/x-pack/plugins/ml/public/components/annotations/annotations_table/annotations_table.js index 4d1cbba243a36..38390b1cd9399 100644 --- a/x-pack/plugins/ml/public/components/annotations/annotations_table/annotations_table.js +++ b/x-pack/plugins/ml/public/components/annotations/annotations_table/annotations_table.js @@ -130,14 +130,17 @@ const AnnotationsTable = injectI18n(class AnnotationsTable extends Component { } } - componentWillUpdate() { + previousJobId = undefined; + componentDidUpdate() { if ( + Array.isArray(this.props.jobs) && this.props.jobs.length > 0 && + this.previousJobId !== this.props.jobs[0].job_id && this.props.annotations === undefined && this.state.isLoading === false && - Array.isArray(this.props.jobs) && this.props.jobs.length > 0 && this.state.jobId !== this.props.jobs[0].job_id ) { annotationsRefresh$.next(); + this.previousJobId = this.props.jobs[0].job_id; } } diff --git a/x-pack/plugins/ml/public/explorer/explorer_utils.js b/x-pack/plugins/ml/public/explorer/explorer_utils.js index 6a116faa9962a..7cffdfea630ee 100644 --- a/x-pack/plugins/ml/public/explorer/explorer_utils.js +++ b/x-pack/plugins/ml/public/explorer/explorer_utils.js @@ -381,7 +381,7 @@ export function processViewByResults( return dataset; } -export async function loadAnnotationsTableData(selectedCells, selectedJobs, interval, bounds) { +export function loadAnnotationsTableData(selectedCells, selectedJobs, interval, bounds) { const jobIds = (selectedCells !== null && selectedCells.viewByFieldName === VIEW_BY_JOB_LABEL) ? selectedCells.lanes : selectedJobs.map(d => d.id); const timeRange = getSelectionTimeRange(selectedCells, interval, bounds); @@ -390,31 +390,41 @@ export async function loadAnnotationsTableData(selectedCells, selectedJobs, inte return Promise.resolve([]); } - const resp = await ml.annotations.getAnnotations({ - jobIds, - earliestMs: timeRange.earliestMs, - latestMs: timeRange.latestMs, - maxAnnotations: ANNOTATIONS_TABLE_DEFAULT_QUERY_SIZE - }); + return new Promise((resolve) => { + ml.annotations.getAnnotations({ + jobIds, + earliestMs: timeRange.earliestMs, + latestMs: timeRange.latestMs, + maxAnnotations: ANNOTATIONS_TABLE_DEFAULT_QUERY_SIZE + }).then((resp) => { + if (resp.error !== undefined || resp.annotations === undefined) { + return resolve([]); + } - const annotationsData = []; - jobIds.forEach((jobId) => { - const jobAnnotations = resp.annotations[jobId]; - if (jobAnnotations !== undefined) { - annotationsData.push(...jobAnnotations); - } - }); + const annotationsData = []; + jobIds.forEach((jobId) => { + const jobAnnotations = resp.annotations[jobId]; + if (jobAnnotations !== undefined) { + annotationsData.push(...jobAnnotations); + } + }); - return Promise.resolve( - annotationsData - .sort((a, b) => { - return a.timestamp - b.timestamp; - }) - .map((d, i) => { - d.key = String.fromCharCode(65 + i); - return d; - }) - ); + return resolve( + 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. + return resolve([]); + }); + }); } export async function loadAnomaliesTableData(selectedCells, selectedJobs, dateFormatTz, interval, bounds, fieldName) { 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 index e45528096ccc6..465d0397228f9 100644 --- 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, @@ -58,8 +56,6 @@ import { import { injectI18n } from '@kbn/i18n/react'; -const mlAnnotationsEnabled = chrome.getInjected('mlAnnotationsEnabled', false); - const focusZoomPanelHeight = 25; const focusChartHeight = 310; const focusHeight = focusZoomPanelHeight + focusChartHeight; @@ -93,6 +89,7 @@ function getSvgHeight() { const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Component { static propTypes = { + annotationsEnabled: PropTypes.bool, annotation: PropTypes.object, autoZoomDuration: PropTypes.number, contextAggregationInterval: PropTypes.object, @@ -126,6 +123,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo componentDidMount() { const { + annotationsEnabled, svgWidth } = this.props; @@ -158,7 +156,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo this.fieldFormat = undefined; // Annotations Brush - if (mlAnnotationsEnabled) { + if (annotationsEnabled) { this.annotateBrush = getAnnotationBrush.call(this); } @@ -205,7 +203,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo this.renderFocusChart(); - if (mlAnnotationsEnabled && this.props.annotation === null) { + if (this.props.annotationsEnabled && this.props.annotation === null) { const chartElement = d3.select(this.rootNode); chartElement.select('g.mlAnnotationBrush').call(this.annotateBrush.extent([0, 0])); } @@ -213,6 +211,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo renderChart() { const { + annotationsEnabled, contextChartData, contextForecastData, detectorIndex, @@ -314,7 +313,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo .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') @@ -390,6 +389,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo // as we want to re-render the paths and points when the zoom area changes. const { + annotationsEnabled, contextForecastData } = this.props; @@ -406,7 +406,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo this.createZoomInfoElements(zoomGroup, fcsWidth); // Create the elements for annotations - if (mlAnnotationsEnabled) { + if (annotationsEnabled) { const annotateBrush = this.annotateBrush.bind(this); fcsGroup.append('g') @@ -488,6 +488,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo renderFocusChart() { const { + annotationsEnabled, focusAggregationInterval, focusAnnotationData, focusChartData, @@ -589,7 +590,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo // 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 revisiting to be a more robust normalization @@ -625,7 +626,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo .classed('hidden', !showModelBounds); } - if (mlAnnotationsEnabled) { + if (annotationsEnabled) { renderAnnotations( focusChart, focusAnnotationData, @@ -638,7 +639,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo // 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') @@ -1229,6 +1230,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo showFocusChartTooltip(marker, circle) { const { + annotationsEnabled, modelPlotEnabled, intl } = this.props; @@ -1369,7 +1371,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo }); } - if (mlAnnotationsEnabled && _.has(marker, 'annotation')) { + if (annotationsEnabled && _.has(marker, 'annotation')) { contents = mlEscape(marker.annotation); contents += `
${moment(marker.timestamp).format('MMMM Do YYYY, HH:mm')}`; diff --git a/x-pack/plugins/ml/public/timeseriesexplorer/components/timeseries_chart/timeseries_chart_directive.js b/x-pack/plugins/ml/public/timeseriesexplorer/components/timeseries_chart/timeseries_chart_directive.js index f097a55a58477..e4bfe294f91e3 100644 --- a/x-pack/plugins/ml/public/timeseriesexplorer/components/timeseries_chart/timeseries_chart_directive.js +++ b/x-pack/plugins/ml/public/timeseriesexplorer/components/timeseries_chart/timeseries_chart_directive.js @@ -26,9 +26,6 @@ const module = uiModules.get('apps/ml'); import { I18nContext } from 'ui/i18n'; -import chrome from 'ui/chrome'; -const mlAnnotationsEnabled = chrome.getInjected('mlAnnotationsEnabled', false); - module.directive('mlTimeseriesChart', function ($timeout) { function link(scope, element) { @@ -44,6 +41,7 @@ module.directive('mlTimeseriesChart', function ($timeout) { svgWidth = Math.max(angular.element('.results-container').width(), 0); const props = { + annotationsEnabled: scope.annotationsEnabled, autoZoomDuration: scope.autoZoomDuration, contextAggregationInterval: scope.contextAggregationInterval, contextChartData: scope.contextChartData, @@ -91,7 +89,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); } @@ -116,6 +115,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 index d73d5ab07ba0d..c475a86247b11 100644 --- a/x-pack/plugins/ml/public/timeseriesexplorer/timeseriesexplorer.html +++ b/x-pack/plugins/ml/public/timeseriesexplorer/timeseriesexplorer.html @@ -161,6 +161,7 @@
{ - 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 index b8ea8e49964c2..97a9c59aab830 100644 --- 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; @@ -55,8 +57,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, @@ -73,13 +73,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.Mock = 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, @@ -107,7 +131,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', @@ -139,8 +162,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, diff --git a/x-pack/plugins/ml/server/models/annotation_service/annotation.ts b/x-pack/plugins/ml/server/models/annotation_service/annotation.ts index aaea6d08c80a5..addcdcb376b93 100644 --- a/x-pack/plugins/ml/server/models/annotation_service/annotation.ts +++ b/x-pack/plugins/ml/server/models/annotation_service/annotation.ts @@ -70,6 +70,7 @@ export type callWithRequestType = ( export function annotationProvider(callWithRequest: callWithRequestType) { async function indexAnnotation(annotation: Annotation, username: string) { if (isAnnotation(annotation) === false) { + // No need to translate, this will not be exposed in the UI. return Promise.reject(new Error('invalid annotation format')); } @@ -211,27 +212,37 @@ export function annotationProvider(callWithRequest: callWithRequestType) { }, }; - const resp = await callWithRequest('search', params); + try { + const resp = await callWithRequest('search', params); - const docs: Annotations = _.get(resp, ['hits', 'hits'], []).map((d: EsResult) => { - // get the original source document and the document id, we need it - // to identify the annotation when editing/deleting it. - return { ...d._source, _id: d._id } as Annotation; - }); + if (resp.error !== undefined && resp.message !== undefined) { + // No need to translate, this will not be exposed in the UI. + throw new Error(`Annotations couldn't be retrieved from Elasticsearch.`); + } - if (isAnnotations(docs) === false) { - throw Boom.badRequest(`Annotations didn't pass integrity check.`); - } + const docs: Annotations = _.get(resp, ['hits', 'hits'], []).map((d: EsResult) => { + // get the original source document and the document id, we need it + // to identify the annotation when editing/deleting it. + return { ...d._source, _id: d._id } as Annotation; + }); - docs.forEach((doc: Annotation) => { - const jobId = doc.job_id; - if (typeof obj.annotations[jobId] === 'undefined') { - obj.annotations[jobId] = []; + if (isAnnotations(docs) === false) { + // No need to translate, this will not be exposed in the UI. + throw new Error(`Annotations didn't pass integrity check.`); } - obj.annotations[jobId].push(doc); - }); - return obj; + docs.forEach((doc: Annotation) => { + const jobId = doc.job_id; + if (typeof obj.annotations[jobId] === 'undefined') { + obj.annotations[jobId] = []; + } + obj.annotations[jobId].push(doc); + }); + + return obj; + } catch (error) { + throw Boom.badRequest(error); + } } async function deleteAnnotation(id: string) {