Model changes for alerts #13233

moolitayer · 2016-12-18T16:10:15Z

Model changes extending ManageIQ alerting capability to support operation teams use cases:

A ticketing system where privileged users could assign alerts for users to work on, assigned users can acknowledge them and everyone can comment on them
Alerts could be coming in from external systems and can have severity / informational url attached to them
Alerts can represent a situation going bad (e.g agent reports overheating CPU) and they can represent the situation returning to normal again (e.g agent reports CPU temperature is normal again)

These new entities are used in #11962 and #13025

moolitayer · 2016-12-18T16:53:56Z

@miq-bot add_label enhancement
@simon3z FYI

moolitayer · 2016-12-19T14:46:24Z

@simon3z @Fryguy please review

moolitayer · 2016-12-25T10:08:47Z

@simon3z PTAL

gtanzillo

This looks good to me 👍 @Fryguy are you ok with merging this?

simon3z · 2017-01-10T08:51:36Z

app/models/miq_alert_status.rb

+  virtual_column :assignee, :type => :string
+
+  def assignee
+    miq_alert_status_actions.where(:action_type => %w(assign unassign)).last.try(:assignee)


@moolitayer no need for explicit ordering here? Is there a guarantee for the actions to be ordered or is it just a coincidence?

Also, to simplify, isn't unassign just an assign to nil? Can't you just use assign?

The order comes from the association there is a test for this here

I chose to use unassign since I must have an unacknowledge, for consistency

Perhaps encapsulate the :action_type => %w(assign unassign), or even more of this logic, in a MiqAlertStatusAction scope.

Ordered association: neat, though I've seen a recommendation to avoid implicit ordering, or capture it in separate scope: http://blog.carbonfive.com/2016/11/16/rails-database-best-practices/.

LGTM either way.

Perhaps encapsulate the :action_type => %w(assign unassign), or even more of this logic, in a MiqAlertStatusAction scope.

I could not find an elegant way of doing that, do you have a suggestion?

Ordered association: neat, though I've seen a recommendation to avoid implicit ordering, or capture it in separate scope: http://blog.carbonfive.com/2016/11/16/rails-database-best-practices/.

LGTM either way.

This seems to be a good use case since I do not have a consumer that would require a different order

Perhaps encapsulate the :action_type => %w(assign unassign), or even more of this logic, in a MiqAlertStatusAction scope.

Yeah, I forgot it's not purely actions responsibility, it's only actions that belong to this status, which is easier expressed on this side.
The most I can think of (untested):

# in MiqAlertStatusAction scope :assignee_changes, -> { where(:action_type => %w(assign unassign)) } # in MiqAlertStatus def assignee miq_alert_status_actions.assignee_changes.last.try(:assignee)

Fine as-is.

moolitayer · 2017-01-10T12:34:10Z

@enoodle @cben @zeari please review

cben

Looks pretty good. Didn't carefully review tests.

How is severity used? Do you plan a severity-changing action?
How is ancestry used?
I'm curious, why maintain acknowledged column in sync on miq_alert_statuses vs computing last ack/unack like you did for assignee?

cben · 2017-01-10T15:28:36Z

app/models/miq_alert_status.rb

+  virtual_column :assignee, :type => :string
+
+  def assignee
+    miq_alert_status_actions.where(:action_type => %w(assign unassign)).last.try(:assignee)


Perhaps encapsulate the :action_type => %w(assign unassign), or even more of this logic, in a MiqAlertStatusAction scope.

Ordered association: neat, though I've seen a recommendation to avoid implicit ordering, or capture it in separate scope: http://blog.carbonfive.com/2016/11/16/rails-database-best-practices/.

LGTM either way.

cben · 2017-01-10T15:33:04Z

app/models/miq_alert_status_action.rb

@@ -0,0 +1,30 @@
+class MiqAlertStatusAction < ApplicationRecord


I know we don't do comments in MIQ :-| but IMHO a few words on a model are worth it.
Specifically, mentioning that this table is intended to be append-only would be useful.

I'd be interested in having that enforced, did not find a way to do that. @kbrock do we have a mechanism for read only records?
They should only be created & deleted but never updated

We've been overriding active record callbacks.
I'm not thrilled with this method, but that is what we've been doing.
Maybe @Fryguy or @chrisarcand has an alternative way

It's easy to mark a relation as read-only, but read only implies you can't update or delete records. I believe you'll have to utilize callbacks as @kbrock mentions if you want to allow deletes but not updates.

@chrisarcand that might be exactly what I need, I need to allow deletion of status actions only when the owning status is destroyed. I could not find a way to use readonly except as a callback:

def readonly? !new_record? end

I have a feeling you meant something else?

cben · 2017-01-10T15:33:46Z

app/models/miq_alert_status_action.rb

+  after_save :update_status_acknowledgement
+
+  def only_assignee_can_acknowledge
+    if action_type == 'acknowledge' && (miq_alert_status.nil? || miq_alert_status.assignee.id != user.id)


why allow miq_alert_status.nil?

ah, you're not "allowing" it to be nil, you're considering it an error.
not sure it's needed (you have validation it's present, you assume it in other places) but harmless.

cben · 2017-01-10T15:47:00Z

app/models/miq_alert_status_action.rb

+    elsif "acknowledge" == action_type
+      miq_alert_status.update(:acknowledged => true)
+    end
+  end


If a non-assignee tries "acknowledge", is only_assignee_can_acknowledge validated before or after this function? Could .update(:acknowledged => true) write to DB before validation failed?

cben · 2017-01-10T15:52:41Z

app/models/miq_alert_status.rb

 class MiqAlertStatus < ApplicationRecord
+  SEVERITY_LEVELS = %w(error warning info).freeze


do you want :accept => SEVERITY_LEVELS validation?

Nice! I seem to have dropped it by mistake. Added a test as well

cben · 2017-01-10T15:55:42Z

app/models/miq_alert_status_action.rb

+
+  def update_status_acknowledgement
+    if %w(assign unassign unacknowledge).include?(action_type)
+      miq_alert_status.update(:acknowledged => false)


assign > ack > unassign would remove ack preserving only_assignee_can_acknowledge invariant. Good.

Can you re-"assign" to other (or even same) person without "unassign" first?

yes you can.

zeari · 2017-01-10T16:11:24Z

app/models/miq_alert_status.rb

+  end
+
+  def assigned?
+    !assignee.nil?


assignee.present? is much nicer

moolitayer · 2017-01-10T17:25:31Z

How is severity used? Do you plan a severity-changing action?

in the pushing side openshift collector logic should allow changing severity based on metadata, details not yet finalized.
can you explain what you mean by severity-changing action?

How is ancestry used?

ancestry will be used to link an alert status with its resolver. More details on it will be added in the resolve collection patch, but wanted to add the base here

I'm curious, why maintain acknowledged column in sync on miq_alert_statuses vs computing last ack/unack like you did for assignee?

Turns out it is quite hard to calculate, since we need to look at entire history of ack unack assign unassign actions Meaning there needs to be an ack that is not followed by unack assign or unassign. Since this property should be supplied in the API for every miq_alert_status I found It helpful to save on the alert status.

moolitayer · 2017-01-10T18:06:36Z

@cben @zeari thanks for reviewing. addressed comments, PTAL

cben · 2017-01-11T09:34:15Z

app/models/miq_alert_status_action.rb

+  after_save :update_status_acknowledgement
+
+  def only_assignee_can_acknowledge
+    if action_type == 'acknowledge' && (miq_alert_status.nil? || miq_alert_status.assignee.id != user.id)


ah, you're not "allowing" it to be nil, you're considering it an error.
not sure it's needed (you have validation it's present, you assume it in other places) but harmless.

cben · 2017-01-11T09:36:35Z

spec/models/miq_alert_status_spec.rb

+  let(:alert)                  { FactoryGirl.create(:miq_alert_status) }
+  let(:user1)                  { FactoryGirl.create(:user, :name => 'user1') }
+  let(:user2)                  { FactoryGirl.create(:user, :name => 'user2') }
+  let(:acknowledgement_ticket) do


s/ticket/action/g

cben · 2017-01-11T10:15:35Z

app/models/miq_alert_status.rb

+  virtual_column :assignee, :type => :string
+
+  def assignee
+    miq_alert_status_actions.where(:action_type => %w(assign unassign)).last.try(:assignee)


Perhaps encapsulate the :action_type => %w(assign unassign), or even more of this logic, in a MiqAlertStatusAction scope.

Yeah, I forgot it's not purely actions responsibility, it's only actions that belong to this status, which is easier expressed on this side.
The most I can think of (untested):

# in MiqAlertStatusAction scope :assignee_changes, -> { where(:action_type => %w(assign unassign)) } # in MiqAlertStatus def assignee miq_alert_status_actions.assignee_changes.last.try(:assignee)

Fine as-is.

moolitayer · 2017-01-11T16:05:02Z

closed by mistake

miq-bot · 2017-01-11T16:07:38Z

Checked commit moolitayer@8056603 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
8 files checked, 1 offense detected

db/migrate/20161113091851_add_miq_alert_status_actions.rb

❗ - Line 5, Col 26 - Style/ExtraSpacing - Unnecessary spacing detected.

moolitayer · 2017-01-11T17:04:24Z

@Fryguy PTAL

moolitayer · 2017-01-11T19:07:50Z

@simon3z @zeari PTAL

moolitayer · 2017-01-12T12:52:44Z

@Fryguy PTAL

cben · 2017-01-15T16:06:12Z

app/models/miq_alert_status_action.rb

+
+  def update_status_acknowledgement
+    if %w(assign unassign unacknowledge).include?(action_type)
+      miq_alert_status.update(:acknowledged => false)


Ah, I see, assign also clears ack, so assign > ack > reassign keeps the invariant. LGTM.

simon3z · 2017-01-17T10:59:11Z

@moolitayer @cben what's the reasons to have different approaches for assignment vs acknowledgement?

IIUC for the assignee you have a virtual column on MiqAlertStatus and for the acknowledgement instead you update a real column on MiqAlertStatus as after_save of MiqAlertStatusAction.

Couldn't you have a virtual column for acknowledgement as well?

moolitayer · 2017-01-17T18:34:15Z

app/models/miq_alert_status_action.rb

+  validates :assignee, :absence => true, :unless => "action_type == 'assign'"
+  validate :only_assignee_can_acknowledge
+
+  after_save :update_status_acknowledgement


@kbrock this saves a column on the owner of this action. Does it make sense to do it this way

moolitayer · 2017-01-17T18:42:45Z

@simon3z having a column for acknowledged is an optimization
I found that calculating it is complex and costly
ack = true iff:
there was an ack action, and since then there have been at most comment actions

simon3z · 2017-01-17T20:45:58Z

LGTM 👍
ping @Fryguy @chessbyte @gtanzillo @kbrock

Fryguy · 2017-01-18T01:39:53Z

app/models/miq_alert_status.rb

  belongs_to :miq_alert
  belongs_to :resource, :polymorphic => true
+  has_many :miq_alert_status_actions, -> { order "created_at" }, :dependent => :destroy
+  virtual_column :assignee, :type => :string


@moolitayer In a follow up PR you can optimize this with a :uses => :miq_alert_status_actions The :uses clause lets the reporting engine know to preload that. Alternately there might be a pure SQL way to do this, which perhaps @kbrock can help you with via the :arel => option.

submitted in #13553

Fryguy · 2017-01-18T01:42:43Z

app/models/miq_alert_status_action.rb

+  validates :miq_alert_status, :presence => true
+  validates :comment, :presence => true, :if => "action_type == 'comment'"
+  validates :assignee, :presence => true, :if => "action_type == 'assign'"
+  validates :assignee, :absence => true, :unless => "action_type == 'assign'"


I feel like there's a nicer way to do these conditionals without giving SQL fragments. @kbrock ?

ugh. sorry. MIA on this one

@moolitayer using a lambda here would make this better - essentially moving this check to ruby. well, at least the if part of the check, presence is in the database.

Sorry for taking long on this one, was unavailable for a while. Sent fix in #13797

Fryguy · 2017-01-18T01:43:35Z

@moolitayer I have a couple comments, but they can be addressed in followup PRs, perhaps with some guidance from @kbrock .

jrafanie · 2017-12-06T16:04:01Z

spec/factories/miq_alert_status_action.rb

+FactoryGirl.define do
+  factory :miq_alert_status_action do
+    action_type 'comment'
+    comment     'https://www.youtube.com/watch?v=dQw4w9WgXcQ'


I'm disappointed @chrisarcand and @Fryguy didn't see this. 👍 @moolitayer

I showed this to you because I was literally, perfectly rick-rolled looking through commits working on something else. 😅

The SOP shows up on the monitoring screen (it's supposed to help you solve the alert)
and we had a couple of rick rolling demos of it which was too much so I removed it from setup examples...

moolitayer changed the title ~~Alert tickets~~ Alert Model changes Dec 18, 2016

miq-bot added the sql migration label Dec 18, 2016

moolitayer force-pushed the alert_tickets branch 3 times, most recently from 6ea83f2 to f1a65dd Compare December 18, 2016 16:47

moolitayer mentioned this pull request Dec 18, 2016

Add alerts api #13025

Merged

moolitayer force-pushed the alert_tickets branch 3 times, most recently from dedf472 to a0083fc Compare December 19, 2016 13:37

chessbyte assigned Fryguy and simon3z Dec 19, 2016

moolitayer mentioned this pull request Dec 19, 2016

[WIP] Add assignee and severity to miq_alert_statuses #11049

Closed

miq-bot added the enhancement label Dec 20, 2016

moolitayer force-pushed the alert_tickets branch from a0083fc to 9e5cb2b Compare December 29, 2016 15:06

moolitayer changed the title ~~Alert Model changes~~ Model changes for alerts Dec 29, 2016

moolitayer mentioned this pull request Dec 29, 2016

Add alert actions api #13325

Merged

moolitayer force-pushed the alert_tickets branch from 9e5cb2b to 621d518 Compare January 5, 2017 12:03

gtanzillo approved these changes Jan 10, 2017

View reviewed changes

simon3z reviewed Jan 10, 2017

View reviewed changes

cben reviewed Jan 10, 2017

View reviewed changes

zeari reviewed Jan 10, 2017

View reviewed changes

app/models/miq_alert_status.rb

end

def assigned?

!assignee.nil?

Copy link

zeari Jan 10, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

assignee.present? is much nicer

moolitayer force-pushed the alert_tickets branch from 621d518 to c3e8873 Compare January 10, 2017 18:04

moolitayer force-pushed the alert_tickets branch from c3e8873 to dd7e092 Compare January 10, 2017 18:08

cben reviewed Jan 11, 2017

View reviewed changes

cben approved these changes Jan 11, 2017

View reviewed changes

moolitayer mentioned this pull request Jan 11, 2017

Purge miq alert statuses #13430

Closed

moolitayer closed this Jan 11, 2017

moolitayer force-pushed the alert_tickets branch from dd7e092 to 8520722 Compare January 11, 2017 16:03

Model changes for Alerts

8056603

moolitayer reopened this Jan 11, 2017

moolitayer closed this Jan 11, 2017

moolitayer reopened this Jan 11, 2017

cben reviewed Jan 15, 2017

View reviewed changes

moolitayer commented Jan 17, 2017

View reviewed changes

Fryguy reviewed Jan 18, 2017

View reviewed changes

Fryguy merged commit 02901ea into ManageIQ:master Jan 18, 2017

Fryguy added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 18, 2017

Fryguy added core core/alerts and removed core labels Jan 18, 2017

moolitayer mentioned this pull request Jan 18, 2017

Optimize virtual attribute by adding :uses clause #13553

Closed

moolitayer mentioned this pull request Mar 13, 2017

Miq Alert Status model changes for resolved alerts #14295

Merged

jrafanie reviewed Dec 6, 2017

View reviewed changes

		@@ -0,0 +1,30 @@
		class MiqAlertStatusAction < ApplicationRecord

		class MiqAlertStatus < ApplicationRecord
		SEVERITY_LEVELS = %w(error warning info).freeze

Model changes for alerts #13233

Model changes for alerts #13233

Conversation

moolitayer commented Dec 18, 2016 • edited Loading

moolitayer commented Dec 18, 2016

moolitayer commented Dec 19, 2016

moolitayer commented Dec 25, 2016

gtanzillo left a 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 Jan 10, 2017

cben left a 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

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

cben Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeari Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

moolitayer commented Jan 10, 2017 • edited Loading

moolitayer commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Jan 11, 2017

miq-bot commented Jan 11, 2017

moolitayer commented Jan 11, 2017

moolitayer commented Jan 11, 2017

moolitayer commented Jan 12, 2017

Choose a reason for hiding this comment

simon3z commented Jan 17, 2017

Choose a reason for hiding this comment

moolitayer commented Jan 17, 2017

simon3z commented Jan 17, 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

Fryguy commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Dec 18, 2016 •

edited

Loading

cben Jan 10, 2017 •

edited

Loading

zeari Jan 10, 2017 •

edited

Loading

moolitayer commented Jan 10, 2017 •

edited

Loading