Merge branch 'bvl-controller-action-attribution' into 'master'

Add a feature category to requests handled by rails controllers

See merge request gitlab-org/gitlab!36141

app/controllers/application_controller.rb

@@ -22,6 +22,7 @@ class ApplicationController < ActionController::Base
   include Impersonation
   include Gitlab::Logging::CloudflareHelper
   include Gitlab::Utils::StrongMemoize
+  include ControllerWithFeatureCategory
 
   before_action :authenticate_user!, except: [:route_not_found]
   before_action :enforce_terms!, if: :should_enforce_terms?

app/controllers/concerns/controller_with_feature_category.rb

+# frozen_string_literal: true
+
+module ControllerWithFeatureCategory
+  extend ActiveSupport::Concern
+  include Gitlab::ClassAttributes
+
+  class_methods do
+    def feature_category(category, config = {})
+      validate_config!(config)
+
+      category_config = Config.new(category, config[:only], config[:except], config[:if], config[:unless])
+      # Add the config to the beginning. That way, the last defined one takes precedence.
+      feature_category_configuration.unshift(category_config)
+    end
+
+    def feature_category_for_action(action)
+      category_config = feature_category_configuration.find { |config| config.matches?(action) }
+
+      category_config&.category || superclass_feature_category_for_action(action)
+    end
+
+    private
+
+    def validate_config!(config)
+      invalid_keys = config.keys - [:only, :except, :if, :unless]
+      if invalid_keys.any?
+        raise ArgumentError, "unknown arguments: #{invalid_keys} "
+      end
+
+      if config.key?(:only) && config.key?(:except)
+        raise ArgumentError, "cannot configure both `only` and `except`"
+      end
+    end
+
+    def feature_category_configuration
+      class_attributes[:feature_category_config] ||= []
+    end
+
+    def superclass_feature_category_for_action(action)
+      return unless superclass.respond_to?(:feature_category_for_action)
+
+      superclass.feature_category_for_action(action)
+    end
+  end
+end

app/controllers/concerns/controller_with_feature_category/config.rb

+# frozen_string_literal: true
+
+module ControllerWithFeatureCategory
+  class Config
+    attr_reader :category
+
+    def initialize(category, only, except, if_proc, unless_proc)
+      @category = category.to_sym
+      @only, @except = only&.map(&:to_s), except&.map(&:to_s)
+      @if_proc, @unless_proc = if_proc, unless_proc
+    end
+
+    def matches?(action)
+      included?(action) && !excluded?(action) &&
+        if_proc?(action) && !unless_proc?(action)
+    end
+
+    private
+
+    attr_reader :only, :except, :if_proc, :unless_proc
+
+    def if_proc?(action)
+      if_proc.nil? || if_proc.call(action)
+    end
+
+    def unless_proc?(action)
+      unless_proc.present? && unless_proc.call(action)
+    end
+
+    def included?(action)
+      only.nil? || only.include?(action)
+    end
+
+    def excluded?(action)
+      except.present? && except.include?(action)
+    end
+  end
+end

app/controllers/projects/merge_requests_controller.rb

@@ -45,6 +45,13 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
   around_action :allow_gitaly_ref_name_caching, only: [:index, :show, :discussions]
 
+  feature_category :source_code_management,
+                   unless: -> (action) { action.ends_with?("_reports") }
+  feature_category :code_testing,
+                   only: [:test_reports, :coverage_reports, :terraform_reports]
+  feature_category :accessibility_testing,
+                   only: [:accessibility_reports]
+
   def index
     @merge_requests = @issuables
 

app/workers/concerns/worker_attributes.rb

@@ -2,6 +2,7 @@
 
 module WorkerAttributes
   extend ActiveSupport::Concern
+  include Gitlab::ClassAttributes
 
   # Resource boundaries that workers can declare through the
   # `resource_boundary` attribute
@@ -30,24 +31,24 @@ module WorkerAttributes
   }.stringify_keys.freeze
 
   class_methods do
-    def feature_category(value)
+    def feature_category(value, *extras)
       raise "Invalid category. Use `feature_category_not_owned!` to mark a worker as not owned" if value == :not_owned
 
-      worker_attributes[:feature_category] = value
+      class_attributes[:feature_category] = value
     end
 
     # Special case: mark this work as not associated with a feature category
     # this should be used for cross-cutting concerns, such as mailer workers.
     def feature_category_not_owned!
-      worker_attributes[:feature_category] = :not_owned
+      class_attributes[:feature_category] = :not_owned
     end
 
     def get_feature_category
-      get_worker_attribute(:feature_category)
+      get_class_attribute(:feature_category)
     end
 
     def feature_category_not_owned?
-      get_worker_attribute(:feature_category) == :not_owned
+      get_feature_category == :not_owned
     end
 
     # This should be set to :high for jobs that need to be run
@@ -61,11 +62,11 @@ module WorkerAttributes
     def urgency(urgency)
       raise "Invalid urgency: #{urgency}" unless VALID_URGENCIES.include?(urgency)
 
-      worker_attributes[:urgency] = urgency
+      class_attributes[:urgency] = urgency
     end
 
     def get_urgency
-      worker_attributes[:urgency] || :low
+      class_attributes[:urgency] || :low
     end
 
     # Set this attribute on a job when it will call to services outside of the
@@ -73,85 +74,64 @@ module WorkerAttributes
     # doc/development/sidekiq_style_guide.md#Jobs-with-External-Dependencies for
     # details
     def worker_has_external_dependencies!
-      worker_attributes[:external_dependencies] = true
+      class_attributes[:external_dependencies] = true
     end
 
     # Returns a truthy value if the worker has external dependencies.
     # See doc/development/sidekiq_style_guide.md#Jobs-with-External-Dependencies
     # for details
     def worker_has_external_dependencies?
-      worker_attributes[:external_dependencies]
+      class_attributes[:external_dependencies]
     end
 
     def worker_resource_boundary(boundary)
       raise "Invalid boundary" unless VALID_RESOURCE_BOUNDARIES.include? boundary
 
-      worker_attributes[:resource_boundary] = boundary
+      class_attributes[:resource_boundary] = boundary
     end
 
     def get_worker_resource_boundary
-      worker_attributes[:resource_boundary] || :unknown
+      class_attributes[:resource_boundary] || :unknown
     end
 
     def idempotent!
-      worker_attributes[:idempotent] = true
+      class_attributes[:idempotent] = true
     end
 
     def idempotent?
-      worker_attributes[:idempotent]
+      class_attributes[:idempotent]
     end
 
     def weight(value)
-      worker_attributes[:weight] = value
+      class_attributes[:weight] = value
     end
 
     def get_weight
-      worker_attributes[:weight] ||
+      class_attributes[:weight] ||
         NAMESPACE_WEIGHTS[queue_namespace] ||
         1
     end
 
     def tags(*values)
-      worker_attributes[:tags] = values
+      class_attributes[:tags] = values
     end
 
     def get_tags
-      Array(worker_attributes[:tags])
+      Array(class_attributes[:tags])
     end
 
     def deduplicate(strategy, options = {})
-      worker_attributes[:deduplication_strategy] = strategy
-      worker_attributes[:deduplication_options] = options
+      class_attributes[:deduplication_strategy] = strategy
+      class_attributes[:deduplication_options] = options
     end
 
     def get_deduplicate_strategy
-      worker_attributes[:deduplication_strategy] ||
+      class_attributes[:deduplication_strategy] ||
         Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob::DEFAULT_STRATEGY
     end
 
     def get_deduplication_options
-      worker_attributes[:deduplication_options] || {}
-    end
-
-    protected
-
-    # Returns a worker attribute declared on this class or its parent class.
-    # This approach allows declared attributes to be inherited by
-    # child classes.
-    def get_worker_attribute(name)
-      worker_attributes[name] || superclass_worker_attributes(name)
-    end
-
-    private
-
-    def worker_attributes
-      @attributes ||= {}
-    end
-
-    def superclass_worker_attributes(name)
-      return unless superclass.include? WorkerAttributes
-
-      superclass.get_worker_attribute(name)
+      class_attributes[:deduplication_options] || {}
     end
   end
 end

ee/app/controllers/ee/projects/merge_requests_controller.rb

@@ -19,6 +19,14 @@ module EE
         before_action :authorize_read_pipeline!, only: [:container_scanning_reports, :dependency_scanning_reports,
                                                         :license_scanning_reports,
                                                         :sast_reports, :secret_detection_reports, :dast_reports, :metrics_reports]
+
+        feature_category :container_scanning, only: [:container_scanning_reports]
+        feature_category :dependency_scanning, only: [:dependency_scanning_reports]
+        feature_category :license_compliance, only: [:license_scanning_reports]
+        feature_category :static_application_security_testing, only: [:sast_reports]
+        feature_category :secret_detection, only: [:secret_detection_reports]
+        feature_category :dynamic_application_security_testing, only: [:dast_reports]
+        feature_category :metrics, only: [:metrics_reports]
       end
 
       def approve

lib/gitlab/class_attributes.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module ClassAttributes
+    extend ActiveSupport::Concern
+
+    class_methods do
+      protected
+
+      # Returns an attribute declared on this class or its parent class.
+      # This approach allows declared attributes to be inherited by
+      # child classes.
+      def get_class_attribute(name)
+        class_attributes[name] || superclass_attributes(name)
+      end
+
+      private
+
+      def class_attributes
+        @class_attributes ||= {}
+      end
+
+      def superclass_attributes(name)
+        return unless superclass.include? Gitlab::ClassAttributes
+
+        superclass.get_class_attribute(name)
+      end
+    end
+  end
+end

lib/gitlab/metrics/background_transaction.rb

@@ -9,7 +9,7 @@ module Gitlab
       end
 
       def labels
-        { controller: @worker_class.name, action: 'perform' }
+        { controller: @worker_class.name, action: 'perform', feature_category: @worker_class.try(:get_feature_category).to_s }
       end
     end
   end

lib/gitlab/metrics/transaction.rb

@@ -7,7 +7,7 @@ module Gitlab
       include Gitlab::Metrics::Methods
 
       # base labels shared among all transactions
-      BASE_LABELS = { controller: nil, action: nil }.freeze
+      BASE_LABELS = { controller: nil, action: nil, feature_category: nil }.freeze
       # labels that potentially contain sensitive information and will be filtered
       FILTERED_LABELS = [:branch, :path].freeze
 

lib/gitlab/metrics/web_transaction.rb

@@ -32,6 +32,10 @@ module Gitlab
 
         action = "#{controller.action_name}"
 
+        # Try to get the feature category, but don't fail when the controller is
+        # not an ApplicationController.
+        feature_category = controller.class.try(:feature_category_for_action, action).to_s
+
         # Devise exposes a method called "request_format" that does the below.
         # However, this method is not available to all controllers (e.g. certain
         # Doorkeeper controllers). As such we use the underlying code directly.
@@ -45,7 +49,7 @@ module Gitlab
           action = "#{action}.#{suffix}"
         end
 
-        { controller: controller.class.name, action: action }
+        { controller: controller.class.name, action: action, feature_category: feature_category }
       end
 
       def labels_from_endpoint
@@ -61,7 +65,10 @@ module Gitlab
 
         if route
           path = endpoint_paths_cache[route.request_method][route.path]
-          { controller: 'Grape', action: "#{route.request_method} #{path}" }
+
+          # Feature categories will be added for grape endpoints in
+          # https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/462
+          { controller: 'Grape', action: "#{route.request_method} #{path}", feature_category: '' }
         end
       end
 

spec/controllers/concerns/controller_with_feature_category/config_spec.rb

+# frozen_string_literal: true
+
+require "fast_spec_helper"
+require "rspec-parameterized"
+require_relative "../../../../app/controllers/concerns/controller_with_feature_category/config"
+
+RSpec.describe ControllerWithFeatureCategory::Config do
+  describe "#matches?" do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:only_actions, :except_actions, :if_proc, :unless_proc, :test_action, :expected) do
+      nil         | nil         | nil   | nil   | "action"   | true
+      [:included] | nil         | nil   | nil   | "action"   | false
+      [:included] | nil         | nil   | nil   | "included" | true
+      nil         | [:excluded] | nil   | nil   | "excluded" | false
+      nil         | nil         | true  | nil   | "action"   | true
+      [:included] | nil         | true  | nil   | "action"   | false
+      [:included] | nil         | true  | nil   | "included" | true
+      nil         | [:excluded] | true  | nil   | "excluded" | false
+      nil         | nil         | false | nil   | "action"   | false
+      [:included] | nil         | false | nil   | "action"   | false
+      [:included] | nil         | false | nil   | "included" | false
+      nil         | [:excluded] | false | nil   | "excluded" | false
+      nil         | nil         | nil   | true  | "action"   | false
+      [:included] | nil         | nil   | true  | "action"   | false
+      [:included] | nil         | nil   | true  | "included" | false
+      nil         | [:excluded] | nil   | true  | "excluded" | false
+      nil         | nil         | nil   | false | "action"   | true
+      [:included] | nil         | nil   | false | "action"   | false
+      [:included] | nil         | nil   | false | "included" | true
+      nil         | [:excluded] | nil   | false | "excluded" | false
+      nil         | nil         | true  | false | "action"   | true
+      [:included] | nil         | true  | false | "action"   | false
+      [:included] | nil         | true  | false | "included" | true
+      nil         | [:excluded] | true  | false | "excluded" | false
+      nil         | nil         | false | true  | "action"   | false
+      [:included] | nil         | false | true  | "action"   | false
+      [:included] | nil         | false | true  | "included" | false
+      nil         | [:excluded] | false | true  | "excluded" | false
+    end
+
+    with_them do
+      let(:config) do
+        if_to_proc = if_proc.nil? ? nil : -> (_) { if_proc }
+        unless_to_proc = unless_proc.nil? ? nil : -> (_) { unless_proc }
+
+        described_class.new(:category, only_actions, except_actions, if_to_proc, unless_to_proc)
+      end
+
+      specify { expect(config.matches?(test_action)).to be(expected) }
+    end
+  end
+end

spec/controllers/concerns/controller_with_feature_category_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require_relative "../../../app/controllers/concerns/controller_with_feature_category"
+require_relative "../../../app/controllers/concerns/controller_with_feature_category/config"
+
+RSpec.describe ControllerWithFeatureCategory do
+  describe ".feature_category_for_action" do
+    let(:base_controller) do
+      Class.new do
+        include ControllerWithFeatureCategory
+      end
+    end
+
+    let(:controller) do
+      Class.new(base_controller) do
+        feature_category :baz
+        feature_category :foo, except: %w(update edit)
+        feature_category :bar, only: %w(index show)
+        feature_category :quux, only: %w(destroy)
+        feature_category :quuz, only: %w(destroy)
+      end
+    end
+
+    let(:subclass) do
+      Class.new(controller) do
+        feature_category :qux, only: %w(index)
+      end
+    end
+
+    it "is nil when nothing was defined" do
+      expect(base_controller.feature_category_for_action("hello")).to be_nil
+    end
+
+    it "returns the expected category", :aggregate_failures do
+      expect(controller.feature_category_for_action("update")).to eq(:baz)
+      expect(controller.feature_category_for_action("hello")).to eq(:foo)
+      expect(controller.feature_category_for_action("index")).to eq(:bar)
+    end
+
+    it "returns the closest match for categories defined in subclasses" do
+      expect(subclass.feature_category_for_action("index")).to eq(:qux)
+      expect(subclass.feature_category_for_action("show")).to eq(:bar)
+    end
+
+    it "returns the last defined feature category when multiple match" do
+      expect(controller.feature_category_for_action("destroy")).to eq(:quuz)
+    end
+
+    it "raises an error when using including and excluding the same action" do
+      expect do
+        Class.new(base_controller) do
+          feature_category :hello, only: [:world], except: [:world]
+        end
+      end.to raise_error(%r(cannot configure both `only` and `except`))
+    end
+
+    it "raises an error when using unknown arguments" do
+      expect do
+        Class.new(base_controller) do
+          feature_category :hello, hello: :world
+        end
+      end.to raise_error(%r(unknown arguments))
+    end
+  end
+end

spec/controllers/every_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe "Every controller" do
+  context "feature categories" do
+    let_it_be(:feature_categories) do
+      YAML.load_file(Rails.root.join('config', 'feature_categories.yml')).map(&:to_sym).to_set
+    end
+
+    let_it_be(:controller_actions) do
+      # This will return tuples of all controller actions defined in the routes
+      # Only for controllers inheriting ApplicationController
+      # Excluding controllers from gems (OAuth, Sidekiq)
+      Rails.application.routes.routes
+        .map { |route| route.required_defaults.presence }
+        .compact
+        .select { |route| route[:controller].present? && route[:action].present? }
+        .map { |route| [constantize_controller(route[:controller]), route[:action]] }
+        .reject { |route| route.first.nil? || !route.first.include?(ControllerWithFeatureCategory) }
+    end
+
+    let_it_be(:routes_without_category) do
+      controller_actions.map do |controller, action|
+        "#{controller}##{action}" unless controller.feature_category_for_action(action)
+      end.compact
+    end
+
+    it "has feature categories" do
+      pending("We'll work on defining categories for all controllers: "\
+              "https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/463")
+
+      expect(routes_without_category).to be_empty, "#{routes_without_category.first(10)} did not have a category"
+    end
+
+    it "completed controllers don't get new routes without categories" do
+      completed_controllers = [Projects::MergeRequestsController].map(&:to_s)
+
+      newly_introduced_missing_category = routes_without_category.select do |route|
+        completed_controllers.any? { |controller| route.start_with?(controller) }
+      end
+
+      expect(newly_introduced_missing_category).to be_empty
+    end
+
+    it "recognizes the feature categories" do
+      routes_unknown_category = controller_actions.map do |controller, action|
+        used_category = controller.feature_category_for_action(action)
+        next unless used_category
+        next if used_category == :not_owned
+
+        ["#{controller}##{action}", used_category] unless feature_categories.include?(used_category)
+      end.compact
+
+      expect(routes_unknown_category).to be_empty, "#{routes_unknown_category.first(10)} had an unknown category"
+    end
+
+    it "doesn't define or exclude categories on removed actions", :aggregate_failures do
+      controller_actions.group_by(&:first).each do |controller, controller_action|
+        existing_actions = controller_action.map(&:last)
+        used_actions = actions_defined_in_feature_category_config(controller)
+        non_existing_used_actions = used_actions - existing_actions
+
+        expect(non_existing_used_actions).to be_empty,
+                                             "#{controller} used #{non_existing_used_actions} to define feature category, but the route does not exist"
+      end
+    end
+  end
+
+  def constantize_controller(name)
+    "#{name.camelize}Controller".constantize
+  rescue NameError
+    nil # some controllers, like the omniauth ones are dynamic
+  end
+
+  def actions_defined_in_feature_category_config(controller)
+    feature_category_configs = controller.send(:class_attributes)[:feature_category_config]
+    feature_category_configs.map do |config|
+      Array(config.send(:only)) + Array(config.send(:except))
+    end.flatten.uniq.map(&:to_s)
+  end
+end

spec/lib/gitlab/class_attributes_spec.rb

+# frozen_string_literal: true
+require 'fast_spec_helper'
+
+RSpec.describe Gitlab::ClassAttributes do
+  let(:klass) do
+    Class.new do
+      include Gitlab::ClassAttributes
+
+      def self.get_attribute(name)
+        get_class_attribute(name)
+      end
+
+      def self.set_attribute(name, value)
+        class_attributes[name] = value
+      end
+    end
+  end
+
+  let(:subclass) { Class.new(klass) }
+
+  describe ".get_class_attribute" do
+    it "returns values set on the class" do
+      klass.set_attribute(:foo, :bar)
+
+      expect(klass.get_attribute(:foo)).to eq(:bar)
+    end
+
+    it "returns values set on a superclass" do
+      klass.set_attribute(:foo, :bar)
+
+      expect(subclass.get_attribute(:foo)).to eq(:bar)
+    end
+
+    it "returns values from the subclass over attributes from a superclass" do
+      klass.set_attribute(:foo, :baz)
+      subclass.set_attribute(:foo, :bar)
+
+      expect(subclass.get_attribute(:foo)).to eq(:bar)
+    end
+  end
+end

spec/lib/gitlab/metrics/background_transaction_spec.rb

@@ -9,7 +9,16 @@ RSpec.describe Gitlab::Metrics::BackgroundTransaction do
 
   describe '#label' do
     it 'returns labels based on class name' do
-      expect(subject.labels).to eq(controller: 'TestWorker', action: 'perform')
+      expect(subject.labels).to eq(controller: 'TestWorker', action: 'perform', feature_category: '')
+    end
+
+    it 'contains only the labels defined for metrics' do
+      expect(subject.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABELS.keys)
+    end
+
+    it 'includes the feature category if there is one' do
+      expect(test_worker_class).to receive(:get_feature_category).and_return('source_code_management')
+      expect(subject.labels).to include(feature_category: 'source_code_management')
     end
   end
 end

spec/lib/gitlab/metrics/web_transaction_spec.rb

@@ -70,6 +70,9 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
   end
 
   describe '#labels' do
+    let(:request) { double(:request, format: double(:format, ref: :html)) }
+    let(:controller_class) { double(:controller_class, name: 'TestController') }
+
     context 'when request goes to Grape endpoint' do
       before do
         route = double(:route, request_method: 'GET', path: '/:version/projects/:id/archive(.:format)')
@@ -77,8 +80,13 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
 
         env['api.endpoint'] = endpoint
       end
+
       it 'provides labels with the method and path of the route in the grape endpoint' do
-        expect(transaction.labels).to eq({ controller: 'Grape', action: 'GET /projects/:id/archive' })
+        expect(transaction.labels).to eq({ controller: 'Grape', action: 'GET /projects/:id/archive', feature_category: '' })
+      end
+
+      it 'contains only the labels defined for transactions' do
+        expect(transaction.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABELS.keys)
       end
 
       it 'does not provide labels if route infos are missing' do
@@ -92,24 +100,25 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
     end
 
     context 'when request goes to ActionController' do
-      let(:request) { double(:request, format: double(:format, ref: :html)) }
-
       before do
-        klass = double(:klass, name: 'TestController')
-        controller = double(:controller, class: klass, action_name: 'show', request: request)
+        controller = double(:controller, class: controller_class, action_name: 'show', request: request)
 
         env['action_controller.instance'] = controller
       end
 
       it 'tags a transaction with the name and action of a controller' do
-        expect(transaction.labels).to eq({ controller: 'TestController', action: 'show' })
+        expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: '' })
+      end
+
+      it 'contains only the labels defined for transactions' do
+        expect(transaction.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABELS.keys)
       end
 
       context 'when the request content type is not :html' do
         let(:request) { double(:request, format: double(:format, ref: :json)) }
 
         it 'appends the mime type to the transaction action' do
-          expect(transaction.labels).to eq({ controller: 'TestController', action: 'show.json' })
+          expect(transaction.labels).to eq({ controller: 'TestController', action: 'show.json', feature_category: '' })
         end
       end
 
@@ -117,7 +126,14 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
         let(:request) { double(:request, format: double(:format, ref: 'http://example.com')) }
 
         it 'does not append the MIME type to the transaction action' do
-          expect(transaction.labels).to eq({ controller: 'TestController', action: 'show' })
+          expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: '' })
+        end
+      end
+
+      context 'when the feature category is known' do
+        it 'includes it in the feature category label' do
+          expect(controller_class).to receive(:feature_category_for_action).with('show').and_return(:source_code_management)
+          expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: "source_code_management" })
         end
       end
     end