Add CustomButton event emiter #17764

tumido · 2018-07-26T14:26:15Z

When CustomButton is invoked, let's create a CustomEvent logging the activity.

Tracked data:

requester (user, group, tenant)
target (id, class)
source (UI or API - currently UI only)
event type (button.trigger.start)
full data (task args, entry point for automate)

Design: GDoc
RFE: BUGZILLA 1511126
Related: ManageIQ/manageiq-ui-classic#4363

tumido · 2018-07-26T14:29:28Z

@miq-bot add_label events, enhancement, automate

tumido · 2018-07-26T14:32:22Z

@gmcculloug , @Fryguy can you please review and let me know if this would suit the needs of this RFE? Is CustomEvent the right record type or should I use rather MiqEvent?

bronaghs · 2018-07-26T15:12:00Z

@miq-bot assign @gmcculloug

tumido · 2018-07-26T17:35:51Z

cc @skateman

skateman · 2018-07-27T09:35:51Z

app/models/custom_button.rb

+        :args                 => args,
+        :automate_entry_point => resource_action.ae_path
+      }
+    )


The CustomEvent model and its parent EventStream is a little bit unclear for me. In the UI we're using EventMixin#has_events? for enabling/disabling timeline buttons for a given entity. This invokes an event_where_clause on the entity that doesn't always operate with the target. For example for providers, it's checking the ems_id that this method doesn't even set. For VMs it's a little trickier, depending on the event type, it checks the vm_or_template_id or the target.

So this is a little confusing ... the ugly solution would be to edit all the event_where_clause methods, but I really hope 🙏 🙏 there's a better solution for this.

ping @bronaghs as @tumido is on PTO and you will probably know who should I ask 😉

oh, probably @gmcculloug

These events are not intended to be displayed on the timelime. That might be the good and bad news. I'll updated the associated doc but the idea is to have a link from the item summary page that will show a list-view of these events.

As far as the modeling I would recommend creating a new event model that is derived from EventStream instead of using the CustomEvent model. Maybe CustomButtonEvent < EventStream

As this will be shared between a number of different models it would make sense to do most of this work in a new mixin.

cc @lfu

@dclarizio pinging you here as you asked me to do timelines 😉

dclarizio · 2018-08-02T15:50:00Z

@gmcculloug I seem to recall that this was mainly for debugging purposes and that we had considered just logging these custom button events. Can we just do that? (Not sure which log, but that's just a detail 😄 )

tumido · 2018-08-07T13:03:43Z

@gmcculloug can you please give me any resolution on this? How this RFE should be implemented?

Simple log entry, as @dclarizio suggests
Add a new model CustomButtonEvent and push the event records in there

Both ways are fine with me, I just need to know which path I should follow.

tumido · 2018-08-07T14:26:08Z

Unless it is decided otherwise, I've added the CustomButtonEvent model and updated the emitter to use it.

lfu · 2018-08-07T19:28:45Z

@gmcculloug If the purpose of this PR is to keep track of what custom buttons have been executed for a VM, maybe we can use AuditEvent which would create an audit_event object in VMDB and leave a log message in audit.log.

These issues should be addressed If we continue with CustomButtonEvent model.

event_type is the identifier of an EventStream object which does not change once created for all other EventStream objects.
:event_type => 'custom_button_event' would set all CustomButtonEvent objects with the same event_type. And seems it would be changed later to specify what playbook was launched.
CustomButtonEvent should be added into automate explorer under /System/Event.

tumido · 2018-08-09T13:28:15Z

@lfu, can you please explain me the first bullet point? I'm not sure what you're asking me to do. As the design document states this PR should cover the first step - catching the events.

To the second bullet point: I'm aware of this need, however I've decided to wait until we have a green light on the CustomButtomEvent model.

lfu · 2018-08-09T19:02:37Z

app/models/custom_button.rb

+      :message     => 'Custom button launched',
+      :source      => source,
+      :target_id   => target.id,
+      :target_type => target.type,


The above two lines can be just:

:target => target,

lfu · 2018-08-09T19:23:29Z

spec/models/custom_button_spec.rb

+            :source     => 'UI',
+            :target_id  => vm.id,
+            :type       => 'CustomButtonEvent',
+            :event_type => 'custom_button_event',


event_type is supposed to specify a uniq ID for the event instance. Now that we have a new class CustomButtonEvent, the event_type value seems redundant in this case.

We need to name event_type better for each CustomButtonEvent instance.

@lfu do you have any suggestions what we can do with that? I can't find any valid and reasonable information which would suit here and is available at the button invocation.

lfu · 2018-08-09T19:24:13Z

spec/models/custom_button_spec.rb

+          expect(CustomButtonEvent.count).to eq(1)
+          expect(CustomButtonEvent.first).to have_attributes(
+            :source     => 'UI',
+            :target_id  => vm.id,


We can also test :target_type here.

lfu · 2018-08-09T19:35:12Z

@tumido The first bullet point is more of a comment regarding the design doc.

Usually event_type is a uniq ID of the event instance. But in this case they would be the same for all CustomButtonEvent instances. And the doc mentioned event_type value would be changed later by automate.

gmcculloug · 2018-08-13T12:34:17Z

The purpose of this is to show a history of custom button actions for a resource. It was discussed and we do not want to build this off of the AuditEvent. Visually the Summary screen for the resource should have a link that takes you to a list-view showing the basic event properties for that resource.

lfu · 2018-08-14T16:19:49Z

spec/models/custom_button_spec.rb

+            :target_id   => vm.id,
+            :target_type => 'VmOrTemplate',
+            :type        => 'CustomButtonEvent',
+            :event_type  => 'custom_button_event',


event_type has changed.

tumido · 2018-08-14T21:34:37Z

@gmcculloug We've been discussing the event_type with @lfu here and on the automate PR ManageIQ/manageiq-content#398 a bit. What's your position on that?

We already have the event class named CustomButtonEvent and the event type should represent on kind of the event in that stream, do it seem redundant to have it named the same. Also in automate it looks silly to have System/Event/CustomButtonEvent with UI.class and custom_button_event in it.

These are the option we have:

Keep custom_button_event
Something else (my suggestion is button_triggered since this particular event is logging the invocation of a button)

What do you think?

gmcculloug · 2018-08-15T21:18:40Z

@lfu I would not get caught up in the assumption that the design doc needs to be followed exactly. It was a first pass to get ideas written out for discussion and scoping. For example, automate changing the event_type is a potential future feature. It is not in scope for this initial implementations.

I would agree that the redundant naming does not make a lot of sense and like the idea of using it to identify that the event is for a "on_launch" or "triggered" action.

Maybe we following something like the OpenStack naming which has *.start and *.end events to support the possibility of having ending events later.

button.trigger.start for now and it could have a corresponding button.trigger.end in the future.

lfu · 2018-08-16T13:26:09Z

button.trigger.start is a good name 👍

miq-bot · 2018-08-16T20:32:23Z

Checked commit tumido@b9cc5a6 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. 👍

gmcculloug

@tumido This PR looks good. Are you planning to add an association between the resources and this new event class? Would that happen in a separate PR?

tumido · 2018-08-17T20:49:07Z

@gmcculloug, sorry I was not planning to do that. I'm actually not sure what's the complete list of resources to associate the events with. Since I'm on PTO next week, can you please delegate that to someone else? Thanks!

gmcculloug · 2018-08-20T12:38:10Z

@tumido Thanks. I will discuss remaining backend work with @lfu.

gmcculloug · 2018-08-20T23:37:34Z

Talk to @lfu. She is going to continue the backend work in another PR. 👍

tumido mentioned this pull request Jul 26, 2018

Add a source definition when invoking CustomButton ManageIQ/manageiq-ui-classic#4363

Merged

miq-bot added automate enhancement core/events labels Jul 26, 2018

miq-bot assigned gmcculloug Jul 26, 2018

skateman reviewed Jul 27, 2018

View reviewed changes

miq-bot added the question label Jul 27, 2018

gmcculloug requested a review from lfu August 2, 2018 13:23

tumido force-pushed the ansible_reporting branch from 5e4160e to 9cea6c4 Compare August 7, 2018 14:24

tumido force-pushed the ansible_reporting branch from 9cea6c4 to 63dcfc0 Compare August 7, 2018 14:28

lfu reviewed Aug 9, 2018

View reviewed changes

tumido force-pushed the ansible_reporting branch from 63dcfc0 to 87dfd93 Compare August 13, 2018 15:01

tumido mentioned this pull request Aug 13, 2018

Add CustomButtonEvent to automate explorer ManageIQ/manageiq-content#398

Merged

tumido force-pushed the ansible_reporting branch 2 times, most recently from 7e1cada to 06ad2d5 Compare August 14, 2018 13:02

lfu reviewed Aug 14, 2018

View reviewed changes

tumido force-pushed the ansible_reporting branch from 06ad2d5 to ac9f797 Compare August 14, 2018 17:56

Add CustomButtonEvent model and emitter

b9cc5a6

tumido force-pushed the ansible_reporting branch from ac9f797 to b9cc5a6 Compare August 16, 2018 20:26

lfu approved these changes Aug 16, 2018

View reviewed changes

gmcculloug reviewed Aug 17, 2018

View reviewed changes

gmcculloug merged commit c08b60e into ManageIQ:master Aug 20, 2018

gmcculloug added this to the Sprint 93 Ending Aug 27, 2018 milestone Aug 20, 2018

This was referenced Aug 21, 2018

Changes to CustomButtonEvent. #17885

Merged

Populate timestamp of CustomButtonEvent. #17899

Merged

lfu mentioned this pull request Aug 29, 2018

Add CustomButtonEvent association to GenericObject. #17924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CustomButton event emiter #17764

Add CustomButton event emiter #17764

tumido commented Jul 26, 2018 •

edited

Loading

tumido commented Jul 26, 2018

tumido commented Jul 26, 2018

bronaghs commented Jul 26, 2018

tumido commented Jul 26, 2018

skateman Jul 27, 2018

skateman Jul 27, 2018

skateman Jul 27, 2018

gmcculloug Aug 2, 2018

skateman Aug 2, 2018

dclarizio commented Aug 2, 2018

tumido commented Aug 7, 2018

tumido commented Aug 7, 2018

lfu commented Aug 7, 2018

tumido commented Aug 9, 2018

lfu Aug 9, 2018

lfu Aug 9, 2018

tumido Aug 13, 2018

lfu Aug 9, 2018

lfu commented Aug 9, 2018

gmcculloug commented Aug 13, 2018

lfu Aug 14, 2018

tumido commented Aug 14, 2018

gmcculloug commented Aug 15, 2018

lfu commented Aug 16, 2018

miq-bot commented Aug 16, 2018

gmcculloug left a comment

tumido commented Aug 17, 2018

gmcculloug commented Aug 20, 2018

gmcculloug commented Aug 20, 2018

Add CustomButton event emiter #17764

Add CustomButton event emiter #17764

Conversation

tumido commented Jul 26, 2018 • edited Loading

tumido commented Jul 26, 2018

tumido commented Jul 26, 2018

bronaghs commented Jul 26, 2018

tumido commented Jul 26, 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

dclarizio commented Aug 2, 2018

tumido commented Aug 7, 2018

tumido commented Aug 7, 2018

lfu commented Aug 7, 2018

tumido commented Aug 9, 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

lfu commented Aug 9, 2018

gmcculloug commented Aug 13, 2018

Choose a reason for hiding this comment

tumido commented Aug 14, 2018

gmcculloug commented Aug 15, 2018

lfu commented Aug 16, 2018

miq-bot commented Aug 16, 2018

gmcculloug left a comment

Choose a reason for hiding this comment

tumido commented Aug 17, 2018

gmcculloug commented Aug 20, 2018

gmcculloug commented Aug 20, 2018

tumido commented Jul 26, 2018 •

edited

Loading