Allow mapping event types to groups using regexes #17772

tadeboro · 2018-07-29T19:13:16Z

Up until now, in order to associate an event type with an event group, we needed to spell out events full name in settings file under appropriate event group category and level.

Some of the providers have hundreds of event types, which makes filling all this information in tedious and error prone. Not to mention that settings file is now one giant pile of event types.

In order to make process of registering event types scale a bit better, this commit adds support for registering a class of event types that match specified regular expressions. Simple example:

:ems:
  :ems_dummy:
    :event_handling:
      :event_groups:
        :addition:
          :critical:
          - dummy_event01_add
          - dummy_event02_add
          :detail:
          - dummy_event03_add
          - dummy_event04_add
          ...
          - dummy_event70_add
          - dm_event_create

This can now be written as

:ems:
  :ems_dummy:
    :event_handling:
      :event_groups:
        :addition:
          :critical:
          - dummy_event01_add
          - dummy_event02_add
          :detail:
          - dm_event_create
          :detail_regex:
          - ^dummy_[0-9]{2}_add$

Regular expression support has been added to all levels, so if there exists a level :lvl, then :lvl_regex is also available.

We took a great deal of care not to break existing code, so non-regex event type mappings take precedence over regular expression matches. For example, in our sample above, dummy_event_02_add gets assigned critical level despite also matching detail regex.

Note that if sets of event types that are matched by regular expressions are not disjoint, then there will be multiple valid event type mappings. In this case, event types that match more than one
group and/or level get assigned an arbitrary one.

@miq-bot assign @gtanzillo

/cc @miha-plesko @agrare @gberginc

miha-plesko · 2018-07-30T06:04:51Z

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1562414

miha-plesko

Thanks @tadeboro for the PR, it's great to see what kind of power we're about to get with regular expressions - I mean we'll assign Nuage events in a single line instead of prevous 1k 👍 Please see my suggestion how to make this PR more readable, looking forward to your opinion.

@gtanzillo @agrare so we had discussion with Fryguy on this topis some time (see #17295) and we agreed that REGEX support is mandatory in order to support Timelines for Nuage because Nuage has like 1000 event types. Kindly ask for review.

miha-plesko · 2018-07-30T06:31:54Z

app/models/event_stream.rb

@@ -36,7 +36,9 @@ class EventStream < ApplicationRecord

  after_commit :emit_notifications, :on => :create

-  GROUP_LEVELS = %i(critical detail warning).freeze
+  GROUP_LEVELS = %w(critical detail warning).map do |lvl|
+    [lvl.to_sym, (lvl + '_regex').to_sym]


I would prefer if we didn't go with 2D array at this point as it's affects code readability and introduces a bunch of annonymous arguments _ below. So IMAO we should just leave a simple %i(critical detail warning) here and update the group-matching .detect statement below to append _regex suffix on the fly, please see comment below.

You are going to have a hard time convincing me that placing a string concatenation inside a loop is a good idea, no matter the readability benefits. Especially since we would be doing same three concatenations over and over again.

If you encode the regex directly as mentioned in #17772 (comment), then I think you don't need to differentiate with an _regex special suffix.

So, I'm picturing a YAML that looks like

:ems: :ems_dummy: :event_handling: :event_groups: :addition: :critical: - dummy_event01_add - dummy_event02_add :detail: - dm_event_create - !ruby/regexp /^dummy_[0-9]{2}_add$/

Then when you loop over that you would do something like

GROUP_LEVELS.detect { |lvl| lvl.any? { |v| v === event_type } }

Note that === does a match? for regexps and == for Strings, so it's a nice operator that works for both.

I will definitely use the serialized version of regex, since this kills two problems I had in one hit: regex validation and avoiding regex construction on each self.group_and_level call.

As for the loop, I would still keep the two loops separated, since giving explicitly named event types precedence over regular expression matches since this gives us guarantee that regular expressions do not break existing functionality. For example, provider introducing a bad regular expression cannot "recategorise" some other provider's events.

This separation is independent of the place where regular expressions are listed, so we can still add them to the configuration file under existing levels.

Right, you can do strings, regexes = events.partition { |e| e.kind_of?(Regexp }, and then process those subcategories separately

I am afraid that partitioning will not work here, since iteration over strings and regexes do not happen inside the same one-level-up iteration. In pseudo-code, we determine the event type group and level like this:

for each event group: for each level: if event type is equal to one of the types in level, set level and group if group is not set: for each event group: for each level: if event type matches one of the type regexes in level, set level and group

miha-plesko · 2018-07-30T06:37:04Z

app/models/event_stream.rb

+
+    group = egroups.find do |_, value|
+      GROUP_LEVELS
+        .detect { |lvl, _| value[lvl]&.include?(event_type) }


Here I'd go with something like

.detect { |lvl| value[lvl]&.include?(event_type) || Regexp.new(value["#{lvl}_regex"]).match(event_type) }

and then we could omit all the egroups memoization, annonymous args _ and the .first.

This would break backwards compatibility, since badly formed regular expressions in one group of event types could take precedence over explicitly named events in other groups. And backwards compatibility is not something I would like to mess with.

Just to give you an example of bad situation, let us assume that one of the providers has this in its settings:

:ems: :ems_dummy: :event_handling: :event_groups: :addition: :critical_regex: - .+

If we are unlucky and this is the first category that is checked, all of the events will be mapped to :addition group with :critical level. This example is an exaggeration, but the the general principle still stands.

We have two loops here for a reason: we would like to give explicitly named types higher precedence over regular expressions.

miha-plesko · 2018-07-30T06:53:05Z

app/models/event_stream.rb

+
+    group ||= egroups.find do |_, value|
+      GROUP_LEVELS
+        .detect { |_, re_lvl| value[re_lvl]&.find { |regex| Regexp.new(regex).match(event_type) } }


So if regex is not given, we end up with Regexp.new(nil) which raises TypeError, right?

If my ruby serves me, then no. If value[re_lvl] returns nil, because regex was not given, then find is not executed at all.

Fryguy · 2018-08-06T15:34:33Z

I haven't read the details, but I wonder if it's better to put an actual Ruby regexp into the yaml, so it can be serialized and deserialized directly (with the side effect of verifying that it's a valid regex)

[1] pry(main)> require 'yaml'
[2] pry(main)> s = {:detail_regex => /^dummy_[0-9]{2}_add$/}.to_yaml
[3] pry(main)> puts s
---
:detail_regex: !ruby/regexp /^dummy_[0-9]{2}_add$/
[4] pry(main)> YAML.load(s)
=> {:detail_regex=>/^dummy_[0-9]{2}_add$/}

Fryguy · 2018-08-06T15:40:19Z

To add to #17772 (comment), you can have an event with simple chars like ., and we don't want those interpreted as regexes. If you encode the regex into the YAML then we can have strings with . that are not confused with regexes.

tadeboro · 2018-08-07T13:55:22Z

@Fryguy Thanks for valuable feedback. It is greatly appreciated. I implemented most of the enhancements you proposed. I only kept the regular expression precedence lower in order to have a tool to cherry-pick some of the event types into different groups/levels.

Fryguy · 2018-08-07T17:20:48Z

app/models/event_stream.rb

+
+    group = egroups.find do |_, value|
+      GROUP_LEVELS
+        .find { |lvl| value[lvl]&.select { |typ| typ.kind_of?(String) }&.include?(event_type) }


.select can't return nil, so you can remove the & before .include?

select is safe, but the [] is not. And if I am not mistaken, &. operator does not short-circuit, which means that include? will still get called on nil.

And this can be simplified to...

GROUP_LEVELS.detect { |lvl| value[lvl]&.any? { |typ| typ.kind_of?(String) && typ == event_type } }

(Technically you don't even need the .kind_of? check because Regexp can never == a String, but I can see leaving it for consistency)

nil&.include?("foo") # => nil

&. short-cirtcuits, but my point is that .select can never return nil. The &. before the select must stay as you have it though because value[lvl] can return nil.

> {a: [1, 2]}[:a].select { |_| false }.include?(1) => false > {a: [1, 2]}[:b].select { |_| false }.include?(1) NoMethodError: private method `select' called for nil:NilClass > {a: [1, 2]}[:b]&.select { |_| false }.include?(1) NoMethodError: undefined method `include?' for nil:NilClass

Now I am almost sure we need that last &.

Fryguy · 2018-08-07T17:22:11Z

app/models/event_stream.rb

+
+    group ||= egroups.find do |_, value|
+      GROUP_LEVELS
+        .find { |lvl| value[lvl]&.select { |typ| typ.kind_of?(Regexp) }&.find { |regex| regex.match(event_type) } }


Same here before the .find after the .select

Additionally, prefer regex.match?(event_type), since you don't actually need the value for the find call.

Actually, now that I'm looking deeper, this code finds the GROUP_LEVEL if any of the deeper arrays match. In this case, I think it's preferable to do...

GROUP_LEVELS.detect { |lvl| value[lvl]&.any? { |typ| typ.kind_of?(Regexp) && regex.match?(event_type) } }

I assumed that code needs to build against ruby 2.3 that has no match? listed in API docs. But maybe this is a documentation issue. Will fix this.

Oh wow you are right! I didn't realize .match? was Ruby 2.4 only, So yeah, leave it.

Fryguy · 2018-08-07T17:23:50Z

app/models/event_stream.rb

+      GROUP_LEVELS
+        .find { |lvl| value[lvl]&.select { |typ| typ.kind_of?(Regexp) }&.find { |regex| regex.match(event_type) } }
+        .tap { |level_found| level = level_found || level }
+    end&.first


Minor, but prefer .detect over .find. I'm surprised the original code had both, but .detect is preferred.

Sure. I did try to make it consistent, but being relatively new to ruby/MIQ, I managed to pick the wrong one.

Up until now, in order to associate an event type with an event group, we needed to spell out events full name in settings file under appropriate event group category and level. Some of the providers have hundreds of event types, which makes filling all this information in tedious and error prone. Not to mention that settings file is now one giant pile of event types. In order to make process of registering event types scale a bit better, this commit adds support for registering a class of event types that match specified regular expressions. Simple example: :ems: :ems_dummy: :event_handling: :event_groups: :addition: :critical: - dummy_event01_add - dummy_event02_add :detail: - dummy_event03_add - dummy_event04_add ... - dummy_event70_add - dm_event_create This can now be written as :ems: :ems_dummy: :event_handling: :event_groups: :addition: :critical: - dummy_event01_add - dummy_event02_add :detail: - dm_event_create - !ruby/regexp /^dummy_event[0-9]{2}_add$/ We took a great deal of care not to break existing code, so non-regex event type mappings take precedence over regular expression matches. For example, in our sample above, dummy_event_02_add gets assigned critical level despite also matching detail regex. Note that if sets of event types that are matched by regular expressions are not disjoint, then there will be multiple valid event type mappings. In this case, event types that match more than one group and/or level get assigned an arbitrary one.

miq-bot · 2018-08-09T03:43:28Z

Checked commit xlab-si@85e3008 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

miq-bot assigned gtanzillo Jul 29, 2018

tadeboro mentioned this pull request Jul 29, 2018

Allow registering events to timelines using regexes ManageIQ/manageiq-ui-classic#4375

Merged

tadeboro force-pushed the event-type-regex branch from c85a04c to 1371b59 Compare July 29, 2018 19:23

miha-plesko reviewed Jul 30, 2018

View reviewed changes

tadeboro force-pushed the event-type-regex branch 2 times, most recently from 95d892a to f44e95e Compare August 6, 2018 04:47

tadeboro mentioned this pull request Aug 6, 2018

[PROPOSAL] Per-provider Event Timelines With Regex Support #17295

Closed

Fryguy assigned Fryguy and unassigned gtanzillo Aug 6, 2018

Fryguy added the enhancement label Aug 6, 2018

tadeboro force-pushed the event-type-regex branch from f44e95e to d35dd63 Compare August 7, 2018 13:53

tadeboro force-pushed the event-type-regex branch from d35dd63 to 9b005ee Compare August 7, 2018 14:05

Fryguy reviewed Aug 7, 2018

View reviewed changes

tadeboro force-pushed the event-type-regex branch 3 times, most recently from 5f3487d to 3b3b07e Compare August 8, 2018 08:31

tadeboro force-pushed the event-type-regex branch from 3b3b07e to 85e3008 Compare August 9, 2018 03:35

Fryguy merged commit 2fefe62 into ManageIQ:master Aug 10, 2018

Fryguy added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 10, 2018

tadeboro deleted the event-type-regex branch August 10, 2018 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow mapping event types to groups using regexes #17772

Allow mapping event types to groups using regexes #17772

tadeboro commented Jul 29, 2018

miha-plesko commented Jul 30, 2018

miha-plesko left a comment

miha-plesko Jul 30, 2018

tadeboro Jul 30, 2018

Fryguy Aug 6, 2018

Fryguy Aug 6, 2018

tadeboro Aug 7, 2018

tadeboro Aug 7, 2018

Fryguy Aug 7, 2018

tadeboro Aug 7, 2018

miha-plesko Jul 30, 2018

tadeboro Jul 30, 2018

miha-plesko Jul 30, 2018

tadeboro Jul 30, 2018

Fryguy commented Aug 6, 2018 •

edited

Loading

Fryguy commented Aug 6, 2018 •

edited

Loading

tadeboro commented Aug 7, 2018

Fryguy Aug 7, 2018

tadeboro Aug 7, 2018

Fryguy Aug 7, 2018 •

edited

Loading

Fryguy Aug 7, 2018

tadeboro Aug 7, 2018 •

edited

Loading

Fryguy Aug 7, 2018

Fryguy Aug 7, 2018 •

edited

Loading

tadeboro Aug 7, 2018

Fryguy Aug 7, 2018

Fryguy Aug 7, 2018

tadeboro Aug 7, 2018

miq-bot commented Aug 9, 2018

Allow mapping event types to groups using regexes #17772

Allow mapping event types to groups using regexes #17772

Conversation

tadeboro commented Jul 29, 2018

miha-plesko commented Jul 30, 2018

miha-plesko 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

Choose a reason for hiding this comment

Fryguy commented Aug 6, 2018 • edited Loading

Fryguy commented Aug 6, 2018 • edited Loading

tadeboro commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeboro Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Aug 7, 2018 • 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

miq-bot commented Aug 9, 2018

Fryguy commented Aug 6, 2018 •

edited

Loading

Fryguy commented Aug 6, 2018 •

edited

Loading

Fryguy Aug 7, 2018 •

edited

Loading

tadeboro Aug 7, 2018 •

edited

Loading

Fryguy Aug 7, 2018 •

edited

Loading