Add hash_expression to MiqAlert #15315

zakiva · 2017-06-06T14:56:07Z

As discussed in #14979, we want to allow a hash typed expression in the alert definition api.
We will have 2 separate columns, one for miq expression and another for hash expression. I'm keeping the interface of writing and reading the expression attribute to keep backward compatibility (so we can avoid refactoring major areas right now).

zakiva · 2017-06-06T15:02:22Z

@simon3z @moolitayer @cben @dkorn Please review

zakiva · 2017-06-06T15:07:40Z

@miq-bot add_label enhancement, alerts

imtayadeway · 2017-06-06T15:09:09Z

app/models/miq_alert.rb

@@ -73,10 +74,14 @@ def set_responds_to_events
    self.responds_to_events = events unless events.nil?
  end

+  def set_expression_type
+    self.expression_type = self.expression.class unless self.expression_type || self.expression.nil?


Is there a need to add this column over, say, memoizing this result? i.e.

def expression_type @expression_type ||= expression.class end

imtayadeway · 2017-06-06T15:18:43Z

I'm not sure I follow the rationale for adding an expression_type to this model. Generally, _type methods are a design smell. To me, this is overloading a column with two distinct domain concepts, and would be better expressed as a distinct column. I don't think adding a column (or future columns) to this table is an expense that needs to be mitigated in this way.

/cc @Fryguy @jrafanie

jrafanie · 2017-06-06T22:16:59Z

I'm not sure I follow the rationale for adding an expression_type to this model. Generally, _type methods are a design smell. To me, this is overloading a column with two distinct domain concepts, and would be better expressed as a distinct column. I don't think adding a column (or future columns) to this table is an expense that needs to be mitigated in this way.

/cc @Fryguy @jrafanie

I don't understand this PR or the related #14979, what feature is missing originally?

I'm concerned because MiqExpression is already complicated because it is essentially user input as an expression. Now, we want to add a user defined hash and have code decide when to use either one? I'm beyond confused. For those of us not on the original PR, can you give pseudo code of what doesn't work now, and how this PR enables that?

cben · 2017-06-07T07:31:05Z

I think we all agree "MiqExpression is already complicated" :-|
Unfortunately, we already have both normal MiqAlerts where expression is a (serialized) MiqExpressions and some MiqAlerts where expression is a (serialized) ruby hash :-(
#14979 description had an example of a hash alert having:

"expression": {"eval_method": "dwh_generic", "mode": "internal", "options": {} }

@moolitayer @dkorn are/will these used for anything beyond hawkular-evaluated alerts?

Our concrete problem is the hash ones can't be round-tripped through the API. (API returns either as json hash, losing the distinction; it converts any incoming hash to MiqExpression.)

This is one option out several ideas how to begin untangling this: #14979 (comment)
cc @abellotti

Talking of code smells, see also #14979 (comment) how miq_alert.rb is full of expression.kind_of? checks... I'm tempted to throw in couple more ideas (subclass MiqAlert itself, extract eval_method to a column), but I don't know enough.

moolitayer · 2017-06-07T08:44:05Z

This change was agreed upon[1] as a perquisite for the alert definitions PR. That happened after a very long time of trying to get feedback in various channels and makes perfect sense.

~~I'm ok with memoization instead of model changes.~~
EDIT: Actually after thinking about it memoization is fundamentally different from what was previously agreed - if we go down this route then the API would include the field to understand how to create the object - but would not persist it. That is a different solution suggested in #14979 (comment). That would also mean this PR is not needed.

@moolitayer @dkorn are/will these used for anything beyond hawkular-evaluated alerts?

Yes, all the automate_expressions in miq_alert are of type Hash

[1] #14979 (comment)

So I'm ok with 3. storing expression_type in model, this would allow us to add other types in future without changing schema.

moolitayer · 2017-06-07T13:47:40Z

@imtayadeway @abellotti Please review and let us know how to proceed with this and #14979 as soon as possible

imtayadeway · 2017-06-07T15:50:23Z

Unfortunately, we already have both normal MiqAlerts where expression is a (serialized) MiqExpressions and some MiqAlerts where expression is a (serialized) ruby hash

This seems to be the root of the problem. I'd like to know what is preventing us from fixing this?

I'm tempted to throw in couple more ideas (subclass MiqAlert itself, extract eval_method to a column)

If I understand this correctly, subclassing MIqAlert would create a Liskov substitution problem. To me, there is no getting around the fact that this column is storing two totally different things, that don't share an interface and can't be used interchangeably. I can't approve a change that preserves this behavior, at least without understanding why we can't address root problem above.

moolitayer · 2017-06-07T16:09:55Z

That goes against what was explicitly requested of us in #14979 (comment)

If you disagree with that please discuss it with @abellotti and let us know what you two decide.

chessbyte · 2017-06-08T12:58:26Z

db/migrate/20170605090919_add_expression_type_to_miq_alert.rb

@@ -0,0 +1,23 @@
+class ::MiqExpression; end


Looks like a major no-no!

Thanks! We suspected so but weren't sure what's a right way.
It's a serialized column, without a stub accessing it will instantiate lib/miq_expression.rb
(uncaught by good_migrations BTW, though it does fail when that (wrongly?) leads to requiring app/models/tag.rb.)
And namespacing the stub inside the migration serialize doesn't pick it. Is there a way to deserialize using custom class namespace?
Or manipulate the YAML with regexps like https://github.com/ManageIQ/manageiq/blob/master/db/migrate/20150625220141_fix_serialized_reports_for_rails_four.rb ?

I believe you can try serializing manually or specify a coder which will use your MiqExpression stub within the migration namespace:

https://www.viget.com/articles/how-i-used-activerecord-serialize-with-a-custom-data-type

I just remembered where we stubbed classes for deserialization (if that turns out to be needed):

manageiq/spec/migrations/20150625220141_fix_serialized_reports_for_rails_four_spec.rb

Lines 6 to 10 in 9fdcde9

stub_const("MiqReport", Class.new(ActiveRecord::Base))

stub_const("Ruport", Module.new)

stub_const("Ruport::Data", Module.new)

stub_const("Ruport::Data::Table", Class.new)

stub_const("Ruport::Data::Record", Class.new)

chessbyte · 2017-06-08T12:59:22Z

db/migrate/20170605090919_add_expression_type_to_miq_alert.rb

+    say_with_time('Add expression type to MiqAlert for existing alerts') do
+      MiqAlert.all.each do |alert|
+        alert.expression_type = Hash if alert.expression.kind_of?(Hash)
+        alert.expression_type = MiqExpression if alert.expression.kind_of?(MiqExpression)


what is this expression_type in SQL? A string? If so, I expect above 2 lines to set it this value to a string, and not a Ruby class.

If we have Ruby objects stored inside the serialized YAML, those are hard to solve in migrations. Easiest solution tends to be to treat the column like a regular string in the stub, and then just look for keywords in the string. For example:

class AddExpressionTypeToMiqAlert < ActiveRecord::Migration[5.0] class MiqAlert < ActiveRecord::Base end def up ... alert.expression_type = alert.expression.include?("MiqExpression") : "MiqExpression" : "Hash" ... end

If you end up doing the string inclusion style, then you can do this with update_all in two calls:

def up ... MiqAlert.where("expression LIKE ?", "%MiqExpression%").update_all(:expression_type => "MiqExpression") MiqAlert.where(:expression_type => nil).update_all(:expression_type => "Hash") end

zakiva · 2017-06-12T13:06:44Z

That goes against what was explicitly requested of us in #14979 (comment)

If you disagree with that please discuss it with @abellotti and let us know what you two decide.

@imtayadeway @abellotti if you don't have any further input, I'm continuing as agreed in #14979 (comment)

Fryguy · 2017-06-13T19:32:58Z

@imtayadeway I'd like for you to do the last review on this, and I'll do a final review before merge.

Fryguy · 2017-06-13T19:43:17Z

spec/migrations/20170605090919_add_expression_type_to_miq_alert_spec.rb

+    end
+
+    it 'alert with miq_expression has expression_type MiqExpression' do
+      miq_expression = YAML.load '--- !ruby/object:MiqExpression


This forces the object to be deserialized, which isn't necessary. You can just write this string directly into the column if you don't serialize in the stub in the first place a la https://github.com/ManageIQ/manageiq/pull/15315/files#r121777567

imtayadeway · 2017-06-13T20:55:11Z

@zakiva I asked above in #15315 (comment) what was preventing us from fixing the root problem that this PR is attempting to accommodate. Are you able to provide details of that, regardless of what was agreed upon with other parties? That would help enormously, thanks!

moolitayer · 2017-06-14T13:44:09Z

@zakiva I asked above in #15315 (comment) what was preventing us from fixing the root problem that this PR is attempting to accommodate. Are you able to provide details of that, regardless of what was agreed upon with other parties? That would help enormously, thanks!

@imtayadeway can you please explain what change you want to see here?

Fryguy · 2017-06-22T17:44:47Z

Database migrations have now been moved to the https://github.com/ManageIQ/manageiq-schema repo. Please see http://talk.manageiq.org/t/new-split-repo-manageiq-schema/2478 for instructions on how to transfer your database migrations. If this PR contains only migrations, I will leave it open for a short time during the transition, after which I will close this if it has not been moved over.

abonas · 2017-06-28T10:35:49Z

@lucasponce fyi - does this change/add anything for hawkular alerts?

lucasponce · 2017-06-28T10:47:17Z

@abonas on a quick look I think this change has not impact on the MiddlewareServer alerts evaluation, middleware classes are handled separately on
https://github.com/zakiva/manageiq/blob/dcff64a0688ed7f3133e53c8c2a5acc541c13406/app/models/miq_alert.rb#L576
and if I understood well the change, the new expression_type column will be used but different use cases.

moolitayer · 2017-06-28T10:57:48Z

This change should be backward compatible (if not it is a bug).

The implementation is going to change (we will have two separate columns for miq_expression and hash typed expressions)

expression is already sometimes an expression and sometimes a Hash (like for hawkular alerts) this PR is going to make the distinction clearer.

moolitayer · 2017-08-03T19:08:41Z

@zakiva I discussed this with @imtayadeway. What we want to see here at this point is two new columns in miq_alerts instead of the current one for miq expressions and one for hash expressions.

In another PR we will try to better encapsulate these types, we can start with the migration of that part.
What we will need is two new tables for these types, each with one column.

@imtayadeway is that accurate?
cc @simon3z

This work will allow us to continue with #14979 which will use to seed policies using ansible.

zakiva · 2017-08-15T14:02:12Z

What we want to see here at this point is two new columns in miq_alerts instead of the current one for miq expressions and one for hash expressions

@moolitayer @imtayadeway I updated the PR according to this approach, please review.

zakiva · 2017-08-21T08:24:54Z

@miq-bot rm_label sql migration

moolitayer · 2017-08-23T09:40:38Z

app/models/miq_alert.rb

+  def validate_single_expression
+    unless miq_expression.nil? || hash_expression.nil?
+      errors.add("Alert", "must not have both miq_expression and hash_expression set")
+    end


👍
It's worth validating that there is exactly one value. That would allow simplifying some code in this PR
(line 555 for example)

Validating that there is exactly one value means that you will not be able to have a nil expression which was possible until now, so I preferred not to change that behavior.

moolitayer · 2017-08-23T09:49:40Z

@zakiva I like that the outside interface has not changed.

moolitayer

LGTM 👍

moolitayer · 2017-08-23T09:51:21Z

@imtayadeway @Fryguy please review

simon3z

LGTM 👍

simon3z · 2017-08-25T10:45:58Z

Ping @imtayadeway @Fryguy

Fryguy · 2017-08-25T16:27:55Z

I re-reviewed the schema change and found some problems with it. Would like those resolve first.

Fryguy · 2017-08-25T16:31:52Z

app/models/miq_alert.rb

@@ -89,6 +103,12 @@ def validate_automate_expressions
    valid
  end

+  def validate_single_expression
+    unless miq_expression.nil? || hash_expression.nil?


Prefer if miq_expression && hash_expression, which to me is a lot clearer in that it is saying "if both things are set", as opposed to "if neither thing is not set" (double-negatives and all that)

Fryguy · 2017-08-25T16:40:07Z

app/models/miq_alert.rb

    when "internal" then return evaluate_internal(target, inputs)
    when "automate" then return evaluate_in_automate(target, inputs)
    when "script"   then return evaluate_script
-    else                 raise "unable to evaluate expression: [#{expression.inspect}], unknown mode"
+    else                 raise "unable to evaluate expression: [#{hash_expression.inspect}], unknown mode"


Maybe not for this PR, but we are mixing evaluation of 2 different expression types into a single method, and that is causing confusion. I think this would be a LOT clearer if the two evaluation strategies were split into methods, with eval_expression branching between those

def eval_expression(target, inputs = {}) case expression when MiqExpression then eval_miq_expression(target, inputs = {}) when Hash then eval_hash_expression(target, inputs = {}) else raise "unable to evaluate expression: [#{expression.inspect}], unknown format" end end

I think I'm also leaning towards using expression where possible in the places where it has been changed to individually check miq_expression, then hash_expression, then nil. Checking for each one in turn is sort of the purpose of the expression method itself.

However, in a method like my suggested eval_hash_expression, where you know you are working with hash_expression, then yes, you would access it directly.

@Fryguy This is defiantly the direction we are aiming at. As @imtayadeway pointed in an earlier discussion, having both expression types in the same column was the root problem here, which we hope to handle in this PR (and its related schema change). The next step will be trying to better encapsulate these types and their evaluation, maybe with two new tables each with one column (it will probably require more discussion). However, since this PR blocks some other items we are working on we would like to move forward with the current implementation as an initial solution if you are OK with it.

Sounds great @zakiva

miq-bot · 2017-08-30T16:18:13Z

Checked commit zakiva@97c94f6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 2 offenses detected

app/models/miq_alert.rb

❗ - Line 397, Col 72 - Style/WordArray - Use %w or %W for an array of words.
❗ - Line 596, Col 88 - Style/RedundantSelf - Redundant self detected.

Fryguy · 2017-08-31T15:23:17Z

Merged schema in ManageIQ/manageiq-schema#49

agrare · 2017-08-31T17:28:25Z

Merging 🔴 to fix db seeding on master

miq-bot added the sql migration label Jun 6, 2017

miq-bot added core/alerts enhancement labels Jun 6, 2017

imtayadeway reviewed Jun 6, 2017

View reviewed changes

chessbyte assigned Fryguy Jun 6, 2017

dkorn mentioned this pull request Jun 8, 2017

Alert Definitions API: add hash typed expression and symbolize options #14979

Closed

chessbyte reviewed Jun 8, 2017

View reviewed changes

Fryguy reviewed Jun 13, 2017

View reviewed changes

zakiva mentioned this pull request Aug 15, 2017

Add hash_expression to MiqAlert ManageIQ/manageiq-schema#49

Merged

zakiva force-pushed the alert_exp_type branch from dcff64a to 4786553 Compare August 15, 2017 12:42

zakiva changed the title ~~Add expression_type column to MiqAlert~~ Add hash_expression to MiqAlert Aug 15, 2017

zakiva force-pushed the alert_exp_type branch 2 times, most recently from e951ce1 to 204559a Compare August 20, 2017 12:53

miq-bot removed the sql migration label Aug 21, 2017

moolitayer reviewed Aug 23, 2017

View reviewed changes

moolitayer approved these changes Aug 23, 2017

View reviewed changes

simon3z approved these changes Aug 23, 2017

View reviewed changes

Fryguy reviewed Aug 25, 2017

View reviewed changes

zakiva force-pushed the alert_exp_type branch from 204559a to 97c94f6 Compare August 30, 2017 16:15

Add hash_expression to MiqAlert

97c94f6

Fryguy closed this Aug 31, 2017

Fryguy reopened this Aug 31, 2017

agrare merged commit ae77e85 into ManageIQ:master Aug 31, 2017

agrare added this to the Sprint 68 Ending Sep 4, 2017 milestone Aug 31, 2017

israel-hdez mentioned this pull request Sep 4, 2017

Changing bracket syntax for method/attribute syntax in update_alerts ManageIQ/manageiq-providers-hawkular#39

Merged

zakiva mentioned this pull request Sep 26, 2017

Alert Definitions: add hash_expression ManageIQ/manageiq-api#76

Merged

	stub_const("MiqReport", Class.new(ActiveRecord::Base))
	stub_const("Ruport", Module.new)
	stub_const("Ruport::Data", Module.new)
	stub_const("Ruport::Data::Table", Class.new)
	stub_const("Ruport::Data::Record", Class.new)

Add hash_expression to MiqAlert #15315

Add hash_expression to MiqAlert #15315

Conversation

zakiva commented Jun 6, 2017 • edited Loading

zakiva commented Jun 6, 2017

zakiva commented Jun 6, 2017

Choose a reason for hiding this comment

imtayadeway commented Jun 6, 2017

jrafanie commented Jun 6, 2017

cben commented Jun 7, 2017 • edited Loading

moolitayer commented Jun 7, 2017 • edited Loading

moolitayer commented Jun 7, 2017

imtayadeway commented Jun 7, 2017

moolitayer commented Jun 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakiva commented Jun 12, 2017

Fryguy commented Jun 13, 2017 • edited Loading

Fryguy Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

imtayadeway commented Jun 13, 2017

moolitayer commented Jun 14, 2017 • edited Loading

Fryguy commented Jun 22, 2017

abonas commented Jun 28, 2017

lucasponce commented Jun 28, 2017

moolitayer commented Jun 28, 2017

moolitayer commented Aug 3, 2017

zakiva commented Aug 15, 2017 • edited Loading

zakiva commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Aug 23, 2017

moolitayer left a comment

Choose a reason for hiding this comment

moolitayer commented Aug 23, 2017

simon3z left a comment

Choose a reason for hiding this comment

simon3z commented Aug 25, 2017

Fryguy commented Aug 25, 2017

Fryguy Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

zakiva Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Aug 30, 2017

Fryguy commented Aug 31, 2017

agrare commented Aug 31, 2017 • edited Loading

zakiva commented Jun 6, 2017 •

edited

Loading

cben commented Jun 7, 2017 •

edited

Loading

moolitayer commented Jun 7, 2017 •

edited

Loading

moolitayer commented Jun 7, 2017 •

edited

Loading

Fryguy commented Jun 13, 2017 •

edited

Loading

Fryguy Jun 13, 2017 •

edited

Loading

moolitayer commented Jun 14, 2017 •

edited

Loading

zakiva commented Aug 15, 2017 •

edited

Loading

Fryguy Aug 25, 2017 •

edited

Loading

Fryguy Aug 25, 2017 •

edited

Loading

zakiva Aug 30, 2017 •

edited

Loading

agrare commented Aug 31, 2017 •

edited

Loading