[ML] Improving existing job check in anomaly detection wizard (#87674)

* [ML] Improving existing job check in anomaly detection wizard

* fixing job id validation

* allow group ids to be reused

* updating module exists endpoint

* fixing issuse with job without group list

* fixing test and translation ids

* fixing validator when model plot is disabled

* changes based on review

* adding group id check to edit job flyout

* small refactor and fixing edit job issue

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
This commit is contained in:
James Gowdy 2021-01-15 12:57:41 +00:00 committed by GitHub
parent 701bd0998d
commit 686ece9aea
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
15 changed files with 258 additions and 135 deletions

View file

@ -0,0 +1,24 @@
/*
* 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 { Job, JobStats } from './anomaly_detection_jobs';
export interface MlJobsResponse {
jobs: Job[];
count: number;
}
export interface MlJobsStatsResponse {
jobs: JobStats[];
count: number;
}
export interface JobsExistResponse {
[jobId: string]: {
exists: boolean;
isGroup: boolean;
};
}

View file

@ -29,6 +29,7 @@ import { saveJob } from './edit_utils';
import { loadFullJob } from '../utils';
import { validateModelMemoryLimit, validateGroupNames, isValidCustomUrls } from '../validate_job';
import { toastNotificationServiceProvider } from '../../../../services/toast_notification_service';
import { ml } from '../../../../services/ml_api_service';
import { withKibana } from '../../../../../../../../../src/plugins/kibana_react/public';
import { collapseLiteralStrings } from '../../../../../../shared_imports';
import { DATAFEED_STATE } from '../../../../../../common/constants/states';
@ -195,16 +196,24 @@ export class EditJobFlyoutUI extends Component {
}
if (jobDetails.jobGroups !== undefined) {
if (jobDetails.jobGroups.some((j) => this.props.allJobIds.includes(j))) {
jobGroupsValidationError = i18n.translate(
'xpack.ml.jobsList.editJobFlyout.groupsAndJobsHasSameIdErrorMessage',
{
defaultMessage:
'A job with this ID already exists. Groups and jobs cannot use the same ID.',
jobGroupsValidationError = validateGroupNames(jobDetails.jobGroups).message;
if (jobGroupsValidationError === '') {
ml.jobs.jobsExist(jobDetails.jobGroups, true).then((resp) => {
const groups = Object.values(resp);
const valid = groups.some((g) => g.exists === true && g.isGroup === false) === false;
if (valid === false) {
this.setState({
jobGroupsValidationError: i18n.translate(
'xpack.ml.jobsList.editJobFlyout.groupsAndJobsHasSameIdErrorMessage',
{
defaultMessage:
'A job with this ID already exists. Groups and jobs cannot use the same ID.',
}
),
isValidJobDetails: false,
});
}
);
} else {
jobGroupsValidationError = validateGroupNames(jobDetails.jobGroups).message;
});
}
}

View file

@ -14,9 +14,15 @@ import {
} from '../../../../../../common/util/job_utils';
import { getNewJobLimits } from '../../../../services/ml_server_info';
import { JobCreator, JobCreatorType, isCategorizationJobCreator } from '../job_creator';
import { populateValidationMessages, checkForExistingJobAndGroupIds } from './util';
import { ExistingJobsAndGroups } from '../../../../services/job_service';
import { cardinalityValidator, CardinalityValidatorResult } from './validators';
import { populateValidationMessages } from './util';
import {
cardinalityValidator,
CardinalityValidatorResult,
jobIdValidator,
groupIdsValidator,
JobExistsResult,
GroupsExistResult,
} from './validators';
import { CATEGORY_EXAMPLES_VALIDATION_STATUS } from '../../../../../../common/constants/categorization_job';
import { JOB_TYPE } from '../../../../../../common/constants/new_job';
@ -25,7 +31,9 @@ import { JOB_TYPE } from '../../../../../../common/constants/new_job';
// after every keystroke
export const VALIDATION_DELAY_MS = 500;
type AsyncValidatorsResult = Partial<CardinalityValidatorResult>;
type AsyncValidatorsResult = Partial<
CardinalityValidatorResult & JobExistsResult & GroupsExistResult
>;
/**
* Union of possible validation results.
@ -69,7 +77,6 @@ export class JobValidator {
private _validateTimeout: ReturnType<typeof setTimeout> | null = null;
private _asyncValidators$: Array<Observable<AsyncValidatorsResult>> = [];
private _asyncValidatorsResult$: Observable<AsyncValidatorsResult>;
private _existingJobsAndGroups: ExistingJobsAndGroups;
private _basicValidations: BasicValidations = {
jobId: { valid: true },
groupIds: { valid: true },
@ -97,7 +104,7 @@ export class JobValidator {
*/
public validationResult$: Observable<JobValidationResult>;
constructor(jobCreator: JobCreatorType, existingJobsAndGroups: ExistingJobsAndGroups) {
constructor(jobCreator: JobCreatorType) {
this._jobCreator = jobCreator;
this._lastJobConfig = this._jobCreator.formattedJobJson;
this._lastDatafeedConfig = this._jobCreator.formattedDatafeedJson;
@ -105,9 +112,12 @@ export class JobValidator {
basic: false,
advanced: false,
};
this._existingJobsAndGroups = existingJobsAndGroups;
this._asyncValidators$ = [cardinalityValidator(this._jobCreatorSubject$)];
this._asyncValidators$ = [
cardinalityValidator(this._jobCreatorSubject$),
jobIdValidator(this._jobCreatorSubject$),
groupIdsValidator(this._jobCreatorSubject$),
];
this._asyncValidatorsResult$ = combineLatest(this._asyncValidators$).pipe(
map((res) => {
@ -208,14 +218,6 @@ export class JobValidator {
datafeedConfig
);
// run addition job and group id validation
const idResults = checkForExistingJobAndGroupIds(
this._jobCreator.jobId,
this._jobCreator.groups,
this._existingJobsAndGroups
);
populateValidationMessages(idResults, this._basicValidations, jobConfig, datafeedConfig);
this._validationSummary.basic = this._isOverallBasicValid();
// Update validation results subject
this._basicValidationResult$.next(this._basicValidations);

View file

@ -13,8 +13,6 @@ import {
} from '../../../../../../common/constants/validation';
import { getNewJobLimits } from '../../../../services/ml_server_info';
import { ValidationResults } from '../../../../../../common/util/job_utils';
import { ExistingJobsAndGroups } from '../../../../services/job_service';
import { JobValidationMessage } from '../../../../../../common/constants/messages';
export function populateValidationMessages(
validationResults: ValidationResults,
@ -204,36 +202,6 @@ export function populateValidationMessages(
}
}
export function checkForExistingJobAndGroupIds(
jobId: string,
groupIds: string[],
existingJobsAndGroups: ExistingJobsAndGroups
): ValidationResults {
const messages: JobValidationMessage[] = [];
// check that job id does not already exist as a job or group or a newly created group
if (
existingJobsAndGroups.jobIds.includes(jobId) ||
existingJobsAndGroups.groupIds.includes(jobId) ||
groupIds.includes(jobId)
) {
messages.push({ id: 'job_id_already_exists' });
}
// check that groups that have been newly added in this job do not already exist as job ids
const newGroups = groupIds.filter((g) => !existingJobsAndGroups.groupIds.includes(g));
if (existingJobsAndGroups.jobIds.some((g) => newGroups.includes(g))) {
messages.push({ id: 'job_group_id_already_exists' });
}
return {
messages,
valid: messages.length === 0,
contains: (id: string) => messages.some((m) => id === m.id),
find: (id: string) => messages.find((m) => id === m.id),
};
}
function invalidTimeIntervalMessage(value: string | undefined) {
return i18n.translate(
'xpack.ml.newJob.wizard.validateJob.frequencyInvalidTimeIntervalFormatErrorMessage',

View file

@ -4,8 +4,9 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { distinctUntilChanged, filter, map, switchMap } from 'rxjs/operators';
import { Observable, Subject } from 'rxjs';
import { i18n } from '@kbn/i18n';
import { distinctUntilChanged, filter, map, pluck, switchMap, startWith } from 'rxjs/operators';
import { combineLatest, Observable, Subject } from 'rxjs';
import {
CardinalityModelPlotHigh,
CardinalityValidationResult,
@ -13,6 +14,7 @@ import {
} from '../../../../services/ml_api_service';
import { JobCreator } from '../job_creator';
import { CombinedJob } from '../../../../../../common/types/anomaly_detection_jobs';
import { BasicValidations } from './job_validator';
export enum VALIDATOR_SEVERITY {
ERROR,
@ -26,8 +28,30 @@ export interface CardinalityValidatorError {
};
}
const jobExistsErrorMessage = i18n.translate(
'xpack.ml.newJob.wizard.validateJob.asyncJobNameAlreadyExists',
{
defaultMessage:
'Job ID already exists. A job ID cannot be the same as an existing job or group.',
}
);
const groupExistsErrorMessage = i18n.translate(
'xpack.ml.newJob.wizard.validateJob.asyncGroupNameAlreadyExists',
{
defaultMessage:
'Group ID already exists. A group ID cannot be the same as an existing group or job.',
}
);
export type CardinalityValidatorResult = CardinalityValidatorError | null;
export type JobExistsResult = {
jobIdExists: BasicValidations['jobId'];
} | null;
export type GroupsExistResult = {
groupIdsExist: BasicValidations['groupIds'];
} | null;
export function isCardinalityModelPlotHigh(
cardinalityValidationResult: CardinalityValidationResult
): cardinalityValidationResult is CardinalityModelPlotHigh {
@ -39,39 +63,95 @@ export function isCardinalityModelPlotHigh(
export function cardinalityValidator(
jobCreator$: Subject<JobCreator>
): Observable<CardinalityValidatorResult> {
return jobCreator$.pipe(
// Perform a cardinality check only with enabled model plot.
filter((jobCreator) => {
return jobCreator?.modelPlot;
}),
map((jobCreator) => {
return {
jobCreator,
analysisConfigString: JSON.stringify(jobCreator.jobConfig.analysis_config),
};
}),
// No need to perform an API call if the analysis configuration hasn't been changed
distinctUntilChanged((prev, curr) => {
return prev.analysisConfigString === curr.analysisConfigString;
}),
switchMap(({ jobCreator }) => {
return ml.validateCardinality$({
...jobCreator.jobConfig,
datafeed_config: jobCreator.datafeedConfig,
} as CombinedJob);
}),
map((validationResults) => {
for (const validationResult of validationResults) {
if (isCardinalityModelPlotHigh(validationResult)) {
return {
highCardinality: {
value: validationResult.modelPlotCardinality,
severity: VALIDATOR_SEVERITY.WARNING,
},
};
}
}
return null;
return combineLatest([
jobCreator$.pipe(pluck('modelPlot')),
jobCreator$.pipe(
filter((jobCreator) => {
return jobCreator?.modelPlot;
}),
map((jobCreator) => {
return {
jobCreator,
analysisConfigString: JSON.stringify(jobCreator.jobConfig.analysis_config, null, 2),
};
}),
distinctUntilChanged((prev, curr) => {
return prev.analysisConfigString === curr.analysisConfigString;
}),
switchMap(({ jobCreator }) => {
// Perform a cardinality check only with enabled model plot.
return ml
.validateCardinality$({
...jobCreator.jobConfig,
datafeed_config: jobCreator.datafeedConfig,
} as CombinedJob)
.pipe(
map((validationResults) => {
for (const validationResult of validationResults) {
if (isCardinalityModelPlotHigh(validationResult)) {
return {
highCardinality: {
value: validationResult.modelPlotCardinality,
severity: VALIDATOR_SEVERITY.WARNING,
},
};
}
}
return null;
})
);
}),
startWith(null)
),
]).pipe(
map(([isModelPlotEnabled, cardinalityValidationResult]) => {
return isModelPlotEnabled ? cardinalityValidationResult : null;
})
);
}
export function jobIdValidator(jobCreator$: Subject<JobCreator>): Observable<JobExistsResult> {
return jobCreator$.pipe(
map((jobCreator) => {
return jobCreator.jobId;
}),
// No need to perform an API call if the analysis configuration hasn't been changed
distinctUntilChanged((prevJobId, currJobId) => prevJobId === currJobId),
switchMap((jobId) => ml.jobs.jobsExist$([jobId], true)),
map((jobExistsResults) => {
const jobs = Object.values(jobExistsResults);
const valid = jobs?.[0].exists === false;
return {
jobIdExists: {
valid,
...(valid ? {} : { message: jobExistsErrorMessage }),
},
};
})
);
}
export function groupIdsValidator(jobCreator$: Subject<JobCreator>): Observable<GroupsExistResult> {
return jobCreator$.pipe(
map((jobCreator) => jobCreator.groups),
// No need to perform an API call if the analysis configuration hasn't been changed
distinctUntilChanged(
(prevGroups, currGroups) => JSON.stringify(prevGroups) === JSON.stringify(currGroups)
),
switchMap((groups) => {
return ml.jobs.jobsExist$(groups, true);
}),
map((jobExistsResults) => {
const groups = Object.values(jobExistsResults);
// only match jobs that exist but aren't groups.
// as we should allow existing groups to be reused.
const valid = groups.some((g) => g.exists === true && g.isGroup === false) === false;
return {
groupIdsExist: {
valid,
...(valid ? {} : { message: groupExistsErrorMessage }),
},
};
})
);
}

View file

@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import React, { FC, useState, useContext, useEffect } from 'react';
import React, { FC, useState, useContext, useEffect, useMemo } from 'react';
import { EuiComboBox, EuiComboBoxOptionOption } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { JobCreatorContext } from '../../../job_creator_context';
@ -17,7 +17,19 @@ export const GroupsInput: FC = () => {
);
const { existingJobsAndGroups } = useContext(JobCreatorContext);
const [selectedGroups, setSelectedGroups] = useState(jobCreator.groups);
const [validation, setValidation] = useState(jobValidator.groupIds);
const validation = useMemo(() => {
const valid =
jobValidator.groupIds.valid === true &&
jobValidator.latestValidationResult.groupIdsExist?.valid === true;
const message =
jobValidator.groupIds.message ?? jobValidator.latestValidationResult.groupIdsExist?.message;
return {
valid,
message,
};
}, [jobValidatorUpdated]);
useEffect(() => {
jobCreator.groups = selectedGroups;
@ -61,10 +73,6 @@ export const GroupsInput: FC = () => {
setSelectedGroups([...selectedOptions, newGroup].map((g) => g.label));
}
useEffect(() => {
setValidation(jobValidator.groupIds);
}, [jobValidatorUpdated]);
return (
<Description validation={validation}>
<EuiComboBox

View file

@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import React, { FC, useState, useContext, useEffect } from 'react';
import React, { FC, useState, useContext, useEffect, useMemo } from 'react';
import { EuiFieldText } from '@elastic/eui';
import { JobCreatorContext } from '../../../job_creator_context';
import { Description } from './description';
@ -14,21 +14,28 @@ export const JobIdInput: FC = () => {
JobCreatorContext
);
const [jobId, setJobId] = useState(jobCreator.jobId);
const [validation, setValidation] = useState(jobValidator.jobId);
const validation = useMemo(() => {
const isEmptyId = jobId === '';
const valid =
isEmptyId === true ||
(jobValidator.jobId.valid === true &&
jobValidator.latestValidationResult.jobIdExists?.valid === true);
const message =
jobValidator.jobId.message ?? jobValidator.latestValidationResult.jobIdExists?.message;
return {
valid,
message,
};
}, [jobValidatorUpdated]);
useEffect(() => {
jobCreator.jobId = jobId;
jobCreatorUpdate();
}, [jobId]);
useEffect(() => {
const isEmptyId = jobId === '';
setValidation({
valid: isEmptyId === true || jobValidator.jobId.valid,
message: isEmptyId === false ? jobValidator.jobId.message : '',
});
}, [jobValidatorUpdated]);
return (
<Description validation={validation}>
<EuiFieldText

View file

@ -40,6 +40,8 @@ export const JobDetailsStep: FC<Props> = ({
jobValidator.jobId.valid &&
jobValidator.modelMemoryLimit.valid &&
jobValidator.groupIds.valid &&
jobValidator.latestValidationResult.jobIdExists?.valid === true &&
jobValidator.latestValidationResult.groupIdsExist?.valid === true &&
jobValidator.validating === false;
setNextActive(active);
}, [jobValidatorUpdated]);

View file

@ -182,7 +182,7 @@ export const Page: FC<PageProps> = ({ existingJobsAndGroups, jobType }) => {
const chartLoader = new ChartLoader(mlContext.currentIndexPattern, mlContext.combinedQuery);
const jobValidator = new JobValidator(jobCreator, existingJobsAndGroups);
const jobValidator = new JobValidator(jobCreator);
const resultsLoader = new ResultsLoader(jobCreator, chartInterval, chartLoader);

View file

@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { Observable } from 'rxjs';
import { HttpService } from '../http_service';
import { basePath } from './index';
@ -23,6 +24,7 @@ import {
} from '../../../../common/types/categories';
import { CATEGORY_EXAMPLES_VALIDATION_STATUS } from '../../../../common/constants/categorization_job';
import { Category } from '../../../../common/types/categories';
import { JobsExistResponse } from '../../../../common/types/job_service';
export const jobsApiProvider = (httpService: HttpService) => ({
jobsSummary(jobIds: string[]) {
@ -138,9 +140,18 @@ export const jobsApiProvider = (httpService: HttpService) => ({
});
},
jobsExist(jobIds: string[]) {
const body = JSON.stringify({ jobIds });
return httpService.http<any>({
jobsExist(jobIds: string[], allSpaces: boolean = false) {
const body = JSON.stringify({ jobIds, allSpaces });
return httpService.http<JobsExistResponse>({
path: `${basePath()}/jobs/jobs_exist`,
method: 'POST',
body,
});
},
jobsExist$(jobIds: string[], allSpaces: boolean = false): Observable<JobsExistResponse> {
const body = JSON.stringify({ jobIds, allSpaces });
return httpService.http$({
path: `${basePath()}/jobs/jobs_exist`,
method: 'POST',
body,

View file

@ -43,7 +43,7 @@ import { fieldsServiceProvider } from '../fields_service';
import { jobServiceProvider } from '../job_service';
import { resultsServiceProvider } from '../results_service';
import { JobExistResult, JobStat } from '../../../common/types/data_recognizer';
import { MlJobsStatsResponse } from '../job_service/jobs';
import { MlJobsStatsResponse } from '../../../common/types/job_service';
import { JobSavedObjectService } from '../../saved_objects';
const ML_DIR = 'ml';
@ -533,7 +533,7 @@ export class DataRecognizer {
const jobInfo = await this._jobsService.jobsExist(jobIds);
// Check if the value for any of the jobs is false.
const doJobsExist = Object.values(jobInfo).includes(false) === false;
const doJobsExist = Object.values(jobInfo).every((j) => j.exists === true);
results.jobsExist = doJobsExist;
if (doJobsExist === true) {

View file

@ -7,7 +7,7 @@
import { CalendarManager } from '../calendar';
import { GLOBAL_CALENDAR } from '../../../common/constants/calendars';
import { Job } from '../../../common/types/anomaly_detection_jobs';
import { MlJobsResponse } from './jobs';
import { MlJobsResponse } from '../../../common/types/job_service';
import type { MlClient } from '../../lib/ml_client';
interface Group {

View file

@ -16,11 +16,14 @@ import { JOB_STATE, DATAFEED_STATE } from '../../../common/constants/states';
import {
MlSummaryJob,
AuditMessage,
Job,
JobStats,
DatafeedWithStats,
CombinedJobWithStats,
} from '../../../common/types/anomaly_detection_jobs';
import {
MlJobsResponse,
MlJobsStatsResponse,
JobsExistResponse,
} from '../../../common/types/job_service';
import { GLOBAL_CALENDAR } from '../../../common/constants/calendars';
import { datafeedsProvider, MlDatafeedsResponse, MlDatafeedsStatsResponse } from './datafeeds';
import { jobAuditMessagesProvider } from '../job_audit_messages';
@ -34,16 +37,6 @@ import {
import { groupsProvider } from './groups';
import type { MlClient } from '../../lib/ml_client';
export interface MlJobsResponse {
jobs: Job[];
count: number;
}
export interface MlJobsStatsResponse {
jobs: JobStats[];
count: number;
}
interface Results {
[id: string]: {
[status: string]: boolean;
@ -420,10 +413,18 @@ export function jobsProvider(client: IScopedClusterClient, mlClient: MlClient) {
// Checks if each of the jobs in the specified list of IDs exist.
// Job IDs in supplied array may contain wildcard '*' characters
// e.g. *_low_request_rate_ecs
async function jobsExist(jobIds: string[] = [], allSpaces: boolean = false) {
const results: { [id: string]: boolean } = {};
async function jobsExist(
jobIds: string[] = [],
allSpaces: boolean = false
): Promise<JobsExistResponse> {
const results: JobsExistResponse = {};
for (const jobId of jobIds) {
try {
if (jobId === '') {
results[jobId] = { exists: false, isGroup: false };
continue;
}
const { body } = allSpaces
? await client.asInternalUser.ml.getJobs<MlJobsResponse>({
job_id: jobId,
@ -431,13 +432,15 @@ export function jobsProvider(client: IScopedClusterClient, mlClient: MlClient) {
: await mlClient.getJobs<MlJobsResponse>({
job_id: jobId,
});
results[jobId] = body.count > 0;
const isGroup = body.jobs.some((j) => j.groups !== undefined && j.groups.includes(jobId));
results[jobId] = { exists: body.count > 0, isGroup };
} catch (e) {
// if a non-wildcarded job id is supplied, the get jobs endpoint will 404
if (e.statusCode !== 404) {
throw e;
}
results[jobId] = false;
results[jobId] = { exists: false, isGroup: false };
}
}
return results;

View file

@ -17,7 +17,7 @@ import {
} from '../../../common/types/anomalies';
import { JOB_ID, PARTITION_FIELD_VALUE } from '../../../common/constants/anomalies';
import { GetStoppedPartitionResult } from '../../../common/types/results';
import { MlJobsResponse } from '../job_service/jobs';
import { MlJobsResponse } from '../../../common/types/job_service';
import type { MlClient } from '../../lib/ml_client';
// Service for carrying out Elasticsearch queries to obtain data for the

View file

@ -17,6 +17,7 @@ export default ({ getService }: FtrProviderContext) => {
const jobIdSpace1 = 'fq_single_space1';
const jobIdSpace2 = 'fq_single_space2';
const groupSpace1 = 'farequote';
const idSpace1 = 'space1';
const idSpace2 = 'space2';
@ -57,17 +58,25 @@ export default ({ getService }: FtrProviderContext) => {
it('should find single job from same space', async () => {
const body = await runRequest(idSpace1, 200, [jobIdSpace1]);
expect(body).to.eql({ [jobIdSpace1]: true });
expect(body).to.eql({ [jobIdSpace1]: { exists: true, isGroup: false } });
});
it('should find single job from same space', async () => {
const body = await runRequest(idSpace1, 200, [groupSpace1]);
expect(body).to.eql({ [groupSpace1]: { exists: true, isGroup: true } });
});
it('should not find single job from different space', async () => {
const body = await runRequest(idSpace2, 200, [jobIdSpace1]);
expect(body).to.eql({ [jobIdSpace1]: false });
expect(body).to.eql({ [jobIdSpace1]: { exists: false, isGroup: false } });
});
it('should only find job from same space when called with a list of jobs', async () => {
const body = await runRequest(idSpace1, 200, [jobIdSpace1, jobIdSpace2]);
expect(body).to.eql({ [jobIdSpace1]: true, [jobIdSpace2]: false });
expect(body).to.eql({
[jobIdSpace1]: { exists: true, isGroup: false },
[jobIdSpace2]: { exists: false, isGroup: false },
});
});
});
};