Merge branch 'etag-notes-polling' into 'master'

Use ETag to improve performance of issue notes polling

Closes #27582

See merge request !9036

app/assets/javascripts/notes.js

@@ -198,7 +198,7 @@ require('./task_list');
       this.refreshing = true;
       return $.ajax({
         url: this.notes_url,
-        data: "last_fetched_at=" + this.last_fetched_at,
+        headers: { "X-Last-Fetched-At": this.last_fetched_at },
         dataType: "json",
         success: (function(_this) {
           return function(data) {

app/controllers/projects/notes_controller.rb

@@ -211,6 +211,11 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def find_current_user_notes
-    @notes = NotesFinder.new(project, current_user, params).execute.inc_author
+    @notes = NotesFinder.new(project, current_user, params.merge(last_fetched_at: last_fetched_at))
+      .execute.inc_author
+  end
+
+  def last_fetched_at
+    request.headers['X-Last-Fetched-At']
   end
 end

app/models/note.rb

@@ -85,6 +85,7 @@ class Note < ActiveRecord::Base
   before_validation :nullify_blank_type, :nullify_blank_line_code
   before_validation :set_discussion_id
   after_save :keep_around_commit, unless: :for_personal_snippet?
+  after_save :expire_etag_cache
 
   class << self
     def model_name
@@ -272,4 +273,16 @@ class Note < ActiveRecord::Base
       self.class.build_discussion_id(noteable_type, noteable_id || commit_id)
     end
   end
+
+  def expire_etag_cache
+    return unless for_issue?
+
+    key = Gitlab::Routing.url_helpers.namespace_project_noteable_notes_path(
+      noteable.project.namespace,
+      noteable.project,
+      target_type: noteable_type.underscore,
+      target_id: noteable.id
+    )
+    Gitlab::EtagCaching::Store.new.touch(key)
+  end
 end

app/views/projects/notes/_notes_with_form.html.haml

@@ -23,4 +23,4 @@
           to post a comment
 
 :javascript
-  var notes = new Notes("#{namespace_project_notes_path(namespace_id: @project.namespace, project_id: @project, target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json}, #{Time.now.to_i}, "#{diff_view}")
+  var notes = new Notes("#{namespace_project_noteable_notes_path(namespace_id: @project.namespace, project_id: @project, target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json}, #{Time.now.to_i}, "#{diff_view}")

changelogs/unreleased/etag-notes-polling.yml

+---
+title: Use ETag to improve performance of issue notes polling
+merge_request: 9036
+author:

config/initializers/etag_caching.rb

+# This middleware has to come after Gitlab::Metrics::RackMiddleware
+# in the middleware stack, because it tracks events with
+# GitLab Performance Monitoring
+Rails.application.config.middleware.use(Gitlab::EtagCaching::Middleware)

config/routes/project.rb

@@ -267,7 +267,7 @@ constraints(ProjectUrlConstrainer.new) do
 
       resources :group_links, only: [:index, :create, :update, :destroy], constraints: { id: /\d+/ }
 
-      resources :notes, only: [:index, :create, :destroy, :update], concerns: :awardable, constraints: { id: /\d+/ } do
+      resources :notes, only: [:create, :destroy, :update], concerns: :awardable, constraints: { id: /\d+/ } do
         member do
           delete :delete_attachment
           post :resolve
@@ -275,6 +275,8 @@ constraints(ProjectUrlConstrainer.new) do
         end
       end
 
+      get 'noteable/:target_type/:target_id/notes' => 'notes#index', as: 'noteable_notes'
+
       resources :boards, only: [:index, :show] do
         scope module: :boards do
           resources :issues, only: [:index, :update]

doc/development/instrumentation.md

@@ -35,7 +35,7 @@ Using this method is in general preferred over directly calling the various
 instrumentation methods.
 
 Method instrumentation should be added in the initializer
-`config/initializers/metrics.rb`.
+`config/initializers/8_metrics.rb`.
 
 ### Examples
 

lib/gitlab/etag_caching/middleware.rb

+module Gitlab
+  module EtagCaching
+    class Middleware
+      RESERVED_WORDS = ProjectPathValidator::RESERVED.map { |word| "/#{word}/" }.join('|')
+      ROUTE_REGEXP = Regexp.union(
+        %r(^(?!.*(#{RESERVED_WORDS})).*/noteable/issue/\d+/notes\z)
+      )
+
+      def initialize(app)
+        @app = app
+      end
+
+      def call(env)
+        return @app.call(env) unless enabled_for_current_route?(env)
+        Gitlab::Metrics.add_event(:etag_caching_middleware_used)
+
+        etag, cached_value_present = get_etag(env)
+        if_none_match = env['HTTP_IF_NONE_MATCH']
+
+        if if_none_match == etag
+          Gitlab::Metrics.add_event(:etag_caching_cache_hit)
+          [304, { 'ETag' => etag }, ['']]
+        else
+          track_cache_miss(if_none_match, cached_value_present)
+
+          status, headers, body = @app.call(env)
+          headers['ETag'] = etag
+          [status, headers, body]
+        end
+      end
+
+      private
+
+      def enabled_for_current_route?(env)
+        ROUTE_REGEXP.match(env['PATH_INFO'])
+      end
+
+      def get_etag(env)
+        cache_key = env['PATH_INFO']
+        store = Store.new
+        current_value = store.get(cache_key)
+        cached_value_present = current_value.present?
+
+        unless cached_value_present
+          current_value = store.touch(cache_key, only_if_missing: true)
+        end
+
+        [weak_etag_format(current_value), cached_value_present]
+      end
+
+      def weak_etag_format(value)
+        %Q{W/"#{value}"}
+      end
+
+      def track_cache_miss(if_none_match, cached_value_present)
+        if if_none_match.blank?
+          Gitlab::Metrics.add_event(:etag_caching_header_missing)
+        elsif !cached_value_present
+          Gitlab::Metrics.add_event(:etag_caching_key_not_found)
+        else
+          Gitlab::Metrics.add_event(:etag_caching_resource_changed)
+        end
+      end
+    end
+  end
+end

lib/gitlab/etag_caching/store.rb

+module Gitlab
+  module EtagCaching
+    class Store
+      EXPIRY_TIME = 10.minutes
+      REDIS_NAMESPACE = 'etag:'.freeze
+
+      def get(key)
+        Gitlab::Redis.with { |redis| redis.get(redis_key(key)) }
+      end
+
+      def touch(key, only_if_missing: false)
+        etag = generate_etag
+
+        Gitlab::Redis.with do |redis|
+          redis.set(redis_key(key), etag, ex: EXPIRY_TIME, nx: only_if_missing)
+        end
+
+        etag
+      end
+
+      private
+
+      def generate_etag
+        SecureRandom.hex
+      end
+
+      def redis_key(key)
+        "#{REDIS_NAMESPACE}#{key}"
+      end
+    end
+  end
+end

spec/controllers/projects/notes_controller_spec.rb

@@ -200,4 +200,31 @@ describe Projects::NotesController do
       end
     end
   end
+
+  describe 'GET index' do
+    let(:last_fetched_at) { '1487756246' }
+    let(:request_params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        target_type: 'issue',
+        target_id: issue.id
+      }
+    end
+
+    before do
+      sign_in(user)
+      project.team << [user, :developer]
+    end
+
+    it 'passes last_fetched_at from headers to NotesFinder' do
+      request.headers['X-Last-Fetched-At'] = last_fetched_at
+
+      expect(NotesFinder).to receive(:new)
+        .with(anything, anything, hash_including(last_fetched_at: last_fetched_at))
+        .and_call_original
+
+      get :index, request_params
+    end
+  end
 end

spec/initializers/metrics_spec.rb → spec/initializers/8_metrics_spec.rb

 require 'spec_helper'
-require_relative '../../config/initializers/metrics'
+require_relative '../../config/initializers/8_metrics'
 
 describe 'instrument_classes', lib: true do
   let(:config) { double(:config) }

spec/lib/gitlab/etag_caching/middleware_spec.rb

+require 'spec_helper'
+
+describe Gitlab::EtagCaching::Middleware do
+  let(:app) { double(:app) }
+  let(:middleware) { described_class.new(app) }
+  let(:app_status_code) { 200 }
+  let(:if_none_match) { nil }
+  let(:enabled_path) { '/gitlab-org/gitlab-ce/noteable/issue/1/notes' }
+
+  context 'when ETag caching is not enabled for current route' do
+    let(:path) { '/gitlab-org/gitlab-ce/tree/master/noteable/issue/1/notes' }
+
+    before do
+      mock_app_response
+    end
+
+    it 'does not add ETag header' do
+      _, headers, _ = middleware.call(build_env(path, if_none_match))
+
+      expect(headers['ETag']).to be_nil
+    end
+
+    it 'passes status code from app' do
+      status, _, _ = middleware.call(build_env(path, if_none_match))
+
+      expect(status).to eq app_status_code
+    end
+  end
+
+  context 'when there is no ETag in store for given resource' do
+    let(:path) { enabled_path }
+
+    before do
+      mock_app_response
+      mock_value_in_store(nil)
+    end
+
+    it 'generates ETag' do
+      expect_any_instance_of(Gitlab::EtagCaching::Store)
+        .to receive(:touch).and_return('123')
+
+      middleware.call(build_env(path, if_none_match))
+    end
+
+    context 'when If-None-Match header was specified' do
+      let(:if_none_match) { 'W/"abc"' }
+
+      it 'tracks "etag_caching_key_not_found" event' do
+        expect(Gitlab::Metrics).to receive(:add_event)
+          .with(:etag_caching_middleware_used)
+        expect(Gitlab::Metrics).to receive(:add_event)
+          .with(:etag_caching_key_not_found)
+
+        middleware.call(build_env(path, if_none_match))
+      end
+    end
+  end
+
+  context 'when there is ETag in store for given resource' do
+    let(:path) { enabled_path }
+
+    before do
+      mock_app_response
+      mock_value_in_store('123')
+    end
+
+    it 'returns this value as header' do
+      _, headers, _ = middleware.call(build_env(path, if_none_match))
+
+      expect(headers['ETag']).to eq 'W/"123"'
+    end
+  end
+
+  context 'when If-None-Match header matches ETag in store' do
+    let(:path) { enabled_path }
+    let(:if_none_match) { 'W/"123"' }
+
+    before do
+      mock_value_in_store('123')
+    end
+
+    it 'does not call app' do
+      expect(app).not_to receive(:call)
+
+      middleware.call(build_env(path, if_none_match))
+    end
+
+    it 'returns status code 304' do
+      status, _, _ = middleware.call(build_env(path, if_none_match))
+
+      expect(status).to eq 304
+    end
+
+    it 'tracks "etag_caching_cache_hit" event' do
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_middleware_used)
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_cache_hit)
+
+      middleware.call(build_env(path, if_none_match))
+    end
+  end
+
+  context 'when If-None-Match header does not match ETag in store' do
+    let(:path) { enabled_path }
+    let(:if_none_match) { 'W/"abc"' }
+
+    before do
+      mock_value_in_store('123')
+    end
+
+    it 'calls app' do
+      expect(app).to receive(:call).and_return([app_status_code, {}, ['body']])
+
+      middleware.call(build_env(path, if_none_match))
+    end
+
+    it 'tracks "etag_caching_resource_changed" event' do
+      mock_app_response
+
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_middleware_used)
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_resource_changed)
+
+      middleware.call(build_env(path, if_none_match))
+    end
+  end
+
+  context 'when If-None-Match header is not specified' do
+    let(:path) { enabled_path }
+
+    before do
+      mock_value_in_store('123')
+      mock_app_response
+    end
+
+    it 'tracks "etag_caching_header_missing" event' do
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_middleware_used)
+      expect(Gitlab::Metrics).to receive(:add_event)
+        .with(:etag_caching_header_missing)
+
+      middleware.call(build_env(path, if_none_match))
+    end
+  end
+
+  def mock_app_response
+    allow(app).to receive(:call).and_return([app_status_code, {}, ['body']])
+  end
+
+  def mock_value_in_store(value)
+    allow_any_instance_of(Gitlab::EtagCaching::Store)
+      .to receive(:get).and_return(value)
+  end
+
+  def build_env(path, if_none_match)
+    {
+      'PATH_INFO' => path,
+      'HTTP_IF_NONE_MATCH' => if_none_match
+    }
+  end
+end

spec/models/note_spec.rb

@@ -387,4 +387,16 @@ describe Note, models: true do
       end
     end
   end
+
+  describe 'expiring ETag cache' do
+    let(:note) { build(:note_on_issue) }
+
+    it "expires cache for note's issue when note is saved" do
+      expect_any_instance_of(Gitlab::EtagCaching::Store)
+        .to receive(:touch)
+        .with("/#{note.project.namespace.to_param}/#{note.project.to_param}/noteable/issue/#{note.noteable.id}/notes")
+
+      note.save!
+    end
+  end
 end

spec/routing/project_routing_spec.rb

@@ -431,12 +431,22 @@ describe 'project routing' do
     end
   end
 
-  #         project_notes GET    /:project_id/notes(.:format)         notes#index
-  #                       POST   /:project_id/notes(.:format)         notes#create
-  #          project_note DELETE /:project_id/notes/:id(.:format)     notes#destroy
+  # project_noteable_notes GET    /:project_id/noteable/:target_type/:target_id/notes notes#index
+  #                        POST   /:project_id/notes(.:format)                        notes#create
+  #           project_note DELETE /:project_id/notes/:id(.:format)                    notes#destroy
   describe Projects::NotesController, 'routing' do
+    it 'to #index' do
+      expect(get('/gitlab/gitlabhq/noteable/issue/1/notes')).to route_to(
+        'projects/notes#index',
+        namespace_id: 'gitlab',
+        project_id: 'gitlabhq',
+        target_type: 'issue',
+        target_id: '1'
+      )
+    end
+
     it_behaves_like 'RESTful project resources' do
-      let(:actions)    { [:index, :create, :destroy] }
+      let(:actions)    { [:create, :destroy] }
       let(:controller) { 'notes' }
     end
   end