Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non blocker #202

Merged
merged 4 commits into from
Mar 9, 2022
Merged

Non blocker #202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a24455
added intergration with non-blocker validation
amitgalitz Mar 7, 2022
506e146
remove unneeded log statements
amitgalitz Mar 7, 2022
63cdb1a
addressed comments
amitgalitz Mar 8, 2022
c2fc4e9
removed log statement
amitgalitz Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions public/models/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,11 @@ export type EntityOptionsMap = {
export type ValidationModelResponse = {
message: String;
sub_issues?: { [key: string]: string };
validationType: string;
};

export interface ValidationSettingResponse {
issueType: string;
message: string;
validationType: string;
}
1 change: 0 additions & 1 deletion public/pages/DefineDetector/utils/constants.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import { FILTER_TYPES, UIFilter } from '../../../models/interfaces';
import { OPERATORS_MAP } from '../../DefineDetector/components/DataFilterList/utils/constant';
import { DetectorDefinitionFormikValues } from '../../DefineDetector/models/interfaces';

export const EMPTY_UI_FILTER: UIFilter = {
filterType: FILTER_TYPES.SIMPLE,
fieldInfo: [],
Expand Down
48 changes: 33 additions & 15 deletions ...ic/pages/ReviewAndCreate/components/DetectorDefinitionFields/DetectorDefinitionFields.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,14 @@ import {
EuiText,
} from '@elastic/eui';
import React from 'react';
import { get } from 'lodash';
import { get, isEqual } from 'lodash';
import {
Detector,
ValidationSettingResponse,
} from '../../../../models/interfaces';
import { FilterDisplayList } from '../FilterDisplayList';
import { ConfigCell, FixedWidthRow } from '../../../../components/ConfigCell';
import { toStringConfigCell } from '../../utils/helpers';

interface DetectorDefinitionFieldsProps {
detector: Detector;
onEditDetectorDefinition(): void;
Expand Down Expand Up @@ -94,19 +93,38 @@ export const DetectorDefinitionFields = (
!props.validDetectorSettings &&
props.validationResponse.hasOwnProperty('message')
) {
return (
<EuiCallOut
title="Issues found in the detector settings"
color="danger"
iconType="alert"
size="s"
style={{ marginBottom: '10px' }}
>
<ul>
<li>{props.validationResponse.message}</li>
</ul>
</EuiCallOut>
);
if (
isEqual(get(props, 'validationResponse.validationType', ''), 'model')
) {
return (
<EuiCallOut
title="We identified some areas that might improve your model"
color="warning"
iconType="iInCircle"
size="s"
style={{ marginBottom: '10px' }}
>
{JSON.stringify(props.validationResponse.message).replace(
ohltyler marked this conversation as resolved.
Show resolved Hide resolved
/\"/g,
''
)}
</EuiCallOut>
);
} else {
return (
<EuiCallOut
title="Issues found in the detector settings"
color="danger"
iconType="alert"
size="s"
style={{ marginBottom: '10px' }}
>
<ul>
<li>{props.validationResponse.message}</li>
</ul>
</EuiCallOut>
);
}
} else {
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion public/pages/ReviewAndCreate/components/FilterDisplayList/FilterDisplayList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const FilterDisplayList = (props: FilterDisplayListProps) => {
let filters = get(props, 'uiMetadata.filters', []);
const oldFilterType = get(props, 'uiMetadata.filterType', undefined);

// We want to show the custom filter if filters is empty and
// We want to show the custom filter if filters is empty and
// props.filterQuery isn't empty.
// Two possible situations for the if branch:
// First, old detectors with custom filters will have no filter list, but
Expand Down
66 changes: 45 additions & 21 deletions ...ic/pages/ReviewAndCreate/components/ModelConfigurationFields/ModelConfigurationFields.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
FeatureAttributes,
ValidationModelResponse,
} from '../../../../models/interfaces';
import { get, sortBy } from 'lodash';
import { get, sortBy, isEqual } from 'lodash';
import ContentPanel from '../../../../components/ContentPanel/ContentPanel';
import { CodeModal } from '../../../../components/CodeModal/CodeModal';
import { AdditionalSettings } from '../AdditionalSettings/AdditionalSettings';
Expand Down Expand Up @@ -253,26 +253,50 @@ export const ModelConfigurationFields = (
!props.validModel &&
props.validationFeatureResponse.hasOwnProperty('message')
) {
return (
<EuiCallOut
title="issues found in the model configuration"
color="danger"
iconType="alert"
size="s"
style={{ marginBottom: '10px' }}
>
{/* Callout can either display feature subissue which related to a specific
feature issue or display a feature_attribute issue that is general like
more then x anomaly feature or dulicate feature names */}
{props.validationFeatureResponse.hasOwnProperty('sub_issues') ? (
getFeatureValidationCallout(props.validationFeatureResponse)
) : (
<ul>
<li>{JSON.stringify(props.validationFeatureResponse.message)}</li>
</ul>
)}
</EuiCallOut>
);
if (
isEqual(
get(props, 'validationFeatureResponse.validationType', ''),
'model'
)
) {
return (
<EuiCallOut
title="We identified some areas that might improve your model"
color="warning"
iconType="iInCircle"
size="s"
style={{ marginBottom: '10px' }}
>
{JSON.stringify(props.validationFeatureResponse.message).replace(
/\"/g,
''
)}
</EuiCallOut>
);
} else {
return (
<EuiCallOut
title="Issues found in the model configuration"
color="danger"
iconType="alert"
size="s"
style={{ marginBottom: '10px' }}
>
{/* Callout can either display feature subissue which related to a specific
feature issue or display a feature_attribute issue that is general like
more then x anomaly feature or dulicate feature names */}
{props.validationFeatureResponse.hasOwnProperty('sub_issues') ? (
getFeatureValidationCallout(props.validationFeatureResponse)
) : (
<ul>
<li>
{JSON.stringify(props.validationFeatureResponse.message)}
</li>
</ul>
)}
</EuiCallOut>
);
}
} else {
return null;
}
Expand Down
38 changes: 28 additions & 10 deletions public/pages/ReviewAndCreate/containers/ReviewAndCreate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import {
EuiTitle,
EuiButtonEmpty,
EuiSpacer,
EuiCallOut,
} from '@elastic/eui';
import {
createDetector,
Expand All @@ -30,7 +29,7 @@ import {
validateDetector,
} from '../../../redux/reducers/ad';
import { Formik, FormikHelpers } from 'formik';
import { get } from 'lodash';
import { get, isEmpty } from 'lodash';
import React, { Fragment, useEffect, useState } from 'react';
import { RouteComponentProps } from 'react-router';
import { useDispatch, useSelector } from 'react-redux';
Expand All @@ -54,7 +53,6 @@ import {
ValidationSettingResponse,
VALIDATION_ISSUE_TYPES,
} from '../../../models/interfaces';
import { isEmpty } from 'lodash';

interface ReviewAndCreateProps extends RouteComponentProps {
setStep(stepNumber: number): void;
Expand Down Expand Up @@ -92,28 +90,49 @@ export function ReviewAndCreate(props: ReviewAndCreateProps) {
// meaning validation has passed and succesful callout will display or validation has failed
// and callouts displaying what the issue is will be displayed instead.
useEffect(() => {
dispatch(validateDetector(formikToDetector(props.values)))
dispatch(validateDetector(formikToDetector(props.values), 'model'))
.then((resp: any) => {
if (isEmpty(Object.keys(resp.response))) {
setValidDetectorSettings(true);
setValidModelConfigurations(true);
} else {
if (resp.response.hasOwnProperty('detector')) {
const issueType = Object.keys(resp.response.detector)[0];
if (resp.response.detector[issueType].hasOwnProperty('message')) {
if (
resp.response.hasOwnProperty('detector') ||
resp.response.hasOwnProperty('model')
) {
const validationType = Object.keys(resp.response)[0];
const issueType = Object.keys(resp.response[validationType])[0];
if (
resp.response[validationType][issueType].hasOwnProperty('message')
) {
const validationMessage =
resp.response.detector[issueType].message;
resp.response[validationType][issueType].message;
const detectorSettingIssue: ValidationSettingResponse = {
issueType: issueType,
message: validationMessage,
validationType: validationType,
};

// These issue types only come up during non-blocker validation after blocker validation has passed
// This means that the configurations don't have any blocking issues but request either timed out during
// non blocking validation or due to an issue in core. This means we aren't able to provide any recommendation
// and user has no way of re-trying except re-rendering page which isn't straightforward. At the moment we will
// hide these failures instead of explaining both levels of validation being done in the backend.
if (issueType == 'aggregation' || issueType == 'timeout') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want to remove the warning toast here? What does an aggregation issue type mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this is due to the fact that if there is an aggregation issue then it might be an issue that arouse from a change in core and user won't have much they can do about it and for timeout they also can't really re-try since action happens automatically when entering ReviewAndCreate page so I thought that I could just make it basically invisible to users instead of additionally giving a warning toast that will confuse them on there next action.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this same if block, I see it sets the variables indicating valid detector settings and model configs, when in fact that might not be true if there was a problem performing the queries, or if it timed out. It seems like a bad user experience to see it showing as valid in the callouts, then failing later when actually creating the detector, or trying to start it.

How about having a separate callout text indicating that validation failed to run? It could be generic so that it covers both aggregation and timeout failures. I think showing the toast with more details on the failure (aggregation or timeout) could be helpful to the user, and would allow for quick user feedback in case there is something wrong with the AD code that we would need to fix (e.g., aggregation failure where the queries being made in validation API call need to be updated after core changes something internal).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed separately, these issue types only come up during non-blocker validation, after blocker validation has passed. So in that case it is mostly validated, just not fully validated / not able to provide any recommendations in case there is data sparsity due to any of the configs / features.

I think the 2 options are (1) hide these failures like you're doing now, or (2) find a way to tell the user that blocking validation passed, but non-blocking validation failed to run properly. For simplicity, I think its ok to go with option 1 for this release, and sync with UX team on a solution to option 2 later on.

Suggest to update the comments on line 117 to reflect something along the lines of this

setValidDetectorSettings(true);
setValidModelConfigurations(true);
return;
}

switch (issueType) {
// need to handle model validation issue case seperatly
case VALIDATION_ISSUE_TYPES.FEATURE_ATTRIBUTES:
case VALIDATION_ISSUE_TYPES.CATEGORY:
case VALIDATION_ISSUE_TYPES.SHINGLE_SIZE_FIELD:
const modelResp = resp.response.detector[
const modelResp = resp.response[validationType][
issueType
] as ValidationModelResponse;
modelResp.validationType = validationType;
setFeatureResponse(modelResp);
setValidDetectorSettings(true);
setValidModelConfigurations(false);
Expand Down Expand Up @@ -313,7 +332,6 @@ export function ReviewAndCreate(props: ReviewAndCreateProps) {
iconType="arrowLeft"
fill={false}
data-test-subj="reviewAndCreatePreviousButton"
//@ts-ignore
onClick={() => {
props.setStep(3);
}}
Expand Down
7 changes: 5 additions & 2 deletions public/redux/reducers/ad.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,10 +377,13 @@ export const createDetector = (requestBody: Detector): APIAction => ({
}),
});

export const validateDetector = (requestBody: Detector): APIAction => ({
export const validateDetector = (
requestBody: Detector,
validationType: string
): APIAction => ({
type: VALIDATE_DETECTOR,
request: (client: HttpSetup) =>
client.post(`..${AD_NODE_API.DETECTOR}/_validate`, {
client.post(`..${AD_NODE_API.DETECTOR}/_validate/${validationType}`, {
body: JSON.stringify(requestBody),
}),
});
Expand Down
8 changes: 7 additions & 1 deletion server/cluster/ad/adPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,13 @@ export default function adPlugin(Client: any, config: any, components: any) {
});
ad.validateDetector = ca({
url: {
fmt: `${API.DETECTOR_BASE}/_validate`,
fmt: `${API.DETECTOR_BASE}/_validate/<%=validationType%>`,
req: {
validationType: {
type: 'string',
required: true,
},
},
},
needBody: true,
method: 'POST',
Expand Down
9 changes: 8 additions & 1 deletion server/routes/ad.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,10 @@ export function registerADRoutes(apiRouter: Router, adService: AdService) {
'/detectors/{detectorId}/_topAnomalies/{isHistorical}',
adService.getTopAnomalyResults
);
apiRouter.post('/detectors/_validate', adService.validateDetector);
apiRouter.post(
'/detectors/_validate/{validationType}',
adService.validateDetector
);
}

export default class AdService {
Expand Down Expand Up @@ -230,13 +233,17 @@ export default class AdService {
opensearchDashboardsResponse: OpenSearchDashboardsResponseFactory
): Promise<IOpenSearchDashboardsResponse<any>> => {
try {
let { validationType } = request.params as {
validationType: string;
};
const requestBody = JSON.stringify(
convertPreviewInputKeysToSnakeCase(request.body)
);
const response = await this.client
.asScoped(request)
.callAsCurrentUser('ad.validateDetector', {
body: requestBody,
validationType: validationType,
});
return opensearchDashboardsResponse.ok({
body: {
Expand Down