From 7799a9bc442738935104d3b047c257e5c5884d95 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Tue, 31 Oct 2017 09:48:10 -0400 Subject: add metrics tagging to the sidekiq middleware --- app/workers/update_merge_requests_worker.rb | 10 +++++ lib/gitlab/metrics/sidekiq_middleware.rb | 2 + spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb | 48 +++++++++++++--------- 3 files changed, 40 insertions(+), 20 deletions(-) diff --git a/app/workers/update_merge_requests_worker.rb b/app/workers/update_merge_requests_worker.rb index 89ae17cef37..f1e43bea8b3 100644 --- a/app/workers/update_merge_requests_worker.rb +++ b/app/workers/update_merge_requests_worker.rb @@ -2,6 +2,12 @@ class UpdateMergeRequestsWorker include Sidekiq::Worker include DedicatedSidekiqQueue + attr_reader :targets # for metrics tags + + def initialize + @targets = {} + end + def perform(project_id, user_id, oldrev, newrev, ref) project = Project.find_by(id: project_id) return unless project @@ -9,6 +15,10 @@ class UpdateMergeRequestsWorker user = User.find_by(id: user_id) return unless user + @targets = { + project_id: project_id, + user_id: user_id + } MergeRequests::RefreshService.new(project, user).execute(oldrev, newrev, ref) end end diff --git a/lib/gitlab/metrics/sidekiq_middleware.rb b/lib/gitlab/metrics/sidekiq_middleware.rb index f9dd8e41912..2febd79e547 100644 --- a/lib/gitlab/metrics/sidekiq_middleware.rb +++ b/lib/gitlab/metrics/sidekiq_middleware.rb @@ -11,6 +11,8 @@ module Gitlab # Old gitlad-shell messages don't provide enqueued_at/created_at attributes trans.set(:sidekiq_queue_duration, Time.now.to_f - (message['enqueued_at'] || message['created_at'] || 0)) trans.run { yield } + + worker.targets.each { |name, target| trans.add_tag(name, target) } if worker.respond_to?(:targets) rescue Exception => error # rubocop: disable Lint/RescueException trans.add_event(:sidekiq_exception) diff --git a/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb b/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb index b576d7173f5..196e0fadda2 100644 --- a/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb +++ b/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb @@ -4,35 +4,30 @@ describe Gitlab::Metrics::SidekiqMiddleware do let(:middleware) { described_class.new } let(:message) { { 'args' => ['test'], 'enqueued_at' => Time.new(2016, 6, 23, 6, 59).to_f } } - describe '#call' do - it 'tracks the transaction' do - worker = double(:worker, class: double(:class, name: 'TestWorker')) + def run(worker, message) + expect(Gitlab::Metrics::Transaction).to receive(:new) + .with('TestWorker#perform') + .and_call_original + + expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:set) + .with(:sidekiq_queue_duration, instance_of(Float)) - expect(Gitlab::Metrics::Transaction).to receive(:new) - .with('TestWorker#perform') - .and_call_original + expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:finish) - expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:set) - .with(:sidekiq_queue_duration, instance_of(Float)) + middleware.call(worker, message, :test) { nil } + end - expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:finish) + describe '#call' do + it 'tracks the transaction' do + worker = double(:worker, class: double(:class, name: 'TestWorker')) - middleware.call(worker, message, :test) { nil } + run(worker, message) end it 'tracks the transaction (for messages without `enqueued_at`)' do worker = double(:worker, class: double(:class, name: 'TestWorker')) - expect(Gitlab::Metrics::Transaction).to receive(:new) - .with('TestWorker#perform') - .and_call_original - - expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:set) - .with(:sidekiq_queue_duration, instance_of(Float)) - - expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:finish) - - middleware.call(worker, {}, :test) { nil } + run(worker, {}) end it 'tracks any raised exceptions' do @@ -50,5 +45,18 @@ describe Gitlab::Metrics::SidekiqMiddleware do expect { middleware.call(worker, message, :test) } .to raise_error(RuntimeError) end + + it 'tags the metrics accordingly' do + targets = { one: 1, two: 2 } + worker = double(:worker, class: double(:class, name: 'TestWorker')) + allow(worker).to receive(:targets).and_return(targets) + + targets.each do |tag, value| + expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:add_tag) + .with(tag, value) + end + + run(worker, message) + end end end -- cgit v1.2.1 From 2b7e03cf699f9d266af585a1a9399c3e219fe063 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Tue, 31 Oct 2017 13:21:08 -0400 Subject: reword `targets` to `metric tags` add changelog --- app/workers/update_merge_requests_worker.rb | 7 ++++--- .../unreleased/35914-merge-request-update-worker-is-slow.yml | 5 +++++ lib/gitlab/metrics/sidekiq_middleware.rb | 2 +- spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb | 6 +++--- 4 files changed, 13 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/35914-merge-request-update-worker-is-slow.yml diff --git a/app/workers/update_merge_requests_worker.rb b/app/workers/update_merge_requests_worker.rb index f1e43bea8b3..e364fb698d3 100644 --- a/app/workers/update_merge_requests_worker.rb +++ b/app/workers/update_merge_requests_worker.rb @@ -2,10 +2,10 @@ class UpdateMergeRequestsWorker include Sidekiq::Worker include DedicatedSidekiqQueue - attr_reader :targets # for metrics tags + attr_reader :metrics_tags def initialize - @targets = {} + @metrics_tags = {} end def perform(project_id, user_id, oldrev, newrev, ref) @@ -15,10 +15,11 @@ class UpdateMergeRequestsWorker user = User.find_by(id: user_id) return unless user - @targets = { + @metrics_tags = { project_id: project_id, user_id: user_id } + MergeRequests::RefreshService.new(project, user).execute(oldrev, newrev, ref) end end diff --git a/changelogs/unreleased/35914-merge-request-update-worker-is-slow.yml b/changelogs/unreleased/35914-merge-request-update-worker-is-slow.yml new file mode 100644 index 00000000000..34bb76195af --- /dev/null +++ b/changelogs/unreleased/35914-merge-request-update-worker-is-slow.yml @@ -0,0 +1,5 @@ +--- +title: Add metric tagging for sidekiq workers +merge_request: 15111 +author: +type: added diff --git a/lib/gitlab/metrics/sidekiq_middleware.rb b/lib/gitlab/metrics/sidekiq_middleware.rb index 2febd79e547..b983a40611f 100644 --- a/lib/gitlab/metrics/sidekiq_middleware.rb +++ b/lib/gitlab/metrics/sidekiq_middleware.rb @@ -12,7 +12,7 @@ module Gitlab trans.set(:sidekiq_queue_duration, Time.now.to_f - (message['enqueued_at'] || message['created_at'] || 0)) trans.run { yield } - worker.targets.each { |name, target| trans.add_tag(name, target) } if worker.respond_to?(:targets) + worker.metrics_tags.each { |tag, value| trans.add_tag(tag, value) } if worker.respond_to?(:metrics_tags) rescue Exception => error # rubocop: disable Lint/RescueException trans.add_event(:sidekiq_exception) diff --git a/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb b/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb index 196e0fadda2..0803ce42fac 100644 --- a/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb +++ b/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb @@ -47,11 +47,11 @@ describe Gitlab::Metrics::SidekiqMiddleware do end it 'tags the metrics accordingly' do - targets = { one: 1, two: 2 } + tags = { one: 1, two: 2 } worker = double(:worker, class: double(:class, name: 'TestWorker')) - allow(worker).to receive(:targets).and_return(targets) + allow(worker).to receive(:metrics_tags).and_return(tags) - targets.each do |tag, value| + tags.each do |tag, value| expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:add_tag) .with(tag, value) end -- cgit v1.2.1 From d5859bb9d59b3750ac6e9b8c4c17d69c4c3ed077 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Wed, 1 Nov 2017 09:07:27 -0400 Subject: rework the UpdateMergeRequestsWorker#metrics_tags --- app/workers/update_merge_requests_worker.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/workers/update_merge_requests_worker.rb b/app/workers/update_merge_requests_worker.rb index e364fb698d3..150788ca611 100644 --- a/app/workers/update_merge_requests_worker.rb +++ b/app/workers/update_merge_requests_worker.rb @@ -2,10 +2,8 @@ class UpdateMergeRequestsWorker include Sidekiq::Worker include DedicatedSidekiqQueue - attr_reader :metrics_tags - - def initialize - @metrics_tags = {} + def metrics_tags + @metrics_tags || {} end def perform(project_id, user_id, oldrev, newrev, ref) -- cgit v1.2.1