Merge branch 'jv-etag-annotations' into 'master'

Annotate Etag cache hits with caller_id

See merge request gitlab-org/gitlab!81303

ee/lib/ee/gitlab/etag_caching/router/restful.rb → ee/lib/ee/gitlab/etag_caching/router/rails.rb

@@ -4,14 +4,15 @@ module EE
   module Gitlab
     module EtagCaching
       module Router
-        module Restful
+        module Rails
           extend ActiveSupport::Concern
 
           EE_ROUTE_DEFINITONS = [
             [
               %r(^/groups/#{::Gitlab::PathRegex.full_namespace_route_regex}/-/epics/\d+/notes\z),
               'epic_notes',
-              'portfolio_management'
+              ::Groups::Epics::NotesController,
+              :index
             ]
           ].freeze
 
@@ -28,7 +29,7 @@ module EE
             end
 
             def ee_routes
-              EE_ROUTE_DEFINITONS.map(&method(:build_route))
+              EE_ROUTE_DEFINITONS.map(&method(:build_rails_route))
             end
           end
         end

ee/spec/lib/ee/gitlab/etag_caching/router/restful_spec.rb → ee/spec/lib/ee/gitlab/etag_caching/router/rails_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::EtagCaching::Router::Restful do
+RSpec.describe Gitlab::EtagCaching::Router::Rails do
   it 'matches epic notes endpoint' do
     result = described_class.match(
       double(path_info: '/groups/my-group/and-subgroup/-/epics/1/notes')

lib/gitlab/etag_caching/middleware.rb

@@ -67,7 +67,10 @@ module Gitlab
 
         add_instrument_for_cache_hit(status_code, route, request)
 
-        Gitlab::ApplicationContext.push(feature_category: route.feature_category)
+        Gitlab::ApplicationContext.push(
+          feature_category: route.feature_category,
+          caller_id: route.caller_id
+        )
 
         new_headers = {
           'ETag' => etag,

lib/gitlab/etag_caching/router.rb

@@ -3,22 +3,33 @@
 module Gitlab
   module EtagCaching
     module Router
-      Route = Struct.new(:regexp, :name, :feature_category, :router) do
+      Route = Struct.new(:router, :regexp, :name, :feature_category, :caller_id) do
         delegate :match, to: :regexp
         delegate :cache_key, to: :router
       end
 
       module Helpers
         def build_route(attrs)
-          EtagCaching::Router::Route.new(*attrs, self)
+          EtagCaching::Router::Route.new(self, *attrs)
+        end
+
+        def build_rails_route(attrs)
+          regexp, name, controller, action_name = *attrs
+          EtagCaching::Router::Route.new(
+            self,
+            regexp,
+            name,
+            controller.feature_category_for_action(action_name).to_s,
+            controller.endpoint_id_for_action(action_name).to_s
+          )
         end
       end
 
-      # Performing RESTful routing match before GraphQL would be more expensive
+      # Performing Rails routing match before GraphQL would be more expensive
       # for the GraphQL requests because we need to traverse all of the RESTful
       # route definitions before falling back to GraphQL.
       def self.match(request)
-        Router::Graphql.match(request) || Router::Restful.match(request)
+        Router::Graphql.match(request) || Router::Rails.match(request)
       end
     end
   end

lib/gitlab/etag_caching/router/restful.rb → lib/gitlab/etag_caching/router/rails.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module EtagCaching
     module Router
-      class Restful
+      class Rails
         extend EtagCaching::Router::Helpers
 
         # We enable an ETag for every request matching the regex.
@@ -23,74 +23,88 @@ module Gitlab
           [
             %r(#{RESERVED_WORDS_PREFIX}/noteable/issue/\d+/notes\z),
             'issue_notes',
-            'team_planning'
+            ::Projects::NotesController,
+            :index
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/noteable/merge_request/\d+/notes\z),
             'merge_request_notes',
-            'code_review'
+            ::Projects::NotesController,
+            :index
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/issues/\d+/realtime_changes\z),
             'issue_title',
-            'team_planning'
+            ::Projects::IssuesController,
+            :realtime_changes
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/commit/\S+/pipelines\.json\z),
             'commit_pipelines',
-            'continuous_integration'
+            ::Projects::CommitController,
+            :pipelines
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/merge_requests/new\.json\z),
             'new_merge_request_pipelines',
-            'continuous_integration'
+            ::Projects::MergeRequests::CreationsController,
+            :new
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/merge_requests/\d+/pipelines\.json\z),
             'merge_request_pipelines',
-            'continuous_integration'
+            ::Projects::MergeRequestsController,
+            :pipelines
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/pipelines\.json\z),
             'project_pipelines',
-            'continuous_integration'
+            ::Projects::PipelinesController,
+            :index
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/pipelines/\d+\.json\z),
             'project_pipeline',
-            'continuous_integration'
+            ::Projects::PipelinesController,
+            :show
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/builds/\d+\.json\z),
             'project_build',
-            'continuous_integration'
+            ::Projects::BuildsController,
+            :show
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/clusters/\d+/environments\z),
             'cluster_environments',
-            'continuous_delivery'
+            ::Groups::ClustersController,
+            :environments
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/-/environments\.json\z),
             'environments',
-            'continuous_delivery'
+            ::Projects::EnvironmentsController,
+            :index
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/import/github/realtime_changes\.json\z),
             'realtime_changes_import_github',
-            'importers'
+            ::Import::GithubController,
+            :realtime_changes
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/import/gitea/realtime_changes\.json\z),
             'realtime_changes_import_gitea',
-            'importers'
+            ::Import::GiteaController,
+            :realtime_changes
           ],
           [
             %r(#{RESERVED_WORDS_PREFIX}/merge_requests/\d+/cached_widget\.json\z),
             'merge_request_widget',
-            'code_review'
+            ::Projects::MergeRequests::ContentController,
+            :cached_widget
           ]
-        ].map(&method(:build_route)).freeze
+        ].map(&method(:build_rails_route)).freeze
 
         # Overridden in EE to add more routes
         def self.all_routes
@@ -109,4 +123,4 @@ module Gitlab
   end
 end
 
-Gitlab::EtagCaching::Router::Restful.prepend_mod_with('Gitlab::EtagCaching::Router::Restful')
+Gitlab::EtagCaching::Router::Rails.prepend_mod_with('Gitlab::EtagCaching::Router::Rails')

spec/lib/gitlab/etag_caching/middleware_spec.rb

@@ -174,7 +174,8 @@ RSpec.describe Gitlab::EtagCaching::Middleware, :clean_gitlab_redis_shared_state
 
     it "pushes route's feature category to the context" do
       expect(Gitlab::ApplicationContext).to receive(:push).with(
-        feature_category: 'team_planning'
+        feature_category: 'team_planning',
+        caller_id: 'Projects::NotesController#index'
       )
 
       _, _, _ = middleware.call(build_request(path, if_none_match))

spec/lib/gitlab/etag_caching/router/restful_spec.rb → spec/lib/gitlab/etag_caching/router/rails_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::EtagCaching::Router::Restful do
+RSpec.describe Gitlab::EtagCaching::Router::Rails do
   it 'matches issue notes endpoint' do
     result = match_route('/my-group/and-subgroup/here-comes-the-project/noteable/issue/1/notes')
 
@@ -114,6 +114,12 @@ RSpec.describe Gitlab::EtagCaching::Router::Restful do
     end
   end
 
+  it 'has a caller_id for every route', :aggregate_failures do
+    described_class::ROUTES.each do |route|
+      expect(route.caller_id).to include('#'), "#{route.name} has caller_id #{route.caller_id}, which is not valid"
+    end
+  end
+
   def match_route(path)
     described_class.match(double(path_info: path))
   end

spec/lib/gitlab/etag_caching/router_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe Gitlab::EtagCaching::Router do
 
         expect(result).to be_present
         expect(result.name).to eq 'project_pipelines'
-        expect(result.router).to eq Gitlab::EtagCaching::Router::Restful
+        expect(result.router).to eq Gitlab::EtagCaching::Router::Rails
       end
     end