Add timeline_options to expose event_streams OPTIONS to the API #21552

jrafanie · 2021-11-10T23:18:48Z

Note, currently there are several methods in EventStream whose implementation is
for EmsEvent and should move there. We'll look at that when we need to
expose/test filtering on MiqEvent levels (success/failure) for the API.

jrafanie · 2021-11-10T23:23:26Z

app/models/ems_event.rb

@@ -1,5 +1,6 @@
 class EmsEvent < EventStream
  include_concern 'Automate'
+  supports :timeline


I didn't know what to call this feature so I went with timeline. Basically, we need to ensure the other EventStream subclasses, such as CustomEvent or RequestEvent, don't show up in the payload until we support them for timelines.

timeline seems fine - @agrare opinions?

It's a little weird to be using supports for this, because it's not really a provider feature, but I think I'm ok with that.

I'm not against using supports for non provider features. If this is just a single subclass check though could you just use EmsEvent for the query? (just a first take without reading how you're using this yet)

yeah, supports is really about the outside callers (like UI and API) having an interface for asking if a feature is supported and being able to filter on it as well as getting why it's not supported for presentation purposes. In this case, that's not what it's being used for, as we aren't going to be presenting anything in the UI for the non-supported classes.

I'm thinking this is much cleaner with a simpler subclasses.select { |e| e.respond_to?(:group_levels_and_details) } construct.

See my comment here too: https://github.com/ManageIQ/manageiq/pull/21552/files#r747633985

Note, the motivation was something different than what the UI was doing, namely:

record.has_events? || record.has_events?(:policy_events) # EmsEvent || MiqEvent

joerafaniello@Joes-MBP-2 manageiq-ui-classic % git grep has_events app/helpers/application_helper/button/availability_zone_timeline.rb: @error_message = _("No Timeline data has been collected for this Availability Zone") unless @record.has_events? app/helpers/application_helper/button/container_timeline.rb: @record.has_events? || @record.has_events?(:policy_events) app/helpers/application_helper/button/ems_cluster_timeline.rb: @error_message = _("No Timeline data has been collected for this Cluster") unless @record.has_events? || @record.has_events?(:policy_events) app/helpers/application_helper/button/ems_storage_timeline.rb: unless @record.has_events? || @record.has_events?(:policy_events) app/helpers/application_helper/button/ems_timeline.rb: unless @record.has_events? || @record.has_events?(:policy_events) app/helpers/application_helper/button/host_timeline.rb: @error_message = _("No Timeline data has been collected for this Host") unless @record.has_events? || @record.has_events?(:policy_events) app/helpers/application_helper/button/miq_template_timeline.rb: unless @record.has_events? || @record.has_events?(:policy_events) app/helpers/application_helper/button/physical_server_timeline.rb: unless @record.has_events? app/helpers/application_helper/button/vm_timeline.rb: @record.has_events? || @record.has_events?(:policy_events)

Granted, the UI is just checking for events for display purposes but obviously, if they wanted to display CustomEvent, CustomButtonEvent, or RequestEvent, these conditionals would grow gnarly.

For the backend code, we don't care about having events for display purposes. We only want to provide the details about options for classes that support timelines. 🤷

I'm thinking this is much cleaner with a simpler subclasses.select { |e| e.respond_to?(:group_levels_and_details) } construct.

I guess I can try doing this in EventStream

def self.timeline_classes subclasses.select { |e| e.respond_to?(:group_levels_and_details) } end

I can worry about what the UI does with checking if there are events for record later.

This should be done, I dropped the private method and simplified things.

jrafanie · 2021-11-11T13:14:25Z

spec/models/event_stream_spec.rb

+
+      expect(options["MiqEvent"].keys.sort).to eq %w[description group_levels group_names]
+      expect(options["MiqEvent"]["description"]).to eq(MiqEvent.description)
+      expect(options["MiqEvent"]["group_levels"].keys.sort).to eq %w[failure success]


Does it make sense to to_i all nested keys to have consistent keys since the event_handling part of the settings.yml has symbols keys, or should the nested keys be strings to match how they'd be accessed over json?

Do you mean to_s all of the keys? If so, I'm not sure.

Yeah, I think I'll just use the existing settings.yml event_handling format and do symbols keys even though there's no such concept on the other side of json.

Either way, I don't like that there's currently both string and symbol keys based on what level of depth.

FYI, this is what I meant... I changed it in a commit I just pushed:

Change mixed string / symbol keys to all symbol keys Before: ``` {"EmsEvent"=> {"description"=>"Management Events", "group_names"=> {:addition=>"Creation/Addition", ... :security=>"Security"}, "group_levels"=> {"critical"=>"Critical", "detail"=>"Detail", "warning"=>"Warning"}}, "MiqEvent"=> {"description"=>"Policy Events", "group_names"=> {"vm_process"=>"VM Lifecycle", ... "auth_validation"=>"Authentication Validation (Provider)"}, "group_levels"=>{"success"=>"Success", "failure"=>"Failure"}}} ``` After: ``` {:EmsEvent=> {:description=>"Management Events", :group_names=> {:addition=>"Creation/Addition", ... :security=>"Security"}, :group_levels=> {:critical=>"Critical", :detail=>"Detail", :warning=>"Warning"}}, :MiqEvent=> {:description=>"Policy Events", :group_names=> {:vm_process=>"VM Lifecycle", ... :auth_validation=>"Authentication Validation (Provider)"}, :group_levels=>{:success=>"Success", :failure=>"Failure"}}} ```

app/models/miq_event.rb

Fryguy · 2021-11-11T14:40:37Z

app/models/miq_event.rb

+
+  def self.group_names_and_levels
+    hsh = {}
+    hsh["group_names"] = MiqEventDefinitionSet.all.map {|s| [s.name, s.description]}.to_h


Suggested change

hsh["group_names"] = MiqEventDefinitionSet.all.map {|s| [s.name, s.description]}.to_h

hash["group_names"] = MiqEventDefinitionSet.all.pluck(:name, :description).to_h

Fryguy · 2021-11-11T14:41:14Z

app/models/miq_policy.rb

+  CONDITION_ALLOW = "Success"
+  CONDITION_DENY  = "Failure"


Suggested change

CONDITION_ALLOW = "Success"

CONDITION_DENY = "Failure"

CONDITION_ALLOW = N_("Success")

CONDITION_DENY = N_("Failure")

Fryguy · 2021-11-11T14:43:54Z

app/models/ems_event.rb

@@ -10,6 +11,24 @@ class EmsEvent < EventStream
    'Rename_Task',
  ]

+  def self.description
+    "Management Events"


Suggested change

"Management Events"

_("Management Events")

Fryguy · 2021-11-11T14:44:50Z

app/models/ems_event.rb

+  end
+
+  def self.group_names_and_levels
+    event_groups.each_with_object({}) do |arr, hsh|


I'm not sure what the array is here, but can we deconstruct it here and give the parameters names?

Something like:

Suggested change

event_groups.each_with_object({}) do |arr, hsh|

event_groups.each_with_object({}) do |(group_name, group_details), hsh|

Fryguy · 2021-11-11T16:13:43Z

app/models/event_stream.rb

@@ -35,14 +36,22 @@ class EventStream < ApplicationRecord

  after_commit :emit_notifications, :on => :create

+  supports_not :timeline, :reason => "Only subclasses can be on a timeline"


For example, this reason will never be presented in the UI nor API, which goes against why you'd need it.

Yeah, that's more for developers. I know supports is a provider concept but it describes what I wanted to do.

I originally had self.timeline_supported? => false in the EventStream class and only set self.timeline_supported? => true for MiqEvent and EmsEvent classes.

I'm not against using something else, but saw it and thought I was reinventing the wheel.

Changed to respond_to? in the timeline_options method.

Fryguy · 2021-11-11T16:16:19Z

app/models/event_stream.rb

  GROUP_LEVELS = %i(critical detail warning).freeze

+  def self.description
+    raise NotImplementedError, _("description must be implemented in a subclass")


I don't think we need to translate this Exception because it should never be presented to a user (i.e. getting a NotImplementedError raise in production is a bug that needs to be fixed). Mainly thinking about translators here, as they won't know what this means in order to translate it.

Fryguy · 2021-11-11T16:17:00Z

app/models/event_stream.rb

+  end
+
+  private_class_method def self.group_names_and_levels
+    raise NotImplementedError, _("group_names_and_levels must be implemented in a subclass")


Same here on the translation thing.

Fryguy · 2021-11-11T16:17:46Z

app/models/event_stream.rb

+    raise NotImplementedError, _("group_names_and_levels must be implemented in a subclass")
+  end
+
+  def self.event_stream_options


wonder if this should be named timeline_options, so it is clear what the purpose is for?

Good point. I guess that's the UI usage of it. I suppose various events could be presented in any format, not just a timeline. For example, you could just tableize the events, level, group/category, etc.

Good idea. I can't come up with a better name so I'm fine with using the only client of this.

Fryguy · 2021-11-16T14:29:11Z

app/models/ems_event.rb

+      group_details.each_key do |level|
+        level = level.to_s
+        next if level == 'name'
+
+        hash[:group_levels] ||= {}
+        hash[:group_levels][level.to_sym] ||= level.capitalize
+      end


Suggested change

group_details.each_key do |level|

level = level.to_s

next if level == 'name'

hash[:group_levels] ||= {}

hash[:group_levels][level.to_sym] ||= level.capitalize

end

group_details.each_key do |level|

next if level == :name

hash[:group_levels] ||= {}

hash[:group_levels][level.to_sym] ||= level.to_s.capitalize

end

Fryguy · 2021-11-16T14:32:10Z

app/models/event_stream.rb

  def group_name
    @group_name ||= self.class.group_name(group)
  end
+
+  def self.timeline_classes
+    @timeline_classes ||= EventStream.subclasses.select { |e| e.respond_to?(:group_names_and_levels) }


Could caching this cause issues with code reload since we are caching classes?

It's a relatively cheap operation that really should only create 1 new Array object, so I think we don't need caching here.

Sure, it was premature. The caller shouldn't be hitting this constantly and it's not a problem once classes are loaded.

Fryguy · 2021-11-16T14:35:03Z

app/models/event_stream.rb

+  end
+
+  def self.timeline_options
+    timeline_classes.map { |c| [c.name.to_sym, c.group_names_and_levels] }.to_h


Is this the only caller of timeline_classes? If so, then a) timeline_classes should be private and b) it might be cheaper to consolidate into something like:

Suggested change

timeline_classes.map { |c| [c.name.to_sym, c.group_names_and_levels] }.to_h

EventStream.subclasses.compact_map { |c| [c.name.to_sym, c.group_names_and_levels] if c.respond_to?(:group_names_and_levels) }.to_h

Sure, I can make it private.

I made it a method because timeline_classes describes what a class needs to do to implement timelines, namely, define group_names_and_levels. In the example b), it feels like we're mixing the concerns of "which classes support timelines" and "how to get the options for it".

Actually, no, I think it makes sense to be able to expose what classes support timelines.

Fryguy · 2021-11-16T14:36:23Z

app/models/miq_policy.rb

+  CONDITION_ALLOW = N_("Success")
+  CONDITION_DENY  = N_("Failure")


it's weird to me that the constant name is one thing but the text is something else. maybe:

Suggested change

CONDITION_ALLOW = N_("Success")

CONDITION_DENY = N_("Failure")

CONDITION_SUCCESS = N_("Success")

CONDITION_FAILURE = N_("Failure")

Yeah, I'm mixing policy terminology that prefers allow/deny over success/failure but I agree, it doesn't make sense at all here.

For issue: ManageIQ/manageiq-api#1090 Note, currently there are several methods in EventStream whose implementation is for EmsEvent and should move there. We'll look at that when we need to expose/test filtering on MiqEvent levels (success/failure) for the API.

jrafanie · 2021-11-16T20:09:36Z

Ok, applied the suggested changes, other than making timeline_classes private.

miq-bot · 2021-11-16T20:11:15Z

Checked commit jrafanie@97324f8 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
5 files checked, 0 offenses detected
Everything looks fine. 🍪

jrafanie added enhancement api labels Nov 10, 2021

jrafanie requested review from agrare, Fryguy and kbrock as code owners November 10, 2021 23:18

jrafanie assigned Fryguy Nov 10, 2021

jrafanie mentioned this pull request Nov 10, 2021

Expose MiqEvent/EmsEvent group/levels in event stream OPTIONS ManageIQ/manageiq-api#1099

Merged

3 tasks

jrafanie commented Nov 10, 2021

View reviewed changes

jrafanie commented Nov 11, 2021

View reviewed changes

Fryguy reviewed Nov 11, 2021

View reviewed changes

app/models/miq_event.rb Outdated Show resolved Hide resolved

Fryguy reviewed Nov 11, 2021

View reviewed changes

jrafanie force-pushed the event_stream_options_for_api branch from 20a87a3 to 7451687 Compare November 11, 2021 22:32

Fryguy reviewed Nov 16, 2021

View reviewed changes

jrafanie force-pushed the event_stream_options_for_api branch 2 times, most recently from d7a753a to 4c1dd49 Compare November 16, 2021 20:03

jrafanie changed the title ~~Add event_stream_options to expose OPTIONS to the API~~ Add timeline_options to expose event_streams OPTIONS to the API Nov 16, 2021

jrafanie force-pushed the event_stream_options_for_api branch from 4c1dd49 to 97324f8 Compare November 16, 2021 20:08

Fryguy approved these changes Nov 16, 2021

View reviewed changes

Fryguy merged commit 7077112 into ManageIQ:master Nov 16, 2021

jrafanie deleted the event_stream_options_for_api branch November 16, 2021 21:46

This was referenced Nov 17, 2021

Api for timeline charts. ManageIQ/manageiq-api#1090

Open

Fix group level and names for miq event #21569

Merged

	hsh["group_names"] = MiqEventDefinitionSet.all.map {\|s\| [s.name, s.description]}.to_h
	hash["group_names"] = MiqEventDefinitionSet.all.pluck(:name, :description).to_h

	event_groups.each_with_object({}) do \|arr, hsh\|
	event_groups.each_with_object({}) do \|(group_name, group_details), hsh\|

		@@ -35,14 +36,22 @@ class EventStream < ApplicationRecord

		after_commit :emit_notifications, :on => :create

		supports_not :timeline, :reason => "Only subclasses can be on a timeline"

	timeline_classes.map { \|c\| [c.name.to_sym, c.group_names_and_levels] }.to_h
	EventStream.subclasses.compact_map { \|c\| [c.name.to_sym, c.group_names_and_levels] if c.respond_to?(:group_names_and_levels) }.to_h

		CONDITION_ALLOW = N_("Success")
		CONDITION_DENY = N_("Failure")

Add timeline_options to expose event_streams OPTIONS to the API #21552

Add timeline_options to expose event_streams OPTIONS to the API #21552

Conversation

jrafanie commented Nov 10, 2021

jrafanie Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Fryguy Nov 11, 2021 • 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

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

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

jrafanie Nov 16, 2021 • 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

jrafanie commented Nov 16, 2021

miq-bot commented Nov 16, 2021

jrafanie Nov 10, 2021 •

edited

Loading

agrare Nov 11, 2021 •

edited

Loading

Fryguy Nov 11, 2021 •

edited

Loading

jrafanie Nov 16, 2021 •

edited

Loading