Default to a 0 evaluation frequency in dwh alerts #678

moolitayer · 2017-03-14T11:37:48Z

See ManageIQ/manageiq#14318 for explanation why a 0 notification frequency makes sense here

moolitayer · 2017-03-14T11:39:52Z

@miq-bot add_label bug, providers/containers

miq-bot · 2017-03-14T11:40:27Z

@moolitayer Cannot apply the following label because they are not recognized: providers/containers

moolitayer · 2017-03-20T14:42:47Z

@simon3z please review, this has no dependencies

moolitayer · 2017-03-20T15:18:40Z

app/controllers/miq_policy_controller/alerts.rb

@@ -486,7 +491,7 @@ def alert_build_pulldowns
      4.minutes.to_i => _("4 Minutes"), 5.minutes.to_i => _("5 Minutes"), 10.minutes.to_i => _("10 Minutes"),
      15.minutes.to_i => _("15 Minutes"), 30.minutes.to_i => _("30 Minutes"), 1.hour.to_i => _("1 Hour"),
      2.hours.to_i => _("2 Hours"), 3.hours.to_i => _("3 Hours"), 4.hours.to_i => _("4 Hours"),
-      6.hours.to_i => _("6 Hours"), 12.hours.to_i => _("12 Hours"), 1.day.to_i => _("1 Day")
+      6.hours.to_i => _("6 Hours"), 12.hours.to_i => _("12 Hours"), 1.day.to_i => _("1 Day"), 0.minutes.to_i => _("0")


@gtanzillo is it ok to add a 0 notification frequency for existing alerts types?

also note it won't be the default

moolitayer · 2017-03-20T15:57:00Z

@cben @zgalor please review

simon3z · 2017-03-20T16:24:48Z

@moolitayer does it deserve a better naming in the UI rather than "0"? Can we think of something more descriptive? (I don't know maybe "Immediate"?)
What about other alerts? Is this option available only for DWH?

moolitayer · 2017-03-21T08:10:25Z

@simon3z I'm not sure what's most usable here. I added a screen shot, the values are
Notification Frequency: 0, Notification Frequency: 1 Minute ...

Another option I can think of is Notification Frequency: Always Evaluate

simon3z · 2017-03-21T08:21:34Z

@moolitayer what about other alerts? Is this option available only for DWH?

@moolitayer for DWH what are the options you display? If "0" is the only one that makes sense, is it the only one available?

moolitayer · 2017-03-21T09:03:47Z

@moolitayer what about other alerts? Is this option available only for DWH?

I asked about that but I guess it is better to only have this options for dwh. Updated the PR.

@moolitayer for DWH what are the options you display? If "0" is the only one that makes sense, is it the only one available?

Updated. Note since our main use case is API I initial created ManageIQ/manageiq#14318 to block non zero alerts in the model

simon3z · 2017-03-21T09:08:19Z

@moolitayer are all these validations enforced for Rest-API as well? (I suppose they should be at model-level for that)

moolitayer · 2017-03-21T09:18:55Z

@simon3z since our main use case is API I initial created ManageIQ/manageiq#14318 to block non zero alerts in the model

simon3z · 2017-03-21T09:34:23Z

@moolitayer not sure what "Always Notify" means, could it be better saying "Notify Always"?
Also, Always/In some cases/Never is not an interval frequency so it doesn't seem to belong there.

moolitayer · 2017-03-21T09:35:45Z

Also, Always/In some cases/Never is not an interval frequency so it doesn't seem to belong there

So back to 0 then?

simon3z · 2017-03-21T09:42:18Z

Also, Always/In some cases/Never is not an interval frequency so it doesn't seem to belong there

So back to 0 then?

@moolitayer I wanted to come up with a better naming... what is this doing? This is evaluating the alerts as soon as received... so maybe something along the lines of "Upon Receipt", "When Received", "Immediate", etc. Maybe someone from UX can help us (provided that we explain what this is about).

moolitayer · 2017-03-21T12:00:27Z

Notification frequency does not describe what this value means unless i'm misreading it.

If an alert's generating event happened and it has not yet been this time since last evaluation[1], it is not evaluated (MiqAlertStatus will not be created, actions will not fire, etc). see https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_alert.rb#L161

Something parallel in ovirt engine's event log is named flood rate.

While this name is what it is the best I can think of is [Always Notify, Always Evaluate, 0, -1]

@simon3z cc

[1] for the same target object ofcurse

simon3z · 2017-03-22T11:04:12Z

@moolitayer since it seems that it's not really configurable (frequency is externally defined in the hawkular triggers) then we should probably hide the option altogether.

moolitayer · 2017-03-22T15:43:09Z

@simon3z nice, done

simon3z

LGTM 👍
cc @gtanzillo

@miq-bot assign gtanzillo

moolitayer · 2017-03-22T16:53:38Z

Rubocop fix

miq-bot · 2017-03-26T13:08:15Z

Checked commit moolitayer@4a4e8bf with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. ⭐

miq-bot · 2017-03-27T15:06:19Z

@moolitayer 'gtanzillo' is an invalid assignee, ignoring...

moolitayer · 2017-03-27T15:08:08Z

attempting to assign again per comment

@miq-bot assign gtanzillo

@gtanzillo Please take a look

miq-bot · 2017-03-27T15:08:26Z

@moolitayer 'gtanzillo' is an invalid assignee, ignoring...

gtanzillo · 2017-04-04T13:11:51Z

app/controllers/miq_policy_controller/alerts.rb

+    if (@edit[:new][:expression][:eval_method] && @edit[:new][:expression][:eval_method] == "hourly_performance") ||
+       @edit[:new][:exp_event] == "_hourly_timer_"
+      1.hour.to_i
+    elsif @edit[:new][:expression][:eval_method] && @edit[:new][:expression][:eval_method] == "dwh_generic"


@moolitayer Are you able to check :always_evaluate here instead? This way the Ui code can also be generic.

The UI code does not send or knows about :always_evaluate, that is only on the server side

@gtanzillo PTAL

@gtanzillo Please take another look.

Understood @moolitayer

gtanzillo · 2017-04-04T13:12:31Z

app/views/miq_policy/_alert_details.html.haml

          - if @edit
            .col-md-8
              - if @edit[:new][:expression][:eval_method] == "hourly_performance"
                - repeat = @sb[:alert][:hourly_repeat_times]
+              - elsif @edit[:new][:expression][:eval_method] == "dwh_generic"


Same here - would be better if you can check :always_evaluate here.

gtanzillo · 2017-04-05T18:54:11Z

app/controllers/miq_policy_controller/alerts.rb

+    if (@edit[:new][:expression][:eval_method] && @edit[:new][:expression][:eval_method] == "hourly_performance") ||
+       @edit[:new][:exp_event] == "_hourly_timer_"
+      1.hour.to_i
+    elsif @edit[:new][:expression][:eval_method] && @edit[:new][:expression][:eval_method] == "dwh_generic"


Understood @moolitayer

moolitayer · 2017-04-06T15:00:13Z

@dclarizio please review

moolitayer mentioned this pull request Mar 14, 2017

Always evaluate datawarehouse_alerts ManageIQ/manageiq#14302

Closed

miq-bot added the bug label Mar 14, 2017

moolitayer changed the title ~~Default to a 0 evaluation frequncy in dwh alerts~~ Default to a 0 evaluation frequency in dwh alerts Mar 14, 2017

moolitayer closed this Mar 14, 2017

moolitayer reopened this Mar 14, 2017

dclarizio added the compute/containers label Mar 15, 2017

dclarizio assigned simon3z Mar 15, 2017

moolitayer mentioned this pull request Mar 16, 2017

Always evaluate datawarehouse_alerts ManageIQ/manageiq#14318

Merged

moolitayer commented Mar 20, 2017

View reviewed changes

moolitayer force-pushed the dwh_frequency branch from 2eec229 to 67bc09e Compare March 21, 2017 09:04

moolitayer force-pushed the dwh_frequency branch from 67bc09e to 9507e9b Compare March 22, 2017 15:43

simon3z approved these changes Mar 22, 2017

View reviewed changes

moolitayer force-pushed the dwh_frequency branch from 9507e9b to 157a73a Compare March 22, 2017 16:53

moolitayer closed this Mar 22, 2017

moolitayer reopened this Mar 22, 2017

moolitayer force-pushed the dwh_frequency branch from 157a73a to 623fe75 Compare March 26, 2017 12:53

Default to a 0 evaluation frequncy in dwh alerts

4a4e8bf

moolitayer force-pushed the dwh_frequency branch from 623fe75 to 4a4e8bf Compare March 26, 2017 13:02

gtanzillo reviewed Apr 4, 2017

View reviewed changes

gtanzillo approved these changes Apr 5, 2017

View reviewed changes

dclarizio assigned dclarizio and unassigned simon3z Apr 6, 2017

dclarizio merged commit 2f6d6af into ManageIQ:master Apr 6, 2017

dclarizio added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to a 0 evaluation frequency in dwh alerts #678

Default to a 0 evaluation frequency in dwh alerts #678

moolitayer commented Mar 14, 2017 •

edited

Loading

moolitayer commented Mar 14, 2017

miq-bot commented Mar 14, 2017

moolitayer commented Mar 20, 2017

moolitayer Mar 20, 2017

moolitayer commented Mar 20, 2017

simon3z commented Mar 20, 2017

moolitayer commented Mar 21, 2017

simon3z commented Mar 21, 2017

moolitayer commented Mar 21, 2017

simon3z commented Mar 21, 2017

moolitayer commented Mar 21, 2017

simon3z commented Mar 21, 2017

moolitayer commented Mar 21, 2017

simon3z commented Mar 21, 2017

moolitayer commented Mar 21, 2017

simon3z commented Mar 22, 2017

moolitayer commented Mar 22, 2017

simon3z left a comment

moolitayer commented Mar 22, 2017

miq-bot commented Mar 26, 2017

miq-bot commented Mar 27, 2017

moolitayer commented Mar 27, 2017

miq-bot commented Mar 27, 2017

gtanzillo Apr 4, 2017

moolitayer Apr 4, 2017

moolitayer Apr 4, 2017

moolitayer Apr 5, 2017

gtanzillo Apr 5, 2017

gtanzillo Apr 4, 2017

gtanzillo Apr 5, 2017

moolitayer commented Apr 6, 2017

Default to a 0 evaluation frequency in dwh alerts #678

Default to a 0 evaluation frequency in dwh alerts #678

Conversation

moolitayer commented Mar 14, 2017 • edited Loading

moolitayer commented Mar 14, 2017

miq-bot commented Mar 14, 2017

moolitayer commented Mar 20, 2017

Choose a reason for hiding this comment

moolitayer commented Mar 20, 2017

simon3z commented Mar 20, 2017

moolitayer commented Mar 21, 2017

simon3z commented Mar 21, 2017

moolitayer commented Mar 21, 2017

simon3z commented Mar 21, 2017

moolitayer commented Mar 21, 2017

simon3z commented Mar 21, 2017

moolitayer commented Mar 21, 2017

simon3z commented Mar 21, 2017

moolitayer commented Mar 21, 2017

simon3z commented Mar 22, 2017

moolitayer commented Mar 22, 2017

simon3z left a comment

Choose a reason for hiding this comment

moolitayer commented Mar 22, 2017

miq-bot commented Mar 26, 2017

miq-bot commented Mar 27, 2017

moolitayer commented Mar 27, 2017

miq-bot commented Mar 27, 2017

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

Choose a reason for hiding this comment

moolitayer commented Apr 6, 2017

moolitayer commented Mar 14, 2017 •

edited

Loading