Merge branch 'set-feature-category-headers-for-etag-caching' into 'master'

Set feature category in metrics for ETag caching hits

See merge request gitlab-org/gitlab!46126

ee/lib/ee/gitlab/etag_caching/router.rb

@@ -7,7 +7,8 @@ module EE
         EE_ROUTES = [
           ::Gitlab::EtagCaching::Router::Route.new(
             %r(^/groups/#{::Gitlab::PathRegex.full_namespace_route_regex}/-/epics/\d+/notes\z),
-            'epic_notes'
+            'epic_notes',
+            'epics'
           )
         ].freeze
 

ee/spec/lib/ee/gitlab/etag_caching/router_spec.rb

@@ -19,4 +19,12 @@ RSpec.describe Gitlab::EtagCaching::Router do
 
     expect(result).to be_blank
   end
+
+  it 'has a valid feature category for every route', :aggregate_failures do
+    feature_categories = YAML.load_file(Rails.root.join('config', 'feature_categories.yml')).to_set
+
+    described_class::EE_ROUTES.each do |route|
+      expect(feature_categories).to include(route.feature_category), "#{route.name} has a category of #{route.feature_category}, which is not valid"
+    end
+  end
 end

lib/gitlab/etag_caching/middleware.rb

@@ -54,7 +54,13 @@ module Gitlab
 
         add_instrument_for_cache_hit(status_code, route, request)
 
-        [status_code, { 'ETag' => etag, 'X-Gitlab-From-Cache' => 'true' }, []]
+        new_headers = {
+          'ETag' => etag,
+          'X-Gitlab-From-Cache' => 'true',
+          ::Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER => route.feature_category
+        }
+
+        [status_code, new_headers, []]
       end
 
       def track_cache_miss(if_none_match, cached_value_present, route)

lib/gitlab/etag_caching/router.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module EtagCaching
     class Router
-      Route = Struct.new(:regexp, :name)
+      Route = Struct.new(:regexp, :name, :feature_category)
       # We enable an ETag for every request matching the regex.
       # To match a regex the path needs to match the following:
       #   - Don't contain a reserved word (expect for the words used in the
@@ -20,59 +20,73 @@ module Gitlab
       ROUTES = [
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/noteable/issue/\d+/notes\z),
-          'issue_notes'
+          'issue_notes',
+          'issue_tracking'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/noteable/merge_request/\d+/notes\z),
-          'merge_request_notes'
+          'merge_request_notes',
+          'code_review'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/issues/\d+/realtime_changes\z),
-          'issue_title'
+          'issue_title',
+          'issue_tracking'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/commit/\S+/pipelines\.json\z),
-          'commit_pipelines'
+          'commit_pipelines',
+          'continuous_integration'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/merge_requests/new\.json\z),
-          'new_merge_request_pipelines'
+          'new_merge_request_pipelines',
+          'continuous_integration'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/merge_requests/\d+/pipelines\.json\z),
-          'merge_request_pipelines'
+          'merge_request_pipelines',
+          'continuous_integration'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/pipelines\.json\z),
-          'project_pipelines'
+          'project_pipelines',
+          'continuous_integration'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/pipelines/\d+\.json\z),
-          'project_pipeline'
+          'project_pipeline',
+          'continuous_integration'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/builds/\d+\.json\z),
-          'project_build'
+          'project_build',
+          'continuous_integration'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/clusters/\d+/environments\z),
-          'cluster_environments'
+          'cluster_environments',
+          'continuous_delivery'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/environments\.json\z),
-          'environments'
+          'environments',
+          'continuous_delivery'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/import/github/realtime_changes\.json\z),
-          'realtime_changes_import_github'
+          'realtime_changes_import_github',
+          'importers'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/import/gitea/realtime_changes\.json\z),
-          'realtime_changes_import_gitea'
+          'realtime_changes_import_gitea',
+          'importers'
         ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/merge_requests/\d+/cached_widget\.json\z),
-          'merge_request_widget'
+          'merge_request_widget',
+          'code_review'
         )
       ].freeze
 

spec/lib/gitlab/etag_caching/middleware_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::EtagCaching::Middleware do
+RSpec.describe Gitlab::EtagCaching::Middleware, :clean_gitlab_redis_shared_state do
   let(:app) { double(:app) }
   let(:middleware) { described_class.new(app) }
   let(:app_status_code) { 200 }
@@ -17,10 +17,12 @@ RSpec.describe Gitlab::EtagCaching::Middleware do
       mock_app_response
     end
 
-    it 'does not add ETag header' do
+    it 'does not add ETag headers' do
       _, headers, _ = middleware.call(build_request(path, if_none_match))
 
       expect(headers['ETag']).to be_nil
+      expect(headers['X-Gitlab-From-Cache']).to be_nil
+      expect(headers[::Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER]).to be_nil
     end
 
     it 'passes status code from app' do
@@ -68,7 +70,7 @@ RSpec.describe Gitlab::EtagCaching::Middleware do
       mock_value_in_store('123')
     end
 
-    it 'returns this value as header' do
+    it 'returns the correct headers' do
       _, headers, _ = middleware.call(build_request(path, if_none_match))
 
       expect(headers['ETag']).to eq 'W/"123"'
@@ -126,6 +128,13 @@ RSpec.describe Gitlab::EtagCaching::Middleware do
       expect(status).to eq 304
     end
 
+    it 'sets correct headers' do
+      _, headers, _ = middleware.call(build_request(path, if_none_match))
+
+      expect(headers).to include('X-Gitlab-From-Cache' => 'true',
+                                 ::Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER => 'issue_tracking')
+    end
+
     it_behaves_like 'sends a process_action.action_controller notification', 304
 
     it 'returns empty body' do

spec/lib/gitlab/etag_caching/router_spec.rb

@@ -127,4 +127,12 @@ RSpec.describe Gitlab::EtagCaching::Router do
     expect(result).to be_present
     expect(result.name).to eq 'project_pipeline'
   end
+
+  it 'has a valid feature category for every route', :aggregate_failures do
+    feature_categories = YAML.load_file(Rails.root.join('config', 'feature_categories.yml')).to_set
+
+    described_class::ROUTES.each do |route|
+      expect(feature_categories).to include(route.feature_category), "#{route.name} has a category of #{route.feature_category}, which is not valid"
+    end
+  end
 end