[ML] Improve support for script and aggregation fields in anomaly detection jobs by qn895 · Pull Request #81923 · elastic/kibana

qn895 · 2020-10-28T16:44:24Z

Summary

This PR improves support for anomaly charts in the Anomaly Explorer and Single Metric Viewer for jobs which use scripted or aggregation fields in their datafeed configuration and where model plot is not enabled.

Fixes using a script_field as a partition_field_name fails validation in anomaly detection advanced job wizard
[ML] Using a script_field as a partition_field_name fails validation in AD advanced job wizard #76075 (we now check for the cardinality using the whole script_field)
Fixes the plotting of values in anomaly charts for jobs, with or without model plot enabled, where datafeeds use aggregations, for cases where the aggregation does not define the partitioning field (by, over or partition field) capabilities - partially addresses [ML] Anomalies chart doesn't plot values for datafeeds comprising aggregations #65223

Before

After

Fix Anomalies chart doesn't plot values for datafeeds comprising scripted fields

Fix datafeed preview to work when something else other than buckets is used for aggregation
Fix anomalies chart advanced wizard not validating correctly for datafeed configs with nested aggregations
Also contains a fix for anomalies not showing in Anomaly Explorer charts when no influencer detected:

Before

After

Added validation if summary count field is missing if datafeed config has aggregation fields

Note that the following items will be addressed in a follow-up:

Fix anomalies chart advanced wizard to work when something else other than buckets is used
Disable links to the Single Metric Viewer for datafeed configs which use aggregations with nested terms for which the plot of metric data is not supported.
Provide better messaging in the Anomaly Explorer for datafeed configs which use aggregations with nested terms for which the plot of metric data is not supported.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

… is not `bucket`

…ggregation-fields

…tion

elasticmachine · 2020-11-04T15:04:16Z

Pinging @elastic/ml-ui (:ml)

jgowdyelastic

Added a few minor comments, but generally LGTM
I'll approve once i've tested it.

jgowdyelastic · 2020-11-05T21:01:43Z

x-pack/plugins/ml/public/application/services/ml_api_service/index.ts

 } from '../../datavisualizer/index_based/common';
-import { DataRecognizerConfigResponse, Module } from '../../../../common/types/modules';
+import {
+  DatafeedOverride,


DatafeedOverride is for overriding the datafeed config when calling the module setup and so i don't think should be used here.
It looks like the standard Datafeed interface would be better suited throughout this PR.

jgowdyelastic · 2020-11-05T21:10:56Z

x-pack/plugins/ml/public/application/services/results_service/results_service.js

                    terms: {
                      field: 'influencer_field_value',
-                      size: maxResults !== undefined ? maxResults : 2,
+                      size: !!maxResults ? maxResults : 2,


this change is subtle but using a falsey check like !! rather than an explicit check for undefined changes its behaviour.
if maxResults is 0 it would now default to 2
Not that 0 is a sensible number, it's just generally dangerous IMO using falsey checks for variables that are numbers.

I actually had to update this to using !! because in the case maxResults = 0, the size will be 0 and will make the query invalid.

Updated here 7355559

jgowdyelastic · 2020-11-05T21:15:22Z

x-pack/plugins/ml/server/models/fields_service/fields_service.ts

      fields: fieldNames,
    });
    const aggregatableFields: string[] = [];
+    const datafeedAggConfig = datafeedConfig?.aggregations ?? datafeedConfig?.aggs;


nit, datafeedAggregations or even just aggregations might be a better name for this as datafeedAggConfig is a bit too similar to datafeedConfig. I missed the difference when first reading it.
The same in other files in this PR.

Updated here f54c8e8

jgowdyelastic · 2020-11-05T21:20:35Z

x-pack/plugins/ml/server/models/job_validation/job_validation.ts

+
+      // if datafeed has aggregation, require job config to include a valid summary_doc_field_name
+      const datafeedAggConfig = job.datafeed_config?.aggregations ?? job.datafeed_config?.aggs;
+      if (datafeedAggConfig !== undefined && !job.analysis_config?.summary_count_field_name) {


in the two conditions here, one checks for undefined and one checks for !.
Is there a reason for the difference? or could they both be undefined checks?

I see that this was two ifs combined based on a previous comment.
being a fussy nitpicker i think they should be the same so not to raised questions further down the line.

Updated here f54c8e8

…ggregation-fields

jgowdyelastic

LGTM

qn895 · 2020-11-17T00:30:43Z

@elasticmachine merge upstream

peteharverson

Tested and LGTM.

In the follow-up we should try and disable links to the Single Metric Viewer for the configs which we don't support and provide e.g. a callout in the Anomaly Explorer to explain why we aren't displaying anomaly charts.

qn895 · 2020-11-17T15:56:46Z

@elasticmachine merge upstream

kibanamachine · 2020-11-17T17:33:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a661a27

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`ml`	1564	1565	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`ml`	5.2MB	5.2MB	+5.9KB

Distributable file count

id	before	after	diff
`default`	42848	42849	+1

History

💚 Build #88207 succeeded 6d50e8e
💚 Build #87382 succeeded cf5d4d1
💚 Build #86774 succeeded 20c1f67
💚 Build #86292 succeeded 7355559
💚 Build #86247 succeeded 9769d94

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…ection jobs (elastic#81923) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…ly detection jobs (#81923) (#83569) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: (51 commits) [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439) adds xpack.security.authc.selector.enabled setting (elastic#83551) skip flaky suite (elastic#77279) [ML] Improve support for script and aggregation fields in anomaly detection jobs (elastic#81923) [Workplace Search] Migrate SourcesLogic from ent-search (elastic#83544) [ML] Add UI test for feature importance features (elastic#82677) [Maps] Improve icons for all layer types (elastic#83503) Replace experimental badge with Beta (elastic#83468) [Fleet][EPM] Unified install and archive (elastic#83384) Move src/legacy/server/keystore to src/cli (elastic#83483) Used SO for saving the API key IDs that should be deleted (elastic#82211) [Uptime] Mock implementation to account for math flakiness test (elastic#83535) [Workplace Search] Enable check for org context based on URL (elastic#83487) [App Search] Added all Document related routes and logic (elastic#83324) [Alerting UI] Fix console error when setting connector params (elastic#83333) [Discover] Allow custom name for fields via index pattern field management (elastic#70039) [Uptime] Fix monitor list down histogram (elastic#83411) remove headers timeout hack, rely on nodejs timeouts (elastic#83419) [ML] Update console autocomplete for ML data frame evaluate API (elastic#83151) [Lens] Color in dimension trigger (elastic#76871) ...

qn895 added 7 commits October 28, 2020 11:38

[ML] Add support for script fields in wizard and in viz

75c4924

[ML] Fix datafeed preview for aggregations not showing if name of agg…

d90f1ec

… is not `bucket`

[ML] Seems like it can be just aggregations

b580708

[ML] Fix AE plotting for aggregated fields

77a7e92

[ML] Export Aggregation interface

ecc77ef

[ML] Fix plotting issue with AE script fields

8cd0732

[ML] Fix typescript

a930e21

qn895 added release_note:enhancement :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 v7.11 labels Oct 28, 2020

qn895 self-assigned this Oct 28, 2020

timroes added v7.11.0 and removed v7.11 labels Oct 29, 2020

qn895 added 4 commits October 30, 2020 09:40

Merge remote-tracking branch 'upstream/master' into ml-fix-script-n-a…

571bf79

…ggregation-fields

Merge upstream/master into ml-fix-script-n-aggregation-fields

25e063d

[ML] Clean up validation for nested aggregation field

0455ba1

[ML] Add validation for missing summary count if datafeed has aggrega…

4a557f5

…tion

qn895 marked this pull request as ready for review November 4, 2020 15:04

qn895 requested a review from a team as a code owner November 4, 2020 15:04

peteharverson requested review from peteharverson and walterra November 4, 2020 17:19

qn895 requested a review from jgowdyelastic November 4, 2020 17:35

qn895 added 5 commits November 4, 2020 14:43

[ML] Fix anomaly search broken because max results is 0

2712c6f

[ML] Add missing_summary_count_field_name validation to validate.ts test

f76039b

[ML] Fix order between datafeedConfig and allowMMLGreaterThanMax

85e1773

[ML] Fix jest tests

79f22dd

[ML] Fix calculateModelMemoryLimitProvider order

739a423

[ML] Fix allowMMLGreaterThanMax changed to undefined previously

9769d94

jgowdyelastic reviewed Nov 5, 2020

View reviewed changes

qn895 added 7 commits November 5, 2020 17:07

[ML] Update datafeedAggregations check

f54c8e8

[ML] Change DatafeedOverride to Datafeed

7355559

[ML] Fix field in aggregatable check

1b5898f

Merge remote-tracking branch 'upstream/master' into ml-fix-script-n-a…

20c1f67

…ggregation-fields

Merge upstream/master into ml-fix-script-n-aggregation-fields

1a3c40f

[ML] Update text description

22f938c

[ML] Update text translations

cf5d4d1

jgowdyelastic approved these changes Nov 11, 2020

View reviewed changes

jgowdyelastic self-requested a review November 11, 2020 21:24

jgowdyelastic approved these changes Nov 12, 2020

View reviewed changes

Merge branch 'master' into ml-fix-script-n-aggregation-fields

6d50e8e

peteharverson changed the title ~~[ML] Add support for script and aggregation fields to Anomaly Detection~~ [ML] Improve support for script and aggregation fields in anomaly detection jobs Nov 17, 2020

peteharverson approved these changes Nov 17, 2020

View reviewed changes

Merge branch 'master' into ml-fix-script-n-aggregation-fields

a661a27

qn895 merged commit 55119c2 into elastic:master Nov 17, 2020

qn895 deleted the ml-fix-script-n-aggregation-fields branch November 17, 2020 17:34

qn895 added a commit to qn895/kibana that referenced this pull request Nov 17, 2020

[ML] Improve support for script and aggregation fields in anomaly det…

005aaa2

…ection jobs (elastic#81923) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

qn895 mentioned this pull request Nov 17, 2020

[7.x] [ML] Improve support for script and aggregation fields in anomaly detection jobs (#81923) #83569

Merged

peteharverson mentioned this pull request Dec 22, 2020

[ML] Add visuals in result dashboards for all ML detector functions #22922

Open

61 tasks

qn895 mentioned this pull request Jan 11, 2021

[ML] Fix Anomaly Explorer & Single Metric Viewer not plotting correctly for detectors with non zero count function #87903

Merged

1 task

Conversation

qn895 commented Oct 28, 2020 • edited by peteharverson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Nov 4, 2020

Uh oh!

jgowdyelastic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic left a comment

Choose a reason for hiding this comment

Uh oh!

qn895 commented Nov 17, 2020

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

qn895 commented Nov 17, 2020

Uh oh!

kibanamachine commented Nov 17, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

qn895 commented Oct 28, 2020 •

edited by peteharverson

Loading