Set feature category in metrics for ETag caching hits

When a request hits a Rails controller, we set the
X-Gitlab-Feature-Category header to the feature category for that path,
which we then use as a label in our Prometheus metrics.

For ETag caching hits, we weren't setting this, because those skip Rails
entirely (that's the point of them!). This annotates the ETag caching
routes with their feature category to set the same header there.

We don't need this for cache misses because they go to Rails anyway.

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