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

Add notification drawer MVP designs #306

Merged
merged 1 commit into from
Jan 21, 2020

Conversation

cshinn
Copy link
Contributor

@cshinn cshinn commented Dec 6, 2019

Adds initial designs for Notification Drawer MVP

Open Questions:

  • What should the masthead icon look like when there are alerts/updates? (Ongoing visual story)
  • Should we use unique severity values and annotations for "First time setup" notifications
  • Is it feasible to use annotations to implement action buttons?

@openshift/team-ux-leads
@openshift/team-ux-review (Admin Console UX team)

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 6, 2019
Copy link
Contributor

@lizsurette lizsurette left a comment

Choose a reason for hiding this comment

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

@cshinn - This looks awesome, thanks for taking on this tricky addition :) I've added some thoughts in comments.


## Masthead Icon
![](img/notifications-closed.png)
- When alerts are firing, a special signifier icon appears in the masthead. Clicking on the icon opens the notification drawer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean just the blue dot over the icon will show when Alerts are firing? Should we update this text to include if there is an available update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an open visual design story for the specifics of this. I plan to update once that's fully decided

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

![](img/notifications-updates.png)
- Updates for the cluster or for specific operators could also be listed here.
- Clicking on one of these cards would take the user to a page with information about the updates.
- Action buttons to install the update immediately could also be provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how closely our use case will align, but I envision 20 operators having an update available...should we include an "Install all updates" eventually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that's a good idea. Is the console capable of "handling" 20 simultaneous updates? or would it queue them or something?

Copy link
Contributor

@lizsurette lizsurette Dec 9, 2019

Choose a reason for hiding this comment

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

Great question :) @tlwu2013 - Do you know the answer to this?

- When there are no critical alerts firing, the `Critical alerts` group should still be present but should contain an empty state pattern.
- The `Available updates` category and any additional categories should be hidden when they are empty.

## First Time Setup
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a great feature, but how about some sort of empty state treatment rather than a firing alert treatment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking something more like a small version of when receivers need to be created in general: http://openshift.github.io/openshift-origin-design/admin-perspective/monitoring/alertmanager-config/img/config-empty.png


## Masthead Icon
![](img/notifications-closed.png)
- When alerts are firing, a special signifier icon appears in the masthead. Clicking on the icon opens the notification drawer.
Copy link
Member

Choose a reason for hiding this comment

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

The masthead icon should probably be stronger when there are critical alerts. They require attention, and it would be really easy to miss or ignore the blue dot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. There's an open visual design story to improve the "there's something here" icon. I'll update this when that's finished

Choose a reason for hiding this comment

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

Yeah, I would expect red or yellow for critical, something subtler for warning only. But the "you need notification sinks configured" is probably going to be warning level.

Also, we need to make sure we use the right terminology. We use "sinks" in the alert but you used "receivers" here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we are also using the term "sink" for Knative Event Sources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been using "Receiver" to align with what the corresponding Alertmanager objects are called https://prometheus.io/docs/alerting/configuration/#receiver

I believe that's the terminology we use on the page where they're configured as well

![](img/notifications-open.png)
- As this drawer is an endopoint for alertmanager, silenced and pending alerts do not appear here.
- Clicking on an alert takes the user to the details page for the alert.
- The alertmanager config could potentially be set up to send non-critical alerts here, but by default that would not be the case.
Copy link
Member

Choose a reason for hiding this comment

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

Why not show all alerts?

Copy link
Contributor Author

@cshinn cshinn Dec 9, 2019

Choose a reason for hiding this comment

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

The urgency of warning alerts is ideally on the scale of several days; ideally we would encourage people to set up an alert receiver to get these into some kind of ticketing system. I worry that including warnings in this drawer would cause alert fatigue and people would start to ignore them.

Choose a reason for hiding this comment

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

I don't think that's the case - for instance, NodeDown is a warning. You should take action on warnings pretty fast. We have silences for a reason. If your nodes are going down, you have a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sichvoge what are your thoughts on how much we should surface here? I guess ideally, the user would have some sort of mobile phone alert set up for things that are really critical and could look to the UI for a list of everything.

## Empty State
![](img/notifications-empty.png)
- When there are no critical alerts firing, the `Critical alerts` group should still be present but should contain an empty state pattern.
- The `Available updates` category and any additional categories should be hidden when they are empty.
Copy link
Member

Choose a reason for hiding this comment

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

Should we show progress during an upgrade?

Copy link
Contributor Author

@cshinn cshinn Dec 9, 2019

Choose a reason for hiding this comment

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

If we have that capability, I think that would be great! Ideally, the messages here would function like those in the "status" section of the cluster overview page.

Choose a reason for hiding this comment

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

I am concerned that this tray should be strongly focused on things that justify an alert. An upgrade is important, but only when it starts failing. We could come up with lots of "current status" things to show here, but I think we should start by only focusing on alerts until the alert system is a) properly integrated, b) well used, and c) covers the use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we currently put an icon in the masthead to indicate that there's an available update. Combining that messaging into this space would keep upgrades from competing for people's attention because a pressing alert would be surfaced above the update notification. It also reduces the amount of stuff in the masthead.

- The alertmanager config could potentially be set up to send non-critical alerts here, but by default that would not be the case.
- The critical alerts group should be expanded by default and all others should be collapsed.

## Available Updates

Choose a reason for hiding this comment

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

This probably should just be an alert anyway. We need to discuss what the design is, but info level alerts (not warning or critical) should be shown, and you should be able to silence them for a while with a reason just like any other alert.

Visually, I would suggest only reporting "there is an update available" vs showing the list. I don't think we have evidence users select any other update than the latest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this section only available to cluster admins, aka specific roles? Is the heading & section not displayed if you don't have access to update anything?

## First Time Setup
![](img/notifications-receivers.png)
- Potentially, alerts with a specific annotation could have the annotation rendered as an action or link button. Otherwise, clicking on the alert itself would take the user to the details page for the alert.
- A special severity level for “getting started” or “first time setup” alerts could cause them to be rendered in a special way.

Choose a reason for hiding this comment

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

For now we can just hardcode by alert name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this section also intended for the cluster admin/specific roles?


## Masthead Icon
![](img/notifications-closed.png)
- When alerts are firing, a special signifier icon appears in the masthead. Clicking on the icon opens the notification drawer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does someone without cluster admin rights will only see alerts that are associated with namespaces that they have read access to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the admin console at least, I believe that all the things that fall under "monitoring" are cluster scoped and only visible to admins. I was under the impression that this initial work would be scoped the same way, but I think a similar pattern could be useful for project-specific alerts when they exist as well

- The alertmanager config could potentially be set up to send non-critical alerts here, but by default that would not be the case.
- The critical alerts group should be expanded by default and all others should be collapsed.

## Available Updates
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this section only available to cluster admins, aka specific roles? Is the heading & section not displayed if you don't have access to update anything?

## First Time Setup
![](img/notifications-receivers.png)
- Potentially, alerts with a specific annotation could have the annotation rendered as an action or link button. Otherwise, clicking on the alert itself would take the user to the details page for the alert.
- A special severity level for “getting started” or “first time setup” alerts could cause them to be rendered in a special way.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this section also intended for the cluster admin/specific roles?

@rhamilto
Copy link
Member

rhamilto commented Jan 3, 2020

What about mobile? I don't see any mention of the drawer at mobile resolutions. Does that mean it doesn't appear?

@cshinn
Copy link
Contributor Author

cshinn commented Jan 3, 2020

What about mobile? I don't see any mention of the drawer at mobile resolutions. Does that mean it doesn't appear?

I think this pattern still has utility at mobile scale. I believe the drawer becomes full-width on small screens in the PF implementation, but the behavior would be the same otherwise. As for the masthead icon, I agree we need to consider how any kind of "badge" would work when collapsed into a menu

@rhamilto
Copy link
Member

rhamilto commented Jan 3, 2020

I agree we need to consider how any kind of "badge" would work when collapsed into a menu.

Which is why I asked. Note we already have a system status icon that appears outside of the mobile kebab. https://user-images.githubusercontent.com/895728/57083069-f79b0680-6cc5-11e9-8ee4-0f56727f97f3.jpeg

@cshinn
Copy link
Contributor Author

cshinn commented Jan 3, 2020

Ideally, we'd use this drawer do display things such as system status, so co-opting that behavior to some extent might be a good idea.

@benjaminapetersen
Copy link

benjaminapetersen commented Jan 9, 2020

@cshinn It doesn't look like there is a definitive answer to the "Admin only" questions.
@jcaianirh is working on implementation and is working through this right now.

Should the notifications bell appear in the dev perspective?

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 16, 2020
@jcaianirh
Copy link
Member

@benjaminapetersen, @cshinn and I had a discussion about this, and decided not to show/hide the notification drawer based on the console, but rather the permission to view monitoring data. As such, the notification drawer is not visible to the dev console. We discussed this with @serenamarie125 and this is ok with her for the time being until a later release where there may be functionality introduced that would benefit the dev console.

Copy link
Member

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

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

Thanks Chris, this is great. It looks like you made most of the updates for 4.4 based on everyone's feedback. I know we will update and enhance in future releases, but for now can we add the note about permissions and who will see the bell in the masthead?

@cshinn cshinn changed the title [WIP] Add notification drawer MVP designs Add notification drawer MVP designs Jan 20, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 20, 2020
Copy link
Member

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

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

LGTM!

@beanh66
Copy link
Member

beanh66 commented Jan 20, 2020

I think this is good to go. @serenamarie125 or @lizsurette mind taking a look?

Copy link
Contributor

@matthewcarleton matthewcarleton left a comment

Choose a reason for hiding this comment

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

Assuming there are further changes to come in following releases based on work that is ongoing on the PF side - this LGTM!

Copy link
Contributor

@lizsurette lizsurette left a comment

Choose a reason for hiding this comment

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

LGTM!

Something to consider for the future...
We noticed with including alerts on the dashboard design that there are often a lot of them firing at once (maybe just our test environments?). But it could be good to consider if we need to reduce the height of each alert to allow for more scale. We can see what typical systems look like. We condensed the events list on the dashboard too for this reason, so this could be a design we reuse here in the drawer.

@matthewcarleton matthewcarleton merged commit 3cb5b74 into openshift:master Jan 21, 2020
@cshinn cshinn deleted the notification-drawer branch January 29, 2020 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet