Commit e0d76a5d authored by Sean McGivern's avatar Sean McGivern

Fix ActionMailer job metric labels

When we emit metrics for Sidekiq workers, we were only adding some
labels for workers defined in our application code.
ActionMailer::MailDeliveryJob comes from Rails - we don't define that
class. But we still want to add these labels to the metrics. The urgency
label in particular is important, as it's used for calculating SLIs;
without that label, we current have no SLIs for the mailers queue.
parent 08b19204
...@@ -1781,9 +1781,9 @@ ...@@ -1781,9 +1781,9 @@
:idempotent: true :idempotent: true
:tags: [] :tags: []
- :name: mailers - :name: mailers
:feature_category: :feature_category: :issue_tracking
:has_external_dependencies: :has_external_dependencies:
:urgency: :urgency: low
:resource_boundary: :resource_boundary:
:weight: 2 :weight: 2
:idempotent: :idempotent:
......
...@@ -13,10 +13,17 @@ module Gitlab ...@@ -13,10 +13,17 @@ module Gitlab
(EE_QUEUE_CONFIG_PATH if Gitlab.ee?) (EE_QUEUE_CONFIG_PATH if Gitlab.ee?)
].compact.freeze ].compact.freeze
DEFAULT_WORKERS = [ # This maps workers not in our application code to queues. We need
DummyWorker.new('default', weight: 1, tags: []), # these queues in our YAML files to ensure we don't accidentally
DummyWorker.new('mailers', weight: 2, tags: []) # miss jobs from these queues.
].map { |worker| Gitlab::SidekiqConfig::Worker.new(worker, ee: false) }.freeze #
# The default queue should be unused, which is why it maps to an
# invalid class name. We keep it in the YAML file for safety, just
# in case anything does get scheduled to run there.
DEFAULT_WORKERS = {
'_' => DummyWorker.new('default', weight: 1, tags: []),
'ActionMailer::MailDeliveryJob' => DummyWorker.new('mailers', feature_category: :issue_tracking, urgency: 'low', weight: 2, tags: [])
}.transform_values { |worker| Gitlab::SidekiqConfig::Worker.new(worker, ee: false) }.freeze
class << self class << self
include Gitlab::SidekiqConfig::CliMethods include Gitlab::SidekiqConfig::CliMethods
...@@ -40,7 +47,7 @@ module Gitlab ...@@ -40,7 +47,7 @@ module Gitlab
def workers def workers
@workers ||= begin @workers ||= begin
result = [] result = []
result.concat(DEFAULT_WORKERS) result.concat(DEFAULT_WORKERS.values)
result.concat(find_workers(Rails.root.join('app', 'workers'), ee: false)) result.concat(find_workers(Rails.root.join('app', 'workers'), ee: false))
if Gitlab.ee? if Gitlab.ee?
......
...@@ -10,6 +10,7 @@ module Gitlab ...@@ -10,6 +10,7 @@ module Gitlab
def create_labels(worker_class, queue, job) def create_labels(worker_class, queue, job)
worker_name = (job['wrapped'].presence || worker_class).to_s worker_name = (job['wrapped'].presence || worker_class).to_s
worker = find_worker(worker_name, worker_class)
labels = { queue: queue.to_s, labels = { queue: queue.to_s,
worker: worker_name, worker: worker_name,
...@@ -18,15 +19,15 @@ module Gitlab ...@@ -18,15 +19,15 @@ module Gitlab
feature_category: "", feature_category: "",
boundary: "" } boundary: "" }
return labels unless worker_class && worker_class.include?(WorkerAttributes) return labels unless worker.respond_to?(:get_urgency)
labels[:urgency] = worker_class.get_urgency.to_s labels[:urgency] = worker.get_urgency.to_s
labels[:external_dependencies] = bool_as_label(worker_class.worker_has_external_dependencies?) labels[:external_dependencies] = bool_as_label(worker.worker_has_external_dependencies?)
feature_category = worker_class.get_feature_category feature_category = worker.get_feature_category
labels[:feature_category] = feature_category.to_s labels[:feature_category] = feature_category.to_s
resource_boundary = worker_class.get_worker_resource_boundary resource_boundary = worker.get_worker_resource_boundary
labels[:boundary] = resource_boundary == :unknown ? "" : resource_boundary.to_s labels[:boundary] = resource_boundary == :unknown ? "" : resource_boundary.to_s
labels labels
...@@ -35,6 +36,10 @@ module Gitlab ...@@ -35,6 +36,10 @@ module Gitlab
def bool_as_label(value) def bool_as_label(value)
value ? TRUE_LABEL : FALSE_LABEL value ? TRUE_LABEL : FALSE_LABEL
end end
def find_worker(worker_name, worker_class)
Gitlab::SidekiqConfig::DEFAULT_WORKERS.fetch(worker_name, worker_class)
end
end end
end end
end end
...@@ -229,6 +229,15 @@ RSpec.describe Gitlab::SidekiqMiddleware::ServerMetrics do ...@@ -229,6 +229,15 @@ RSpec.describe Gitlab::SidekiqMiddleware::ServerMetrics do
it_behaves_like "a metrics middleware" it_behaves_like "a metrics middleware"
end end
context 'for ActionMailer::MailDeliveryJob' do
let(:job) { { 'class' => ActionMailer::MailDeliveryJob } }
let(:worker) { ActionMailer::MailDeliveryJob.new }
let(:worker_class) { ActionMailer::MailDeliveryJob }
let(:labels) { default_labels.merge(feature_category: 'issue_tracking') }
it_behaves_like 'a metrics middleware'
end
context "when workers are attributed" do context "when workers are attributed" do
def create_attributed_worker_class(urgency, external_dependencies, resource_boundary, category) def create_attributed_worker_class(urgency, external_dependencies, resource_boundary, category)
Class.new do Class.new do
......
...@@ -4,7 +4,7 @@ require 'spec_helper' ...@@ -4,7 +4,7 @@ require 'spec_helper'
RSpec.describe 'Every Sidekiq worker' do RSpec.describe 'Every Sidekiq worker' do
let(:workers_without_defaults) do let(:workers_without_defaults) do
Gitlab::SidekiqConfig.workers - Gitlab::SidekiqConfig::DEFAULT_WORKERS Gitlab::SidekiqConfig.workers - Gitlab::SidekiqConfig::DEFAULT_WORKERS.values
end end
it 'does not use the default queue' do it 'does not use the default queue' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment