From 0a31efb57768345e2b3350a493021a26b54994a3 Mon Sep 17 00:00:00 2001 From: Adam Niedzielski Date: Tue, 7 Feb 2017 18:02:02 +0100 Subject: [PATCH 1/4] Remove query parameters from notes polling endpoint to make caching easier --- app/assets/javascripts/notes.js | 2 +- app/controllers/projects/notes_controller.rb | 7 ++++- .../projects/notes/_notes_with_form.html.haml | 2 +- config/routes/project.rb | 4 ++- .../projects/notes_controller_spec.rb | 27 +++++++++++++++++++ spec/routing/project_routing_spec.rb | 18 ++++++++++--- 6 files changed, 52 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 47fa0f2eb96f..df7a7d2a459f 100644 --- a/app/assets/javascripts/notes.js +++ b/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) { diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 5cf3a7f593b5..d00177e7612f 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -211,6 +211,11 @@ def note_params 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 diff --git a/app/views/projects/notes/_notes_with_form.html.haml b/app/views/projects/notes/_notes_with_form.html.haml index 08c73d94a099..90a150aa74c7 100644 --- a/app/views/projects/notes/_notes_with_form.html.haml +++ b/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}") diff --git a/config/routes/project.rb b/config/routes/project.rb index 84f123ff7172..a9f95c09bef3 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -265,7 +265,7 @@ 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 @@ -273,6 +273,8 @@ 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] diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index dc597202050f..d80780b1d90e 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -200,4 +200,31 @@ 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 diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index a5bc62ef6c2b..d31f1bdfb7c6 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -431,12 +431,22 @@ 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 -- GitLab From 61c9604721dbda53eb4a7111d16c1b19292f9766 Mon Sep 17 00:00:00 2001 From: Adam Niedzielski Date: Tue, 7 Feb 2017 18:06:08 +0100 Subject: [PATCH 2/4] Add middleware for ETag caching with Redis --- changelogs/unreleased/etag-notes-polling.yml | 4 + config/initializers/etag_caching.rb | 4 + lib/gitlab/etag_caching/middleware.rb | 66 +++++++ lib/gitlab/etag_caching/store.rb | 32 ++++ .../gitlab/etag_caching/middleware_spec.rb | 163 ++++++++++++++++++ 5 files changed, 269 insertions(+) create mode 100644 changelogs/unreleased/etag-notes-polling.yml create mode 100644 config/initializers/etag_caching.rb create mode 100644 lib/gitlab/etag_caching/middleware.rb create mode 100644 lib/gitlab/etag_caching/store.rb create mode 100644 spec/lib/gitlab/etag_caching/middleware_spec.rb diff --git a/changelogs/unreleased/etag-notes-polling.yml b/changelogs/unreleased/etag-notes-polling.yml new file mode 100644 index 000000000000..53990821d253 --- /dev/null +++ b/changelogs/unreleased/etag-notes-polling.yml @@ -0,0 +1,4 @@ +--- +title: Use ETag to improve performance of issue notes polling +merge_request: 9036 +author: diff --git a/config/initializers/etag_caching.rb b/config/initializers/etag_caching.rb new file mode 100644 index 000000000000..eba888011418 --- /dev/null +++ b/config/initializers/etag_caching.rb @@ -0,0 +1,4 @@ +# 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) diff --git a/lib/gitlab/etag_caching/middleware.rb b/lib/gitlab/etag_caching/middleware.rb new file mode 100644 index 000000000000..0f24f9bbfde6 --- /dev/null +++ b/lib/gitlab/etag_caching/middleware.rb @@ -0,0 +1,66 @@ +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 diff --git a/lib/gitlab/etag_caching/store.rb b/lib/gitlab/etag_caching/store.rb new file mode 100644 index 000000000000..9532e432f787 --- /dev/null +++ b/lib/gitlab/etag_caching/store.rb @@ -0,0 +1,32 @@ +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 diff --git a/spec/lib/gitlab/etag_caching/middleware_spec.rb b/spec/lib/gitlab/etag_caching/middleware_spec.rb new file mode 100644 index 000000000000..8b5bfc4dbb00 --- /dev/null +++ b/spec/lib/gitlab/etag_caching/middleware_spec.rb @@ -0,0 +1,163 @@ +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 -- GitLab From c661df356156240deeef7c2734670ba5f2b4b04b Mon Sep 17 00:00:00 2001 From: Adam Niedzielski Date: Wed, 8 Feb 2017 18:04:16 +0100 Subject: [PATCH 3/4] Invalidate ETag cache when note changes --- app/models/note.rb | 13 +++++++++++++ spec/models/note_spec.rb | 12 ++++++++++++ 2 files changed, 25 insertions(+) diff --git a/app/models/note.rb b/app/models/note.rb index 4c97e4a986c5..e22e96aec6fe 100644 --- a/app/models/note.rb +++ b/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 @@ def build_discussion_id 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 diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 1cde9e049518..33536487c417 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -387,4 +387,16 @@ 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 -- GitLab From ee318727774dbec75d5506a4b4749fc4236206d5 Mon Sep 17 00:00:00 2001 From: Adam Niedzielski Date: Wed, 1 Mar 2017 18:15:28 +0100 Subject: [PATCH 4/4] Execute metrics initializer earlier This makes sure that Gitlab::Metrics::RackMiddleware is added before Gitlab::EtagCaching::Middleware. --- config/initializers/{metrics.rb => 8_metrics.rb} | 0 doc/development/instrumentation.md | 2 +- spec/initializers/{metrics_spec.rb => 8_metrics_spec.rb} | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename config/initializers/{metrics.rb => 8_metrics.rb} (100%) rename spec/initializers/{metrics_spec.rb => 8_metrics_spec.rb} (87%) diff --git a/config/initializers/metrics.rb b/config/initializers/8_metrics.rb similarity index 100% rename from config/initializers/metrics.rb rename to config/initializers/8_metrics.rb diff --git a/doc/development/instrumentation.md b/doc/development/instrumentation.md index b8669964c84b..a14c0752366b 100644 --- a/doc/development/instrumentation.md +++ b/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 diff --git a/spec/initializers/metrics_spec.rb b/spec/initializers/8_metrics_spec.rb similarity index 87% rename from spec/initializers/metrics_spec.rb rename to spec/initializers/8_metrics_spec.rb index bb5951623700..570754621f39 100644 --- a/spec/initializers/metrics_spec.rb +++ b/spec/initializers/8_metrics_spec.rb @@ -1,5 +1,5 @@ require 'spec_helper' -require_relative '../../config/initializers/metrics' +require_relative '../../config/initializers/8_metrics' describe 'instrument_classes', lib: true do let(:config) { double(:config) } -- GitLab