[ML] Entity filter for the Notifications page #142778

darnautov · 2022-10-05T17:08:10Z

Summary

Even though it's possible to add job_id to the query string from the beginning, it wasn't obvious due to lacking of the UI filter.
This PR adds a custom control that allows selection of different ML entities.

What's supported

Allows picking entities from the current space
Restores filters state from the query string
Adds/removes OR clause on change
Persisted URL state

Corner cases

There is no uniqueness of entity IDs across different types, i.e. it's possible to have anomaly detection and data frame analytics jobs with the same ID, e.g. my_job. In this case, the entity picker will highlight all entities with provided ID as selected that mentioned in the query string.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

elasticmachine · 2022-10-06T16:27:23Z

Pinging @elastic/ml-ui (:ml)

…tity-selector

jgowdyelastic

Tested, added some minor comments, but overall LGTM

jgowdyelastic · 2022-10-10T16:42:35Z

x-pack/plugins/ml/public/application/notifications/components/entity_filter.tsx

+        }, newQuery);
+      }
+
+      onChange!(newQuery);


if there is a risk onChange might be undefined, it might be safer to wrap this in a check.

There is an issue with the types provided by import type { CustomComponentProps } from '@elastic/eui/src/components/search_bar/filters/custom_component_filter';. onChange is always defined in this case.

jgowdyelastic · 2022-10-10T16:52:57Z

x-pack/plugins/ml/public/application/notifications/components/ml_entity_selector.tsx

+import { countBy } from 'lodash';
+import { useMlApiContext } from '../../contexts/kibana';
+
+type EntityType = 'anomaly_detector' | 'data_frame_analytics' | 'trained_models';


In other places in the notifications page inference is used.
I wonder if we're introducing confusion by using the label Trained models in this menu, even though it's probably clearer and we used Trained model everywhere else in the UI.
It's a shame the notifications use inference as the type.

Not asking for a change here, just thinking out loud.

Agree, it's indeed confusing. Everywhere in the UI code, I refer to this as trained models.
On the data level for the .ml-notifications* it's inference, hence I decided to keep it this way. If I'm not mistaken it's the only exception in the user-facing labels so far.

x-pack/plugins/ml/public/application/notifications/components/ml_entity_selector.tsx

peteharverson · 2022-10-10T16:57:29Z

x-pack/plugins/ml/public/application/notifications/components/entity_filter.tsx

+import { MlEntitySelector, MlEntitySelectorProps } from './ml_entity_selector';
+
+/**
+ * Custom filter component to use with {@link EuiInMemoryTable}


Is this component limited to working with notifications only? If not, could it be moved to the top-level components folder?

I presume this is the only use case for this component, so I'd rather keep it as is for now.

OK! We can always move it at a later date when we find another use case for it.

x-pack/plugins/ml/public/application/notifications/components/ml_entity_selector.tsx

x-pack/plugins/ml/public/application/notifications/components/notifications_list.tsx

peteharverson · 2022-10-11T09:00:09Z

x-pack/plugins/ml/public/application/notifications/components/ml_entity_selector.tsx

+      isLoading={isLoading}
+      singleSelection={!multiSelect}
+      selectedOptions={selectedEntities}
+      options={options}


Entities that had notifications, but have since been deleted, cannot be selected. Is there a way to populate it with all entities that have had notifications?

Note that I hadn't synchronized the saved objects for the gallery_delete job here. Therefore the old saved object for this job still existed. Do you think we should be showing notifications for a deleted job in this case @jgowdyelastic ?

Not sure it's possible, because of the space awareness. It'd require an extra API call to retrieve all unique terms for job_id from .ml-notifications*, but eventually, we won't be able to resolve what IDs are available for the user (because Kibana saved objects are already deleted).

I think the alternative is too tricky to implement. We'd have to check every job to see whether it does still exist.
Just to clarify what's happening here...
When a job is deleted, it happens in the background in es, Kibana does not know when the job has actually been deleted so we can't be sure when to delete the saved object.
It was decided that leaving these orphaned saved objects behind was not a problem. They are deleted on sync or when a new job arrives with the same name.

The reason gallery_delete is showing up here is because the saved object still exists.
IMO the behaviour here is ok. It's not ideal, but the alternative of checking whether a job really still exists before listing its messages would add an overhead.

kibana-ci · 2022-10-11T14:12:07Z

💚 Build Succeeded

Buildkite Build
Commit: 0e8b635

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`ml`	1626	1629	+3

Async chunks

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

id	before	after	diff
`ml`	3.4MB	3.4MB	+3.8KB

History

💔 Build #79348 failed 06efa1e
💚 Build #79303 succeeded 6fffca8
💚 Build #79117 succeeded 08e0a0d
💚 Build #78807 succeeded d05f195
💛 Build #78477 was flaky 204a114

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

cc @darnautov

peteharverson

Tested latest changes and LGTM

darnautov added 5 commits October 5, 2022 14:02

do not show callout on loading

a63a0df

unit tests

c49d92b

wip ml entity selector

7a3fe5e

all options, preserve state wip

7dadc09

support simple selection and wildcards

204a114

darnautov self-assigned this Oct 5, 2022

handle duplicates

a515243

darnautov marked this pull request as ready for review October 6, 2022 16:27

darnautov requested a review from a team as a code owner October 6, 2022 16:27

darnautov added release_note:enhancement :ml Team:ML Team label for ML (also use :ml) v8.6.0 labels Oct 6, 2022

darnautov requested review from peteharverson, jgowdyelastic and lcawl and removed request for lcawl October 6, 2022 16:27

darnautov added 2 commits October 6, 2022 18:42

update selectedOptions prop

d05f195

Merge remote-tracking branch 'upstream/main' into ml-notifications-en…

08e0a0d

…tity-selector

jgowdyelastic approved these changes Oct 10, 2022

View reviewed changes

peteharverson reviewed Oct 10, 2022

View reviewed changes

fix hook deps, add error handling

1daee30

peteharverson reviewed Oct 11, 2022

View reviewed changes

x-pack/plugins/ml/public/application/notifications/components/notifications_list.tsx Show resolved Hide resolved

fix empty counts

6fffca8

peteharverson reviewed Oct 11, 2022

View reviewed changes

darnautov added 2 commits October 11, 2022 13:17

unit test

7f84c91

move component

37a8c11

darnautov requested a review from peteharverson October 11, 2022 11:23

rename translation key

115cdae

darnautov added 3 commits October 11, 2022 13:30

add extra assertion

b313c14

more tests

06efa1e

fix export

0e8b635

darnautov mentioned this pull request Oct 11, 2022

[ML] Increase Test Coverage 8.6.0 #142456

Closed

12 tasks

peteharverson approved these changes Oct 11, 2022

View reviewed changes

darnautov merged commit 1d08141 into elastic:main Oct 11, 2022

darnautov deleted the ml-notifications-entity-selector branch October 11, 2022 15:57

kibanamachine added the backport:skip This commit does not require backporting label Oct 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Entity filter for the Notifications page #142778

[ML] Entity filter for the Notifications page #142778

darnautov commented Oct 5, 2022 •

edited

Loading

elasticmachine commented Oct 6, 2022

jgowdyelastic left a comment

jgowdyelastic Oct 10, 2022

darnautov Oct 11, 2022

jgowdyelastic Oct 10, 2022

darnautov Oct 11, 2022 •

edited

Loading

peteharverson Oct 10, 2022

darnautov Oct 11, 2022

peteharverson Oct 11, 2022

peteharverson Oct 11, 2022

peteharverson Oct 11, 2022

darnautov Oct 11, 2022 •

edited

Loading

jgowdyelastic Oct 11, 2022 •

edited

Loading

kibana-ci commented Oct 11, 2022

peteharverson left a comment

[ML] Entity filter for the Notifications page #142778

[ML] Entity filter for the Notifications page #142778

Conversation

darnautov commented Oct 5, 2022 • edited Loading

Summary

What's supported

Corner cases

Checklist

elasticmachine commented Oct 6, 2022

jgowdyelastic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darnautov Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darnautov Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

jgowdyelastic Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Oct 11, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

peteharverson left a comment

Choose a reason for hiding this comment

darnautov commented Oct 5, 2022 •

edited

Loading

darnautov Oct 11, 2022 •

edited

Loading

darnautov Oct 11, 2022 •

edited

Loading

jgowdyelastic Oct 11, 2022 •

edited

Loading