From 755dc7e8ee8308d2220a5028d5fb099f07389633 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Thu, 17 Dec 2020 18:58:37 +0100 Subject: [PATCH] [ML] Anomaly Detection: Fix validation error when no data in index. (#86114) For cardinality checks, an empty index of the fields checks not returning any results would block the user from moving to the next step in the Anomaly Detection job wizard. This PR fixes it by adding more fine grained checks and only returning warning-level messages for the above cases. A warning-level message will allow the user to continue to the next step in the wizard. --- .../plugins/ml/common/constants/messages.ts | 32 ++++ .../validate_job/validate_job_view.js | 141 ++++++++++++------ .../validate_job/validate_job_view.test.js | 5 +- .../components/validation_step/validation.tsx | 10 +- .../job_validation/job_validation.test.ts | 2 +- .../validate_cardinality.test.ts | 32 ++++ .../job_validation/validate_cardinality.ts | 69 +++++---- 7 files changed, 207 insertions(+), 84 deletions(-) diff --git a/x-pack/plugins/ml/common/constants/messages.ts b/x-pack/plugins/ml/common/constants/messages.ts index 1027ee5bf9a89d..0e7303c6d73175 100644 --- a/x-pack/plugins/ml/common/constants/messages.ts +++ b/x-pack/plugins/ml/common/constants/messages.ts @@ -90,6 +90,38 @@ export const getMessages = once(() => { url: 'https://www.elastic.co/guide/en/machine-learning/{{version}}/ml-configuring-aggregation.html', }, + cardinality_no_results: { + status: VALIDATION_STATUS.WARNING, + heading: i18n.translate( + 'xpack.ml.models.jobValidation.messages.cardinalityNoResultsHeading', + { + defaultMessage: 'Field cardinality', + } + ), + text: i18n.translate('xpack.ml.models.jobValidation.messages.cardinalityNoResultsMessage', { + defaultMessage: `Cardinality checks could not be run. The query to validate fields didn't return any documents.`, + }), + }, + cardinality_field_not_exists: { + status: VALIDATION_STATUS.WARNING, + heading: i18n.translate( + 'xpack.ml.models.jobValidation.messages.cardinalityFieldNotExistsHeading', + { + defaultMessage: 'Field cardinality', + } + ), + text: i18n.translate( + 'xpack.ml.models.jobValidation.messages.cardinalityFieldNotExistsMessage', + { + defaultMessage: `Cardinality checks could not be run for field {fieldName}. The query to validate the field didn't return any documents.`, + values: { + fieldName: '"{{fieldName}}"', + }, + } + ), + url: + 'https://www.elastic.co/guide/en/machine-learning/{{version}}/ml-configuring-aggregation.html', + }, cardinality_by_field: { status: VALIDATION_STATUS.WARNING, text: i18n.translate('xpack.ml.models.jobValidation.messages.cardinalityByFieldMessage', { diff --git a/x-pack/plugins/ml/public/application/components/validate_job/validate_job_view.js b/x-pack/plugins/ml/public/application/components/validate_job/validate_job_view.js index 0c079bc11cffc0..1ce4188d8e0632 100644 --- a/x-pack/plugins/ml/public/application/components/validate_job/validate_job_view.js +++ b/x-pack/plugins/ml/public/application/components/validate_job/validate_job_view.js @@ -208,60 +208,103 @@ export class ValidateJobUI extends Component { const duration = typeof getDuration === 'function' ? getDuration() : undefined; const fields = this.props.fields; + // Run job validation only if a job config has been passed on and the duration makes sense to run it. + // Otherwise we skip the call and display a generic warning, but let the user move on to the next wizard step. if (typeof job === 'object') { - let shouldShowLoadingIndicator = true; - - this.props.ml - .validateJob({ duration, fields, job }) - .then((messages) => { - shouldShowLoadingIndicator = false; - this.setState({ - ...this.state, - ui: { - ...this.state.ui, - iconType: statusToEuiIconType(getMostSevereMessageStatus(messages)), - isLoading: false, - isModalVisible: true, - }, - data: { - messages, - success: true, - }, - title: job.job_id, - }); - if (typeof this.props.setIsValid === 'function') { - this.props.setIsValid( - messages.some((m) => m.status === VALIDATION_STATUS.ERROR) === false + if (typeof duration === 'object' && duration.start !== null && duration.end !== null) { + let shouldShowLoadingIndicator = true; + + this.props.ml + .validateJob({ duration, fields, job }) + .then((messages) => { + shouldShowLoadingIndicator = false; + + const messagesContainError = messages.some((m) => m.status === VALIDATION_STATUS.ERROR); + + if (messagesContainError) { + messages.push({ + id: 'job_validation_includes_error', + text: i18n.translate('xpack.ml.validateJob.jobValidationIncludesErrorText', { + defaultMessage: + 'Job validation has failed, but you can still continue and create the job. Please be aware the job may encounter problems when running.', + }), + status: VALIDATION_STATUS.WARNING, + }); + } + + this.setState({ + ...this.state, + ui: { + ...this.state.ui, + iconType: statusToEuiIconType(getMostSevereMessageStatus(messages)), + isLoading: false, + isModalVisible: true, + }, + data: { + messages, + success: true, + }, + title: job.job_id, + }); + if (typeof this.props.setIsValid === 'function') { + this.props.setIsValid(!messagesContainError); + } + }) + .catch((error) => { + const { toasts } = this.props.kibana.services.notifications; + const toastNotificationService = toastNotificationServiceProvider(toasts); + toastNotificationService.displayErrorToast( + error, + i18n.translate('xpack.ml.jobService.validateJobErrorTitle', { + defaultMessage: 'Job Validation Error', + }) ); + }); + + // wait for 250ms before triggering the loading indicator + // to avoid flickering when there's a loading time below + // 250ms for the job validation data + const delay = 250; + setTimeout(() => { + if (shouldShowLoadingIndicator) { + this.setState({ + ...this.state, + ui: { + ...this.state.ui, + isLoading: true, + isModalVisible: false, + }, + }); } - }) - .catch((error) => { - const { toasts } = this.props.kibana.services.notifications; - const toastNotificationService = toastNotificationServiceProvider(toasts); - toastNotificationService.displayErrorToast( - error, - i18n.translate('xpack.ml.jobService.validateJobErrorTitle', { - defaultMessage: 'Job Validation Error', - }) - ); + }, delay); + } else { + this.setState({ + ...this.state, + ui: { + ...this.state.ui, + iconType: statusToEuiIconType(VALIDATION_STATUS.WARNING), + isLoading: false, + isModalVisible: true, + }, + data: { + messages: [ + { + id: 'job_validation_skipped', + text: i18n.translate('xpack.ml.validateJob.jobValidationSkippedText', { + defaultMessage: + 'Job validation could not be run because of insufficient sample data. Please be aware the job may encounter problems when running.', + }), + status: VALIDATION_STATUS.WARNING, + }, + ], + success: true, + }, + title: job.job_id, }); - - // wait for 250ms before triggering the loading indicator - // to avoid flickering when there's a loading time below - // 250ms for the job validation data - const delay = 250; - setTimeout(() => { - if (shouldShowLoadingIndicator) { - this.setState({ - ...this.state, - ui: { - ...this.state.ui, - isLoading: true, - isModalVisible: false, - }, - }); + if (typeof this.props.setIsValid === 'function') { + this.props.setIsValid(true); } - }, delay); + } } }; diff --git a/x-pack/plugins/ml/public/application/components/validate_job/validate_job_view.test.js b/x-pack/plugins/ml/public/application/components/validate_job/validate_job_view.test.js index f2f785d91dcac3..7e473f12ee50de 100644 --- a/x-pack/plugins/ml/public/application/components/validate_job/validate_job_view.test.js +++ b/x-pack/plugins/ml/public/application/components/validate_job/validate_job_view.test.js @@ -27,6 +27,7 @@ const job = { }; const getJobConfig = () => job; +const getDuration = () => ({ start: 0, end: 1 }); function prepareTest(messages) { const p = Promise.resolve(messages); @@ -40,7 +41,9 @@ function prepareTest(messages) { }, }; - const component = ; + const component = ( + + ); const wrapper = shallowWithIntl(component); diff --git a/x-pack/plugins/ml/public/application/jobs/new_job/pages/components/validation_step/validation.tsx b/x-pack/plugins/ml/public/application/jobs/new_job/pages/components/validation_step/validation.tsx index 3bde32f40eeb5a..224e2eacb21e0b 100644 --- a/x-pack/plugins/ml/public/application/jobs/new_job/pages/components/validation_step/validation.tsx +++ b/x-pack/plugins/ml/public/application/jobs/new_job/pages/components/validation_step/validation.tsx @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import React, { Fragment, FC, useContext, useState, useEffect } from 'react'; +import React, { Fragment, FC, useContext, useEffect } from 'react'; import { WizardNav } from '../wizard_nav'; import { WIZARD_STEPS, StepProps } from '../step_types'; import { JobCreatorContext } from '../job_creator_context'; @@ -21,7 +21,6 @@ const idFilterList = [ export const ValidationStep: FC = ({ setCurrentStep, isCurrentStep }) => { const { jobCreator, jobCreatorUpdate, jobValidator } = useContext(JobCreatorContext); - const [nextActive, setNextActive] = useState(false); if (jobCreator.type === JOB_TYPE.ADVANCED) { // for advanced jobs, ignore time range warning as the @@ -50,13 +49,8 @@ export const ValidationStep: FC = ({ setCurrentStep, isCurrentStep }) }, []); // keep a record of the advanced validation in the jobValidator - // and disable the next button if any advanced checks have failed. - // note, it is not currently possible to get to a state where any of the - // advanced validation checks return an error because they are all - // caught in previous basic checks function setIsValid(valid: boolean) { jobValidator.advancedValid = valid; - setNextActive(valid); } return ( @@ -74,7 +68,7 @@ export const ValidationStep: FC = ({ setCurrentStep, isCurrentStep }) setCurrentStep(WIZARD_STEPS.JOB_DETAILS)} next={() => setCurrentStep(WIZARD_STEPS.SUMMARY)} - nextActive={nextActive} + nextActive={true} /> )} diff --git a/x-pack/plugins/ml/server/models/job_validation/job_validation.test.ts b/x-pack/plugins/ml/server/models/job_validation/job_validation.test.ts index d397d39d32b6bf..8e88d1c1d05376 100644 --- a/x-pack/plugins/ml/server/models/job_validation/job_validation.test.ts +++ b/x-pack/plugins/ml/server/models/job_validation/job_validation.test.ts @@ -12,7 +12,7 @@ import type { MlClient } from '../../lib/ml_client'; const callAs = { fieldCaps: () => Promise.resolve({ body: { fields: [] } }), - search: () => Promise.resolve({ body: { hits: { total: { value: 0, relation: 'eq' } } } }), + search: () => Promise.resolve({ body: { hits: { total: { value: 1, relation: 'eq' } } } }), }; const mlClusterClient = ({ diff --git a/x-pack/plugins/ml/server/models/job_validation/validate_cardinality.test.ts b/x-pack/plugins/ml/server/models/job_validation/validate_cardinality.test.ts index 2e2a9e21aa9594..3996f42c489265 100644 --- a/x-pack/plugins/ml/server/models/job_validation/validate_cardinality.test.ts +++ b/x-pack/plugins/ml/server/models/job_validation/validate_cardinality.test.ts @@ -189,6 +189,38 @@ describe('ML - validateCardinality', () => { ); }); + it('cardinality no results', () => { + const job = getJobConfig('partition_field_name'); + + const mockCardinality = cloneDeep(mockResponses); + mockCardinality.search.hits.total.value = 0; + mockCardinality.search.aggregations.airline_count.doc_count = 0; + + return validateCardinality( + mlClusterClientFactory(mockCardinality), + (job as unknown) as CombinedJob + ).then((messages) => { + const ids = messages.map((m) => m.id); + expect(ids).toStrictEqual(['cardinality_no_results']); + }); + }); + + it('cardinality field not exists', () => { + const job = getJobConfig('partition_field_name'); + + const mockCardinality = cloneDeep(mockResponses); + mockCardinality.search.hits.total.value = 1; + mockCardinality.search.aggregations.airline_count.doc_count = 0; + + return validateCardinality( + mlClusterClientFactory(mockCardinality), + (job as unknown) as CombinedJob + ).then((messages) => { + const ids = messages.map((m) => m.id); + expect(ids).toStrictEqual(['cardinality_field_not_exists']); + }); + }); + it('fields not aggregatable', () => { const job = getJobConfig('partition_field_name'); job.analysis_config.detectors.push({ diff --git a/x-pack/plugins/ml/server/models/job_validation/validate_cardinality.ts b/x-pack/plugins/ml/server/models/job_validation/validate_cardinality.ts index 822d1a1081874a..3afb4035546668 100644 --- a/x-pack/plugins/ml/server/models/job_validation/validate_cardinality.ts +++ b/x-pack/plugins/ml/server/models/job_validation/validate_cardinality.ts @@ -132,35 +132,54 @@ const validateFactory = (client: IScopedClusterClient, job: CombinedJob): Valida datafeedConfig ); - uniqueFieldNames.forEach((uniqueFieldName) => { - const field = stats.aggregatableExistsFields.find( - (fieldData) => fieldData.fieldName === uniqueFieldName - ); - if (field !== undefined && typeof field === 'object' && field.stats) { - modelPlotCardinality += - modelPlotConfigFieldCount > 0 ? modelPlotConfigFieldCount : field.stats.cardinality!; - - if (isInvalid(field.stats.cardinality!)) { - messages.push({ - id: messageId || (`cardinality_${type}_field` as MessageId), - fieldName: uniqueFieldName, - }); - } - } else { - // only report uniqueFieldName as not aggregatable if it's not part - // of a valid categorization configuration and if it's not a scripted field or runtime mapping. - if ( - !isValidCategorizationConfig(job, uniqueFieldName) && - !isScriptField(job, uniqueFieldName) && - !isRuntimeMapping(job, uniqueFieldName) - ) { + if (stats.totalCount === 0) { + messages.push({ + id: 'cardinality_no_results', + }); + } else { + uniqueFieldNames.forEach((uniqueFieldName) => { + const aggregatableNotExistsField = stats.aggregatableNotExistsFields.find( + (fieldData) => fieldData.fieldName === uniqueFieldName + ); + + if (aggregatableNotExistsField !== undefined) { messages.push({ - id: 'field_not_aggregatable', + id: 'cardinality_field_not_exists', fieldName: uniqueFieldName, }); + } else { + const field = stats.aggregatableExistsFields.find( + (fieldData) => fieldData.fieldName === uniqueFieldName + ); + if (field !== undefined && typeof field === 'object' && field.stats) { + modelPlotCardinality += + modelPlotConfigFieldCount > 0 + ? modelPlotConfigFieldCount + : field.stats.cardinality!; + + if (isInvalid(field.stats.cardinality!)) { + messages.push({ + id: messageId || (`cardinality_${type}_field` as MessageId), + fieldName: uniqueFieldName, + }); + } + } else { + // only report uniqueFieldName as not aggregatable if it's not part + // of a valid categorization configuration and if it's not a scripted field or runtime mapping. + if ( + !isValidCategorizationConfig(job, uniqueFieldName) && + !isScriptField(job, uniqueFieldName) && + !isRuntimeMapping(job, uniqueFieldName) + ) { + messages.push({ + id: 'field_not_aggregatable', + fieldName: uniqueFieldName, + }); + } + } } - } - }); + }); + } } catch (e) { // checkAggregatableFieldsExist may return an error if 'fielddata' is // disabled for text fields (which is the default). If there was only