diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2017-03-06 13:42:08 +0300 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2017-03-06 13:42:08 +0300 |
commit | 633aaf2b6123359e4c7bfce441022a68b2f21222 (patch) | |
tree | 255a09114c072005de5a6e3524efd59189df88d5 | |
parent | 4a6bc1f6a415b82e88473f3bd076f95fb4973562 (diff) | |
parent | ee318727774dbec75d5506a4b4749fc4236206d5 (diff) |
Merge branch 'etag-notes-polling' into 'master'
Use ETag to improve performance of issue notes polling
Closes #27582
See merge request !9036
-rw-r--r-- | app/assets/javascripts/notes.js | 2 | ||||
-rw-r--r-- | app/controllers/projects/notes_controller.rb | 7 | ||||
-rw-r--r-- | app/models/note.rb | 13 | ||||
-rw-r--r-- | app/views/projects/notes/_notes_with_form.html.haml | 2 | ||||
-rw-r--r-- | changelogs/unreleased/etag-notes-polling.yml | 4 | ||||
-rw-r--r-- | config/initializers/8_metrics.rb (renamed from config/initializers/metrics.rb) | 0 | ||||
-rw-r--r-- | config/initializers/etag_caching.rb | 4 | ||||
-rw-r--r-- | config/routes/project.rb | 4 | ||||
-rw-r--r-- | doc/development/instrumentation.md | 2 | ||||
-rw-r--r-- | lib/gitlab/etag_caching/middleware.rb | 66 | ||||
-rw-r--r-- | lib/gitlab/etag_caching/store.rb | 32 | ||||
-rw-r--r-- | spec/controllers/projects/notes_controller_spec.rb | 27 | ||||
-rw-r--r-- | spec/initializers/8_metrics_spec.rb (renamed from spec/initializers/metrics_spec.rb) | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/etag_caching/middleware_spec.rb | 163 | ||||
-rw-r--r-- | spec/models/note_spec.rb | 12 | ||||
-rw-r--r-- | spec/routing/project_routing_spec.rb | 18 |
16 files changed, 348 insertions, 10 deletions
diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 47fa0f2eb96..df7a7d2a459 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 5cf3a7f593b..d00177e7612 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/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 diff --git a/app/models/note.rb b/app/models/note.rb index 4c97e4a986c..e22e96aec6f 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 @@ 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 diff --git a/app/views/projects/notes/_notes_with_form.html.haml b/app/views/projects/notes/_notes_with_form.html.haml index 08c73d94a09..90a150aa74c 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/changelogs/unreleased/etag-notes-polling.yml b/changelogs/unreleased/etag-notes-polling.yml new file mode 100644 index 00000000000..53990821d25 --- /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/metrics.rb b/config/initializers/8_metrics.rb index a1517e6afc8..a1517e6afc8 100644 --- a/config/initializers/metrics.rb +++ b/config/initializers/8_metrics.rb diff --git a/config/initializers/etag_caching.rb b/config/initializers/etag_caching.rb new file mode 100644 index 00000000000..eba88801141 --- /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/config/routes/project.rb b/config/routes/project.rb index 2703bf4ab46..7dc7963ab88 100644 --- a/config/routes/project.rb +++ b/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] diff --git a/doc/development/instrumentation.md b/doc/development/instrumentation.md index b8669964c84..a14c0752366 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/lib/gitlab/etag_caching/middleware.rb b/lib/gitlab/etag_caching/middleware.rb new file mode 100644 index 00000000000..0f24f9bbfde --- /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 00000000000..9532e432f78 --- /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/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index dc597202050..d80780b1d90 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/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 diff --git a/spec/initializers/metrics_spec.rb b/spec/initializers/8_metrics_spec.rb index bb595162370..570754621f3 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) } 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 00000000000..8b5bfc4dbb0 --- /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 diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 1cde9e04951..33536487c41 100644 --- a/spec/models/note_spec.rb +++ b/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 diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index a5bc62ef6c2..d31f1bdfb7c 100644 --- a/spec/routing/project_routing_spec.rb +++ b/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 |