From 2559d905518bb8db9b90e9d50932e9dad8c5336b Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Wed, 19 Aug 2020 12:07:15 +0200 Subject: [PATCH] [ML] Anomaly Explorer / Single Metric Viewer: Fix error reporting for annotations. (#74953) Fixes error reporting when annotations fail to load for Anomaly Explorer and Single Metric Viewer. Previously, Anomaly Explorer ended up with a completely empty page when annotations failed to load. Single Metric Viewer would not fail to load, but it would make no difference for the user if existing annotations failed to load of if there were simply no existing annotations. Only in dev console an error message would be visible. Now a callout is shown when annotations fail to load. --- x-pack/plugins/ml/common/types/annotations.ts | 10 +- .../public/application/explorer/explorer.js | 32 ++++- .../application/explorer/explorer_utils.d.ts | 3 +- .../application/explorer/explorer_utils.js | 17 ++- .../reducers/explorer_reducer/state.ts | 8 +- .../timeseries_chart/timeseries_chart.js | 6 +- .../timeseriesexplorer/timeseriesexplorer.js | 33 ++++++ .../get_focus_data.ts | 42 ++++--- .../apps/ml/anomaly_detection/annotations.ts | 109 ++++++++++++++++++ .../ml/anomaly_detection/anomaly_explorer.ts | 4 + .../apps/ml/anomaly_detection/index.ts | 1 + .../anomaly_detection/single_metric_viewer.ts | 4 + .../services/ml/anomaly_explorer.ts | 10 ++ .../test/functional/services/ml/navigation.ts | 18 +++ .../services/ml/single_metric_viewer.ts | 6 + .../functional/services/ml/test_resources.ts | 86 ++++++++++++++ 16 files changed, 357 insertions(+), 32 deletions(-) create mode 100644 x-pack/test/functional/apps/ml/anomaly_detection/annotations.ts diff --git a/x-pack/plugins/ml/common/types/annotations.ts b/x-pack/plugins/ml/common/types/annotations.ts index 159a598f16bf55..eba40c2502f574 100644 --- a/x-pack/plugins/ml/common/types/annotations.ts +++ b/x-pack/plugins/ml/common/types/annotations.ts @@ -103,8 +103,7 @@ export function isAnnotation(arg: any): arg is Annotation { ); } -// eslint-disable-next-line @typescript-eslint/no-empty-interface -export interface Annotations extends Array {} +export type Annotations = Annotation[]; export function isAnnotations(arg: any): arg is Annotations { if (Array.isArray(arg) === false) { @@ -134,5 +133,12 @@ export type EsAggregationResult = Record; export interface GetAnnotationsResponse { aggregations?: EsAggregationResult; annotations: Record; + error?: string; success: boolean; } + +export interface AnnotationsTable { + annotationsData: Annotations; + aggregations: EsAggregationResult; + error?: string; +} diff --git a/x-pack/plugins/ml/public/application/explorer/explorer.js b/x-pack/plugins/ml/public/application/explorer/explorer.js index 4e27c176315060..06cec14578f2a7 100644 --- a/x-pack/plugins/ml/public/application/explorer/explorer.js +++ b/x-pack/plugins/ml/public/application/explorer/explorer.js @@ -15,6 +15,7 @@ import { FormattedMessage } from '@kbn/i18n/react'; import { htmlIdGenerator, + EuiCallOut, EuiFlexGroup, EuiFlexItem, EuiFormRow, @@ -221,7 +222,7 @@ export class Explorer extends React.Component { selectedJobs, tableData, } = this.props.explorerState; - const { annotationsData, aggregations } = annotations; + const { annotationsData, aggregations, error: annotationsError } = annotations; const jobSelectorProps = { dateFormatTz: getDateFormatTz(), @@ -302,9 +303,36 @@ export class Explorer extends React.Component { setSelectedCells={this.props.setSelectedCells} /> - {annotationsData.length > 0 && ( + {annotationsError !== undefined && ( <> + +

+ +

+ + +

{annotationsError}

+ + + + + )} + {annotationsData.length > 0 && ( + <> + Promise; +) => Promise; export declare interface AnomaliesTableData { anomalies: any[]; diff --git a/x-pack/plugins/ml/public/application/explorer/explorer_utils.js b/x-pack/plugins/ml/public/application/explorer/explorer_utils.js index 03a251d3e2f794..08830decc9449d 100644 --- a/x-pack/plugins/ml/public/application/explorer/explorer_utils.js +++ b/x-pack/plugins/ml/public/application/explorer/explorer_utils.js @@ -16,6 +16,7 @@ import { ANOMALIES_TABLE_DEFAULT_QUERY_SIZE, } from '../../../common/constants/search'; import { getEntityFieldList } from '../../../common/util/anomaly_utils'; +import { extractErrorMessage } from '../../../common/util/errors'; import { isSourceDataChartableForDetector, isModelPlotChartableForDetector, @@ -406,7 +407,12 @@ export function loadAnnotationsTableData(selectedCells, selectedJobs, interval, .toPromise() .then((resp) => { if (resp.error !== undefined || resp.annotations === undefined) { - return resolve([]); + const errorMessage = extractErrorMessage(resp.error); + return resolve({ + annotationsData: [], + aggregations: {}, + error: errorMessage !== '' ? errorMessage : undefined, + }); } const annotationsData = []; @@ -430,9 +436,12 @@ export function loadAnnotationsTableData(selectedCells, selectedJobs, interval, }); }) .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([]); + const errorMessage = extractErrorMessage(resp); + return resolve({ + annotationsData: [], + aggregations: {}, + error: errorMessage !== '' ? errorMessage : undefined, + }); }); }); } diff --git a/x-pack/plugins/ml/public/application/explorer/reducers/explorer_reducer/state.ts b/x-pack/plugins/ml/public/application/explorer/reducers/explorer_reducer/state.ts index 889d572f4fabcc..ea9a8b5c18054a 100644 --- a/x-pack/plugins/ml/public/application/explorer/reducers/explorer_reducer/state.ts +++ b/x-pack/plugins/ml/public/application/explorer/reducers/explorer_reducer/state.ts @@ -21,14 +21,11 @@ import { SwimlaneData, ViewBySwimLaneData, } from '../../explorer_utils'; -import { Annotations, EsAggregationResult } from '../../../../../common/types/annotations'; +import { AnnotationsTable } from '../../../../../common/types/annotations'; import { SWIM_LANE_DEFAULT_PAGE_SIZE } from '../../explorer_constants'; export interface ExplorerState { - annotations: { - annotationsData: Annotations; - aggregations: EsAggregationResult; - }; + annotations: AnnotationsTable; bounds: TimeRangeBounds | undefined; chartsData: ExplorerChartsData; fieldFormatsLoading: boolean; @@ -67,6 +64,7 @@ function getDefaultIndexPattern() { export function getExplorerDefaultState(): ExplorerState { return { annotations: { + error: undefined, annotationsData: [], aggregations: {}, }, diff --git a/x-pack/plugins/ml/public/application/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js b/x-pack/plugins/ml/public/application/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js index 7ec59f4acbc519..c1afb2994c92f9 100644 --- a/x-pack/plugins/ml/public/application/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js +++ b/x-pack/plugins/ml/public/application/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js @@ -552,7 +552,7 @@ class TimeseriesChartIntl extends Component { renderFocusChart() { const { focusAggregationInterval, - focusAnnotationData, + focusAnnotationData: focusAnnotationDataOriginalPropValue, focusChartData, focusForecastData, modelPlotEnabled, @@ -565,6 +565,10 @@ class TimeseriesChartIntl extends Component { zoomToFocusLoaded, } = this.props; + const focusAnnotationData = Array.isArray(focusAnnotationDataOriginalPropValue) + ? focusAnnotationDataOriginalPropValue + : []; + if (focusChartData === undefined) { return; } diff --git a/x-pack/plugins/ml/public/application/timeseriesexplorer/timeseriesexplorer.js b/x-pack/plugins/ml/public/application/timeseriesexplorer/timeseriesexplorer.js index 95dc1ed6988f6c..83a789074d353c 100644 --- a/x-pack/plugins/ml/public/application/timeseriesexplorer/timeseriesexplorer.js +++ b/x-pack/plugins/ml/public/application/timeseriesexplorer/timeseriesexplorer.js @@ -27,6 +27,7 @@ import { EuiFormRow, EuiSelect, EuiSpacer, + EuiPanel, EuiTitle, EuiAccordion, EuiBadge, @@ -1028,6 +1029,7 @@ export class TimeSeriesExplorer extends React.Component { dataNotChartable, entityValues, focusAggregationInterval, + focusAnnotationError, focusAnnotationData, focusAggregations, focusChartData, @@ -1316,6 +1318,36 @@ export class TimeSeriesExplorer extends React.Component { )} + {focusAnnotationError !== undefined && ( + <> + +

+ +

+ + + +

{focusAnnotationError}

+ + + + + )} {focusAnnotationData && focusAnnotationData.length > 0 && ( } + data-test-subj="mlAnomalyExplorerAnnotations loaded" > { - // silent fail - return of({ - annotations: {} as Record, + catchError((resp) => + of({ + annotations: {}, aggregations: {}, + error: extractErrorMessage(resp), success: false, - }); - }) + } as GetAnnotationsResponse) + ) ), // Plus query for forecast data if there is a forecastId stored in the appState. forecastId !== undefined @@ -152,16 +154,22 @@ export function getFocusData( }; if (annotations) { - refreshFocusData.focusAnnotationData = (annotations.annotations[selectedJob.job_id] ?? []) - .sort((a, b) => { - return a.timestamp - b.timestamp; - }) - .map((d, i: number) => { - d.key = (i + 1).toString(); - return d; - }); + if (annotations.error !== undefined) { + refreshFocusData.focusAnnotationError = annotations.error; + refreshFocusData.focusAnnotationData = []; + refreshFocusData.focusAggregations = {}; + } else { + refreshFocusData.focusAnnotationData = (annotations.annotations[selectedJob.job_id] ?? []) + .sort((a, b) => { + return a.timestamp - b.timestamp; + }) + .map((d, i: number) => { + d.key = (i + 1).toString(); + return d; + }); - refreshFocusData.focusAggregations = annotations.aggregations; + refreshFocusData.focusAggregations = annotations.aggregations; + } } if (forecastData) { diff --git a/x-pack/test/functional/apps/ml/anomaly_detection/annotations.ts b/x-pack/test/functional/apps/ml/anomaly_detection/annotations.ts new file mode 100644 index 00000000000000..202910622fb64e --- /dev/null +++ b/x-pack/test/functional/apps/ml/anomaly_detection/annotations.ts @@ -0,0 +1,109 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +import expect from '@kbn/expect'; + +import { FtrProviderContext } from '../../../ftr_provider_context'; +import { Job, Datafeed } from '../../../../../plugins/ml/common/types/anomaly_detection_jobs'; + +const JOB_CONFIG: Job = { + job_id: `fq_single_1_smv`, + description: 'mean(responsetime) on farequote dataset with 15m bucket span', + groups: ['farequote', 'automated', 'single-metric'], + analysis_config: { + bucket_span: '15m', + influencers: [], + detectors: [ + { + function: 'mean', + field_name: 'responsetime', + }, + ], + }, + data_description: { time_field: '@timestamp' }, + analysis_limits: { model_memory_limit: '10mb' }, + model_plot_config: { enabled: true }, +}; + +const DATAFEED_CONFIG: Datafeed = { + datafeed_id: 'datafeed-fq_single_1_smv', + indices: ['ft_farequote'], + job_id: 'fq_single_1_smv', + query: { bool: { must: [{ match_all: {} }] } }, +}; + +export default function ({ getService }: FtrProviderContext) { + const esArchiver = getService('esArchiver'); + const ml = getService('ml'); + + describe('annotations', function () { + this.tags(['mlqa']); + before(async () => { + await esArchiver.loadIfNeeded('ml/farequote'); + await ml.testResources.createIndexPatternIfNeeded('ft_farequote', '@timestamp'); + await ml.testResources.setKibanaTimeZoneToUTC(); + + await ml.api.createAndRunAnomalyDetectionLookbackJob(JOB_CONFIG, DATAFEED_CONFIG); + // Points the read/write aliases of annotations to an index with wrong mappings + // so we can simulate errors when requesting annotations. + await ml.testResources.setupBrokenAnnotationsIndexState(JOB_CONFIG.job_id); + await ml.securityUI.loginAsMlPowerUser(); + }); + + after(async () => { + await ml.api.cleanMlIndices(); + }); + + it('loads from job list row link', async () => { + await ml.navigation.navigateToMl(); + await ml.navigation.navigateToJobManagement(); + + await ml.jobTable.waitForJobsToLoad(); + await ml.jobTable.filterWithSearchString(JOB_CONFIG.job_id); + const rows = await ml.jobTable.parseJobTable(); + expect(rows.filter((row) => row.id === JOB_CONFIG.job_id)).to.have.length(1); + + await ml.jobTable.clickOpenJobInSingleMetricViewerButton(JOB_CONFIG.job_id); + await ml.common.waitForMlLoadingIndicatorToDisappear(); + }); + + it('pre-fills the job selection', async () => { + await ml.jobSelection.assertJobSelection([JOB_CONFIG.job_id]); + }); + + it('pre-fills the detector input', async () => { + await ml.singleMetricViewer.assertDetectorInputExsist(); + await ml.singleMetricViewer.assertDetectorInputValue('0'); + }); + + it('should display the annotations section showing an error', async () => { + await ml.singleMetricViewer.assertAnnotationsExists('error'); + }); + + it('should navigate to anomaly explorer', async () => { + await ml.navigation.navigateToAnomalyExplorerViaSingleMetricViewer(); + }); + + it('should display the annotations section showing an error', async () => { + await ml.anomalyExplorer.assertAnnotationsPanelExists('error'); + }); + + it('should display the annotations section without an error', async () => { + // restores the aliases to point to the original working annotations index + // so we can run tests against successfully loaded annotations sections. + await ml.testResources.restoreAnnotationsIndexState(); + await ml.anomalyExplorer.refreshPage(); + await ml.anomalyExplorer.assertAnnotationsPanelExists('loaded'); + }); + + it('should navigate to single metric viewer', async () => { + await ml.navigation.navigateToSingleMetricViewerViaAnomalyExplorer(); + }); + + it('should display the annotations section without an error', async () => { + await ml.singleMetricViewer.assertAnnotationsExists('loaded'); + }); + }); +} diff --git a/x-pack/test/functional/apps/ml/anomaly_detection/anomaly_explorer.ts b/x-pack/test/functional/apps/ml/anomaly_detection/anomaly_explorer.ts index 89308938cfab05..cbee36abef78d4 100644 --- a/x-pack/test/functional/apps/ml/anomaly_detection/anomaly_explorer.ts +++ b/x-pack/test/functional/apps/ml/anomaly_detection/anomaly_explorer.ts @@ -118,6 +118,10 @@ export default function ({ getService }: FtrProviderContext) { await ml.anomalyExplorer.assertSwimlaneViewByExists(); }); + it('should display the annotations panel', async () => { + await ml.anomalyExplorer.assertAnnotationsPanelExists('loaded'); + }); + it('displays the anomalies table', async () => { await ml.anomaliesTable.assertTableExists(); }); diff --git a/x-pack/test/functional/apps/ml/anomaly_detection/index.ts b/x-pack/test/functional/apps/ml/anomaly_detection/index.ts index b5e6e426e2ead1..0983ebd79dd903 100644 --- a/x-pack/test/functional/apps/ml/anomaly_detection/index.ts +++ b/x-pack/test/functional/apps/ml/anomaly_detection/index.ts @@ -18,5 +18,6 @@ export default function ({ loadTestFile }: FtrProviderContext) { loadTestFile(require.resolve('./anomaly_explorer')); loadTestFile(require.resolve('./categorization_job')); loadTestFile(require.resolve('./date_nanos_job')); + loadTestFile(require.resolve('./annotations')); }); } diff --git a/x-pack/test/functional/apps/ml/anomaly_detection/single_metric_viewer.ts b/x-pack/test/functional/apps/ml/anomaly_detection/single_metric_viewer.ts index db511c5d75f396..3855bd0c884cdb 100644 --- a/x-pack/test/functional/apps/ml/anomaly_detection/single_metric_viewer.ts +++ b/x-pack/test/functional/apps/ml/anomaly_detection/single_metric_viewer.ts @@ -79,6 +79,10 @@ export default function ({ getService }: FtrProviderContext) { await ml.singleMetricViewer.assertChartExsist(); }); + it('should display the annotations section', async () => { + await ml.singleMetricViewer.assertAnnotationsExists('loaded'); + }); + it('displays the anomalies table', async () => { await ml.anomaliesTable.assertTableExists(); }); diff --git a/x-pack/test/functional/services/ml/anomaly_explorer.ts b/x-pack/test/functional/services/ml/anomaly_explorer.ts index 80df235bf6ff8d..1a6d5cd09f2e27 100644 --- a/x-pack/test/functional/services/ml/anomaly_explorer.ts +++ b/x-pack/test/functional/services/ml/anomaly_explorer.ts @@ -67,6 +67,12 @@ export function MachineLearningAnomalyExplorerProvider({ getService }: FtrProvid await testSubjects.existOrFail('mlAnomalyExplorerSwimlaneViewBy'); }, + async assertAnnotationsPanelExists(state: string) { + await testSubjects.existOrFail(`mlAnomalyExplorerAnnotationsPanel ${state}`, { + timeout: 30 * 1000, + }); + }, + async openAddToDashboardControl() { await testSubjects.click('mlAnomalyTimelinePanelMenu'); await testSubjects.click('mlAnomalyTimelinePanelAddToDashboardButton'); @@ -89,6 +95,10 @@ export function MachineLearningAnomalyExplorerProvider({ getService }: FtrProvid ); }, + async refreshPage() { + await testSubjects.click('superDatePickerApplyTimeButton'); + }, + async waitForDashboardsToLoad() { await testSubjects.existOrFail('~mlDashboardSelectionTable', { timeout: 60 * 1000 }); }, diff --git a/x-pack/test/functional/services/ml/navigation.ts b/x-pack/test/functional/services/ml/navigation.ts index f52197d4b2256d..116c9deb7c2dc6 100644 --- a/x-pack/test/functional/services/ml/navigation.ts +++ b/x-pack/test/functional/services/ml/navigation.ts @@ -103,5 +103,23 @@ export function MachineLearningNavigationProvider({ await testSubjects.existOrFail('mlNoDataFrameAnalyticsFound'); }); }, + + async navigateToAnomalyExplorerViaSingleMetricViewer() { + // clicks the `Anomaly Explorer` icon on the button group to switch result views + await testSubjects.click('mlAnomalyResultsViewSelectorExplorer'); + await retry.tryForTime(60 * 1000, async () => { + // verify that the anomaly explorer page is visible + await testSubjects.existOrFail('mlPageAnomalyExplorer'); + }); + }, + + async navigateToSingleMetricViewerViaAnomalyExplorer() { + // clicks the `Single Metric Viewere` icon on the button group to switch result views + await testSubjects.click('mlAnomalyResultsViewSelectorSingleMetricViewer'); + await retry.tryForTime(60 * 1000, async () => { + // verify that the single metric viewer page is visible + await testSubjects.existOrFail('mlPageSingleMetricViewer'); + }); + }, }; } diff --git a/x-pack/test/functional/services/ml/single_metric_viewer.ts b/x-pack/test/functional/services/ml/single_metric_viewer.ts index b2c3e19020e62d..a65ac09a0b0564 100644 --- a/x-pack/test/functional/services/ml/single_metric_viewer.ts +++ b/x-pack/test/functional/services/ml/single_metric_viewer.ts @@ -49,5 +49,11 @@ export function MachineLearningSingleMetricViewerProvider({ getService }: FtrPro async assertChartExsist() { await testSubjects.existOrFail('mlSingleMetricViewerChart'); }, + + async assertAnnotationsExists(state: string) { + await testSubjects.existOrFail(`mlAnomalyExplorerAnnotations ${state}`, { + timeout: 30 * 1000, + }); + }, }; } diff --git a/x-pack/test/functional/services/ml/test_resources.ts b/x-pack/test/functional/services/ml/test_resources.ts index 942dc4a80d4ac8..675ec890b9edf1 100644 --- a/x-pack/test/functional/services/ml/test_resources.ts +++ b/x-pack/test/functional/services/ml/test_resources.ts @@ -20,6 +20,7 @@ export enum SavedObjectType { export type MlTestResourcesi = ProvidedType; export function MachineLearningTestResourcesProvider({ getService }: FtrProviderContext) { + const es = getService('es'); const kibanaServer = getService('kibanaServer'); const log = getService('log'); const supertest = getService('supertest'); @@ -166,6 +167,91 @@ export function MachineLearningTestResourcesProvider({ getService }: FtrProvider } }, + async setupBrokenAnnotationsIndexState(jobId: string) { + // Creates a temporary annotations index with unsupported mappings. + await es.indices.create({ + index: '.ml-annotations-6-wrong-mapping', + body: { + settings: { + number_of_shards: 1, + }, + mappings: { + properties: { + field1: { type: 'text' }, + }, + }, + }, + }); + + // Ingests an annotation that will cause dynamic mapping to pick up the wrong field type. + es.create({ + id: 'annotation_with_wrong_mapping', + index: '.ml-annotations-6-wrong-mapping', + body: { + annotation: 'Annotation with wrong mapping', + create_time: 1597393915910, + create_username: '_xpack', + timestamp: 1549756800000, + end_timestamp: 1549756800000, + job_id: jobId, + modified_time: 1597393915910, + modified_username: '_xpack', + type: 'annotation', + event: 'user', + detector_index: 0, + }, + }); + + // Points the read/write aliases for annotations to the broken annotations index + // so we can run tests against a state where annotation endpoints return errors. + await es.indices.updateAliases({ + body: { + actions: [ + { + add: { + index: '.ml-annotations-6-wrong-mapping', + alias: '.ml-annotations-read', + is_hidden: true, + }, + }, + { remove: { index: '.ml-annotations-6', alias: '.ml-annotations-read' } }, + { + add: { + index: '.ml-annotations-6-wrong-mapping', + alias: '.ml-annotations-write', + is_hidden: true, + }, + }, + { remove: { index: '.ml-annotations-6', alias: '.ml-annotations-write' } }, + ], + }, + }); + }, + + async restoreAnnotationsIndexState() { + // restore the original working state of pointing read/write aliases + // to the right annotations index. + await es.indices.updateAliases({ + body: { + actions: [ + { add: { index: '.ml-annotations-6', alias: '.ml-annotations-read', is_hidden: true } }, + { remove: { index: '.ml-annotations-6-wrong-mapping', alias: '.ml-annotations-read' } }, + { + add: { index: '.ml-annotations-6', alias: '.ml-annotations-write', is_hidden: true }, + }, + { + remove: { index: '.ml-annotations-6-wrong-mapping', alias: '.ml-annotations-write' }, + }, + ], + }, + }); + + // deletes the temporary annotations index with wrong mappings + await es.indices.delete({ + index: '.ml-annotations-6-wrong-mapping', + }); + }, + async updateSavedSearchRequestBody(body: object, indexPatternTitle: string): Promise { const indexPatternId = await this.getIndexPatternId(indexPatternTitle); if (indexPatternId === undefined) {