Pass metadata from an EmsEvent to an alert #14136

moolitayer · 2017-03-02T15:23:15Z

Allow passing some additional metadata from an EmsEvent to the MiqAlertStatus it generates.

extracted from #12773

moolitayer · 2017-03-02T15:25:48Z

app/models/miq_alert.rb

@@ -193,16 +193,22 @@ def evaluate(target, inputs = {})
    # If we are alerting, invoke the alert actions, then add a status so we can limit how often to alert
    # Otherwise, destroy this alert's statuses for our target
    invoke_actions(target, inputs) if result
-    add_status_post_evaluate(target, result, inputs[:description])
-
+    add_status_post_evaluate(target, result, inputs[:ems_event])


the inputs[:description] is a bug I recently introduced in #13653

Can you extract this change into a different commit so info is retained on the change?

moolitayer · 2017-03-02T15:29:09Z

app/models/miq_alert.rb

+      event.try(:message),
+      event.try(:full_data).try(:[], :severity),
+      event.try(:full_data).try(:[], :url),
+    ]
    status = miq_alert_statuses.find_or_initialize_by(:resource => target)
    status.result = result
    status.ems_id = target.try(:ems_id)
    status.description = status_description || description


Actually this might interfere with existing alerts (change their message undesirably)
I can

only set it for events of a specific kind (datawarehouse_event)

use some easly distinguishable metadata that won't be there by mistake, e.g
event.try(:full_data).try(:[], :miq_alert_message)

I like only using it on datawarehouse_event. It generates an inconsistency, but it will help prepare migrating the change for other events if necessary.

I think I would prefer something like

event.try(:full_data).try(:[], :alert_status_description)

Due to the inconsistency you mentioned. @durandom what do you think?

how about only setting the message unless description.present?

won't work for me since we have a generic description on alerts
(might event be mandatory)

Then I think it would be best to go the route of

only set it for events of a specific kind (datawarehouse_event)

cfcosta

Looks good.

cfcosta · 2017-03-02T17:55:28Z

app/models/miq_alert.rb

@@ -193,16 +193,22 @@ def evaluate(target, inputs = {})
    # If we are alerting, invoke the alert actions, then add a status so we can limit how often to alert
    # Otherwise, destroy this alert's statuses for our target
    invoke_actions(target, inputs) if result
-    add_status_post_evaluate(target, result, inputs[:description])
-
+    add_status_post_evaluate(target, result, inputs[:ems_event])


Can you extract this change into a different commit so info is retained on the change?

cfcosta · 2017-03-02T17:56:42Z

app/models/miq_alert.rb

+      event.try(:message),
+      event.try(:full_data).try(:[], :severity),
+      event.try(:full_data).try(:[], :url),
+    ]
    status = miq_alert_statuses.find_or_initialize_by(:resource => target)
    status.result = result
    status.ems_id = target.try(:ems_id)
    status.description = status_description || description


I like only using it on datawarehouse_event. It generates an inconsistency, but it will help prepare migrating the change for other events if necessary.

moolitayer · 2017-03-03T13:02:58Z

Addressed comment

moolitayer · 2017-03-03T13:45:59Z

The same spec fails on master

$ git fetch upstream
$ git checkout upstream/master -B master
$ bundle exec bin/setup
$ bundle exec rspec spec/lib/miq_automation_engine/miq_ae_service_spec.rb
undefined method `attribute_names' for #<Module:0x00000017c4fba0>

durandom

I'm basically 👍 with it, although there are more and more providers specifics crawling into EmsEvent - but it's not the fault of this PR

@miq-bot assign @gtanzillo
@gtanzillo merge?

moolitayer · 2017-03-06T10:54:18Z

@durandom @gtanzillo we could potentially go for a different approach -
event.try(:full_data).try(:[], :miq_alert_status) # {:url => 'a', :description => 'b'}
and then override what ever is there.

Benefits:

not having provider specifics in EmsEvent
more extensive (can override all fields)

Drawbacks:

storing the same data twice

I have a feeling the current approach will require more PRs of this nature in the future.

gmcculloug · 2017-03-06T18:59:25Z

app/models/ems_event.rb

@@ -164,6 +164,16 @@ def self.first_chained_event(ems_id, chain_id)
    EmsEvent.where(:ems_id => ems_id, :chain_id => chain_id).order(:id).first
  end

+  def self.parse_event_metadata(event) # event might be nil!
+    status_description = nil
+    if event.try(:event_type) == "datawarehouse_alert"


Should this be an instance method? Then you do not have to worry about event being nil and the caller below can change to: event.parse_event_metadata if event.respond_to?(:parse_event_metadata)

great suggestion

moolitayer · 2017-03-07T09:44:29Z

app/models/miq_alert.rb

    result
  end

-  def add_status_post_evaluate(target, result, status_description)
+  def add_status_post_evaluate(target, result, event)
+    status_description, severity, url = event.parse_event_metadata if event.respond_to?(:parse_event_metadata)
    status = miq_alert_statuses.find_or_initialize_by(:resource => target)
    status.result = result
    status.ems_id = target.try(:ems_id)
    status.description = status_description || description


Is it worth to change the above to

status_description.blank? description : status_description

Prefer to do it in another pr though.

moolitayer · 2017-03-07T09:45:38Z

Addressed comments. @gmcculloug please take a look

moolitayer · 2017-03-07T14:48:36Z

I just added an override for another field: result.
That would be needed for #14121

durandom · 2017-03-08T09:56:44Z

app/models/miq_alert.rb

    status = miq_alert_statuses.find_or_initialize_by(:resource => target)
-    status.result = result
+    status.result = status_result.nil? ? result : status_result


can't this be just?

status.result = status_result || result

I wanted to override the value, if it is true or false, not if it is nil.
I came to realize that it's not a good idea to override the evaluation result,
and I'll take care of that as part of the evaluation. (removing out of this pr in favor of)

durandom · 2017-03-08T09:59:54Z

app/models/miq_alert.rb

    result
  end

-  def add_status_post_evaluate(target, result, status_description)
+  def add_status_post_evaluate(target, result, event)
+    status_description, severity, url, status_result =


could be just

status_description, severity, url, status_result = event.try!(:parse_event_metadata)

moolitayer · 2017-03-08T12:24:22Z

@durandom @gmcculloug reverted to a previous version since I don't think overriding result is a good idea.

miq-bot · 2017-03-08T14:52:59Z

Checked commits moolitayer/manageiq@3e2e70f~...0fbf598 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. ⭐

moolitayer commented Mar 2, 2017

View reviewed changes

moolitayer mentioned this pull request Mar 2, 2017

Event collection for the datawarehouse provider #12773

Merged

cfcosta reviewed Mar 2, 2017

View reviewed changes

moolitayer changed the title ~~Pass metadata from an EmsEvent to a new alert~~ Pass metadata from an EmsEvent to an alert Mar 3, 2017

moolitayer force-pushed the alert_meta branch from ccbf955 to ff52c69 Compare March 3, 2017 12:38

moolitayer closed this Mar 3, 2017

moolitayer reopened this Mar 3, 2017

moolitayer force-pushed the alert_meta branch 2 times, most recently from f9ab18e to e34ad70 Compare March 5, 2017 11:47

durandom approved these changes Mar 6, 2017

View reviewed changes

chessbyte assigned gmcculloug Mar 6, 2017

gmcculloug reviewed Mar 6, 2017

View reviewed changes

moolitayer force-pushed the alert_meta branch from e34ad70 to d6cdce9 Compare March 7, 2017 09:38

moolitayer commented Mar 7, 2017

View reviewed changes

moolitayer force-pushed the alert_meta branch from d6cdce9 to cdadf42 Compare March 7, 2017 14:47

durandom reviewed Mar 8, 2017

View reviewed changes

moolitayer force-pushed the alert_meta branch from cdadf42 to fb1b8ba Compare March 8, 2017 12:23

Mooli Tayer added 2 commits March 8, 2017 16:49

Fix event description extraction

3e2e70f

Pass metadata from an EmsEvent to an alert

0fbf598

moolitayer force-pushed the alert_meta branch from fb1b8ba to 0fbf598 Compare March 8, 2017 14:49

gmcculloug merged commit e910b42 into ManageIQ:master Mar 8, 2017

gmcculloug added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 8, 2017

gmcculloug added core/alerts enhancement core/events labels Mar 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass metadata from an EmsEvent to an alert #14136

Pass metadata from an EmsEvent to an alert #14136

moolitayer commented Mar 2, 2017 •

edited

Loading

moolitayer Mar 2, 2017

cfcosta Mar 2, 2017

moolitayer Mar 2, 2017 •

edited

Loading

cfcosta Mar 2, 2017

moolitayer Mar 3, 2017

durandom Mar 3, 2017 •

edited

Loading

moolitayer Mar 3, 2017

gtanzillo Mar 3, 2017

moolitayer Mar 3, 2017

cfcosta left a comment

cfcosta Mar 2, 2017

cfcosta Mar 2, 2017

moolitayer commented Mar 3, 2017

moolitayer commented Mar 3, 2017

durandom left a comment

moolitayer commented Mar 6, 2017 •

edited

Loading

gmcculloug Mar 6, 2017 •

edited

Loading

durandom Mar 6, 2017

moolitayer Mar 7, 2017

moolitayer commented Mar 7, 2017 •

edited

Loading

moolitayer commented Mar 7, 2017

durandom Mar 8, 2017

moolitayer Mar 8, 2017

durandom Mar 8, 2017

moolitayer commented Mar 8, 2017

miq-bot commented Mar 8, 2017

Pass metadata from an EmsEvent to an alert #14136

Pass metadata from an EmsEvent to an alert #14136

Conversation

moolitayer commented Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom Mar 3, 2017 • edited Loading

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

cfcosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Mar 3, 2017

moolitayer commented Mar 3, 2017

durandom left a comment

Choose a reason for hiding this comment

moolitayer commented Mar 6, 2017 • edited Loading

gmcculloug Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Mar 7, 2017 • edited Loading

moolitayer commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Mar 8, 2017

miq-bot commented Mar 8, 2017

moolitayer commented Mar 2, 2017 •

edited

Loading

moolitayer Mar 2, 2017 •

edited

Loading

durandom Mar 3, 2017 •

edited

Loading

moolitayer commented Mar 6, 2017 •

edited

Loading

gmcculloug Mar 6, 2017 •

edited

Loading

moolitayer commented Mar 7, 2017 •

edited

Loading