Merge branch 'feature-categories-api' into 'master'

Feature categories for API requests

See merge request gitlab-org/gitlab!45260

app/controllers/application_controller.rb

@@ -22,7 +22,7 @@ class ApplicationController < ActionController::Base
   include Impersonation
   include Gitlab::Logging::CloudflareHelper
   include Gitlab::Utils::StrongMemoize
-  include ControllerWithFeatureCategory
+  include ::Gitlab::WithFeatureCategory
 
   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, actions = [])
-      feature_category_configuration[category] ||= []
-      feature_category_configuration[category] += actions.map(&:to_s)
-
-      validate_config!(feature_category_configuration)
-    end
-
-    def feature_category_for_action(action)
-      category_config = feature_category_configuration.find do |_, actions|
-        actions.empty? || actions.include?(action)
-      end
-
-      category_config&.first || superclass_feature_category_for_action(action)
-    end
-
-    private
-
-    def validate_config!(config)
-      empty = config.find { |_, actions| actions.empty? }
-      duplicate_actions = config.values.flatten.group_by(&:itself).select { |_, v| v.count > 1 }.keys
-
-      if config.length > 1 && empty
-        raise ArgumentError, "#{empty.first} is defined for all actions, but other categories are set"
-      end
-
-      if duplicate_actions.any?
-        raise ArgumentError, "Actions have multiple feature categories: #{duplicate_actions.join(', ')}"
-      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

lib/api/api.rb

@@ -48,11 +48,17 @@ module API
     before do
       coerce_nil_params_to_array!
 
+      api_endpoint = env['api.endpoint']
+      feature_category = api_endpoint.options[:for].try(:feature_category_for_app, api_endpoint).to_s
+
+      header[Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER] = feature_category
+
       Gitlab::ApplicationContext.push(
         user: -> { @current_user },
         project: -> { @project },
         namespace: -> { @group },
-        caller_id: route.origin
+        caller_id: route.origin,
+        feature_category: feature_category
       )
     end
 

lib/api/base.rb

@@ -2,5 +2,30 @@
 
 module API
   class Base < Grape::API::Instance # rubocop:disable API/Base
+    include ::Gitlab::WithFeatureCategory
+
+    class << self
+      def feature_category_for_app(app)
+        feature_category_for_action(path_for_app(app))
+      end
+
+      def path_for_app(app)
+        normalize_path(app.namespace, app.options[:path].first)
+      end
+
+      def route(methods, paths = ['/'], route_options = {}, &block)
+        if category = route_options.delete(:feature_category)
+          feature_category(category, Array(paths).map { |path| normalize_path(namespace, path) })
+        end
+
+        super
+      end
+
+      private
+
+      def normalize_path(namespace, path)
+        [namespace.presence, path.to_s.chomp('/').presence].compact.join('/')
+      end
+    end
   end
 end

lib/api/internal/base.rb

@@ -7,10 +7,16 @@ module API
       before { authenticate_by_gitlab_shell_token! }
 
       before do
+        api_endpoint = env['api.endpoint']
+        feature_category = api_endpoint.options[:for].try(:feature_category_for_app, api_endpoint).to_s
+
+        header[Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER] = feature_category
+
         Gitlab::ApplicationContext.push(
           user: -> { actor&.user },
           project: -> { project },
-          caller_id: route.origin
+          caller_id: route.origin,
+          feature_category: feature_category
         )
       end
 

lib/api/issues.rb

@@ -6,6 +6,8 @@ module API
     helpers Helpers::IssuesHelpers
     helpers Helpers::RateLimiter
 
+    feature_category :issue_tracking
+
     before { authenticate_non_get! }
 
     helpers do

lib/gitlab/metrics/requests_rack_middleware.rb

@@ -63,7 +63,7 @@ module Gitlab
           if health_endpoint
             RequestsRackMiddleware.http_health_requests_total.increment(status: status, method: method)
           else
-            RequestsRackMiddleware.http_request_total.increment(status: status, method: method, feature_category: feature_category || FEATURE_CATEGORY_DEFAULT)
+            RequestsRackMiddleware.http_request_total.increment(status: status, method: method, feature_category: feature_category.presence || FEATURE_CATEGORY_DEFAULT)
           end
         end
       end

lib/gitlab/with_feature_category.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module WithFeatureCategory
+    extend ActiveSupport::Concern
+    include Gitlab::ClassAttributes
+
+    class_methods do
+      def feature_category(category, actions = [])
+        feature_category_configuration[category] ||= []
+        feature_category_configuration[category] += actions.map(&:to_s)
+
+        validate_config!(feature_category_configuration)
+      end
+
+      def feature_category_for_action(action)
+        category_config = feature_category_configuration.find do |_, actions|
+          actions.empty? || actions.include?(action)
+        end
+
+        category_config&.first || superclass_feature_category_for_action(action)
+      end
+
+      private
+
+      def validate_config!(config)
+        empty = config.find { |_, actions| actions.empty? }
+        duplicate_actions = config.values.map(&:uniq).flatten.group_by(&:itself).select { |_, v| v.count > 1 }.keys
+
+        if config.length > 1 && empty
+          raise ArgumentError, "#{empty.first} is defined for all actions, but other categories are set"
+        end
+
+        if duplicate_actions.any?
+          raise ArgumentError, "Actions have multiple feature categories: #{duplicate_actions.join(', ')}"
+        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
+end

spec/controllers/every_controller_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe "Every controller" do
         .compact
         .select { |route| route[:controller].present? && route[:action].present? }
         .map { |route| [constantize_controller(route[:controller]), route[:action]] }
-        .select { |(controller, action)| controller&.include?(ControllerWithFeatureCategory) }
+        .select { |(controller, action)| controller&.include?(::Gitlab::WithFeatureCategory) }
         .reject { |(controller, action)| controller == ApplicationController || controller == Devise::UnlocksController }
     end
 

spec/lib/api/every_api_endpoint_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Every API endpoint' 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(:api_endpoints) do
+      API::API.routes.map do |route|
+        [route.app.options[:for], API::Base.path_for_app(route.app)]
+      end
+    end
+
+    let_it_be(:routes_without_category) do
+      api_endpoints.map do |(klass, path)|
+        next if klass.try(:feature_category_for_action, path)
+        # We'll add the rest in https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/463
+        next unless klass == ::API::Users || klass == ::API::Issues
+
+        "#{klass}##{path}"
+      end.compact.uniq
+    end
+
+    it "has feature categories" do
+      expect(routes_without_category).to be_empty, "#{routes_without_category} did not have a category"
+    end
+
+    it "recognizes the feature categories" do
+      routes_unknown_category = api_endpoints.map do |(klass, path)|
+        used_category = klass.try(:feature_category_for_action, path)
+        next unless used_category
+        next if used_category == :not_owned
+
+        [path, 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
+
+    # This is required for API::Base.path_for_app to work, as it picks
+    # the first path
+    it "has no routes with multiple paths" do
+      routes_with_multiple_paths = API::API.routes.select { |route| route.app.options[:path].length != 1 }
+      failure_routes = routes_with_multiple_paths.map { |route| "#{route.app.options[:for]}:[#{route.app.options[:path].join(', ')}]" }
+
+      expect(routes_with_multiple_paths).to be_empty, "#{failure_routes} have multiple paths"
+    end
+
+    it "doesn't define or exclude categories on removed actions", :aggregate_failures do
+      api_endpoints.group_by(&:first).each do |klass, paths|
+        existing_paths = paths.map(&:last)
+        used_paths = paths_defined_in_feature_category_config(klass)
+        non_existing_used_paths = used_paths - existing_paths
+
+        expect(non_existing_used_paths).to be_empty,
+                                           "#{klass} used #{non_existing_used_paths} to define feature category, but the route does not exist"
+      end
+    end
+  end
+
+  def paths_defined_in_feature_category_config(klass)
+    (klass.try(:class_attributes) || {}).fetch(:feature_category_config, {})
+      .values
+      .flatten
+      .map(&:to_s)
+  end
+end

spec/lib/gitlab/metrics/requests_rack_middleware_spec.rb

@@ -113,24 +113,38 @@ RSpec.describe Gitlab::Metrics::RequestsRackMiddleware do
       end
     end
 
-    context 'when a feature category header is present' do
-      before do
-        allow(app).to receive(:call).and_return([200, { described_class::FEATURE_CATEGORY_HEADER => 'issue_tracking' }, nil])
-      end
+    context 'feature category header' do
+      context 'when a feature category header is present' do
+        before do
+          allow(app).to receive(:call).and_return([200, { described_class::FEATURE_CATEGORY_HEADER => 'issue_tracking' }, nil])
+        end
 
-      it 'adds the feature category to the labels for http_request_total' do
-        expect(described_class).to receive_message_chain(:http_request_total, :increment).with(method: 'get', status: 200, feature_category: 'issue_tracking')
+        it 'adds the feature category to the labels for http_request_total' do
+          expect(described_class).to receive_message_chain(:http_request_total, :increment).with(method: 'get', status: 200, feature_category: 'issue_tracking')
 
-        subject.call(env)
+          subject.call(env)
+        end
+
+        it 'does not record a feature category for health check endpoints' do
+          env['PATH_INFO'] = '/-/liveness'
+
+          expect(described_class).to receive_message_chain(:http_health_requests_total, :increment).with(method: 'get', status: 200)
+          expect(described_class).not_to receive(:http_request_total)
+
+          subject.call(env)
+        end
       end
 
-      it 'does not record a feature category for health check endpoints' do
-        env['PATH_INFO'] = '/-/liveness'
+      context 'when the feature category header is an empty string' do
+        before do
+          allow(app).to receive(:call).and_return([200, { described_class::FEATURE_CATEGORY_HEADER => '' }, nil])
+        end
 
-        expect(described_class).to receive_message_chain(:http_health_requests_total, :increment).with(method: 'get', status: 200)
-        expect(described_class).not_to receive(:http_request_total)
+        it 'sets the feature category to unknown' do
+          expect(described_class).to receive_message_chain(:http_request_total, :increment).with(method: 'get', status: 200, feature_category: 'unknown')
 
-        subject.call(env)
+          subject.call(env)
+        end
       end
     end
 

spec/controllers/concerns/controller_with_feature_category_spec.rb → spec/lib/gitlab/with_feature_category_spec.rb

 # frozen_string_literal: true
 
 require 'fast_spec_helper'
-require_relative "../../../app/controllers/concerns/controller_with_feature_category"
+require_relative "../../../lib/gitlab/with_feature_category"
 
-RSpec.describe ControllerWithFeatureCategory do
+RSpec.describe Gitlab::WithFeatureCategory do
   describe ".feature_category_for_action" do
     let(:base_controller) do
       Class.new do
-        include ControllerWithFeatureCategory
+        include ::Gitlab::WithFeatureCategory
       end
     end
 
@@ -56,5 +56,14 @@ RSpec.describe ControllerWithFeatureCategory do
         end
       end.to raise_error(ArgumentError, "Actions have multiple feature categories: world")
     end
+
+    it "does not raise an error when multiple calls define the same action and feature category" do
+      expect do
+        Class.new(base_controller) do
+          feature_category :hello, [:world]
+          feature_category :hello, ["world"]
+        end
+      end.not_to raise_error
+    end
   end
 end

spec/requests/api/api_spec.rb

@@ -92,4 +92,36 @@ RSpec.describe API::API do
       end
     end
   end
+
+  context 'application context' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:user) { project.owner }
+
+    it 'logs all application context fields' do
+      allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do
+        Labkit::Context.current.to_h.tap do |log_context|
+          expect(log_context).to match('correlation_id' => an_instance_of(String),
+                                       'meta.caller_id' => '/api/:version/projects/:id/issues',
+                                       'meta.project' => project.full_path,
+                                       'meta.root_namespace' => project.namespace.full_path,
+                                       'meta.user' => user.username,
+                                       'meta.feature_category' => 'issue_tracking')
+        end
+      end
+
+      get(api("/projects/#{project.id}/issues", user))
+    end
+
+    it 'skips fields that do not apply' do
+      allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do
+        Labkit::Context.current.to_h.tap do |log_context|
+          expect(log_context).to match('correlation_id' => an_instance_of(String),
+                                       'meta.caller_id' => '/api/:version/users',
+                                       'meta.feature_category' => 'users')
+        end
+      end
+
+      get(api('/users'))
+    end
+  end
 end