Reducing the data persited in the full_data columm #185

CharlleDaniel · 2018-05-23T14:09:15Z

This PR is able to:

Reduce the data persisted in the full_data column to save only the necessary data.
Fix the method get_last_ems_ref
- The method is using the function maximum to compare strings and return true for a case like as "8000" > "10000".

Fryguy · 2018-05-23T14:30:40Z

Does this require a data migration for existing full_data columns (put another way...have events already been released, and of so how do you deal with existing ones)?

Fryguy · 2018-05-23T14:32:32Z

app/models/manageiq/providers/lenovo/physical_infra_manager/event_catcher/stream.rb

-  def get_last_cnn_from_events(ems_id)
-    EventStream.where(:ems_id => ems_id).maximum('ems_ref') || 1
+  def get_last_ems_ref(ems_id)
+    EventStream.where(:ems_id => ems_id).maximum('CAST(ems_ref AS int)') || 1


ems_id is already a bigint in the database, so I don't understand the statement about fixing string comparisons.

hey, the maximum is applied on the ems_ref and it is a string.

so, I just cast the ems_ref to int

It sounds like you are doing something similar to what the chain_id column is for, so perhaps it's better to store it there? The purpose of the chain_id is to store a chain of events that make up a sequence, and all events in the same chain will have the same id. Additionally, the chain_id is typically a constantly growing id, which is perfect for finding the maximum of. @agrare Thoughts on using chain_id instead?

Additionally, my other concern here is that every event addition will require a somewhat complex operation. This table is one of our largest tables (multiple millions of rows) and maximum is a table scan when done in this way with casting.

Hm I think this is a little different than chaining events, all of these events are independent and it looks like get_last_ems_ref is used when asking the provider for events aka give me all events that occurred since X.

It looks like we call this get_last_cnn_from_events everytime we collect events but probably we should be calling that once when the collector starts up then save the highest cnn as a variable so we aren't hitting the event_streams table on every batch collection.

@agrare good point, I will try create a cache to improve performance.

CharlleDaniel · 2018-05-23T15:05:04Z

@Fryguy about you comment I think that refactor impact only the new events, because the old events still gonna have these extra data and I'm only reducing the number of things that be persisted in the full_data column. Also I investigate and we aren't using nothing of the full_data column today, before this PR we were saving all the attributes of the XClarityClient::Event object and now we only saving the interesting to Lenovo in the full_data.

The old full_data:

{
   "action": 100,
   "args": [],
   "bayText": "Not Available",
   "chassisText": "Not Available",
   "cn": "13",
   "commonEventID": "FQXHMCR0007I",
   "componentID": "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF",
   "eventClass": 800,
   "eventDate": "2017-04-13T13:32:25Z",
   "eventID": "FQXHMCR0007I",
   "eventSourceText": "Management",
   "failFRUs": "",
   "failSNs": "",
   "flags": "",
   "fruSerialNumberText": "Not Available",
   "localLogID": "",
   "localLogSequence": "",
   "location": "",
   "msg": "Management server date and time is synchronized to the NTP server.",
   "msgID": "",
   "mtm": "",
   "originatorUUID": "",
   "parameters": {   },
   "senderUUID": "",
   "serialnum": "",
   "service": 100,
   "serviceabilityText": "Not Required",
   "severity": 200,
   "severityText": "Informational",
   "sourceID": "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF",
   "sourceLogID": "",
   "sourceLogSequence": 0,
   "systemFruNumberText": "Not Available",
   "systemName": "Management Server",
   "systemSerialNumberText": "Not Available",
   "systemText": "Management Server",
   "systemTypeText": "Management",
   "systemTypeModelText": "Not Available",
   "timeStamp": "2017-04-13T13:32:25Z",
   "typeText": "System",
   "userid": "",
   "userIDIndex": 1
}

The new full_data:

{
  "event_type": "snmp_linkUp",
  "ems_ref": "47953",
  "source": "LenovoXclarity",
  "message": "The communication link with ifIndex 100001 is up.",
  "timestamp": "2018-05-18T01:41:35Z",
  "component_id": "00000000000010008000A48CDB984C00",
  "severity": 200,
  "severity_type": "Informational",
  "sender_uuid": "00000000000010008000A48CDB984C00",
  "sender_name": "ThinkAgile-VX-NE1032-SW02",
  "sender_model": "7159-HD1",
  "sender_type": "Switch",
  "type": "Switch"
}

Fryguy · 2018-05-31T17:04:14Z

@CharlleDaniel What is the purpose of the get_last_ems_ref?

CharlleDaniel · 2018-06-04T12:23:44Z

@Fryguy Morning, the method purpose is get the bigger ems_ref and use it to get the new events since this value (ems_ref). I'm just fixing the string comparison and changing the method name to make more sense.

agrare · 2018-06-04T12:41:50Z

app/models/manageiq/providers/lenovo/physical_infra_manager/event_catcher/stream.rb

+  def parse_events(events)
+    events.collect do |data|
+      event = ManageIQ::Providers::Lenovo::PhysicalInfraManager::EventCatcher::Event.new(data).to_hash
+      ManageIQ::Providers::Lenovo::PhysicalInfraManager::EventParser.event_to_hash(event, @ems.id)


What is the point of splitting these two up? The whole point of the EventParser is to parse the raw event into a hash so I don't see the utility in adding ManageIQ::Providers::Lenovo::PhysicalInfraManager::EventCatcher::Event.

I created this class to filter only the used attributes, but I will refactor this.

agrare · 2018-06-04T12:42:27Z

@CharlleDaniel what do you think about https://github.com/ManageIQ/manageiq-providers-lenovo/pull/185/files#r192171630 specifically only calling get_last_ems_ref once on startup?

agrare · 2018-06-05T12:14:57Z

app/models/manageiq/providers/lenovo/physical_infra_manager/event_catcher/stream.rb

@@ -54,7 +69,7 @@ def create_event_connection(ems)
                :port => ems.endpoints.first.port)
  end

-  def get_last_cnn_from_events(ems_id)
-    EventStream.where(:ems_id => ems_id).maximum('ems_ref') || 1
+  def bigger_ems_ref(ems_id)


I think last_event_ems_ref or something would be a better name.

@agrare It's fine, I changed this name to last_event_ems_ref, thanks 👍

agrare · 2018-06-05T12:19:22Z

app/models/manageiq/providers/lenovo/physical_infra_manager/event_catcher/stream.rb

+    end
+
+    # Update the @bigger_ems_ref with the new bigger ems_ref if to exist new events
+    if events.any?


Since the above loop doesn't filter out any events wouldn't this check be better up at the start of the function?
You can just return if events.blank?

@agrare so, could you review again ?

agrare · 2018-06-05T15:53:29Z

app/models/manageiq/providers/lenovo/physical_infra_manager/event_catcher/stream.rb

    fields
  end

+  def parse_events(events)


can you call this raw_events? Parsing events and returning events is confusing

sure, done.

miq-bot · 2018-06-05T15:53:31Z

Checked commits CharlleDaniel/manageiq-providers-lenovo@9e9a560~...cd1c7bf with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

agrare · 2018-06-05T15:53:53Z

app/models/manageiq/providers/lenovo/physical_infra_manager/event_catcher/stream.rb

    fields
  end

+  def parse_events(events)
+    if events.any?


prefer return if events.blank? instead of having the whole method in a single conditional

agrare · 2018-06-05T16:00:34Z

app/models/manageiq/providers/lenovo/physical_infra_manager/event_catcher/stream.rb

    fields
  end

+  def raw_events(events)


Oh no other way around, the method is fine I meant change the input and the output variables to have different names.

👍 and parse_events was a better method name

@agrare hahaha 😄 now I changed the variables name and back the method name to parse_events.

CharlleDaniel · 2018-06-05T16:08:59Z

@agrare Do you have any request change now? 😰

agrare · 2018-06-05T16:12:58Z

app/models/manageiq/providers/lenovo/physical_infra_manager/event_catcher/stream.rb

-  def get_last_cnn_from_events(ems_id)
-    EventStream.where(:ems_id => ems_id).maximum('ems_ref') || 1
+  def last_event_ems_ref(ems_id)
+    EventStream.where(:ems_id => ems_id).maximum('CAST(ems_ref AS int)')


This is going to be expensive but now that it is only done once on startup it should be manageable. If this proves to be an issue we can add a column to the events stream table to store this specifically.

Thank you. I think all the thinks are ok, but if we need in the future add a new column to store this, let me know I can do this. 👍

Reducing the data persited in the full_data columm

CharlleDaniel force-pushed the full_data_columm_cleaning branch from 8985d7a to 2b1dda5 Compare May 23, 2018 14:31

Fryguy reviewed May 23, 2018

View reviewed changes

CharlleDaniel force-pushed the full_data_columm_cleaning branch from 2b1dda5 to 521fe7d Compare May 23, 2018 14:36

CharlleDaniel force-pushed the full_data_columm_cleaning branch from 521fe7d to 412987a Compare May 23, 2018 15:10

CharlleDaniel changed the title ~~Cleaning of the data persited in the full_data columm~~ Reduce of the data persited in the full_data columm May 24, 2018

CharlleDaniel changed the title ~~Reduce of the data persited in the full_data columm~~ Reducing the data persited in the full_data columm May 24, 2018

agrare reviewed Jun 4, 2018

View reviewed changes

CharlleDaniel changed the title ~~Reducing the data persited in the full_data columm~~ [WIP] Reducing the data persited in the full_data columm Jun 4, 2018

miq-bot added the wip label Jun 4, 2018

agrare self-assigned this Jun 4, 2018

CharlleDaniel force-pushed the full_data_columm_cleaning branch from 2b3a6a3 to 331cb3d Compare June 4, 2018 20:39

Cleaning of the data persited in the full_data columm

9e9a560

CharlleDaniel force-pushed the full_data_columm_cleaning branch from 331cb3d to 28ba18f Compare June 4, 2018 20:56

agrare reviewed Jun 5, 2018

View reviewed changes

CharlleDaniel force-pushed the full_data_columm_cleaning branch 2 times, most recently from a4be0be to 618d58f Compare June 5, 2018 15:37

CharlleDaniel changed the title ~~[WIP] Reducing the data persited in the full_data columm~~ Reducing the data persited in the full_data columm Jun 5, 2018

miq-bot removed the wip label Jun 5, 2018

CharlleDaniel force-pushed the full_data_columm_cleaning branch 3 times, most recently from 4a3136e to cd1c7bf Compare June 5, 2018 15:47

agrare reviewed Jun 5, 2018

View reviewed changes

CharlleDaniel force-pushed the full_data_columm_cleaning branch 2 times, most recently from 1846ba6 to 1b4388a Compare June 5, 2018 15:59

agrare reviewed Jun 5, 2018

View reviewed changes

CharlleDaniel force-pushed the full_data_columm_cleaning branch 2 times, most recently from 1c7418f to c7f75ab Compare June 5, 2018 16:04

create a cache to persiste the bigger ems_ref

c7f75ab

agrare reviewed Jun 5, 2018

View reviewed changes

agrare approved these changes Jun 5, 2018

View reviewed changes

agrare merged commit c7f75ab into ManageIQ:master Jun 5, 2018

agrare added a commit that referenced this pull request Jun 5, 2018

Merge pull request #185 from CharlleDaniel/full_data_columm_cleaning

bb5f4ac

Reducing the data persited in the full_data columm

agrare added the enhancement label Jun 5, 2018

agrare added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reducing the data persited in the full_data columm #185

Reducing the data persited in the full_data columm #185

CharlleDaniel commented May 23, 2018 •

edited

Loading

Fryguy commented May 23, 2018

Fryguy May 23, 2018

CharlleDaniel May 23, 2018

CharlleDaniel May 23, 2018

Fryguy May 31, 2018

Fryguy May 31, 2018

agrare May 31, 2018

CharlleDaniel Jun 4, 2018 •

edited

Loading

CharlleDaniel commented May 23, 2018 •

edited

Loading

Fryguy commented May 31, 2018

CharlleDaniel commented Jun 4, 2018

agrare Jun 4, 2018

CharlleDaniel Jun 4, 2018 •

edited

Loading

agrare commented Jun 4, 2018

agrare Jun 5, 2018

CharlleDaniel Jun 5, 2018

agrare Jun 5, 2018

CharlleDaniel Jun 5, 2018

agrare Jun 5, 2018

CharlleDaniel Jun 5, 2018

agrare Jun 5, 2018

miq-bot commented Jun 5, 2018

agrare Jun 5, 2018

CharlleDaniel Jun 5, 2018

agrare Jun 5, 2018

agrare Jun 5, 2018

CharlleDaniel Jun 5, 2018

CharlleDaniel commented Jun 5, 2018

agrare Jun 5, 2018

CharlleDaniel Jun 5, 2018 •

edited

Loading

Reducing the data persited in the full_data columm #185

Reducing the data persited in the full_data columm #185

Conversation

CharlleDaniel commented May 23, 2018 • edited Loading

Fryguy commented May 23, 2018

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

CharlleDaniel Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

CharlleDaniel commented May 23, 2018 • edited Loading

Fryguy commented May 31, 2018

CharlleDaniel commented Jun 4, 2018

Choose a reason for hiding this comment

CharlleDaniel Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

agrare commented Jun 4, 2018

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

miq-bot commented Jun 5, 2018

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

CharlleDaniel commented Jun 5, 2018

Choose a reason for hiding this comment

CharlleDaniel Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

CharlleDaniel commented May 23, 2018 •

edited

Loading

CharlleDaniel Jun 4, 2018 •

edited

Loading

CharlleDaniel commented May 23, 2018 •

edited

Loading

CharlleDaniel Jun 4, 2018 •

edited

Loading

CharlleDaniel Jun 5, 2018 •

edited

Loading