Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pressing Copy this Report in a middleware report does nothing #1740

Closed
Fryguy opened this issue Jul 21, 2017 · 31 comments · Fixed by #2430
Closed

Pressing Copy this Report in a middleware report does nothing #1740

Fryguy opened this issue Jul 21, 2017 · 31 comments · Fixed by #2430

Comments

@Fryguy
Copy link
Member

Fryguy commented Jul 21, 2017

CloudIntel -> Reports -> Performance By Asset Type -> Middleware (Server or Datasources) -> pick a report -> Configuration -> Copy this Report

The screen blinks but nothing happens (I'm not sure if there are errors logged as I'm using a system that is not mine). This is because the Middleware reports are based on Live Metrics and are not creatable by the user. Since they are not creatable, they should not be Copyable, so we should probably grey out this button with a tooltip explanation.

@abonas Please review

@abonas
Copy link
Member

abonas commented Aug 13, 2017

@Fryguy why was this not labeled as middleware?
@miq-bot add_label middleware

@abonas
Copy link
Member

abonas commented Aug 13, 2017

@Fryguy regardless of the fact that nothing happens and that should be investigated - what do you mean by "not creatable so should not be copyable"? there are a lot of reports that are out of the box in miq (not created by user), so what do you define by "not creatable"?

@abonas
Copy link
Member

abonas commented Aug 13, 2017

the error is around this part
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/report_controller/reports/editor.rb#L1278

the exception that I see in the log is:
[----] D, [2017-08-13T15:09:14.489129 #7493:2d64e10] DEBUG -- : Classification Inst Including Associations (5.5ms - 103rows)
[----] F, [2017-08-13T15:09:14.499129 #7493:2d64e10] FATAL -- : Error caught: [NoMethodError] undefined method -' for nil:NilClass /home/abonas/code/manageiq/manageiq/plugins/manageiq-ui-classic/app/controllers/report_controller/reports/editor.rb:1265:in set_form_vars'
/home/abonas/code/manageiq/manageiq/plugins/manageiq-ui-classic/app/controllers/report_controller/reports/editor.rb:38:in miq_report_copy' /home/abonas/code/manageiq/manageiq/plugins/manageiq-ui-classic/app/controllers/application_controller/explorer.rb:182:in generic_x_button'
/home/abonas/code/manageiq/manageiq/plugins/manageiq-ui-classic/app/controllers/report_controller.rb:62:in x_button' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_controller/metal/basic_implicit_render.rb:4:in send_action'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/abstract_controller/base.rb:188:in process_action' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_controller/metal/rendering.rb:30:in process_action'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/abstract_controller/callbacks.rb:20:in block in process_action' /home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/callbacks.rb:126:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/callbacks.rb:506:in block (2 levels) in compile' /home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/callbacks.rb:455:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/callbacks.rb:101:in __run_callbacks__' /home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/callbacks.rb:750:in _run_process_action_callbacks'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/callbacks.rb:90:in run_callbacks' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/abstract_controller/callbacks.rb:19:in process_action'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_controller/metal/rescue.rb:20:in process_action' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_controller/metal/instrumentation.rb:32:in block in process_action'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/notifications.rb:164:in block in instrument' /home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/notifications/instrumenter.rb:21:in instrument'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/notifications.rb:164:in instrument' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_controller/metal/instrumentation.rb:30:in process_action'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_controller/metal/params_wrapper.rb:248:in process_action' /home/abonas/.rvm/gems/ruby-2.3.0/gems/activerecord-5.0.5/lib/active_record/railties/controller_runtime.rb:18:in process_action'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/abstract_controller/base.rb:126:in process' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionview-5.0.5/lib/action_view/rendering.rb:30:in process'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_controller/metal.rb:190:in dispatch' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_controller/metal.rb:262:in dispatch'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/routing/route_set.rb:50:in dispatch' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/routing/route_set.rb:32:in serve'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/journey/router.rb:39:in block in serve' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/journey/router.rb:26:in each'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/journey/router.rb:26:in serve' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/routing/route_set.rb:727:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/secure_headers-3.0.3/lib/secure_headers/middleware.rb:10:in call' /home/abonas/.rvm/gems/ruby-2.3.0/gems/rack-2.0.3/lib/rack/etag.rb:25:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/rack-2.0.3/lib/rack/conditional_get.rb:38:in call' /home/abonas/.rvm/gems/ruby-2.3.0/gems/rack-2.0.3/lib/rack/head.rb:12:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/rack-2.0.3/lib/rack/session/abstract/id.rb:232:in context' /home/abonas/.rvm/gems/ruby-2.3.0/gems/rack-2.0.3/lib/rack/session/abstract/id.rb:226:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/middleware/cookies.rb:613:in call' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/middleware/callbacks.rb:38:in block in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/callbacks.rb:97:in __run_callbacks__' /home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/callbacks.rb:750:in _run_call_callbacks'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/activesupport-5.0.5/lib/active_support/callbacks.rb:90:in run_callbacks' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/middleware/callbacks.rb:36:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/middleware/executor.rb:12:in call' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/middleware/remote_ip.rb:79:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/middleware/debug_exceptions.rb:49:in call' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/middleware/show_exceptions.rb:31:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/railties-5.0.5/lib/rails/rack/logger.rb:36:in call_app' /home/abonas/.rvm/gems/ruby-2.3.0/gems/railties-5.0.5/lib/rails/rack/logger.rb:26:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/sprockets-rails-3.2.0/lib/sprockets/rails/quiet_assets.rb:13:in call' /home/abonas/.rvm/gems/ruby-2.3.0/gems/actionpack-5.0.5/lib/action_dispatch/middleware/request_id.rb:24:in call'
/home/abonas/.rvm/gems/ruby-2.3.0/gems/rack-2.0.3/lib/rack/method_override.rb:22:in `call'

@abonas
Copy link
Member

abonas commented Aug 13, 2017

@lucasponce do you remember any difference in how miq_report for live metrics is stored in db vs a regular report? although the error is in ui, iiuc it is originated by some lack of config on miq_report

@lucasponce
Copy link
Contributor

When we created this logic, once the report was generated it was stored in the miq_report model as html_details.
"Copy" function is not copying this stored report or it is triggering a new one ?
Can reports be generated normally and the only issues is with the "Copy" button ?

@abonas
Copy link
Member

abonas commented Aug 14, 2017

@lucasponce there's miq_report and miq_report_results - isn't the generated one going to results table?

@lucasponce
Copy link
Contributor

@abonas Correct. I meant miq_report* model/tables. I usually started from miq_report entities on a debug session and tracing the results stored in the model.

@abonas
Copy link
Member

abonas commented Aug 14, 2017

@lucasponce thanks. from the exception that I pasted above it looks like copying a report definition is failing with an exception on something empty in a miq_report (and not its result iiic_

@abonas
Copy link
Member

abonas commented Aug 14, 2017

@lucasponce from debugging this I see a difference in how the rpt object is represented comparing a middleware report to another non middleware report.
Example:
for report : Host CPU Usage per VM
rpt.db_options
{:start_offset=>172800, :end_offset=>0, :interval=>"daily"}

but for the middleware reports:
rpt.db_options
{:rpt_type=>"middleware", :options=>{:interval=>"every minute", :start_offset=>3600, :end_offset=>0}}

so rpt.db_options[:start_offset] for middleware report is always empty and hence the code fails on copy.
the question is - was this an intentional difference during design?

@lucasponce
Copy link
Contributor

lucasponce commented Aug 14, 2017

@abonas I think that was not intentional.
The rationale for this design was to examine how the generator invoked similar classes
https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_report/generator.rb#L184
and follow that pattern in the middleware base ones
https://github.com/ManageIQ/manageiq/blob/master/app/models/middleware_performance.rb
I don't remember this change of format, it sounds to me that probably this has been refactored and has changed.

@abonas
Copy link
Member

abonas commented Aug 14, 2017

@lucasponce @josejulio where is start_offset and end_offset assigned a value? I don't see it in the above class, only see it being consumed

@lucasponce
Copy link
Contributor

@abonas IIRC these variables are defined in the report configuration
https://github.com/ManageIQ/manageiq/blob/master/product/reports/670_Performance%20by%20Asset%20Type%20-%20Middleware%20Servers/110_JVM%20Heap%20Usage%20-%20daily%20over%20last%20week.yaml
So, at the moment is requested the star_offset and end_offset are used to pass these parameters in the classes described.
But this flow is managed by the Report UI and Report Engine.

@abonas
Copy link
Member

abonas commented Aug 15, 2017

@lucasponce right. what I still don't understand (and that is also what's causing this to crash) is why sometimes the start/end offset are on db_options, and sometimes they are on db_options.options

@lucasponce
Copy link
Contributor

@abonas I think this was motivated due this logic
https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_report/generator.rb#L199
I can be wrong but there were some logic related if the reports are generated using a custom_results_method and in this case, the db_options.options format should be expected.

@abonas
Copy link
Member

abonas commented Aug 17, 2017

@Fryguy who can assist understanding this issue with reports definition? the error that you reported results from this logic difference.
cc @isimluk - I think you might have changed this relevant area of code in this PR?

@Fryguy
Copy link
Member Author

Fryguy commented Aug 23, 2017

By not creatable I mean that if you go to create a new report, you can't create a "Middleware Servers" report, because it's not in the "Base the report on" dropdown (which makes sense because these are not typical reports). The existing reports are created out of the box, so they have bypassed that restriction by the nature of being seeded. By copyable I mean that by choosing copy in the UI, you create a duplicate record that you can edit and change, however even if you could copy it, you can't modify it at present, because there is no edit capability. IMO, if you can't create a type of report, you shouldn't be able to copy that type of report as that is just another form of "creation".

@Fryguy
Copy link
Member Author

Fryguy commented Aug 23, 2017

cc @gtanzillo Can you help on the reports definition side of things?

@gtanzillo
Copy link
Member

I think it would be best to move the items under db_options.options up to db_options because that's where the report editor expects them. I looked at some (but not all) of the the original (circa 2009) performance related reports and they all had :interval, start_offset and end_offset directly under db_options.

I made the change to this report and I was able to copy it. The seeding process will take care of updating the instance in the Db. You'll also need to update the code in build_results_for_report_middleware to expect those items in the new location in db_options.

diff --git a/product/reports/670_Performance by Asset Type - Middleware Servers/150_Transactions - hourly over last day.yaml b/product/reports/670_Performance by Asset Type - Middleware Servers/150_Transactions - hourly over last day.yaml
index 82d186f..0b81aea 100644
--- a/product/reports/670_Performance by Asset Type - Middleware Servers/150_Transactions - hourly over last day.yaml   
+++ b/product/reports/670_Performance by Asset Type - Middleware Servers/150_Transactions - hourly over last day.yaml   
@@ -37,10 +37,9 @@ rpt_type: Custom
 filename:
 db_options:
   :rpt_type: middleware
-  :options:
-    :interval: hourly
-    :start_offset: 86400
-    :end_offset: 0
+  :interval: hourly
+  :start_offset: 86400
+  :end_offset: 0
 include: {}
 
 db: MiddlewareServerPerformance

@Fryguy
Copy link
Member Author

Fryguy commented Aug 24, 2017

@gtanzillo Fixing the bug aside, what happens when you copy it...can you then edit it?

@gtanzillo
Copy link
Member

@Fryguy After copying I was able to edit the report definition and save it. (with the change above in place)

@abonas
Copy link
Member

abonas commented Aug 27, 2017

By not creatable I mean that if you go to create a new report, you can't create a "Middleware Servers" report, because it's not in the "Base the report on" dropdown (which makes sense because these are not typical reports). The existing reports are created out of the box, so they have bypassed

but the middleware reports are also created out of the box, so what am I missing?
cc @lucasponce

that restriction by the nature of being seeded. By copyable I mean that by choosing copy in the UI, you create a duplicate record that you can edit and change, however even if you could copy it, you can't modify it at present, because there is no edit capability. IMO, if you can't create a type of report, you shouldn't be able to copy that type of report as that is just another form of "creation".

@abonas
Copy link
Member

abonas commented Aug 27, 2017

@Fryguy After copying I was able to edit the report definition and save it. (with the change above in place)

@gtanzillo thanks. it will require change in some logic additionally so the report generation will not be broken of course.

@abonas
Copy link
Member

abonas commented Aug 27, 2017

@cfcosta Caina I'm looping you in here, please drive this.
Essentially we understand why this fails from code perspective and how to fix it so it won't fail (code pointers and pointers to reports config above)
What is missing right now is understanding whether the functionality should be allowed. If the conclusion is that it's not - another code change to grey out the option should be done.

@lucasponce
Copy link
Contributor

but the middleware reports are also created out of the box, so what am I missing?

I am not certain but I understand we are talking about the report generator here ?
This is a feature where an user (IIRC) can select entities and compose a custom query against the model selecting fields.
In case of middleware reports, I agree this feature is not enabled as middleware reports are backed against a virtual model, so user only would have available the pre-defined reports.

@abonas
Copy link
Member

abonas commented Aug 28, 2017

@lucasponce so what you are saying is that the copy should be disabled and we should not fix/move things from options to dbtotions?

@lucasponce
Copy link
Contributor

lucasponce commented Aug 28, 2017

@lucasponce so what you are saying is that the copy should be disabled and we should not fix/move things from options to dbtotions?

Probably, that could be easier option, I think, unless I miss something, my point is that we can maintain the predefined reports and not let user to generate/customize middleware reports yet.
I think that the MiQ generator is designed thinking that all entities at the end are fetched from database, so mixing virtual models that at the end needs to call an external system without SQL semantic perhaps would bring some refactoring not in the provider itself but in the miq core logic.

So, this issue is interesting and would let us to collect user requeriments. I think that maintained the pre-defined reports (or adding more) could be a valid tradeoff.

@abonas
Copy link
Member

abonas commented Aug 28, 2017

thanks @lucasponce. so it sounds that right now it is ok to make the reports non copy-able.
@jdoyleoss I'd like to confirm with you first before we proceed with this direction

@jdoyleoss
Copy link

+1 let's make then non-copyable. I'll log another requirement where we can scope and manger the priority of 'copy and modify'.

@abonas
Copy link
Member

abonas commented Oct 17, 2017

+1 let's make then non-copyable. I'll log another requirement where we can scope and manger the priority of 'copy and modify'.

@jdoyleoss I didn't find the JIRA for that, can you please mention it?

@abonas
Copy link
Member

abonas commented Oct 17, 2017

@miq-bot assign @aljesusg

@jdoyleoss
Copy link

I agree we can proceed as non-copyable for now. I logged a JIRA for future consideration.
https://issues.jboss.org/browse/JMAN4-232

aljesusg pushed a commit to aljesusg/manageiq-ui-classic that referenced this issue Oct 17, 2017
aljesusg pushed a commit to aljesusg/manageiq-ui-classic that referenced this issue Oct 17, 2017
martinpovolny added a commit that referenced this issue Oct 25, 2017
Disable button Copy  for Middleware Reports - Fix issue #1740
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants