Skip to content

[ML] Add links to rule editor for quick edit of value or filter#22990

Merged
peteharverson merged 2 commits intoelastic:masterfrom
peteharverson:ml-quick-rule-links
Sep 13, 2018
Merged

[ML] Add links to rule editor for quick edit of value or filter#22990
peteharverson merged 2 commits intoelastic:masterfrom
peteharverson:ml-quick-rule-links

Conversation

@peteharverson
Copy link
Copy Markdown
Contributor

Adds functionality to the rule editor flyout for quick edits to the numeric value of a condition, or to add the partitioning field value of the selected anomaly to the filter list used in the scope.

image

The numeric link is available if the rule has a single numeric condition. It is not displayed if the rule has multiple conditions to avoid presenting the user with a long list of options with potential confusion as to exactly which value is being edited. The full editor can continue to be used for editing rules with multiple conditions.

The 'add to filter list' link is shown if the rule has a scope section with a single partitioning field key, andf the partitioning field value is not already in the filter list. As above it is not displayed if the scope has multiple partitioning field keys to simplify the edit options available.

The PR also adds the display of the actual and typical values of the selected anomaly to the top of flyout, to help guide the user in selecting a suitable condition value (the edit condition value link defaults to the value of the selected anomaly). Also adds tests for the rule_editor utils.js file, and extra Jest tests for the new and edited components.

Addresses the first two items in #21843

@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/ml-ui

Copy link
Copy Markdown
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

Added a few comments, but LGTM

return null;
const detector = job.analysis_config.detectors[anomaly.detectorIndex];
const rules = detector.custom_rules;
if (rules !== undefined && ruleIndex <= rules.length) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is there a risk of an overflow here?
does the ruleIndex start at 0?

if (scope !== undefined && Object.keys(scope).length === 1) {
const partitionFieldName = Object.keys(scope)[0];
const partitionFieldValue = this.props.anomaly.source[partitionFieldName];
const filterId = scope[partitionFieldName].filter_id;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is there a risk that scope[partitionFieldName] could be undefined?
a check might be good here, just to be defensive.

updateRuleAtIndex } = this.props;

const detector = job.analysis_config.detectors[anomaly.detectorIndex];
const editedRule = cloneDeep(detector.custom_rules[ruleIndex]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

custom_rules doesn't always exist in the job config.
unless it's being manually added in a step i've missed or perhaps this code will never be reached if it doesn't exist?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The updateConditionValue function is only called from the EditConditionLink component which is only available for editing an existing rule. So yes, this code will only ever be called for a detector which already has a custom_rules property.


// Updates an ML filter used in the scope part of a rule,
// adding an item to the filter with the specified ID.
export function updateFilterAddItem(item, filterId) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a clearer name for this function might be addItemToFilter

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded


.select-rule-action-panel {
padding-top:10px;
padding:10px 0px;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

uber nit: could have a space after padding:.

Copy link
Copy Markdown
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM

@peteharverson peteharverson merged commit c272a1b into elastic:master Sep 13, 2018
@peteharverson peteharverson deleted the ml-quick-rule-links branch September 13, 2018 14:54
peteharverson added a commit to peteharverson/kibana that referenced this pull request Sep 13, 2018
…tic#22990)

* [ML] Add links to rule editor for quick edit of value or filter

* [ML] Updates to rule editor quick links following review
peteharverson added a commit that referenced this pull request Sep 13, 2018
…) (#22997)

* [ML] Add links to rule editor for quick edit of value or filter

* [ML] Updates to rule editor quick links following review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants