{"action":"created","issue":{"url":"https://api.github.com/repos/ManageIQ/manageiq/issues/11502","repository_url":"https://api.github.com/repos/ManageIQ/manageiq","labels_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/11502/labels{/name}","comments_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/11502/comments","events_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/11502/events","html_url":"https://github.com/ManageIQ/manageiq/pull/11502","id":179050748,"number":11502,"title":"Update Service Aggregation Methods to use SQL","user":{"login":"NickLaMuro","id":314014,"avatar_url":"https://avatars.githubusercontent.com/u/314014?v=3","gravatar_id":"","url":"https://api.github.com/users/NickLaMuro","html_url":"https://github.com/NickLaMuro","followers_url":"https://api.github.com/users/NickLaMuro/followers","following_url":"https://api.github.com/users/NickLaMuro/following{/other_user}","gists_url":"https://api.github.com/users/NickLaMuro/gists{/gist_id}","starred_url":"https://api.github.com/users/NickLaMuro/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/NickLaMuro/subscriptions","organizations_url":"https://api.github.com/users/NickLaMuro/orgs","repos_url":"https://api.github.com/users/NickLaMuro/repos","events_url":"https://api.github.com/users/NickLaMuro/events{/privacy}","received_events_url":"https://api.github.com/users/NickLaMuro/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/ManageIQ/manageiq/labels/performance","name":"performance","color":"fad8c7"},{"url":"https://api.github.com/repos/ManageIQ/manageiq/labels/refactoring","name":"refactoring","color":"fbca04"},{"url":"https://api.github.com/repos/ManageIQ/manageiq/labels/ui","name":"ui","color":"5319e7"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-09-24T20:41:32Z","updated_at":"2016-09-24T20:51:57Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ManageIQ/manageiq/pulls/11502","html_url":"https://github.com/ManageIQ/manageiq/pull/11502","diff_url":"https://github.com/ManageIQ/manageiq/pull/11502.diff","patch_url":"https://github.com/ManageIQ/manageiq/pull/11502.patch"},"body":"Purpose\n-------\nDisplaying the number \"Totals for Service VMs\" on the `/service/explorer/:id` page is expensive at scale.  The following table rows:\n\n* CPU\n* Memory\n* Disk Count\n* Disk Space Allocated\n* Disk Space Used\n* Memory on Disk\n\nCurrently requiring load in each hardware record for each vm record, and sometimes each disk record for those hardware records, causing a huge `N*N(N)? + 1`™ when a service has a lot of VMs.\n\n\nOverview (aka: WTF is even going on here...)\n--------------------------------------------\n\nThis is going to be a long, almost blog-post like, explanation here, so hold on to your butts...\n\n\n**How we currently aggregate these stats...**\n\nTaking a step back and looking a particular example from one of the table rows above, to calculate (aggregate) the number of CPUs for a given service currently, you would call `my_service.aggregate_all_vm_cpus` and get returned the number of CPUs that are used by ALL VMs that were created using this service.  What happens under the hood is the following:\n\n* `my_service.all_vms`\n  - This is a `virtual_has_many`, but basically is makes use of `ancestry` on services to return all of the VMs that are associated with this service , and any other service that is a descendant of this one (uses the `.direct_vms` method on each service and child service in `app/models/service.rb` to do this).\n* `.map(&:hardware)`\n  - In reality, we are using a `virtual_column` here, but because it isn't making use of any arel, we are effectively doing a map.\n* `.inject(0) {|sum, hw| sum += hw.cpu_total_cores.to_i`\n\nWith anything having to do with `Disks`, we are also effectively adding another `.flat_map(&:disks)` following the `.map(&:hardware)` step, which then totals up the values from the disk records.  As you probably can probably guess, this is extremely expensive with a service with many VMs.\n\n\n**Representing the same thing with SQL**\n\nTo represent the `aggregate_all_vm_cpus` as a column in SQL, you can represent `Service.select(:id, :aggregate_all_vm_cpus).first` as the following:\n\n```SQL\nSELECT id,\n      (\n       --- Return only the SUM of all of the CPU cores to the main query (`.inject(0) { ... }`)\n       SELECT SUM(hardwares.cpu_total_cores)\n\n       --- Make sure we alias the table name so it doesn't conflict with the outer one\n       FROM services aggregate_all_vm_cpus_services\n\n       --- Join on services_resources that have a type of 'VmOrTemplate' (part of `.direct_vms`)\n       JOIN service_resources ON service_resources.service_id = aggregate_all_vm_cpus_services.id\n                             AND service_resources.resource_type = 'VmOrTemplate' \n\n       --- Include vms and their associated hardware (`.map(&:hardware)`)\n       JOIN vms               ON vms.id = service_resources.resource_id\n       JOIN hardwares         ON hardwares.vm_or_template_id = vms.id\n\n       --- Include services that are the current service, and descendants of that service (`.all_vms`)\n       WHERE ((\"aggregate_all_vm_cpus_services\".\"id\" = services.id \n               OR \"aggregate_all_vm_cpus_services\".\"ancestry\" ILIKE CONCAT(services.id, '/%'))\n              OR \"aggregate_all_vm_cpus_services\".\"ancestry\" = CAST(services.id AS VARCHAR))) AS aggregate_all_vm_cpus\nFROM services\nWHERE id = <my_service.id>;\n```\n\nThis is not ideal, and should not be used when against a index action, as this nested `SELECT` would be run individually for each service, but for a show, it is about a 20ms nested select in the query for a service with 1800 VMs associated with it and it's children.\n\nTo do this for `Disk Count`, another join would need to be done for disks against the `hardware` records.\n\n\n**Moving this into `virtual_attributes`**\n\nPart the reasoning behind doing this as a nested `SELECT` statement is it allows it to be used as a `virtual_attribute`/`virtual_column`.  This allows it to be queried against and selected as if it were a column on the `services` table:\n\n```ruby\nirb> Service.select(:id, :aggregate_all_vm_cpus).where(:id => 123).first.attributes[\"aggregate_all_vm_cpus\"]\n=> 12345\n```\n\nAnd multiple of these can also then be used together in a single query:\n\n```ruby\nirb> Service.select(:id, :aggregate_all_vm_cpus, :aggregate_all_vm_disk_count).where(:id => 123).first.attributes\n  Service Load (146.1ms)  SELECT  \"services\".\"id\", ((SELECT ...\n=> {\"id\" => 123, \"aggregate_all_vm_cpus\" => 12345, \"aggregate_all_vm_disk_count\" => 4321}\n```\n\nWhich now takes a fraction of a second instead of multiple seconds when grabbing each record individually.  This did require modifying the underlying methods in order to check for the existence of the attribute before aggregating in ruby.\n\n\n**Working it into `/service/explorer/:id`**\n\nFor any of these new `virtual_attribute` arel methods to actually have an affect, they need to be included in the select in some way, otherwise they will just default to the original `N+1` method of aggregating the data.  This is done when calling `get_node_info` in the `if params[:id]` of the `explorer` action, which then calls `show_record`, then `identify_service`, then `identify_record` (defined in `ApplicationController::CiProcessing`), and finally calling `find_by_id_filtered` (defined in the main `ApplicationController`.\n\nFor simplicity, the `with_aggregates` scope was created to make including the columns easier.  This needed to be added to the query that returned the record that was acted against in the views so the aggregates aren't recalculated, so modifying `find_by_id_filtered` was necessary to prevent this.\n\n\n\nMetrics\n-------\nThe following metrics should be a clear indication of the improvements provided by this change.\n\n**Before**\n\n|       ms |  queries | query (ms) |   rows |\n|     ---: |     ---: |       ---: |   ---: |\n| 32,534.7 |      278 |    2,922.5 | 36,626 |\n\n\n**After**\n\n|       ms |  queries | query (ms) |   rows |\n|     ---: |     ---: |       ---: |   ---: |\n| 15,958.3 |       44 |    1,304.6 | 29,709 |\n\n\nConsiderations and Mentionables\n-------------------------------\n\n* Truth be told, we are technically moving the N+1 here from the application into the database, it just so happens that the DB is much more efficient at running this kind of stuff than the application is, and doesn't have nearly the object overhead that comes with running this in the DB.  That said, there will be probably a bit more stress put on the DB to execute these queries, but nothing in the sense of \"multi second table locking\", and overall it should be less of an impact on resources.\n* These are DEFINITELY not queries that you want to run in a index type fashion, as they will not scale. This is because the nested selects will individually run for each service record returned.  In this case, it will end up in a tie up a process, so these methods need to be used with care.\n* Optimally, sharing the `JOIN`s here would be ideal as they would prevent making multiple loops in the database for each aggregate column.  I was originally under the impression that the current UI doesn't facilitate for something like that easily, but it might be possible based on how I have implemented the scopes for this and the changes to the controller.  This would be a minor improvement at this point, and the `virtual_column` arel improvements might still want to be included regardless.\n* The overwriting/modifications of `find_by_id_filtered` is ugly at best.  Unfortunately, we are inheriting a lot from the ApplicationController when finding records for \"show\" routes.  I was mostly going for allowing for additions (scopes on the finder), and overriding as few methods as possible when doing so.  `find_by_id_filtered` should still be the method that is used though at the end of the day and make sure we are finding only what the user has access to, and splitting it up would cause more chances for this filtering to be done incorrectly.\n  - Something I noticed while working on that method is it makes two calls to the same record.  One for the `find_by`, and the other when tossed into `Rbac.filtered`.  Furthermore, this also blows away any scopes that might have been applied in the first finder `find_by` when tossed into Rbac, meaning I had to apply the `:named_scope` to Rbac or it wouldn't take.  Since normally this is used with just a model's class and an ID passed in, this is fine, but I found it confusing myself when trying to apply the scope originally to the first query."},"comment":{"url":"https://api.github.com/repos/ManageIQ/manageiq/issues/comments/249386926","html_url":"https://github.com/ManageIQ/manageiq/pull/11502#issuecomment-249386926","issue_url":"https://api.github.com/repos/ManageIQ/manageiq/issues/11502","id":249386926,"user":{"login":"miq-bot","id":7806257,"avatar_url":"https://avatars.githubusercontent.com/u/7806257?v=3","gravatar_id":"","url":"https://api.github.com/users/miq-bot","html_url":"https://github.com/miq-bot","followers_url":"https://api.github.com/users/miq-bot/followers","following_url":"https://api.github.com/users/miq-bot/following{/other_user}","gists_url":"https://api.github.com/users/miq-bot/gists{/gist_id}","starred_url":"https://api.github.com/users/miq-bot/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/miq-bot/subscriptions","organizations_url":"https://api.github.com/users/miq-bot/orgs","repos_url":"https://api.github.com/users/miq-bot/repos","events_url":"https://api.github.com/users/miq-bot/events{/privacy}","received_events_url":"https://api.github.com/users/miq-bot/received_events","type":"User","site_admin":false},"created_at":"2016-09-24T20:51:57Z","updated_at":"2016-09-24T20:51:57Z","body":"<rubocop />Checked commits https://github.com/NickLaMuro/manageiq/compare/0d0a91fd1b8d1a48169764e53323b0268c7ff213~...772bd200fe5439cb9daa42824b0b4d070e767e6c with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1\n3 files checked, 29 offenses detected\n\n**app/controllers/application_controller.rb**\n- [ ] :exclamation: - [Line 2273](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/controllers/application_controller.rb#L2273), Col 42 - [Style/SpaceAroundEqualsInParameterDefault](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/SpaceAroundEqualsInParameterDefault) - Surrounding space missing in default value assignment.\n- [ ] :grey_exclamation: - [Line 2273](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/controllers/application_controller.rb#L2273), Col 3 - [Metrics/AbcSize](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Metrics/AbcSize) - Assignment Branch Condition size for find_by_id_filtered is too high. [20.32/20]\n\n**app/models/service/aggregation.rb**\n- [ ] :warning: - [Line 123](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L123), Col 7 - [Lint/UselessAssignment](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Lint/UselessAssignment) - Useless assignment to variable - `arel`.\n- [ ] :warning: - [Line 154](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L154), Col 80 - [Lint/UselessAssignment](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Lint/UselessAssignment) - Useless assignment to variable - `aggregate`. Use just operator `+`.\n- [ ] :warning: - [Line 162](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L162), Col 43 - [Lint/UselessAssignment](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Lint/UselessAssignment) - Useless assignment to variable - `aggregate`. Use just operator `+`.\n- [ ] :warning: - [Line 167](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L167), Col 86 - [Lint/UselessAssignment](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Lint/UselessAssignment) - Useless assignment to variable - `aggregate`. Use just operator `+`.\n- [ ] :warning: - [Line 171](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L171), Col 96 - [Lint/UselessAssignment](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Lint/UselessAssignment) - Useless assignment to variable - `aggregate`. Use just operator `+`.\n- [ ] :warning: - [Line 175](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L175), Col 91 - [Lint/UselessAssignment](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Lint/UselessAssignment) - Useless assignment to variable - `aggregate`. Use just operator `+`.\n- [ ] :warning: - [Line 183](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L183), Col 43 - [Lint/UselessAssignment](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Lint/UselessAssignment) - Useless assignment to variable - `aggregate`. Use just operator `+`.\n- [ ] :exclamation: - [Line 107](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L107), Col 13 - [Style/MultilineMethodCallIndentation](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/MultilineMethodCallIndentation) - Align `.join` with `.project` on line 106.\n- [ ] :exclamation: - [Line 109](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L109), Col 13 - [Style/MultilineMethodCallIndentation](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/MultilineMethodCallIndentation) - Align `.join` with `.project` on line 106.\n- [ ] :exclamation: - [Line 110](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L110), Col 13 - [Style/MultilineMethodCallIndentation](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/MultilineMethodCallIndentation) - Align `.join` with `.project` on line 106.\n- [ ] :exclamation: - [Line 38](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L38), Col 82 - [Style/SpaceAroundEqualsInParameterDefault](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/SpaceAroundEqualsInParameterDefault) - Surrounding space missing in default value assignment.\n- [ ] :exclamation: - [Line 64](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L64), Col 26 - [Style/SpaceInsideBrackets](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/SpaceInsideBrackets) - Space inside square brackets detected.\n- [ ] :exclamation: - [Line 64](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L64), Col 55 - [Style/SpaceInsideBrackets](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/SpaceInsideBrackets) - Space inside square brackets detected.\n- [ ] :exclamation: - [Line 66](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L66), Col 9 - [Style/AlignParameters](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/AlignParameters) - Align the parameters of a method call if they span more than one line.\n- [ ] :exclamation: - [Line 67](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L67), Col 7 - [Style/ClosingParenthesisIndentation](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/ClosingParenthesisIndentation) - Align `)` with `(`.\n- [ ] :exclamation: - [Line 74](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L74), Col 26 - [Style/SpaceInsideBrackets](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/SpaceInsideBrackets) - Space inside square brackets detected.\n- [ ] :exclamation: - [Line 74](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L74), Col 87 - [Style/SpaceInsideBrackets](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/SpaceInsideBrackets) - Space inside square brackets detected.\n- [ ] :exclamation: - [Line 76](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L76), Col 9 - [Style/AlignParameters](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/AlignParameters) - Align the parameters of a method call if they span more than one line.\n- [ ] :exclamation: - [Line 77](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L77), Col 7 - [Style/ClosingParenthesisIndentation](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/ClosingParenthesisIndentation) - Align `)` with `(`.\n- [ ] :exclamation: - [Line 81](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L81), Col 78 - [Style/SpaceAroundEqualsInParameterDefault](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/SpaceAroundEqualsInParameterDefault) - Surrounding space missing in default value assignment.\n- [ ] :exclamation: - [Line 95](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L95), Col 21 - [Style/ExtraSpacing](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/ExtraSpacing) - Unnecessary spacing detected.\n- [ ] :exclamation: - [Line 95](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L95), Col 24 - [Style/SpaceAroundOperators](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Style/SpaceAroundOperators) - Operator `=` should be surrounded by a single space.\n- [ ] :grey_exclamation: - [Line 171](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L171), Col 121 - [Metrics/LineLength](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Metrics/LineLength) - Line is too long. [140/120]\n- [ ] :grey_exclamation: - [Line 175](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L175), Col 121 - [Metrics/LineLength](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Metrics/LineLength) - Line is too long. [130/120]\n- [ ] :grey_exclamation: - [Line 23](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L23), Col 121 - [Metrics/LineLength](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Metrics/LineLength) - Line is too long. [136/120]\n- [ ] :grey_exclamation: - [Line 81](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L81), Col 5 - [Metrics/AbcSize](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Metrics/AbcSize) - Assignment Branch Condition size for aggregate_hardware_arel is too high. [52.67/20]\n- [ ] :grey_exclamation: - [Line 81](https://github.com/NickLaMuro/manageiq/blob/772bd200fe5439cb9daa42824b0b4d070e767e6c/app/models/service/aggregation.rb#L81), Col 5 - [Metrics/MethodLength](http://rubydoc.info/gems/rubocop/0.37.2/RuboCop/Cop/Metrics/MethodLength) - Method has too many lines. [27/25]\n"}}
{"actor":{"display_login":"miq-bot"}}