github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-01-19 04:42:55 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

DEV: Introduce a problem checks API (#25783)

Browse Source 
Previously, problem checks were all added as either class methods or blocks in AdminDashboardData. Another set of class methods were used to add and run problem checks.

As of this PR, problem checks are promoted to first-class citizens. Each problem check receives their own class. This class of course contains the implementation for running the check, but also configuration items like retry strategies (for scheduled checks.)

In addition, the parent class ProblemCheck also serves as a registry for checks. For example we can get a list of all existing check classes through ProblemCheck.checks, or just the ones running on a schedule through ProblemCheck.scheduled.

After this refactor, the task of adding a new check is significantly simplified. You add a class that inherits ProblemCheck, you implement it, add a test, and you're good to go.
This commit is contained in:
Ted Johansson 2024-02-23 11:20:32 +08:00 committed by GitHub
parent 1ad8e85b37
commit a72dc2f420
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 370 additions and 304 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
app/jobs/regular/problem_check.rb
View File

@ -13,17 +13,23 @@ module Jobs
retry_count = args[:retry_count].to_i
identifier = args[:check_identifier].to_sym
check = AdminDashboardData.problem_scheduled_check_klasses[identifier]
check = ::ProblemCheck[identifier]
AdminDashboardData.execute_scheduled_check(identifier) do |problems|
raise RetrySignal if retry_count < check.max_retries
end
problems = check.call
raise RetrySignal if problems.present? && retry_count < check.max_retries
problems.each { |problem| AdminDashboardData.add_found_scheduled_check_problem(problem) }
rescue RetrySignal
Jobs.enqueue_in(
check.retry_wait,
check.retry_after,
:problem_check,
args.merge(retry_count: retry_count + 1).stringify_keys,
)
rescue StandardError => err
Discourse.warn_exception(
err,
message: "A scheduled admin dashboard problem check (#{identifier}) errored.",
)
end
end
end

9
app/jobs/scheduled/problem_checks.rb
View File

@ -2,8 +2,8 @@
module Jobs
# This job runs all of the scheduled problem checks for the admin dashboard
# on a regular basis. To add a problem check for this scheduled job run
# call AdminDashboardData.add_scheduled_problem_check
# on a regular basis. To add a problem check, add a new class that inherits
# the `ProblemCheck` base class.
class ProblemChecks < ::Jobs::Scheduled
sidekiq_options retry: false
@ -13,7 +13,10 @@ module Jobs
# This way if the problems have been solved in the meantime, then they will
# not be re-added by the relevant checker, and will be cleared.
AdminDashboardData.clear_found_scheduled_check_problems
AdminDashboardData.execute_scheduled_checks
::ProblemCheck.scheduled.each do |check|
Jobs.enqueue(:problem_check, check_identifier: check.identifier.to_s)
end
end
end
end

66
app/models/admin_dashboard_data.rb
View File

@ -3,11 +3,7 @@
class AdminDashboardData
include StatsCacheable
cattr_reader :problem_syms,
:problem_blocks,
:problem_messages,
:problem_scheduled_check_blocks,
:problem_scheduled_check_klasses
cattr_reader :problem_syms, :problem_blocks, :problem_messages
class Problem
VALID_PRIORITIES = %w[low high].freeze
@ -84,11 +80,6 @@ class AdminDashboardData
@@problem_blocks << blk if blk
end
def self.add_scheduled_problem_check(check_identifier, klass = nil, &blk)
@@problem_scheduled_check_klasses[check_identifier] = klass
@@problem_scheduled_check_blocks[check_identifier] = blk
end
def self.add_found_scheduled_check_problem(problem)
problems = load_found_scheduled_check_problems
if problem.identifier.present?
@ -129,57 +120,6 @@ class AdminDashboardData
end
end
def self.register_default_scheduled_problem_checks
add_scheduled_problem_check(:group_smtp_credentials, GroupEmailCredentialsCheck) do
problems = GroupEmailCredentialsCheck.run
problems.map do |p|
problem_message =
I18n.t(
"dashboard.group_email_credentials_warning",
{
base_path: Discourse.base_path,
group_name: p[:group_name],
group_full_name: p[:group_full_name],
error: p[:message],
},
)
Problem.new(
problem_message,
priority: "high",
identifier: "group_#{p[:group_id]}_email_credentials",
)
end
end
end
def self.execute_scheduled_checks
problem_scheduled_check_blocks.keys.each do |check_identifier|
Jobs.enqueue(:problem_check, check_identifier: check_identifier.to_s)
end
end
def self.execute_scheduled_check(identifier)
check = problem_scheduled_check_blocks[identifier]
problems = instance_exec(&check)
yield(problems) if block_given? && problems.present?
Array
.wrap(problems)
.compact
.each do |problem|
next if !problem.is_a?(Problem)
add_found_scheduled_check_problem(problem)
end
rescue StandardError => err
Discourse.warn_exception(
err,
message: "A scheduled admin dashboard problem check (#{identifier}) errored.",
)
end
##
# We call this method in the class definition below
# so all of the problem checks in this class are registered on
@ -192,8 +132,6 @@ class AdminDashboardData
def self.reset_problem_checks
@@problem_syms = []
@@problem_blocks = []
@@problem_scheduled_check_blocks = {}
@@problem_scheduled_check_klasses = {}
@@problem_messages = %w[
dashboard.bad_favicon_url
@ -222,8 +160,6 @@ class AdminDashboardData
:translation_overrides_check,
:ember_version_check
register_default_scheduled_problem_checks
add_problem_check { sidekiq_check || queue_size_check }
end
reset_problem_checks

65
lib/group_email_credentials_check.rb
View File

@ -1,65 +0,0 @@
# frozen_string_literal: true
##
# If group SMTP or IMAP has been configured, we want to make sure the
# credentials are always valid otherwise emails will not be sending out
# from group inboxes. This check is run as part of scheduled AdminDashboardData
# problem checks, and if any credentials have issues they will show up on
# the admin dashboard as a high priority issue.
class GroupEmailCredentialsCheck
def self.max_retries = 2
def self.retry_wait = 30.seconds
def self.run
errors = []
if SiteSetting.enable_smtp
Group.with_smtp_configured.find_each do |group|
errors << try_validate(group) do
EmailSettingsValidator.validate_smtp(
host: group.smtp_server,
port: group.smtp_port,
username: group.email_username,
password: group.email_password,
)
end
end
end
if SiteSetting.enable_imap
Group.with_imap_configured.find_each do |group|
errors << try_validate(group) do
EmailSettingsValidator.validate_imap(
host: group.smtp_server,
port: group.smtp_port,
username: group.email_username,
password: group.email_password,
)
end
end
end
errors.compact
end
def self.try_validate(group, &blk)
begin
blk.call
nil
rescue *EmailSettingsExceptionHandler::EXPECTED_EXCEPTIONS => err
{
group_id: group.id,
group_name: group.name,
group_full_name: group.full_name,
message: EmailSettingsExceptionHandler.friendly_exception_message(err, group.smtp_server),
}
rescue => err
Discourse.warn_exception(
err,
message:
"Unexpected error when checking SMTP credentials for group #{group.id} (#{group.name}).",
)
nil
end
end
end

53
lib/problem_check.rb Normal file
View File

@ -0,0 +1,53 @@
# frozen_string_literal: true
class ProblemCheck
include ActiveSupport::Configurable
# Determines if the check should be performed at a regular interval, and if
# so how often. If left blank, the check will be performed every time the
# admin dashboard is loaded, or the data is otherwise requested.
#
config_accessor :perform_every, default: nil, instance_writer: false
# How many times the check should retry before registering a problem. Only
# works for scheduled checks.
#
config_accessor :max_retries, default: 2, instance_writer: false
# The retry delay after a failed check. Only works for scheduled checks with
# more than one retry configured.
#
config_accessor :retry_after, default: 30.seconds, instance_writer: false
def self.[](key)
key = key.to_sym
checks.find { |c| c.identifier == key }
end
def self.checks
descendants
end
def self.scheduled
checks.select(&:scheduled?)
end
def self.identifier
name.demodulize.underscore.to_sym
end
delegate :identifier, to: :class
def self.scheduled?
perform_every.present?
end
delegate :scheduled?, to: :class
def self.call
new.call
end
def call
raise NotImplementedError
end
end

74
lib/problem_check/group_email_credentials.rb Normal file
View File

@ -0,0 +1,74 @@
# frozen_string_literal: true
##
# If group SMTP or IMAP has been configured, we want to make sure the
# credentials are always valid otherwise emails will not be sending out
# from group inboxes. This check is run as part of scheduled admin
# problem checks, and if any credentials have issues they will show up on
# the admin dashboard as a high priority issue.
class ProblemCheck::GroupEmailCredentials < ProblemCheck
self.perform_every = 30.minutes
def call
[*smtp_errors, *imap_errors]
end
private
def smtp_errors
return [] if !SiteSetting.enable_smtp
Group.with_smtp_configured.find_each.filter_map do |group|
try_validate(group) do
EmailSettingsValidator.validate_smtp(
host: group.smtp_server,
port: group.smtp_port,
username: group.email_username,
password: group.email_password,
)
end
end
end
def imap_errors
return [] if !SiteSetting.enable_imap
Group.with_imap_configured.find_each.filter_map do |group|
try_validate(group) do
EmailSettingsValidator.validate_imap(
host: group.imap_server,
port: group.imap_port,
username: group.email_username,
password: group.email_password,
)
end
end
end
def try_validate(group, &blk)
begin
blk.call
nil
rescue *EmailSettingsExceptionHandler::EXPECTED_EXCEPTIONS => err
message =
I18n.t(
"dashboard.group_email_credentials_warning",
{
base_path: Discourse.base_path,
group_name: group.name,
group_full_name: group.full_name,
error: EmailSettingsExceptionHandler.friendly_exception_message(err, group.smtp_server),
},
)
Problem.new(message, priority: "high", identifier: "group_#{group.id}_email_credentials")
rescue => err
Discourse.warn_exception(
err,
message:
"Unexpected error when checking SMTP credentials for group #{group.id} (#{group.name}).",
)
nil
end
end
end

30
lib/problem_check/problem.rb Normal file
View File

@ -0,0 +1,30 @@
# frozen_string_literal: true
class ProblemCheck::Problem
PRIORITIES = %w[low high].freeze
attr_reader :message, :priority, :identifier
def initialize(message, priority: "low", identifier: nil)
@message = message
@priority = PRIORITIES.include?(priority) ? priority : "low"
@identifier = identifier
end
def to_s
@message
end
def to_h
{ message: message, priority: priority, identifier: identifier }
end
alias_method :attributes, :to_h
def self.from_h(h)
h = h.with_indifferent_access
return if h[:message].blank?
new(h[:message], priority: h[:priority], identifier: h[:identifier])
end
end

192
spec/jobs/problem_check_spec.rb
View File

@ -3,98 +3,128 @@
RSpec.describe Jobs::ProblemCheck do
after do
Discourse.redis.flushdb
AdminDashboardData.reset_problem_checks
::ProblemCheck.send(:remove_const, "TestCheck")
end
class TestCheck
def self.max_retries = 0
def self.retry_wait = 30.seconds
end
context "when there are problems" do
before do
::ProblemCheck::TestCheck =
Class.new(::ProblemCheck) do
self.max_retries = 0
it "runs the scheduled problem check that has been added and adds the messages to the load_found_scheduled_check_problems array" do
AdminDashboardData.add_scheduled_problem_check(:test_identifier, TestCheck) do
AdminDashboardData::Problem.new("big problem")
def call
[
::ProblemCheck::Problem.new("Big problem"),
::ProblemCheck::Problem.new(
"Yuge problem",
priority: "high",
identifier: "config_is_a_mess",
),
]
end
end
end
described_class.new.execute(check_identifier: :test_identifier)
problems = AdminDashboardData.load_found_scheduled_check_problems
expect(problems.count).to eq(1)
expect(problems.first).to be_a(AdminDashboardData::Problem)
expect(problems.first.to_s).to eq("big problem")
end
it "adds the messages to the Redis problems array" do
described_class.new.execute(check_identifier: :test_check)
it "can handle the problem check returning multiple problems" do
AdminDashboardData.add_scheduled_problem_check(:test_identifier, TestCheck) do
[
AdminDashboardData::Problem.new("big problem"),
AdminDashboardData::Problem.new(
"yuge problem",
priority: "high",
identifier: "config_is_a_mess",
),
]
end
problems = AdminDashboardData.load_found_scheduled_check_problems
described_class.new.execute(check_identifier: :test_identifier)
problems = AdminDashboardData.load_found_scheduled_check_problems
expect(problems.map(&:to_s)).to match_array(["big problem", "yuge problem"])
end
it "does not add the same problem twice if the identifier already exists" do
AdminDashboardData.add_scheduled_problem_check(:test_identifier, TestCheck) do
[
AdminDashboardData::Problem.new(
"yuge problem",
priority: "high",
identifier: "config_is_a_mess",
),
AdminDashboardData::Problem.new(
"nasty problem",
priority: "high",
identifier: "config_is_a_mess",
),
]
end
described_class.new.execute(check_identifier: :test_identifier)
problems = AdminDashboardData.load_found_scheduled_check_problems
expect(problems.map(&:to_s)).to match_array(["yuge problem"])
end
it "schedules a retry if there are attempts remaining" do
AdminDashboardData.add_scheduled_problem_check(:test_identifier, TestCheck) do
AdminDashboardData::Problem.new("big problem")
end
TestCheck.stubs(:max_retries).returns(1)
expect_enqueued_with(
job: :problem_check,
args: {
check_identifier: :test_identifier,
retry_count: 1,
},
) { described_class.new.execute(check_identifier: :test_identifier) }
end
it "does not schedule a retry if there are no more attempts remaining" do
AdminDashboardData.add_scheduled_problem_check(:test_identifier, TestCheck) do
AdminDashboardData::Problem.new("big problem")
end
TestCheck.stubs(:max_retries).returns(1)
expect_not_enqueued_with(job: :problem_check) do
described_class.new.execute(check_identifier: :test_identifier, retry_count: 1)
expect(problems.map(&:to_s)).to contain_exactly("Big problem", "Yuge problem")
end
end
it "handles errors from a troublesome check" do
AdminDashboardData.add_scheduled_problem_check(:test_identifier, TestCheck) do
raise StandardError.new("something went wrong")
context "with multiple problems with the same identifier" do
before do
::ProblemCheck::TestCheck =
Class.new(::ProblemCheck) do
self.max_retries = 0
def call
[
::ProblemCheck::Problem.new(
"Yuge problem",
priority: "high",
identifier: "config_is_a_mess",
),
::ProblemCheck::Problem.new(
"Nasty problem",
priority: "high",
identifier: "config_is_a_mess",
),
]
end
end
end
described_class.new.execute(check_identifier: :test_identifier)
expect(AdminDashboardData.load_found_scheduled_check_problems.count).to eq(0)
it "does not add the same problem twice" do
described_class.new.execute(check_identifier: :test_check)
problems = AdminDashboardData.load_found_scheduled_check_problems
expect(problems.map(&:to_s)).to match_array(["Yuge problem"])
end
end
context "when there are retries remaining" do
before do
::ProblemCheck::TestCheck =
Class.new(::ProblemCheck) do
self.max_retries = 2
def call
[::ProblemCheck::Problem.new("Yuge problem")]
end
end
end
it "schedules a retry" do
expect_enqueued_with(
job: :problem_check,
args: {
check_identifier: :test_check,
retry_count: 1,
},
) { described_class.new.execute(check_identifier: :test_check) }
end
end
context "when there are no retries remaining" do
before do
::ProblemCheck::TestCheck =
Class.new(::ProblemCheck) do
self.max_retries = 1
def call
[::ProblemCheck::Problem.new("Yuge problem")]
end
end
end
it "does not schedule a retry" do
expect_not_enqueued_with(job: :problem_check) do
described_class.new.execute(check_identifier: :test_identifier, retry_count: 1)
end
end
end
context "when the check unexpectedly errors out" do
before do
::ProblemCheck::TestCheck =
Class.new(::ProblemCheck) do
self.max_retries = 1
def call
raise StandardError.new("Something went wrong")
end
end
end
it "does not add a problem to the Redis array" do
described_class.new.execute(check_identifier: :test_check)
expect(AdminDashboardData.load_found_scheduled_check_problems).to be_empty
end
end
end

42
spec/jobs/problem_checks_spec.rb
View File

@ -1,31 +1,37 @@
# frozen_string_literal: true
RSpec.describe Jobs::ProblemChecks do
before { Jobs.run_immediately! }
before do
::ProblemCheck::ScheduledCheck =
Class.new(ProblemCheck) do
self.perform_every = 30.minutes
def call = []
end
::ProblemCheck::NonScheduledCheck = Class.new(ProblemCheck) { def call = [] }
end
after do
Discourse.redis.flushdb
AdminDashboardData.reset_problem_checks
ProblemCheck.send(:remove_const, "ScheduledCheck")
ProblemCheck.send(:remove_const, "NonScheduledCheck")
end
class TestCheck
def self.max_retries = 0
def self.retry_wait = 0.seconds
end
it "starts with a blank slate every time the checks are run to avoid duplicate problems and to clear no longer firing problems" do
problem_should_fire = true
AdminDashboardData.reset_problem_checks
AdminDashboardData.add_scheduled_problem_check(:test_identifier, TestCheck) do
if problem_should_fire
problem_should_fire = false
AdminDashboardData::Problem.new("yuge problem", priority: "high")
end
it "schedules the individual scheduled checks" do
expect_enqueued_with(job: :problem_check, args: { check_identifier: "scheduled_check" }) do
described_class.new.execute([])
end
end
described_class.new.execute(nil)
expect(AdminDashboardData.load_found_scheduled_check_problems.count).to eq(1)
described_class.new.execute(nil)
expect(AdminDashboardData.load_found_scheduled_check_problems.count).to eq(0)
it "does not schedule non-scheduled checks" do
expect_not_enqueued_with(
job: :problem_check,
args: {
check_identifier: "non_scheduled_check",
},
) { described_class.new.execute([]) }
end
end

44
spec/lib/group_email_credentials_check_spec.rb → spec/lib/problem_check/group_email_credentials_spec.rb
View File

@ -3,16 +3,16 @@
require "net/smtp"
require "net/imap"
RSpec.describe GroupEmailCredentialsCheck do
RSpec.describe ProblemCheck::GroupEmailCredentials do
fab!(:group1) { Fabricate(:group) }
fab!(:group2) { Fabricate(:smtp_group) }
fab!(:group3) { Fabricate(:imap_group) }
describe "#run" do
describe "#call" do
it "does nothing if SMTP is disabled for the site" do
expect_no_validate_any
SiteSetting.enable_smtp = false
expect(described_class.run).to eq([])
expect(described_class.new.call).to eq([])
end
context "with smtp and imap enabled for the site" do
@ -25,10 +25,10 @@ RSpec.describe GroupEmailCredentialsCheck do
expect_no_validate_any
group2.update!(smtp_enabled: false)
group3.update!(smtp_enabled: false, imap_enabled: false)
expect(described_class.run).to eq([])
expect(described_class.new.call).to eq([])
end
it "returns an error message and the group ID if the group's SMTP settings error" do
it "returns a problem with the group's SMTP settings error" do
EmailSettingsValidator
.expects(:validate_smtp)
.raises(Net::SMTPAuthenticationError.new("bad credentials"))
@ -37,15 +37,13 @@ RSpec.describe GroupEmailCredentialsCheck do
.at_least_once
EmailSettingsValidator.stubs(:validate_imap).returns(true)
expect(described_class.run).to eq(
[
{
group_full_name: group2.full_name,
group_name: group2.name,
group_id: group2.id,
message: I18n.t("email_settings.smtp_authentication_error"),
},
],
expect(described_class.new.call).to contain_exactly(
have_attributes(
identifier: "group_#{group2.id}_email_credentials",
priority: "high",
message:
"There was an issue with the email credentials for the group <a href=\"/g/#{group2.name}/manage/email\"></a>. No emails will be sent from the group inbox until this problem is addressed. There was an issue with the SMTP credentials provided, check the username and password and try again.",
),
)
end
@ -56,15 +54,13 @@ RSpec.describe GroupEmailCredentialsCheck do
.raises(Net::IMAP::NoResponseError.new(stub(data: stub(text: "Invalid credentials"))))
.once
expect(described_class.run).to eq(
[
{
group_full_name: group3.full_name,
group_name: group3.name,
group_id: group3.id,
message: I18n.t("email_settings.imap_authentication_error"),
},
],
expect(described_class.new.call).to contain_exactly(
have_attributes(
identifier: "group_#{group3.id}_email_credentials",
priority: "high",
message:
"There was an issue with the email credentials for the group <a href=\"/g/#{group3.name}/manage/email\"></a>. No emails will be sent from the group inbox until this problem is addressed. There was an issue with the IMAP credentials provided, check the username and password and try again.",
),
)
end
@ -73,7 +69,7 @@ RSpec.describe GroupEmailCredentialsCheck do
EmailSettingsValidator.stubs(:validate_smtp).returns(true)
EmailSettingsValidator.expects(:validate_imap).never
expect(described_class.run).to eq([])
expect(described_class.new.call).to eq([])
end
end
end

40
spec/lib/problem_check_spec.rb Normal file
View File

@ -0,0 +1,40 @@
# frozen_string_literal: true
RSpec.describe ProblemCheck do
# rubocop:disable RSpec/BeforeAfterAll
before(:all) do
ScheduledCheck = Class.new(described_class) { self.perform_every = 30.minutes }
UnscheduledCheck = Class.new(described_class)
end
after(:all) do
Object.send(:remove_const, ScheduledCheck.name)
Object.send(:remove_const, UnscheduledCheck.name)
end
# rubocop:enable RSpec/BeforeAfterAll
let(:scheduled_check) { ScheduledCheck }
let(:unscheduled_check) { UnscheduledCheck }
describe ".[]" do
it { expect(described_class[:scheduled_check]).to eq(scheduled_check) }
it { expect(described_class[:foo]).to eq(nil) }
end
describe ".identifier" do
it { expect(scheduled_check.identifier).to eq(:scheduled_check) }
end
describe ".checks" do
it { expect(described_class.checks).to contain_exactly(scheduled_check, unscheduled_check) }
end
describe ".scheduled" do
it { expect(described_class.scheduled).to contain_exactly(scheduled_check) }
end
describe ".scheduled?" do
it { expect(scheduled_check).to be_scheduled }
it { expect(unscheduled_check).to_not be_scheduled }
end
end

43
spec/models/admin_dashboard_data_spec.rb
View File

@ -82,49 +82,6 @@ RSpec.describe AdminDashboardData do
include_examples "stats cacheable"
end
describe ".execute_scheduled_checks" do
let(:blk) { -> {} }
before { AdminDashboardData.add_scheduled_problem_check(:foo, &blk) }
after { AdminDashboardData.reset_problem_checks }
it do
expect_enqueued_with(job: :problem_check, args: { check_identifier: :foo }) do
AdminDashboardData.execute_scheduled_checks
end
end
end
describe ".execute_scheduled_check" do
context "when problems are found" do
let(:blk) { -> { self::Problem.new("Problem") } }
before do
AdminDashboardData.add_scheduled_problem_check(:foo, &blk)
AdminDashboardData.expects(:add_found_scheduled_check_problem).once
end
after { AdminDashboardData.reset_problem_checks }
it do
expect(described_class.execute_scheduled_check(:foo)).to all(be_a(described_class::Problem))
end
end
context "when check errors out" do
let(:blk) { -> { raise StandardError } }
before do
AdminDashboardData.add_scheduled_problem_check(:foo, &blk)
Discourse.expects(:warn_exception).once
end
after { AdminDashboardData.reset_problem_checks }
it { expect(described_class.execute_scheduled_check(:foo)).to eq(nil) }
end
end
describe "#problem_message_check" do
let(:key) { AdminDashboardData.problem_messages.first }