FEATURE: Add success label to (sidekiq|scheduled)_job_count (#137)

Before this commit, `scheduled_job_count` represents the total number of
scheduled jobs executed regardless of its execution status while
`sidekiq_job_count` represents the total number of sidekiq job executed
successfuly only.

In order to allow the error rate for a particular job, we need to be
able to differentiate between successes and failures. This commit
seeks to do so by adding a `success` label to the `sidekiq_job_count`
and `scheduled_job_count` counters.

Author: tgxworld

lib/collector.rb

@@ -195,7 +195,8 @@ def ensure_global_metrics
 
     def process_job(metric)
       ensure_job_metrics
-      hash = { job_name: metric.job_name }
+      hash = { job_name: metric.job_name, success: metric.success }
+
       if metric.scheduled
         @scheduled_job_duration_seconds.observe(metric.duration, hash)
         @scheduled_job_count.observe(metric.count, hash)
@@ -209,10 +210,13 @@ def ensure_job_metrics
       unless @scheduled_job_count
         @scheduled_job_duration_seconds =
           Counter.new("scheduled_job_duration_seconds", "Total time spent in scheduled jobs")
+
         @scheduled_job_count =
-          Counter.new("scheduled_job_count", "Total number of scheduled jobs executued")
+          Counter.new("scheduled_job_count", "Total number of scheduled jobs executed")
+
         @sidekiq_job_duration_seconds =
           Counter.new("sidekiq_job_duration_seconds", "Total time spent in sidekiq jobs")
+
         @sidekiq_job_count =
           Counter.new("sidekiq_job_count", "Total number of sidekiq jobs executed")
       end

lib/internal_metric/job.rb

@@ -2,6 +2,6 @@
 
 module DiscoursePrometheus::InternalMetric
   class Job < Base
-    attribute :job_name, :scheduled, :duration, :count
+    attribute :job_name, :scheduled, :duration, :count, :success
   end
 end

plugin.rb

@@ -65,15 +65,27 @@ module ::DiscoursePrometheus
     metric.job_name = stat.name
     metric.duration = stat.duration_ms * 0.001
     metric.count = 1
+    metric.success = stats.success
     $prometheus_client.send_json metric.to_h unless Rails.env.test?
   end
 
-  on(:sidekiq_job_ran) do |worker, msg, queue, duration|
+  on(:sidekiq_job_ran) do |worker, _msg, _queue, duration|
     metric = DiscoursePrometheus::InternalMetric::Job.new
     metric.scheduled = false
     metric.duration = duration
     metric.count = 1
     metric.job_name = worker.class.to_s
+    metric.success = true
+    $prometheus_client.send_json metric.to_h unless Rails.env.test?
+  end
+
+  on(:sidekiq_job_error) do |worker, _msg, _queue, duration|
+    metric = DiscoursePrometheus::InternalMetric::Job.new
+    metric.scheduled = false
+    metric.duration = duration
+    metric.count = 1
+    metric.job_name = worker.class.to_s
+    metric.success = false
     $prometheus_client.send_json metric.to_h unless Rails.env.test?
   end
 end

spec/lib/collector_spec.rb

@@ -46,22 +46,78 @@
     expect(counter.data).to eq(nil => 3)
   end
 
-  it "handles scheduled job metrics" do
-    metric = DiscoursePrometheus::InternalMetric::Job.new
+  it "handles sidekiq job metrics" do
+    metric_1 = DiscoursePrometheus::InternalMetric::Job.new
+    metric_1.scheduled = false
+    metric_1.job_name = "Bob"
+    metric_1.duration = 1.778
+    metric_1.count = 1
+    metric_1.success = true
+
+    collector.process(metric_1.to_json)
+    metrics = collector.prometheus_metrics
 
-    metric.scheduled = true
-    metric.job_name = "Bob"
-    metric.duration = 1.778
-    metric.count = 1
+    metric_2 = DiscoursePrometheus::InternalMetric::Job.new
+    metric_2.scheduled = false
+    metric_2.job_name = "Bob"
+    metric_2.duration = 0.5
+    metric_2.count = 1
+    metric_2.success = false
+    collector.process(metric_2.to_json)
+
+    metric_3 = DiscoursePrometheus::InternalMetric::Job.new
+    metric_3.scheduled = false
+    metric_3.job_name = "Bob"
+    metric_3.duration = 1.5
+    metric_3.count = 1
+    metric_3.success = false
+    collector.process(metric_3.to_json)
+
+    duration = metrics.find { |m| m.name == "sidekiq_job_duration_seconds" }
+    sidekiq_job_count = metrics.find { |m| m.name == "sidekiq_job_count" }
+
+    expect(duration.data).to eq(
+      { job_name: "Bob", success: true } => metric_1.duration,
+      { job_name: "Bob", success: false } => metric_2.duration + metric_3.duration,
+    )
+
+    expect(sidekiq_job_count.data).to eq(
+      { job_name: "Bob", success: false } => 2,
+      { job_name: "Bob", success: true } => 1,
+    )
+  end
+
+  it "handles scheduled job metrics" do
+    metric_1 = DiscoursePrometheus::InternalMetric::Job.new
+    metric_1.scheduled = true
+    metric_1.job_name = "Bob"
+    metric_1.duration = 1.778
+    metric_1.success = true
+    metric_1.count = 1
+    collector.process(metric_1.to_json)
+
+    metric_2 = DiscoursePrometheus::InternalMetric::Job.new
+    metric_2.scheduled = true
+    metric_2.job_name = "Bob"
+    metric_2.duration = 1.123123
+    metric_2.success = false
+    metric_2.count = 1
+    collector.process(metric_2.to_json)
 
-    collector.process(metric.to_json)
     metrics = collector.prometheus_metrics
 
     duration = metrics.find { |m| m.name == "scheduled_job_duration_seconds" }
     count = metrics.find { |m| m.name == "scheduled_job_count" }
 
-    expect(duration.data).to eq({ job_name: "Bob" } => 1.778)
-    expect(count.data).to eq({ job_name: "Bob" } => 1)
+    expect(duration.data).to eq(
+      { job_name: "Bob", success: true } => metric_1.duration,
+      { job_name: "Bob", success: false } => metric_2.duration,
+    )
+
+    expect(count.data).to eq(
+      { job_name: "Bob", success: true } => 1,
+      { job_name: "Bob", success: false } => 1,
+    )
   end
 
   it "handles job initialization metrics" do
@@ -71,15 +127,16 @@
     metric.job_name = "Bob"
     metric.count = 0
     metric.duration = 0
+    metric.success = true
 
     collector.process(metric.to_json)
     metrics = collector.prometheus_metrics
 
     duration = metrics.find { |m| m.name == "scheduled_job_duration_seconds" }
     count = metrics.find { |m| m.name == "scheduled_job_count" }
 
-    expect(duration.data).to eq({ job_name: "Bob" } => 0)
-    expect(count.data).to eq({ job_name: "Bob" } => 0)
+    expect(duration.data).to eq({ job_name: "Bob", success: true } => 0)
+    expect(count.data).to eq({ job_name: "Bob", success: true } => 0)
   end
 
   it "handles process metrics" do

spec/lib/internal_metric/base_spec.rb

@@ -7,13 +7,15 @@
     job.scheduled = true
     job.duration = 100.1
     job.count = 1
+    job.success = false
 
     expect(job.to_h).to eq(
       job_name: "bob",
       scheduled: true,
       duration: 100.1,
       count: 1,
       _type: "Job",
+      success: false,
     )
   end