Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Adds window size as advanced setting in model configuration. #287

Merged
merged 1 commit into from
Sep 4, 2020
Merged

Adds window size as advanced setting in model configuration. #287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions public/models/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ export type Detector = {
featureAttributes: FeatureAttributes[];
windowDelay: { period: Schedule };
detectionInterval: { period: Schedule };
shingleSize: number;
uiMetadata: UiMetaData;
lastUpdateTime: number;
enabled?: boolean;
Expand Down
11 changes: 11 additions & 0 deletions public/pages/DetectorConfig/DetectorConfig.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,14 @@
line-height: 24px;
text-align: center;
}

.header-single-value-euiBasicTable {
.euiTableHeaderCell, .euiTableRowCell {
border: 0;
.euiTableCellContent {
padding-top: 0;
padding-bottom: 0;
}
}

}
64 changes: 45 additions & 19 deletions public/pages/DetectorConfig/containers/Features.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,15 @@ import {
EuiIcon,
EuiButton,
EuiEmptyPrompt,
EuiSpacer,
} from '@elastic/eui';
import {
Detector,
FEATURE_TYPE,
FeatureAttributes,
} from '../../../models/interfaces';
import { get, sortBy } from 'lodash';
import { PLUGIN_NAME } from '../../../utils/constants';
import { PLUGIN_NAME, SHINGLE_SIZE } from '../../../utils/constants';
import ContentPanel from '../../../components/ContentPanel/ContentPanel';
import { CodeModal } from '../components/CodeModal/CodeModal';
import { getTitleWithCount } from '../../../utils/utils';
Expand Down Expand Up @@ -93,6 +94,7 @@ export class Features extends Component<FeaturesProps, FeaturesState> {

public render() {
const featureAttributes = get(this.props.detector, 'featureAttributes', []);
const shingleSize = get(this.props.detector, 'shingleSize', SHINGLE_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is shingleSize optional for backend Java Detector class? If yes, suggest to set shingleSize=8 for java detector instance.
So if we change the default SHINGLE_SIZE in future, we don't need to worry about the backward compatibility about old detectors. For example, user create detector with default SHINGLE_SIZE 8 now, later we change the default SHINGLE_SIZE as 4. If we don't set this detector's shingle size as 8 in data model, we may reset it as 4, and model will fail as checkpoint can't match current detector shingle size configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you clarify what change you're suggesting in the frontend code? Are you suggesting the SHINGLE_SIZE constant be removed from the frontend?

The shingleSize parameter is optional in the API, and the backend already defaults the shingle size to 8 if the parameter is not provided in the request. However, this change will only be applied to detectors which get created or updated after this backend change was made.

The reason the default shingle size is also specified in the frontend is precisely for backwards-compatibility. Detectors that were created prior to the recent backend change and have not been updated since then will not have a "shingleSize" field in the stored detector configuration, and it would not be a good user experience to see a "null" or blank in the Window Size field when they view their model configurations in the UI. Even if there is no "shingleSize" field stored on the detector configuration, the user should be informed by the UI that a shingle size of 8 is being used for that detector. In addition, the shingle size needs to be known by the frontend to calculate detector initialization time (see isDetectorInitOverTime). (The SHINGLE_SIZE constant was actually already existing in the code for this reason. This constant did not get added in this change.)

If they save any changes on the model configuration page for the existing detector, even if they didn't change the window size, going forward the "shingleSize" field will get stored on the detector configuration for the future, but until all existing detectors have been updated to have that field, the default value is needed to ensure the model configuration display the correct value and calculates the correct detector initialization time.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said "suggest to set shingleSize=8 for java detector instance." now you have already set it as 8, "the backend already defaults the shingle size to 8 if the parameter is not provided in the request". So that will be ok.


const sorting = {
sort: {
Expand Down Expand Up @@ -170,7 +172,7 @@ export class Features extends Component<FeaturesProps, FeaturesState> {
},
{
field: 'state',
name: 'State',
name: 'Feature state',
},
];

Expand All @@ -182,17 +184,21 @@ export class Features extends Component<FeaturesProps, FeaturesState> {

const featureNum = Object.keys(featureAttributes).length;

const setParamsText = `Set the index fields that you want to find anomalies for by defining
the model features. You can also set other model parameters such as
window size.`

const previewText = `After you set the model features and other optional parameters, you can
preview your anomalies from a sample feature output.`
Comment on lines +187 to +192
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these wordings reviewed by tech writer?

Also, since those are constants, you may name it like SET_PARAMS_TEXT = '....'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wording is based exactly on the UX mockups. I can ask the UX designer if the mockups were reviewed by a tech writer.

Sure, I will change the variable names for these constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The UX designer confirmed that the mockups have been reviewed by a tech writer.


return (
<ContentPanel
title={getTitleWithCount('Features', featureNum)}
title="Model configuration"
titleSize="s"
subTitle={
<EuiText className="anomaly-distribution-subtitle">
<p>
Specify index fields that you want to find anomalies for by
defining features. A detector can discover anomalies for up to 5
features. Once you define the features, you can preview your
anomalies from a sample feature output.{' '}
{`${setParamsText} ${previewText} `}
<EuiLink
href="https://opendistro.github.io/for-elasticsearch-docs/docs/ad/"
target="_blank"
Expand All @@ -211,14 +217,15 @@ export class Features extends Component<FeaturesProps, FeaturesState> {
<EuiEmptyPrompt
title={
<span className="emptyFeatureTitle">
Features are required to run a detector
Model parameters are required to run a detector
</span>
}
body={
<EuiText className="emptyFeatureBody">
Specify index fields that you want to find anomalies for by
defining features. Once you define the features, you can preview
your anomalies from a sample feature output.
{setParamsText}
<br/>
<br/>
{previewText}
</EuiText>
}
actions={[
Expand All @@ -227,18 +234,37 @@ export class Features extends Component<FeaturesProps, FeaturesState> {
href={`${PLUGIN_NAME}#/detectors/${this.props.detectorId}/features`}
fill
>
Add feature
Configure model
</EuiButton>,
]}
/>
) : (
<EuiBasicTable
items={sortedItems}
columns={columns}
cellProps={getCellProps}
sorting={sorting}
onChange={this.handleTableChange}
/>
<div>
<ContentPanel
title={getTitleWithCount('Features', featureNum)}
titleSize="s"
>
<EuiBasicTable
items={sortedItems}
columns={columns}
cellProps={getCellProps}
sorting={sorting}
onChange={this.handleTableChange}
/>
</ContentPanel>
<EuiSpacer size="m"/>
<ContentPanel
title="Additional settings"
titleSize="s"
>
<EuiBasicTable
Copy link
Contributor

Choose a reason for hiding this comment

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

We use EuiBasicTable as we have multiple features, so we can put features in separate row. Should we use the same UX design for Advanced setting ? How about use the similar design of Detector configuration ? Can you confirm with UX designer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll reach out to the UX designer to clarify. In the mockups, the Advanced setting section of this panel is stylistically identical to the features table in this panel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I synced up with the UX designer, and we will change the Advanced settings to be a content panel nested inside the Model configuration panel rather than a table.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, content panel should be good. Be sure to put both features and advanced setting to nested content panels.

className="header-single-value-euiBasicTable"
items={[{ windowSize: shingleSize }]}
columns={[{ field: 'windowSize', name: 'Window size'}]}
cellProps={getCellProps}
/>
</ContentPanel>
</div>
)}
</ContentPanel>
);
Expand Down
3 changes: 0 additions & 3 deletions public/pages/DetectorConfig/containers/MetaData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,8 @@ import {
EuiButton,
EuiFormRowProps,
} from '@elastic/eui';
import { PLUGIN_NAME } from '../../../utils/constants';
import {
Detector,
Schedule,
UiMetaData,
FILTER_TYPES,
UIFilter,
} from '../../../models/interfaces';
Expand Down
17 changes: 8 additions & 9 deletions public/pages/DetectorConfig/containers/__tests__/DetectorConfig.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import {
render,
fireEvent,
wait,
waitForElement,
} from '@testing-library/react';
// @ts-ignore
import { toastNotifications } from 'ui/notify';
Expand All @@ -41,7 +40,6 @@ import {
} from '../../../../models/interfaces';
import {
getRandomDetector,
getRandomFeature,
} from '../../../../redux/reducers/__tests__/utils';
import configureStore from '../../../../redux/configureStore';
import { httpClientMock } from '../../../../../test/mocks';
Expand Down Expand Up @@ -161,14 +159,15 @@ describe('<DetectorConfig /> spec', () => {
...getRandomDetector(true),
uiMetadata: {} as UiMetaData,
featureAttributes: [],
shingleSize: 8,
};
const { getByText } = renderWithRouter(randomDetector);
const { getByText, queryByText } = renderWithRouter(randomDetector);
await wait(() => {
getByText('Features are required to run a detector');
getByText(
'Specify index fields that you want to find anomalies for by defining features. Once you define the features, you can preview your anomalies from a sample feature output.'
getByText('Model parameters are required to run a detector');
queryByText(
'Set the index fields'
);
getByText('Features');
getByText('Model configuration');
getByText(randomDetector.name);
getByText(randomDetector.indices[0]);
getByText(toString(randomDetector.detectionInterval));
Expand All @@ -178,8 +177,8 @@ describe('<DetectorConfig /> spec', () => {
getByText(randomDetector.description);
// filter should be -
getByText('-');
getByText(
'Specify index fields that you want to find anomalies for by defining features. A detector can discover anomalies for up to 5 features. Once you define the features, you can preview your anomalies from a sample feature output.'
queryByText(
'Set the index fields'
);
});
});
Expand Down
2 changes: 1 addition & 1 deletion public/pages/DetectorDetail/components/DetectorControls/DetectorControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export const DetectorControls = (props: DetectorControls) => {
data-test-subj="editFeature"
onClick={props.onEditFeatures}
>
Edit features
Edit model configuration
</EuiContextMenuItem>

<EuiContextMenuItem
Expand Down
3 changes: 2 additions & 1 deletion public/pages/DetectorResults/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
} from './constants';
import { DETECTOR_STATE, SHINGLE_SIZE } from '../../../utils/constants';
import moment, { Moment } from 'moment';
import { get } from 'lodash';
import { DETECTOR_INIT_FAILURES } from '../../../pages/DetectorDetail/utils/constants';

export const IS_INIT_OVERTIME_FIELD = 'isInitOvertime';
Expand Down Expand Up @@ -50,7 +51,7 @@ const isDetectorInitOverTime = (currentTime: Moment, detector: Detector) => {
//@ts-ignore
currentTime
.subtract(
SHINGLE_SIZE * detector.detectionInterval.period.interval,
get(detector, 'shingleSize', SHINGLE_SIZE) * detector.detectionInterval.period.interval,
detector.detectionInterval.period.unit.toLowerCase()
)
//@ts-ignore
Expand Down
Loading