diff --git a/x-pack/plugins/ml/common/constants/messages.ts b/x-pack/plugins/ml/common/constants/messages.ts index 1027ee5bf9a89..0e7303c6d7317 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 0c079bc11cffc..1ce4188d8e063 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 f2f785d91dcac..7e473f12ee50d 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 3bde32f40eeb5..224e2eacb21e0 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 d397d39d32b6b..8e88d1c1d0537 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 2e2a9e21aa959..3996f42c48926 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 822d1a1081874..3afb403554666 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