From 4ca41e0af2374d0766af3a8f6dafbf9ceaf532d2 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Wed, 20 Mar 2024 08:52:25 +0800 Subject: [PATCH] DEV: Promote block problem checks to ProblemCheck (#26193) In #26122 we promoted all problem checks defined as class methods on AdminDashboardData to their own first-class ProblemCheck instances. This PR continues that by promoting problem checks that are implemented as blocks as well. This includes updating a couple plugins that have problem checks. --- app/models/admin_dashboard_data.rb | 27 ++----- app/models/problem_check.rb | 4 +- app/services/problem_check/sidekiq_check.rb | 26 +++++++ spec/models/admin_dashboard_data_spec.rb | 43 ----------- .../problem_check/sidekiq_check_spec.rb | 73 +++++++++++++++++++ 5 files changed, 106 insertions(+), 67 deletions(-) create mode 100644 app/services/problem_check/sidekiq_check.rb create mode 100644 spec/services/problem_check/sidekiq_check_spec.rb diff --git a/app/models/admin_dashboard_data.rb b/app/models/admin_dashboard_data.rb index 6842391deb5..aa9e26f33ea 100644 --- a/app/models/admin_dashboard_data.rb +++ b/app/models/admin_dashboard_data.rb @@ -3,7 +3,7 @@ class AdminDashboardData include StatsCacheable - cattr_reader :problem_blocks, :problem_messages + cattr_reader :problem_messages # kept for backward compatibility GLOBAL_REPORTS ||= [] @@ -25,10 +25,6 @@ class AdminDashboardData def problems problems = [] - self.class.problem_blocks.each do |blk| - message = instance_exec(&blk) - problems << ProblemCheck::Problem.new(message) if message.present? - end self.class.problem_messages.each do |i18n_key| message = self.class.problem_message_check(i18n_key) problems << ProblemCheck::Problem.new(message) if message.present? @@ -48,7 +44,10 @@ class AdminDashboardData end def self.add_problem_check(*syms, &blk) - @@problem_blocks << blk if blk + Discourse.deprecate( + "`AdminDashboardData#add_problem_check` is deprecated. Implement a class that inherits `ProblemCheck` instead.", + drop_from: "3.3", + ) end def self.add_found_scheduled_check_problem(problem) @@ -101,15 +100,11 @@ class AdminDashboardData # tests. It will also fire multiple times in development mode because # classes are not cached. def self.reset_problem_checks - @@problem_blocks = [] - @@problem_messages = %w[ dashboard.bad_favicon_url dashboard.poll_pop3_timeout dashboard.poll_pop3_auth_error ] - - add_problem_check { sidekiq_check || queue_size_check } end reset_problem_checks @@ -175,16 +170,4 @@ class AdminDashboardData def self.problem_message_key(i18n_key) "#{PROBLEM_MESSAGE_PREFIX}#{i18n_key}" end - - def sidekiq_check - last_job_performed_at = Jobs.last_job_performed_at - if Jobs.queued > 0 && (last_job_performed_at.nil? || last_job_performed_at < 2.minutes.ago) - I18n.t("dashboard.sidekiq_warning") - end - end - - def queue_size_check - queue_size = Jobs.queued - I18n.t("dashboard.queue_size_warning", queue_size: queue_size) if queue_size >= 100_000 - end end diff --git a/app/models/problem_check.rb b/app/models/problem_check.rb index 8bc0ea67ff9..82e8cd124bb 100644 --- a/app/models/problem_check.rb +++ b/app/models/problem_check.rb @@ -70,14 +70,14 @@ class ProblemCheck private - def problem(override_key = nil) + def problem(override_key = nil, override_data = {}) [ Problem.new( message || I18n.t( override_key || translation_key, base_path: Discourse.base_path, - **translation_data.symbolize_keys, + **override_data.merge(translation_data).symbolize_keys, ), priority: self.config.priority, identifier:, diff --git a/app/services/problem_check/sidekiq_check.rb b/app/services/problem_check/sidekiq_check.rb new file mode 100644 index 00000000000..5dd9337df78 --- /dev/null +++ b/app/services/problem_check/sidekiq_check.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class ProblemCheck::SidekiqCheck < ProblemCheck + self.priority = "low" + + def call + return problem("dashboard.sidekiq_warning") if jobs_in_queue? && !jobs_performed_recently? + return problem("dashboard.queue_size_warning", queue_size: Jobs.queued) if massive_queue? + + no_problem + end + + private + + def massive_queue? + Jobs.queued >= 100_000 + end + + def jobs_in_queue? + Jobs.queued > 0 + end + + def jobs_performed_recently? + Jobs.last_job_performed_at.present? && Jobs.last_job_performed_at > 2.minutes.ago + end +end diff --git a/spec/models/admin_dashboard_data_spec.rb b/spec/models/admin_dashboard_data_spec.rb index 22f8fb96bd1..78119cb60ab 100644 --- a/spec/models/admin_dashboard_data_spec.rb +++ b/spec/models/admin_dashboard_data_spec.rb @@ -22,15 +22,6 @@ RSpec.describe AdminDashboardData do expect(problems).not_to include(I18n.t("errors.messages.invalid")) end end - - describe "adding new checks" do - it "calls the passed block" do - AdminDashboardData.add_problem_check { "a problem was found" } - - problems = AdminDashboardData.fetch_problems - expect(problems.map(&:to_s)).to include("a problem was found") - end - end end describe "adding scheduled checks" do @@ -91,38 +82,4 @@ RSpec.describe AdminDashboardData do ) end end - - describe "sidekiq_check" do - subject(:check) { described_class.new.sidekiq_check } - - it "returns nil when sidekiq processed a job recently" do - Jobs.stubs(:last_job_performed_at).returns(1.minute.ago) - Jobs.stubs(:queued).returns(0) - expect(check).to be_nil - end - - it "returns nil when last job processed was a long time ago, but no jobs are queued" do - Jobs.stubs(:last_job_performed_at).returns(7.days.ago) - Jobs.stubs(:queued).returns(0) - expect(check).to be_nil - end - - it "returns nil when no jobs have ever been processed, but no jobs are queued" do - Jobs.stubs(:last_job_performed_at).returns(nil) - Jobs.stubs(:queued).returns(0) - expect(check).to be_nil - end - - it "returns a string when no jobs were processed recently and some jobs are queued" do - Jobs.stubs(:last_job_performed_at).returns(20.minutes.ago) - Jobs.stubs(:queued).returns(1) - expect(check).to_not be_nil - end - - it "returns a string when no jobs have ever been processed, and some jobs are queued" do - Jobs.stubs(:last_job_performed_at).returns(nil) - Jobs.stubs(:queued).returns(1) - expect(check).to_not be_nil - end - end end diff --git a/spec/services/problem_check/sidekiq_check_spec.rb b/spec/services/problem_check/sidekiq_check_spec.rb new file mode 100644 index 00000000000..cc9930da72d --- /dev/null +++ b/spec/services/problem_check/sidekiq_check_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +RSpec.describe ProblemCheck::SidekiqCheck do + subject(:check) { described_class.new } + + describe ".call" do + context "when Sidekiq processed a job recently" do + before do + Jobs.stubs(:last_job_performed_at).returns(1.minute.ago) + Jobs.stubs(:queued).returns(1) + end + + it { expect(check).to be_chill_about_it } + end + + context "when last job processed was a long time ago, but no jobs are queued" do + before do + Jobs.stubs(:last_job_performed_at).returns(7.days.ago) + Jobs.stubs(:queued).returns(0) + end + + it { expect(check).to be_chill_about_it } + end + + context "when no jobs have ever been processed, but no jobs are queued" do + before do + Jobs.stubs(:last_job_performed_at).returns(nil) + Jobs.stubs(:queued).returns(0) + end + + it { expect(check).to be_chill_about_it } + end + + context "when no jobs were processed recently and some jobs are queued" do + before do + Jobs.stubs(:last_job_performed_at).returns(20.minutes.ago) + Jobs.stubs(:queued).returns(1) + end + + it do + expect(check).to have_a_problem.with_priority("low").with_message( + 'Sidekiq is not running. Many tasks, like sending emails, are executed asynchronously by Sidekiq. Please ensure at least one Sidekiq process is running. Learn about Sidekiq here.', + ) + end + end + + context "when no jobs have ever been processed, and some jobs are queued" do + before do + Jobs.stubs(:last_job_performed_at).returns(nil) + Jobs.stubs(:queued).returns(1) + end + + it do + expect(check).to have_a_problem.with_priority("low").with_message( + 'Sidekiq is not running. Many tasks, like sending emails, are executed asynchronously by Sidekiq. Please ensure at least one Sidekiq process is running. Learn about Sidekiq here.', + ) + end + end + + context "when there's a massive pile-up in the queue" do + before do + Jobs.stubs(:last_job_performed_at).returns(1.second.ago) + Jobs.stubs(:queued).returns(100_000) + end + + it do + expect(check).to have_a_problem.with_priority("low").with_message( + "The number of queued jobs is 100000, which is high. This could indicate a problem with the Sidekiq process(es), or you may need to add more Sidekiq workers.", + ) + end + end + end +end