[ML] Add runtime support for anomaly charts & add composite validations by qn895 · Pull Request #96348 · elastic/kibana

qn895 · 2021-04-06T19:22:14Z

Summary

Part of #77462. This PR adds runtime support for anomaly charts without model plot enabled.

It also adds extra validations for datafeed with composite aggregations to require only terms and date_histogram fields in the composite sources.

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

… for composite

elasticmachine · 2021-04-06T20:29:09Z

Pinging @elastic/ml-ui (:ml)

x-pack/plugins/ml/common/util/job_utils.ts

darnautov · 2021-04-07T06:20:21Z

x-pack/plugins/ml/common/util/job_utils.ts

+    isPopulatedObject(buckets, ['composite']) &&
+    isPopulatedObject(buckets.composite, ['sources'])


what do you think about extending isPopulatedObject guard to support nested props?

Suggested change

isPopulatedObject(buckets, ['composite']) &&

isPopulatedObject(buckets.composite, ['sources'])

isPopulatedObject(buckets, ['composite.sources'])

I think this is a good idea although I think we probably should have a different API for handling that type of nested cases as it is possible for an object to have keys with dots in the name.
const myObj = {"composite.sources": "value1", "composite": { "source": "value2" }}

x-pack/plugins/ml/common/util/job_utils.ts

x-pack/plugins/ml/common/util/object_utils.ts

…-runtime

darnautov · 2021-04-08T08:28:25Z

x-pack/plugins/ml/common/util/job_utils.ts

+  if (
+    isPopulatedObject(buckets, ['composite']) &&
+    isPopulatedObject(buckets.composite, ['sources']) &&
+    buckets.composite.sources !== undefined


shall we check for an array instead?

Suggested change

buckets.composite.sources !== undefined

Array.isArray(buckets.composite.sources)

Updated here 4987fc2 (#96348)

peteharverson · 2021-04-08T13:50:03Z

The charts in the Anomaly Explorer are showing up empty for me for a population job using runtime fields:

Job config is:

  "analysis_config": {
    "bucket_span": "15m",
    "detectors": [
      {
        "detector_description": "mean(responsetime_big) over airline_lower",
        "function": "mean",
        "field_name": "responsetime_big",
        "over_field_name": "airline_lower",
        "detector_index": 0
      }
    ],
    "influencers": [
      "airline_lower"
    ]
  },

The chart in the Single Metric Viewer is plotting correctly.

Update: confirmed fixed by 9b8746b

…-runtime

peteharverson

Tested latest edits and LGTM

walterra · 2021-04-12T15:08:48Z

x-pack/plugins/ml/common/util/job_utils.ts

-    });
+  // if job has at least one composite source that is not terms or date_histogram
+  const aggs = getDatafeedAggregations(job.datafeed_config);
+  if (aggs !== undefined) {


Nit: Suggest to use isPopulatedObject() here.

Updated here 058c5dd (#96348)

walterra · 2021-04-12T15:09:19Z

x-pack/plugins/ml/common/util/job_utils.ts

+  if (aggs !== undefined) {
+    const aggBucketsName = getFirstKeyInObject(aggs);
+    if (aggBucketsName !== undefined && aggs[aggBucketsName] !== undefined) {
+      // if fieldName is a aggregated field under nested terms using bucket_script


Nit: Missing n for an aggregated field.

Updated here 058c5dd (#96348)

walterra · 2021-04-12T15:11:03Z

x-pack/plugins/ml/common/util/job_utils.ts

+        return i18n.translate(
+          'xpack.ml.timeSeriesJob.jobWithUnsupportedCompositeAggregationMessage',
+          {
+            defaultMessage: 'the datafeed contains unsupported composite sources',


Where does this show up? Should the first char be capitalized and the sentence end with a dot?

Originally, the full message will show up like this. But I realized the extra message is redundant so I removed it here 058c5dd (#96348).

walterra · 2021-04-12T15:14:03Z

x-pack/plugins/ml/common/util/object_utils.ts

+ */
+export const getFirstKeyInObject = (arg: unknown): string | undefined => {
+  if (isPopulatedObject(arg)) {
+    const keys = Object.keys(arg);


Since JS doesn't guarantee the order of keys, should we sort the keys alphabetically to make sure the function returns 100% consistent results?

This is a good point. However, I don't think we should sort it alphabetically by default. We can have an extra argument like sorted or have another getFirstSortedKeyInObject function if that's needed. In this case where this function is used, for example if the aggregation config is like below, my_buckets should be used instead of buckets. Also in this specific case where we are accessing the datafeed config, it's already been validated and should be guaranteed to have only one definition in the object - we just don't know what the key is called.

"aggregations": { "my_buckets": { .... }, "buckets": { .... } },

walterra

Latest changes LGTM.

kibanamachine · 2021-04-12T17:59:18Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`ml`	6.0MB	6.0MB	+91.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`ml`	66.9KB	67.1KB	+207.0B

History

💚 Build #118727 succeeded c1263ef
💚 Build #118281 succeeded 9b8746b
💔 Build #118016 failed 2a9d1fc
💔 Build #117689 failed b3238ec
💔 Build #117662 failed f33f2e5

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

cc @qn895

kibanamachine · 2021-04-12T18:02:43Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 96348

…ns (elastic#96348)

…ns (#96348) (#96880)

qn895 added 2 commits April 6, 2021 14:12

[ML] Add runtime support for anomaly charts w/o mp && add extra check…

968f419

… for composite

[ML] Fix notes

5f2b609

qn895 added release_note:enhancement :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 v7.13.0 labels Apr 6, 2021

qn895 requested review from darnautov, peteharverson and walterra April 6, 2021 19:22

qn895 self-assigned this Apr 6, 2021

[ML] Move getFirstKey to object utils, add test

f33f2e5

qn895 mentioned this pull request Apr 6, 2021

[ML] Support runtime fields #77462

Closed

12 tasks

qn895 marked this pull request as ready for review April 6, 2021 20:29

qn895 requested a review from a team as a code owner April 6, 2021 20:29

[ML] Fix order && i18n

b3238ec

darnautov reviewed Apr 7, 2021

View reviewed changes

qn895 added 3 commits April 7, 2021 11:45

[ML] Add comments, update to some, fix types

25af35d

Merge remote-tracking branch 'upstream/master' into ml-anomaly-charts…

d5ad930

…-runtime

[ML] Update types

2a9d1fc

darnautov reviewed Apr 8, 2021

View reviewed changes

peteharverson mentioned this pull request Apr 8, 2021

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

Open

61 tasks

qn895 added 4 commits April 8, 2021 09:05

Merge remote-tracking branch 'upstream/master' into ml-anomaly-charts…

2a72077

…-runtime

[ML] Update logic

4987fc2

[ML] Fix population chart empty with rt field

9b8746b

Merge remote upstream/master into branch

c1263ef

peteharverson approved these changes Apr 12, 2021

View reviewed changes

walterra reviewed Apr 12, 2021

View reviewed changes

[ML] Update message

058c5dd

walterra approved these changes Apr 12, 2021

View reviewed changes

qn895 added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 12, 2021

qn895 merged commit e7f5d07 into elastic:master Apr 12, 2021

qn895 deleted the ml-anomaly-charts-runtime branch April 12, 2021 17:59

qn895 mentioned this pull request Apr 12, 2021

[7.x] [ML] Add runtime support for anomaly charts & add composite validations (#96348) #96880

Merged

qn895 added a commit to qn895/kibana that referenced this pull request Apr 12, 2021

[ML] Add runtime support for anomaly charts & add composite validatio…

e871186

…ns (elastic#96348)

qn895 added a commit that referenced this pull request Apr 12, 2021

[ML] Add runtime support for anomaly charts & add composite validatio…

5dde995

…ns (#96348) (#96880)

qn895 mentioned this pull request Apr 13, 2021

[ML] validate datafeed composite aggregation support in Single metric viewer #93610

Closed

peteharverson added release_note:feature Makes this part of the condensed release notes and removed release_note:enhancement labels May 4, 2021

		isPopulatedObject(buckets, ['composite']) &&
		isPopulatedObject(buckets.composite, ['sources'])

	isPopulatedObject(buckets, ['composite']) &&
	isPopulatedObject(buckets.composite, ['sources'])
	isPopulatedObject(buckets, ['composite.sources'])

	buckets.composite.sources !== undefined
	Array.isArray(buckets.composite.sources)

Conversation

qn895 commented Apr 6, 2021

Summary

Checklist

Uh oh!

elasticmachine commented Apr 6, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteharverson commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peteharverson 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!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 12, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

Uh oh!

kibanamachine commented Apr 12, 2021

💔 Backport failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

peteharverson commented Apr 8, 2021 •

edited

Loading