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.

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

[ML] Anomaly Detection: Annotations enhancements #70198

Merged
merged 34 commits into from
Jul 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
ff3f180
[ML] Improve Annotations
qn895 Jun 29, 2020
9ffddf3
[ML] Clean up logs & update snapshot
qn895 Jun 29, 2020
e517350
[ML] Add unique ID
qn895 Jun 29, 2020
20ce417
[ML] Add user(0) if no bucket for user found
qn895 Jun 29, 2020
2501b5f
[ML] Add dynamic columns to annotation table
qn895 Jun 29, 2020
e0ce77f
[ML] Refactor unique aggregations to bundle with getAnnotations
qn895 Jun 30, 2020
f96e042
[ML] Fix type issue & aria obj warnings
qn895 Jun 30, 2020
4877939
Merge remote-tracking branch 'upstream/master' into transform-annotat…
qn895 Jun 30, 2020
ffd474a
[ML] Update snapshot for annotation_table
qn895 Jun 30, 2020
2ee31d4
[ML] Add EUI panel around Annotations in Anomaly Explorer
qn895 Jul 1, 2020
36a5a44
[ML] Refactor ANNOTATION_EVENT_USER & minor fixes
qn895 Jul 1, 2020
def94c3
[ML] Fix annotations_table queryText
qn895 Jul 2, 2020
bbdbec6
[ML] Fix types of annotation fields & add Total badge
qn895 Jul 2, 2020
55aa5c8
[ML] Add internationalization to series
qn895 Jul 2, 2020
a56fa66
[ML] Fix partition field type
qn895 Jul 2, 2020
07e396d
[ML] Remove Annotations panel if no focus annotations data
qn895 Jul 2, 2020
002eaf3
[ML] Remove annotations in SMV if no annotations
qn895 Jul 2, 2020
5b6249d
[ML] Refactor to make the toggle button visibly active
qn895 Jul 2, 2020
010c096
[ML] Refactor to make the toggle button visibly active
qn895 Jul 2, 2020
0d945fa
[ML] Update snapshot for annotations_table
qn895 Jul 2, 2020
b69be14
Merge remote-tracking branch 'upstream/master' into transform-annotat…
qn895 Jul 8, 2020
1f2684d
[ML] Pete's feedback
qn895 Jul 8, 2020
e57889f
[ML] Update snapshot, fix i18n, persist event field when editing
qn895 Jul 8, 2020
dfe3950
[ML] Remove partitions if apply to series is unchecked
qn895 Jul 9, 2020
aa98817
[ML] Dynamic partition columns for SMV & default query to include del…
qn895 Jul 9, 2020
02024e5
[ML] Fix annotation table in job list not having terms
qn895 Jul 9, 2020
0e24cb3
[ML] Change SMV annotations to only show series + non-partitioned
qn895 Jul 9, 2020
687fed9
Merge branch 'master' into transform-annotations-enhancements
elasticmachine Jul 13, 2020
de5e991
Merge remote-tracking branch 'upstream/master' into transform-annotat…
qn895 Jul 13, 2020
fe58c7d
Merge remote-tracking branch 'upstream/master' into transform-annotat…
qn895 Jul 13, 2020
c06b83b
Merge remote-tracking branch 'upstream/master' into transform-annotat…
qn895 Jul 13, 2020
4871783
[ML] Fix issue with EUI upgrade and current_series table
qn895 Jul 14, 2020
ad7a947
[ML] Update snapshot for annotations_table
qn895 Jul 14, 2020
00b6c88
Merge remote-tracking branch 'upstream/master' into transform-annotat…
qn895 Jul 14, 2020
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
3 changes: 3 additions & 0 deletions x-pack/plugins/ml/common/constants/annotations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,6 @@ export const ANNOTATION_USER_UNKNOWN = '<user unknown>';

// UI enforced limit to the maximum number of characters that can be entered for an annotation.
export const ANNOTATION_MAX_LENGTH_CHARS = 1000;

export const ANNOTATION_EVENT_USER = 'user';
export const ANNOTATION_EVENT_DELAYED_DATA = 'delayed_data';
2 changes: 2 additions & 0 deletions x-pack/plugins/ml/common/constants/anomalies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,5 @@ export enum ANOMALY_THRESHOLD {
WARNING = 3,
LOW = 0,
}

export const PARTITION_FIELDS = ['partition_field', 'over_field', 'by_field'] as const;
45 changes: 44 additions & 1 deletion x-pack/plugins/ml/common/types/annotations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,20 @@
// ]
// }

import { PartitionFieldsType } from './anomalies';
import { ANNOTATION_TYPE } from '../constants/annotations';

export type AnnotationFieldName = 'partition_field_name' | 'over_field_name' | 'by_field_name';
export type AnnotationFieldValue = 'partition_field_value' | 'over_field_value' | 'by_field_value';

export function getAnnotationFieldName(fieldType: PartitionFieldsType): AnnotationFieldName {
return `${fieldType}_name` as AnnotationFieldName;
}

export function getAnnotationFieldValue(fieldType: PartitionFieldsType): AnnotationFieldValue {
return `${fieldType}_value` as AnnotationFieldValue;
}

export interface Annotation {
_id?: string;
create_time?: number;
Expand All @@ -73,8 +85,15 @@ export interface Annotation {
annotation: string;
job_id: string;
type: ANNOTATION_TYPE.ANNOTATION | ANNOTATION_TYPE.COMMENT;
event?: string;
walterra marked this conversation as resolved.
Show resolved Hide resolved
detector_index?: number;
partition_field_name?: string;
partition_field_value?: string;
over_field_name?: string;
over_field_value?: string;
by_field_name?: string;
by_field_value?: string;
}

export function isAnnotation(arg: any): arg is Annotation {
return (
arg.timestamp !== undefined &&
Expand All @@ -93,3 +112,27 @@ export function isAnnotations(arg: any): arg is Annotations {
}
return arg.every((d: Annotation) => isAnnotation(d));
}

export interface FieldToBucket {
Copy link
Contributor

Choose a reason for hiding this comment

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

The interfaces defined here will be applicable for other types than annotations I'm sure, but till we hit that use case, probably makes sense to leave them defined in here. I had a quick look to see if I could find something similar in our existing types, and the Kibana data plugin, but couldn't find anything.

field: string;
missing?: string | number;
}

export interface FieldToBucketResult {
key: string;
doc_count: number;
}

export interface TermAggregationResult {
doc_count_error_upper_bound: number;
sum_other_doc_count: number;
buckets: FieldToBucketResult[];
}

export type EsAggregationResult = Record<string, TermAggregationResult>;

export interface GetAnnotationsResponse {
aggregations?: EsAggregationResult;
annotations: Record<string, Annotations>;
success: boolean;
}
4 changes: 4 additions & 0 deletions x-pack/plugins/ml/common/types/anomalies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { PARTITION_FIELDS } from '../constants/anomalies';

export interface Influencer {
influencer_field_name: string;
influencer_field_values: string[];
Expand Down Expand Up @@ -53,3 +55,5 @@ export interface AnomaliesTableRecord {
typicalSort?: any;
metricDescriptionSort?: number;
}

export type PartitionFieldsType = typeof PARTITION_FIELDS[number];
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ import { formatHumanReadableDateTimeSeconds } from '../../../util/date_utils';

interface Props {
annotation: Annotation;
detectorDescription?: string;
}

export const AnnotationDescriptionList = ({ annotation }: Props) => {
export const AnnotationDescriptionList = ({ annotation, detectorDescription }: Props) => {
const listItems = [
{
title: i18n.translate('xpack.ml.timeSeriesExplorer.annotationDescriptionList.jobIdTitle', {
Expand Down Expand Up @@ -81,6 +82,33 @@ export const AnnotationDescriptionList = ({ annotation }: Props) => {
description: annotation.modified_username,
});
}
if (detectorDescription !== undefined) {
listItems.push({
title: i18n.translate('xpack.ml.timeSeriesExplorer.annotationDescriptionList.detectorTitle', {
defaultMessage: 'Detector',
}),
description: detectorDescription,
});
}

if (annotation.partition_field_name !== undefined) {
listItems.push({
title: annotation.partition_field_name,
description: annotation.partition_field_value,
});
}
if (annotation.over_field_name !== undefined) {
listItems.push({
title: annotation.over_field_name,
description: annotation.over_field_value,
});
}
if (annotation.by_field_name !== undefined) {
listItems.push({
title: annotation.by_field_name,
description: annotation.by_field_value,
});
}

return (
<EuiDescriptionList
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import React, { Component, Fragment, FC, ReactNode } from 'react';
import useObservable from 'react-use/lib/useObservable';
import * as Rx from 'rxjs';
import { cloneDeep } from 'lodash';

import {
EuiButton,
Expand All @@ -21,35 +22,62 @@ import {
EuiSpacer,
EuiTextArea,
EuiTitle,
EuiCheckbox,
} from '@elastic/eui';

import { CommonProps } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';

import { ANNOTATION_MAX_LENGTH_CHARS } from '../../../../../common/constants/annotations';
import {
ANNOTATION_MAX_LENGTH_CHARS,
ANNOTATION_EVENT_USER,
} from '../../../../../common/constants/annotations';
import {
annotation$,
annotationsRefreshed,
AnnotationState,
} from '../../../services/annotations_service';
import { AnnotationDescriptionList } from '../annotation_description_list';
import { DeleteAnnotationModal } from '../delete_annotation_modal';

import { ml } from '../../../services/ml_api_service';
import { getToastNotifications } from '../../../util/dependency_cache';
import {
getAnnotationFieldName,
getAnnotationFieldValue,
} from '../../../../../common/types/annotations';
import { PartitionFieldsType } from '../../../../../common/types/anomalies';
import { PARTITION_FIELDS } from '../../../../../common/constants/anomalies';

interface ViewableDetector {
index: number;
detector_description: string;
}

interface Entity {
fieldName: string;
fieldType: string;
fieldValue: string;
}

interface Props {
annotation: AnnotationState;
chartDetails: {
entityData: { entities: Entity[] };
functionLabel: string;
};
detectorIndex: number;
detectors: ViewableDetector[];
}

interface State {
isDeleteModalVisible: boolean;
applyAnnotationToSeries: boolean;
}

class AnnotationFlyoutUI extends Component<CommonProps & Props> {
public state: State = {
isDeleteModalVisible: false,
applyAnnotationToSeries: true,
};

public annotationSub: Rx.Subscription | null = null;
Expand Down Expand Up @@ -150,11 +178,31 @@ class AnnotationFlyoutUI extends Component<CommonProps & Props> {
};

public saveOrUpdateAnnotation = () => {
const { annotation } = this.props;

if (annotation === null) {
const { annotation: originalAnnotation, chartDetails, detectorIndex } = this.props;
if (originalAnnotation === null) {
return;
}
const annotation = cloneDeep(originalAnnotation);

if (this.state.applyAnnotationToSeries && chartDetails?.entityData?.entities) {
chartDetails.entityData.entities.forEach((entity: Entity) => {
const { fieldName, fieldValue } = entity;
const fieldType = entity.fieldType as PartitionFieldsType;
annotation[getAnnotationFieldName(fieldType)] = fieldName;
annotation[getAnnotationFieldValue(fieldType)] = fieldValue;
});
annotation.detector_index = detectorIndex;
}
// if unchecked, remove all the partitions before indexing
if (!this.state.applyAnnotationToSeries) {
delete annotation.detector_index;
PARTITION_FIELDS.forEach((fieldType) => {
delete annotation[getAnnotationFieldName(fieldType)];
delete annotation[getAnnotationFieldValue(fieldType)];
});
}
// Mark the annotation created by `user` if and only if annotation is being created, not updated
annotation.event = annotation.event ?? ANNOTATION_EVENT_USER;

annotation$.next(null);

Expand Down Expand Up @@ -214,7 +262,7 @@ class AnnotationFlyoutUI extends Component<CommonProps & Props> {
};

public render(): ReactNode {
const { annotation } = this.props;
const { annotation, detectors, detectorIndex } = this.props;
const { isDeleteModalVisible } = this.state;

if (annotation === null) {
Expand Down Expand Up @@ -242,10 +290,13 @@ class AnnotationFlyoutUI extends Component<CommonProps & Props> {
}
);
}
const detector = detectors ? detectors.find((d) => d.index === detectorIndex) : undefined;
const detectorDescription =
detector && 'detector_description' in detector ? detector.detector_description : '';

return (
<Fragment>
<EuiFlyout onClose={this.cancelEditingHandler} size="s" aria-labelledby="Add annotation">
<EuiFlyout onClose={this.cancelEditingHandler} size="m" aria-labelledby="Add annotation">
<EuiFlyoutHeader hasBorder>
<EuiTitle size="s">
<h2 id="mlAnnotationFlyoutTitle">
Expand All @@ -264,7 +315,10 @@ class AnnotationFlyoutUI extends Component<CommonProps & Props> {
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
<AnnotationDescriptionList annotation={annotation} />
<AnnotationDescriptionList
annotation={annotation}
detectorDescription={detectorDescription}
/>
<EuiSpacer size="m" />
<EuiFormRow
label={
Expand All @@ -286,6 +340,23 @@ class AnnotationFlyoutUI extends Component<CommonProps & Props> {
value={annotation.annotation}
/>
</EuiFormRow>
<EuiFormRow>
peteharverson marked this conversation as resolved.
Show resolved Hide resolved
<EuiCheckbox
id={'xpack.ml.annotationFlyout.applyToPartition'}
label={
<FormattedMessage
id="xpack.ml.annotationFlyout.applyToPartitionTextLabel"
defaultMessage="Apply annotation to this series"
/>
}
checked={this.state.applyAnnotationToSeries}
onChange={() =>
this.setState({
applyAnnotationToSeries: !this.state.applyAnnotationToSeries,
Copy link
Contributor

Choose a reason for hiding this comment

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

When editing a series level annotation, this checkbox should be initialized with the checked state. For me, it is normally opening unchecked.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is now checked on by default here 1f2684d

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed, if editing an existing series level annotation, if the user then unticks the 'apply to series' checkbox, we should clear the partitioning and detector index fields of the annotation.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@qn895 , I'm not sure if this screenshot is still current. But I'm noticing the footer actions. The 'Delete' action should be next to the 'Update' button instead of even spacing between all buttons. You also do not need the cross next to 'Cancel'

For example... (checkmark not needed in your case)
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested latest edits, and the partitioning and detector fields are now removed as expected if I untick the 'apply to series' checkbox when editing an annotation.

})
}
/>
</EuiFormRow>
</EuiFlyoutBody>
<EuiFlyoutFooter>
<EuiFlexGroup justifyContent="spaceBetween">
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading