[Dashboard] optimization

[macejmic]

What this PR does / why we need it:

Performance problems with the dashboard.

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-3592

Tested with 1K products and backends -> time to load page reduced from 34 seconds(on 3scale 2.8.2) to 14 secconds
Tested with 1.5K backends -> time to load page reduced from 29 seconds(on 3scale 2.8.2) to 9 secconds
Tested with 1.5K products -> time to load page reduced from more than 1 minute/or internal error displayed due to long loading time (on 3scale 2.8.2) to 19 secconds

[Martouta]

My 1st reaction was "what is that?" so I went to https://github.com/k0kubun/activerecord-precounter and saw:

Yet Another N+1 COUNT Query Killer for ActiveRecord, counter_cache alternative.
ActiveRecord::Precounter allows you to cache count of associated records by eager loading.

Great, but we already use counter_cache for some things, and I know that they behave VERY differently, so I wondered if we really need both or what was the plan for the future.

We spoke about it and counter_cache should eventually be completely replaced for this one because it is not as handy. Once this PR is merged, we will have a Jira issue for this goal 👍

[Martouta]

This doesn't have a test and got too complicated/dangerous to don't have a test 😓
I wrote some in test/helpers/menu_helper_test.rb locally to be able to change the code having the tests.

require 'test_helper'

class MenuHelperTest < ActionView::TestCase
  def setup
    @provider = FactoryBot.create(:provider_account)
  end

  attr_reader :provider

  test 'switch_link with finance globally disabled' do
    expects(:forcibly_denied_switch?).with(:finance).returns(true)
    assert_nil switch_link('Billing', root_path, switch: :finance, upgrade_notice: true)
  end

  test 'switch_link with finance globally enabled' do
    expects(:forcibly_denied_switch?).with(:finance).returns(false)
    link = switch_link('Billing', root_path, switch: :finance, upgrade_notice: true)
    assert_equal upgrade_notice_link(:finance, 'Billing'), link
  end

  test 'forcibly_denied_switch?' do
    Settings.stubs(globally_denied_switches: [])
    assert @provider.settings.finance.denied?
    refute forcibly_denied_switch?(:finance)

    Settings.stubs(globally_denied_switches: [:finance])
    refute forcibly_denied_switch?(:finance)
  end

  test 'forcibly_denied_switch? for master' do
    ThreeScale.config.stubs(onpremises: false)
    @provider = master_account
    assert @provider.settings.finance.denied?
    refute forcibly_denied_switch?(:finance)

    ThreeScale.config.stubs(onpremises: true)
    assert @provider.settings.finance.denied?
    assert forcibly_denied_switch?(:finance)
  end

  test 'api_selector_services for tenant logged in without api_as_product' do
    FactoryBot.create(:simple_service, account: provider)
    FactoryBot.create(:simple_service, account: provider, state: Service::DELETE_STATE)

    rolling_updates_on
    rolling_update(:api_as_product, enabled: false)

    assert_equal provider.accessible_services.size, api_selector_services.size
    api_selector_services.each do |api_decorated|
      assert_equal ServiceDecorator, api_decorated.class
      assert_includes provider.accessible_services.pluck(:id), api_decorated.id
    end
  end

  test 'api_selector_services for tenant logged in with api_as_product' do
    FactoryBot.create(:simple_service, account: provider)
    FactoryBot.create(:simple_service, account: provider, state: Service::DELETE_STATE)
    FactoryBot.create(:backend_api, account: provider)
    FactoryBot.create(:backend_api, account: provider, state: BackendApi::DELETED_STATE)

    rolling_updates_on
    rolling_update(:api_as_product, enabled: true)

    services_decorated, backend_apis_decorated = api_selector_services.partition { |api_decorated| api_decorated.class == ServiceDecorator }
    assert_equal provider.accessible_services.size, services_decorated.size
    assert_equal provider.backend_apis.accessible.size, backend_apis_decorated.size
    assert_same_elements provider.accessible_services.pluck(:id), services_decorated.map(&:id)
    assert_same_elements provider.backend_apis.accessible.pluck(:id), backend_apis_decorated.map(&:id)
    assert backend_apis_decorated.all? { |api_decorated| api_decorated.class == BackendApiDecorator }
  end

  test 'api_selector_services if not logged in' do
    @current_account = nil
    assert_empty api_selector_services
  end

  test 'api_selector_services in developer portal' do
    @current_account = FactoryBot.create(:simple_buyer, provider_account: provider)
    assert_empty api_selector_services
  end


  protected

  def current_user
    @current_user ||= current_account.try!(:admin_user)
  end

  def current_account
    return @current_account if defined? @current_account
    @current_account = @provider
  end

  def ability
    @ability ||= Ability.new(current_user)
  end

  def can?(*args)
    ability.can?(*args)
  end
end

[Martouta]

There is a bug here 😞 it should be only the accessible backend_apis.
@api_selector_services.concat(current_user.account.backend_apis.accessible.decorate) if current_account.provider_can_use?(:api_as_product)

[Martouta]

There is a bug here 😞site_account.provider? is always true because site_account is the account that possesses the domain where the user in, that means:

• For DevPortal: It is true because it is the tenant account that has this developer portal as domain, but in developer portal, the user checking the portal is a buyer, so this conditional is wrong.
• For AdminPortal of a tenant: It is true for obvious reasons 😄
• For AdminPortal of a master: It is true because a master is also considered a provider.

So it should be one of these instead:

@api_selector_services ||= (current_account && current_account.try(:provider?) ? current_user.accessible_services : Service.none).decorate

@api_selector_services ||= (current_account && current_account.provider? ? current_user.accessible_services : Service.none).decorate

[Martouta]

I know that it is not your fault, but this name at this point is a bit misleading... it sounds like it is only services 😕

[Martouta]

This can never be nil or false, so we do not need to do it like this. We can have it simpler. Plus, checking first site_account.provider? && logged_in? and later (not inside that if, but AFTER it) if current_account.provider_can_use?(:api_as_product) is redundant and even error-prone knowing that current_account could be nil.

So, I propose something like this:

def api_selector_services
  @api_selector_services ||= if current_account.try(:provider?)
     accessible_services.concat(accessible_backend_apis)
  else
    Service.none.decorate
  end
end

private

def accessible_services
  current_user.accessible_services.decorate
end

def accessible_backend_apis
  if current_account.provider_can_use?(:api_as_product)
    current_user.account.backend_apis.accessible
  else
    BackendApi.none
  end.decorate
end

[Martouta]

This needs a test in test/unit/helpers/provider/admin/dashboards_helper_test.rb

And I do not think that checking the type is ideal 🤔 maybe we could have 2 separate methods, reusing the code in common.

[Martouta]

Are you sure that this performs better than this?:

        - if service.cinstances_with_traffic.exists? || (apiap? && service.sandbox_proxy_configs.exists?)

Because the exists? works with LIMIT 1 which implies that instead of counting them all, it only checks until it find the first one if there is any 😄

[macejmic]

so i finally have an asnwer: https://issues.redhat.com/browse/THREESCALE-3592?focusedCommentId=14116380&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14116380

[Martouta]

Same as #1856 (comment)

Just leaving comments :)

[Martouta]

Awesome 👏 💪 🥇