Always evaluate datawarehouse_alerts #14318
Conversation
|
@miq-bot add_label bug, providers/containers |
moolitayer
force-pushed
the
always_evaluate_dwh
branch
3 times, most recently
from
e933060
to
a38ba91
Compare
|
@simon3z please review |
|
@simon3z please review when you can |
|
@moolitayer I commented this in other PRs, I don't like the direction to embed specific logic of DWH in generic classes (as MiqAlert). Even more so if we do it by string comparison (a not even with a constant) 😢 |
app/models/miq_alert.rb
Outdated
| @@ -72,6 +73,14 @@ def set_responds_to_events | |||
| self.responds_to_events = events unless events.nil? | |||
| end | |||
|
|
|||
| def validate_delay_next_evaluation | |||
| next_frequency = (options || {}).fetch_path(:notifications, :delay_next_evaluation) | |||
| evaluation_method = expression[:eval_method] if expression.kind_of?(Hash) | |||
There was a problem hiding this comment.
The suffix if means evaluation_method may be uninitialized, relies on uninitialized vars being nil in Ruby (depends on textual order, works in this case). Personally I would write it another way, but that's a matter of opinion, your call.
E.g. scope everything under if expression.kind_of?(Hash) && expression[:eval_method] == "dwh_generic" on top.
There was a problem hiding this comment.
I will wait on this one since this will likely take a different approach all together per comment
app/models/miq_alert.rb
Outdated
| next_frequency = (options || {}).fetch_path(:notifications, :delay_next_evaluation) | ||
| evaluation_method = expression[:eval_method] if expression.kind_of?(Hash) | ||
| if !evaluation_method.nil? && evaluation_method == "dwh_generic" && !next_frequency.nil? && next_frequency != 0 | ||
| errors.add(:notifications, "Datawarehouse alerts must have a 0 notification frequency") |
There was a problem hiding this comment.
Confusing, "0 frequency" means never notify. Perhaps something like "0 notification interval"?
Also consider s/next_frequency/next_interval/ or next_delay...
There was a problem hiding this comment.
Yes those names are much better, unfortunately that is the name in the UI.
At this point we might ask to change that, see ManageIQ/manageiq-ui-classic#678 (comment)
|
@simon3z This refers to manageiq/app/models/miq_alert.rb Line 441 in a8c79fb |
|
@gtanzillo can you take a look and comment on the direction of this pr? can you suggest a direction for #14318 (comment) ? I'm really not sure what would be a good solution for this |
|
@moolitayer I'm also uncomfortable with special casing DWH alerts. But, a suitable alternative doesn't come to mind. I'll need to give it some thought. Also, can you elaborate what |
|
@gtanzillo yes delay_next_evaluation is only used here as far as I can see. Lets say X = alert.delay_next_evaluation > 0, and a first event goes into the system triggering alert actions. Skipping evaluation in our use case is never desirable as every incoming alert should be translated to a status regardless of time. |
|
@gtanzillo please review ManageIQ/manageiq-ui-classic#678 as well, it is closely related. |
app/models/miq_alert.rb
Outdated
| def validate_delay_next_evaluation | ||
| next_frequency = (options || {}).fetch_path(:notifications, :delay_next_evaluation) | ||
| evaluation_method = expression[:eval_method] if expression.kind_of?(Hash) | ||
| if !evaluation_method.nil? && evaluation_method == "dwh_generic" && !next_frequency.nil? && next_frequency != 0 |
There was a problem hiding this comment.
@moolitayer Instead of hardcoding dwh_generic here, how about adding a flag to the definition of the expression indicating that is should always be evaluated. Something like this -
diff --git a/app/models/miq_alert.rb b/app/models/miq_alert.rb
index f4db271..0027abf 100644
--- a/app/models/miq_alert.rb
+++ b/app/models/miq_alert.rb
@@ -442,7 +442,7 @@ class MiqAlert < ApplicationRecord
{:name => :value_mw_garbage_collector, :description => _("Duration Per Minute (ms)"), :numeric => true}
]},
{:name => "dwh_generic", :description => _("All Datawarehouse alerts"), :db => ["ContainerNode"], :responds_to_events => "datawarehouse_alert",
- :options => []}
+ :options => [], :always_evaluate => true}
]
endThen you can check that here with something like expression_by_name('dwh_generic')[:always_evaluate]. The same can be done on the UI side. What do you think?
moolitayer
force-pushed
the
always_evaluate_dwh
branch
from
a38ba91
to
7a20dc8
Compare
|
@gtanzillo thanks, please take a look |
|
@gtanzillo if I understand correctly this should not effect ManageIQ/manageiq-ui-classic#678 could you review that one as well please? (since only the :delay_next_evaluation and :name fields are sent in creation, the :always_evaluate is only on the backend ) |
moolitayer
force-pushed
the
always_evaluate_dwh
branch
from
7a20dc8
to
6d0abfa
Compare
|
@gtanzillo Please review |
|
Checked commit moolitayer@6d0abfa with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@gtanzillo Is this ready for merge or does it need additional reviews? |
Since hawkular alerts are evaluated in an external system, each alerts coming into the system already represents a created alert. Since an opening or resolving alert is picked only once up by the system, skipping an evaluation will always cause us to miss the alert. This PR works in conjunction with ManageIQ/manageiq-ui-classic#678 allowing to add 0 frequency alerts (although rest is more important for us)