Event collection for the datawarehouse provider #12773

moolitayer · 2016-11-21T16:01:06Z

The Datawarehouse manager added in #12205 collects events that end up belonging to other providers. Those events are passed to the target providers/objects based on metadata they contain. A new field was added to event streams to keep track of the ext_management_system that carried the event in.

miq-bot · 2016-12-09T16:02:15Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

moolitayer · 2016-12-11T19:31:41Z

@miq-bot remove_label wip
@miq-bot add_label events

moolitayer · 2016-12-11T19:32:01Z

@simon3z @lucasponce please review

simon3z · 2016-12-12T10:47:24Z

@moolitayer we poll for alerts right? How do we know if an alert was collected already or not? (You mentioned a timestamp once IIRC, although it would be great if we found something more reliable)

moolitayer · 2016-12-12T12:07:25Z

@simon3z we should never get the same alert since we always curl for alerts after the latest one we met.

moolitayer · 2016-12-12T12:49:04Z

@durandom rubocop is pissed off on my usage of loggers since they are global variables. Should that be ignored/resolved ?

durandom · 2016-12-12T13:19:56Z

loggers since they are global variables.

Add $dwh_log to .rubocop.yml

simon3z · 2016-12-12T14:55:02Z

@simon3z we should never get the same alert since we always curl for alerts after the latest one we met.

@moolitayer "after" time-wise? Meaning that you use a timestamp?

moolitayer · 2016-12-19T17:51:50Z

@moolitayer "after" time-wise? Meaning that you use a timestamp?

Yes @simon3z

moolitayer · 2017-03-06T17:11:32Z

@durandom @simon3z There are no more outstanding issues with this PR AFAIK. I also tested it just now and made sure it works (and fixed a small problem caused by missing parameter to blacklist?)

simon3z

LGTM 👍
cc @chessbyte

@miq-bot assign durandom
Ready for review/merge @durandom

simon3z · 2017-03-06T20:08:16Z

@cben can you review?

durandom

This has come a loooong way :)

LGTM

@cben if you are ok with the changes in the connect logic please assign it to @blomquisg to merge

moolitayer · 2017-03-07T12:10:06Z

bring missing settings section back
unite to use one settings section for event catching related properties

cben · 2017-03-07T12:24:59Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

-      klass = case type
+    def self.raw_connect(options)
+      options[:type] ||= :alerts
+      options[:tenant] ||= '_system'


This mutates the hash passed in. No immediate bug but IMO better avoided, e.g.

type = options[:type] || :alerts tenant = options[:tenant] || '_system'

and then use type, tenant below.

type = options.fetch(:type) { :alerts } tenant = options.fetch(:tenant) { '_system' }

This is the "more correct" way of doing this, because if the user sets the key to nil or false they mean it, and we such treat it as such.

cben · 2017-03-07T12:25:51Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

-        options[:type]
+      memo_options = options.slice(:type, :tenant)
+      @clients[memo_options.freeze] ||= self.class.raw_connect(
+        memo_options.merge(


Very nice 😃

cben · 2017-03-07T12:29:15Z

app/models/manageiq/providers/hawkular/datawarehouse_manager/event_catcher/runner.rb

+               :value_tag_name => 'nodename',
+             )
+             else
+               $datawarehouse_log.error("unexpected target: [#{tags[TAG_TYPE]}]")


what value does target take then?
add explicit nil line (if that's what you meant), or an early return.

cben · 2017-03-07T12:34:54Z

app/models/manageiq/providers/hawkular/datawarehouse_manager/event_catcher/runner.rb

+             end
+    instance = nil
+    instance = target.klass_name.constantize.find_by(target.find_key => tags[target.value_tag_name]) if target
+    $datawarehouse_log.error("Could not find hawkular alert target from tags: [#{tags}]") unless instance


I find the error logic hard to follow here. Actually all logic.
IIUC there is single happy path: type tag is "node" -> ContainerNode.find_by(:name => tags['nodename']).
Do you need all the indirection via an OpenStruct?

I expect this would get more complicated going forward (we are expecting more targets).
I prefer to express this complexity with data and not code (we can add more data and this code would work)

cben · 2017-03-07T12:37:41Z

app/models/manageiq/providers/hawkular/datawarehouse_manager/event_catcher/runner.rb

+
+  def event_to_hash(event, current_ems_id)
+    event.severity = map_severity(event.severity) # gets into event.full_data
+    event.url = event.tags[TAG_URL]


again, consider not mutating argument. I wouldn't expect side effects from a function named foo_to_hash.
can do :full_data => event.to_h.merge(...)

moolitayer · 2017-03-07T13:08:24Z

@cben thanks, addressed comments (and tested) can you give this another quick look? I'm hoping to be done with this soon

moolitayer · 2017-03-07T13:20:46Z

@blomquisg assigning to per #12773 (review), Please review/merge

@miq-bot assign @blomquisg

cfcosta · 2017-02-13T11:12:46Z

app/models/manageiq/providers/hawkular/common/event_catcher/runner_mixin.rb

@@ -0,0 +1,30 @@
+module ManageIQ::Providers::Hawkular::Common::RunnerMixin


Naming something *Mixin is poor ruby style. Not a blocker, but definitely something to keep in mind.

cfcosta · 2017-02-13T11:14:56Z

app/models/manageiq/providers/hawkular/common/event_catcher/stream_mixin.rb

@@ -0,0 +1,17 @@
+module ManageIQ::Providers::Hawkular::Common::StreamMixin


Why not implement http://ruby-doc.org/core-2.4.0/Enumerable.html into this mixin? That way you get more than just #each for free, while also having the code be idiomatic ruby.

cfcosta · 2017-02-27T15:13:48Z

app/models/manageiq/providers/hawkular/common/event_catcher/runner_mixin.rb

+  extend ActiveSupport::Concern
+
+  def log_handle
+    self.class.log_handle


I don't like naming things handle. It'd be better to have them as logger and event_monitor. The handle part has no semantic meaning, because you can understand without they are for without that part.

cfcosta · 2017-03-07T13:18:36Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

-      klass = case type
+    def self.raw_connect(options)
+      options[:type] ||= :alerts
+      options[:tenant] ||= '_system'


type = options.fetch(:type) { :alerts } tenant = options.fetch(:tenant) { '_system' }

This is the "more correct" way of doing this, because if the user sets the key to nil or false they mean it, and we such treat it as such.

blomquisg

Marking this Approved. I have a few small nitpicks, but generally, I'm fine with it.

blomquisg · 2017-03-07T14:26:56Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

@@ -38,38 +39,42 @@ def self.verify_ssl_mode
    end

    # Hawkular Client
-    def self.raw_connect(hostname, port, token, type = :alerts)
+    def self.raw_connect(options)
+      type = options[:type] || :alerts


In all other providers, this key is called :service. Does this have to be :type? Or, can it be :service instead?

EDIT
I just realized that the type was a preexisting method param. I won't hold up this PR on my comment here. @lucasponce can you take a note here and switch this to :service in a separate PR?

blomquisg · 2017-03-07T14:39:30Z

app/models/manageiq/providers/hawkular/common/event_catcher/runner_mixin.rb

+    end
+  end
+
+  def blacklist?(event_type)


blacklist? takes an event_type and whitelist? takes an event. Principle of least surprised suggests that these would take the same parameter type.

Also, a small nitpick. Past tense makes the names work better as predicate methods: blacklisted? and whitelisted? I don't really care if you ignore me on this one, just thought I'd point it out.

I addressed the blacklist|whitelist vs blacklisted|whitelisted issue.
As for the blacklist?(event_type) vs whitelist?(event) this stems from hawkular's current implementation. We had a long discussion on whether we can improve that (#12773 (comment) and above). It seems at this point we can not, but I will keep my my open for an opportunity.

The difference between event_type and event is because whitelist checks a raw event where it is a little harder to figure out the type

blomquisg · 2017-03-07T14:44:24Z

app/models/manageiq/providers/hawkular/datawarehouse_manager/event_catcher/runner.rb

+    when "CRITICAL" then 'error'
+    else
+      $datawarehouse_log.error("Could not map hawkular severity [#{hwk_severity}], using error")
+      'error'


What impact does severity have? Does it make more sense to just use "unknown"?

these seventies are carried to the seventies recently added to miq_alert_status (designed with three levels in mind). Since that is what the screen knows to handle, I prefer to go worst case if we cannot determine that.

miq-bot · 2017-03-07T15:07:30Z

Checked commits moolitayer/manageiq@cd0659a~...d9d2d03 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 0 offenses detected
Everything looks good. 🏆

durandom · 2017-03-08T07:33:16Z

@moolitayer 🏆 you did it 👏

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

Thanks @durandom 🍺

chessbyte added the wip label Nov 21, 2016

moolitayer force-pushed the datawarehouse_events branch 4 times, most recently from 27fc133 to 83678da Compare December 4, 2016 19:08

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

miq-bot added the unmergeable label Dec 9, 2016

moolitayer force-pushed the datawarehouse_events branch from 83678da to 36de491 Compare December 11, 2016 19:16

miq-bot removed the unmergeable label Dec 11, 2016

moolitayer force-pushed the datawarehouse_events branch 2 times, most recently from a7f5076 to 7548b17 Compare December 11, 2016 19:27

moolitayer changed the title ~~[WIP] Event collection for the datawarehouse provider.~~ Event collection for the datawarehouse provider. Dec 11, 2016

miq-bot added core/events and removed wip labels Dec 11, 2016

moolitayer force-pushed the datawarehouse_events branch 2 times, most recently from 5ca8055 to 792f659 Compare December 11, 2016 19:36

moolitayer force-pushed the datawarehouse_events branch from 792f659 to 38b0a99 Compare December 12, 2016 12:47

moolitayer force-pushed the datawarehouse_events branch from 38b0a99 to ccc8a33 Compare December 12, 2016 13:18

moolitayer force-pushed the datawarehouse_events branch from ccc8a33 to 60fe3e4 Compare December 12, 2016 13:27

chessbyte assigned simon3z Dec 12, 2016

moolitayer force-pushed the datawarehouse_events branch from 60fe3e4 to 42164eb Compare December 12, 2016 17:06

Fix no arg client creation

cd0659a

moolitayer force-pushed the datawarehouse_events branch from 831ee7c to b807a42 Compare March 6, 2017 17:08

moolitayer force-pushed the datawarehouse_events branch from b807a42 to ee719e6 Compare March 6, 2017 18:10

simon3z approved these changes Mar 6, 2017

View reviewed changes

durandom approved these changes Mar 6, 2017

View reviewed changes

cben reviewed Mar 7, 2017

View reviewed changes

moolitayer force-pushed the datawarehouse_events branch from dbb461f to 5b3ea33 Compare March 7, 2017 13:04

cben approved these changes Mar 7, 2017

View reviewed changes

miq-bot assigned blomquisg and unassigned simon3z Mar 7, 2017

cfcosta reviewed Mar 7, 2017

View reviewed changes

blomquisg approved these changes Mar 7, 2017

View reviewed changes

Mooli Tayer added 6 commits March 7, 2017 16:58

add common hawkular components

dd592ec

Support different tenants for hawkular client

0247262

Add skeletal hawkular event collection

3b0252b

Datawarehouse event collection

9434718

Add explict setting for polling interval

029fff1

Unify setting sections

d9d2d03

moolitayer force-pushed the datawarehouse_events branch from 5b3ea33 to d9d2d03 Compare March 7, 2017 15:04

blomquisg merged commit 36a423e into ManageIQ:master Mar 7, 2017

blomquisg added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 7, 2017

moolitayer mentioned this pull request Apr 9, 2017

[WIP] Tag seen alerts in hawkular #14671

Closed

		@@ -0,0 +1,30 @@
		module ManageIQ::Providers::Hawkular::Common::RunnerMixin

		@@ -0,0 +1,17 @@
		module ManageIQ::Providers::Hawkular::Common::StreamMixin

Event collection for the datawarehouse provider #12773

Event collection for the datawarehouse provider #12773

Conversation

moolitayer commented Nov 21, 2016 • edited Loading

miq-bot commented Dec 9, 2016

moolitayer commented Dec 11, 2016

moolitayer commented Dec 11, 2016

simon3z commented Dec 12, 2016 • edited Loading

moolitayer commented Dec 12, 2016

moolitayer commented Dec 12, 2016

durandom commented Dec 12, 2016

simon3z commented Dec 12, 2016 • edited Loading

moolitayer commented Dec 19, 2016

moolitayer commented Mar 6, 2017

simon3z left a comment

Choose a reason for hiding this comment

simon3z commented Mar 6, 2017

durandom left a comment

Choose a reason for hiding this comment

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

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 Mar 7, 2017

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

Choose a reason for hiding this comment

blomquisg 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

moolitayer Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Mar 7, 2017

durandom commented Mar 8, 2017

moolitayer commented Mar 8, 2017

moolitayer commented Nov 21, 2016 •

edited

Loading

simon3z commented Dec 12, 2016 •

edited

Loading

simon3z commented Dec 12, 2016 •

edited

Loading

moolitayer Mar 7, 2017 •

edited

Loading