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] Notifications indicator #140944

Merged
merged 12 commits into from
Sep 20, 2022

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Sep 19, 2022

Summary

Resolves #135417

Adds a notifications indicator to the side nav menu.

How it works

  • When the user visits the Notifications page, the app stashes the timestamp of the latest notification visible on the screen in the local storage
  • The app polling the .ml-notifications* index with an interval of 1 minute for new notifications that occurred after the stored timestamp
  • If there are only notifications with an info level, displays the generic notification indicator

image

  • If there are errors or warnings, it shows a counter in the side nav.

image

  • When the user visits the page again, sets the time bounds based on the last checked notification.

How to test

  • To trigger an info message, you can create any job or deploy model, start/stop datafeed, etc.
  • To trigger an error, you can delete the source index of the anomaly detection job. Setting frequency for this job to something small should make it less time-consuming.

Checklist

@darnautov darnautov added :ml release_note:feature Makes this part of the condensed release notes Team:ML Team label for ML (also use :ml) v8.5.0 labels Sep 19, 2022
@darnautov darnautov self-assigned this Sep 19, 2022
@darnautov darnautov marked this pull request as ready for review September 19, 2022 13:08
@darnautov darnautov requested a review from a team as a code owner September 19, 2022 13:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
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.

Looks good overall! Left some comments with suggestions. Did some local testing and was able to trigger the notifications icon including errors.

Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

One comment on something that Walter has already spotted but I added a suggestion anyway. Otherwise UI text LGTM!

Copy link
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.

Latest changes LGTM.

if (globalState?.time || !lastCheckedAt) return;

timeFilter.setTime({
from: moment(lastCheckedAt).toISOString(),
Copy link
Contributor

@peteharverson peteharverson Sep 20, 2022

Choose a reason for hiding this comment

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

Would it be better to remember the last setting in the time picker rather than setting it to the time of the latest checked notification - for example I end up continually resetting to Last 24 hours when I switch back to the page?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add an extra column to the table (8.6+) which indicates if a notification is 'new'? At that point it might be worth changing the behavior to preserve the time picker setting from the last visit to the page. Till then, this current behavior is a good way of showing the unread notifications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I implemented it like this initially but then decided to change to behaviour here.
Because imagine you have a time picker set to some time in the past (this can easily happen when you open anomaly explorer). And when you click the Notification link in the side nav, it's going to preserve this time range and it might be confusing for the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I wasn't thinking of using the time range from the global state, but stashing the last applied time range on the notifications page, where a common use case might be 'Last 1 hour' for example.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1618 1623 +5

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.3MB 3.3MB +3.1KB
Unknown metric groups

ESLint disabled line counts

id before after diff
ml 574 577 +3

Total ESLint disabled count

id before after diff
ml 577 580 +3

History

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

cc @darnautov

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM.

Let's wait for more feedback on how the time range is set in the page. Some way of indicating unread notifications is a possible for a follow-up in 8.6+.

@darnautov darnautov merged commit ab0c7b4 into elastic:main Sep 20, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 20, 2022
@darnautov darnautov deleted the ml-135417-notifications-indicator branch September 20, 2022 11:58
@lcawl lcawl added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:feature Makes this part of the condensed release notes labels Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting :ml release_note:skip Skip the PR/issue when compiling release notes Team:ML Team label for ML (also use :ml) v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ML] Notifications page
8 participants