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

Show monitoring screen by default #14976

Merged
merged 1 commit into from
Sep 3, 2017

Conversation

moolitayer
Copy link

@moolitayer moolitayer commented May 3, 2017

This suggest showing the new alerts screen by default.
Before this can go in:

  • Approve this screen for non container providers[1]
  • hide "most recent alerts" link

[1]
Main differences in existing alerts:

  • alerts aren't being resolved and will remain in the screen (miq_alert_status.resolved vs miq_alert_status.result)
  • Overview screen is per ems, need to handle alerts with no ems if any, probably create a default group of some kind.
  • no severity, should default to error.
  • message is fine, the miq_alert_status.message message will come from the miq_alert.message
  • no url (cm-ops are using url to a document on the specific issue)

Screenshots

alerts_overview

alerts_list

alerts_list_expanded

alerts_action

alerts_assignment

Playing with the screen

  • Add a setting:
:prototype:
 :monitoring: true

@moolitayer
Copy link
Author

cc @simon3z @gtanzillo @gmcculloug

@moolitayer
Copy link
Author

@simon3z how do we move this forward? (we should have someone from core/other providers to work with)

@simon3z
Copy link
Contributor

simon3z commented May 17, 2017

@simon3z how do we move this forward? (we should have someone from core/other providers to work with)

@moolitayer this is currently discussed at ManageIQ core level with @Loicavenel.

@moolitayer would this show all the alerts from all the providers?
(And of course we know that auto-resolve is something we have to solve for other providers)

The plan was to have a common story to enable this (maybe we could have enabled it only for OpenShift if the Alert path forward was more well defined).

@moolitayer
Copy link
Author

  • alerts that are shown in the screen are those that belong to an ems.
  • alerts that belong to an ems are those that happen for an object that has an ems_id field[1]

We probably will need another solution for resolving alerts for systems that do not support it.

For example, I'm still trying to figure out what should be the Prometheus flow; they allow silencing alerts until a time in the future, but I find it hard to believe that's all they do.

Just a reminder, Kenny(I think it was Kenny) from ops mentioned they could also use silencing if it's until a certain date

[1] https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_alert.rb#L221
This actually gives one item to do here: we need to make sure the screen returns alerts whose target entity IS the ems

@moolitayer
Copy link
Author

@Loicavenel this uses the same table as other alerts, and we added some fields.

Some are populated for all providers:
ems_id, description

Some will be populated only in the cm ops flow:
severity, url, resolved

Those would be empty for other providers and need more implementation

@moolitayer
Copy link
Author

@Loicavenel @simon3z bump

1 similar comment
@chessbyte
Copy link
Member

@Loicavenel @simon3z bump

@simon3z
Copy link
Contributor

simon3z commented Jun 5, 2017

@Loicavenel FYI we'll move forward with this as soon as @moolitayer finalized his alerts collection side.
I suppose you'll need to sync with John before that because we need this in 4.6.

@Loicavenel
Copy link

@simon3z I have a first discussion with Dan C yesterday on it, he is looking in the effort to be able send CF Alert to this UI and update the severity and User should be able to Resolve it via an action.
@moolitayer does all Alerts will show in the UI or only OpenShift (Hawkular) Alert?

@moolitayer
Copy link
Author

@Loicavenel see this comment on which alerts should show up on the screen

@Loicavenel
Copy link

@moolitayer Ok, so, if for other provider I want to see it, do I need to populate all fields ? Severity & Resolved will make sense but URL is more complex..

@dclarizio for visibility, we discuss this yesterday

@moolitayer
Copy link
Author

Severity has a default. URL and resolved are optional.

There is however an unexpected behavior currently where if resolved is nil and not false (which would be the case for all non hawkular/openshift alerts) then the alert is not shown. I'm looking into it

@moolitayer
Copy link
Author

There is however an unexpected behavior currently where if resolved is nil and not false (which would be the case for all non hawkular/openshift alerts) then the alert is not shown. I'm looking into it

ManageIQ/manageiq-ui-classic#1503

@moolitayer
Copy link
Author

@simon3z @Loicavenel bump

1 similar comment
@moolitayer
Copy link
Author

@simon3z @Loicavenel bump

@simon3z
Copy link
Contributor

simon3z commented Aug 23, 2017

@moolitayer I think we can remove this from prototype entirely. Also remove the check in the UI.

@@ -1037,7 +1037,6 @@
:container_deployment_wizard: false
:datawarehouse_manager: false
:prototype:
Copy link
Author

@moolitayer moolitayer Aug 23, 2017

Choose a reason for hiding this comment

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

I left the prototype section here although it's empty. It will be useful next time someone has a prototype.

@miq-bot
Copy link
Member

miq-bot commented Aug 23, 2017

Checked commit moolitayer@c88df59 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@moolitayer
Copy link
Author

moolitayer commented Aug 23, 2017

UI change in ManageIQ/manageiq-ui-classic#1982

@Loicavenel
Copy link

@moolitayer why should hide more recent Alerts?

@moolitayer
Copy link
Author

@Loicavenel that screen hasn't been designed and implemented

@h-kataria
Copy link
Contributor

@moolitayer purpose of this PR is not clear to me, Monitoring tab will be hidden by default before/after changes in this PR because of this check https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/presenters/menu/default_menu.rb#L267

@moolitayer
Copy link
Author

@h-kataria that check is removed in ManageIQ/manageiq-ui-classic#1982.
The discussion about adding the screen by default is done mostly here.

@moolitayer
Copy link
Author

Added instructions on how to view the screen in the first comment

@h-kataria h-kataria added this to the Sprint 68 Ending Sep 4, 2017 milestone Sep 3, 2017
@h-kataria
Copy link
Contributor

looks good

@h-kataria h-kataria merged commit 28a0164 into ManageIQ:master Sep 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants