From 1bcb521fbf24fafc92eea7f523c1889679627814 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Tue, 27 Feb 2024 11:17:39 +0800 Subject: [PATCH] DEV: Add DB backed problem checks to support perform_every config (#25834) As part of problem checks refactoring, we're moving some data to be DB backed. In this PR it's the tracking of problem check execution. When was it last run, when was the last problem, when should it run next, how many consecutive checks had problems, etc. This allows us to implement the perform_every feature in scheduled problem checks for checks that don't need to be run every 10 minutes. --- app/jobs/regular/problem_check.rb | 7 +- app/jobs/scheduled/problem_checks.rb | 7 +- app/models/problem_check_tracker.rb | 47 +++++++++ ...223052820_create_problem_check_trackers.rb | 16 +++ .../problem_check_tracker_fabricator.rb | 3 + spec/jobs/problem_check_spec.rb | 18 +++- spec/jobs/problem_checks_spec.rb | 49 +++++++--- spec/models/problem_check_tracker_spec.rb | 98 +++++++++++++++++++ 8 files changed, 230 insertions(+), 15 deletions(-) create mode 100644 app/models/problem_check_tracker.rb create mode 100644 db/migrate/20240223052820_create_problem_check_trackers.rb create mode 100644 spec/fabricators/problem_check_tracker_fabricator.rb create mode 100644 spec/models/problem_check_tracker_spec.rb diff --git a/app/jobs/regular/problem_check.rb b/app/jobs/regular/problem_check.rb index f20e0cc5ad0..b4596a874c1 100644 --- a/app/jobs/regular/problem_check.rb +++ b/app/jobs/regular/problem_check.rb @@ -18,7 +18,12 @@ module Jobs problems = check.call raise RetrySignal if problems.present? && retry_count < check.max_retries - problems.each { |problem| AdminDashboardData.add_found_scheduled_check_problem(problem) } + if problems.present? + problems.each { |problem| AdminDashboardData.add_found_scheduled_check_problem(problem) } + ProblemCheckTracker[identifier].problem!(next_run_at: check.perform_every.from_now) + else + ProblemCheckTracker[identifier].no_problem!(next_run_at: check.perform_every.from_now) + end rescue RetrySignal Jobs.enqueue_in( check.retry_after, diff --git a/app/jobs/scheduled/problem_checks.rb b/app/jobs/scheduled/problem_checks.rb index 8950138faf5..89161925e49 100644 --- a/app/jobs/scheduled/problem_checks.rb +++ b/app/jobs/scheduled/problem_checks.rb @@ -14,7 +14,12 @@ module Jobs # not be re-added by the relevant checker, and will be cleared. AdminDashboardData.clear_found_scheduled_check_problems - ::ProblemCheck.scheduled.each do |check| + scheduled_checks = + ProblemCheckTracker.all.filter_map do |tracker| + tracker.check if tracker.check.scheduled? && tracker.ready_to_run? + end + + scheduled_checks.each do |check| Jobs.enqueue(:problem_check, check_identifier: check.identifier.to_s) end end diff --git a/app/models/problem_check_tracker.rb b/app/models/problem_check_tracker.rb new file mode 100644 index 00000000000..a8f19b1bb5c --- /dev/null +++ b/app/models/problem_check_tracker.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +class ProblemCheckTracker < ActiveRecord::Base + validates :identifier, presence: true, uniqueness: true + validates :blips, presence: true, numericality: { greater_than_or_equal_to: 0 } + + def self.[](identifier) + find_or_create_by(identifier:) + end + + def ready_to_run? + next_run_at.blank? || next_run_at.past? + end + + def problem!(next_run_at: nil) + now = Time.current + + update!(blips: blips + 1, last_run_at: now, last_problem_at: now, next_run_at:) + end + + def no_problem!(next_run_at: nil) + now = Time.current + + update!(blips: 0, last_run_at: now, last_success_at: now, next_run_at:) + end + + def check + ProblemCheck[identifier] + end +end + +# == Schema Information +# +# Table name: problem_check_trackers +# +# id :bigint not null, primary key +# identifier :string not null +# blips :integer default(0), not null +# last_run_at :datetime +# next_run_at :datetime +# last_success_at :datetime +# last_problem_at :datetime +# +# Indexes +# +# index_problem_check_trackers_on_identifier (identifier) UNIQUE +# diff --git a/db/migrate/20240223052820_create_problem_check_trackers.rb b/db/migrate/20240223052820_create_problem_check_trackers.rb new file mode 100644 index 00000000000..12e5ac5c0da --- /dev/null +++ b/db/migrate/20240223052820_create_problem_check_trackers.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class CreateProblemCheckTrackers < ActiveRecord::Migration[7.0] + def change + create_table :problem_check_trackers do |t| + t.string :identifier, null: false, index: { unique: true } + + t.integer :blips, null: false, default: 0 + + t.datetime :last_run_at + t.datetime :next_run_at + t.datetime :last_success_at + t.datetime :last_problem_at + end + end +end diff --git a/spec/fabricators/problem_check_tracker_fabricator.rb b/spec/fabricators/problem_check_tracker_fabricator.rb new file mode 100644 index 00000000000..77f4a819f38 --- /dev/null +++ b/spec/fabricators/problem_check_tracker_fabricator.rb @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +Fabricator(:problem_check_tracker) {} diff --git a/spec/jobs/problem_check_spec.rb b/spec/jobs/problem_check_spec.rb index 8f4d9d60b5f..86c6d128045 100644 --- a/spec/jobs/problem_check_spec.rb +++ b/spec/jobs/problem_check_spec.rb @@ -11,6 +11,7 @@ RSpec.describe Jobs::ProblemCheck do before do ::ProblemCheck::TestCheck = Class.new(::ProblemCheck) do + self.perform_every = 30.minutes self.max_retries = 0 def call @@ -39,6 +40,7 @@ RSpec.describe Jobs::ProblemCheck do before do ::ProblemCheck::TestCheck = Class.new(::ProblemCheck) do + self.perform_every = 30.minutes self.max_retries = 0 def call @@ -71,6 +73,7 @@ RSpec.describe Jobs::ProblemCheck do before do ::ProblemCheck::TestCheck = Class.new(::ProblemCheck) do + self.perform_every = 30.minutes self.max_retries = 2 def call @@ -79,6 +82,12 @@ RSpec.describe Jobs::ProblemCheck do end end + it "does not yet update the problem check tracker" do + expect { + described_class.new.execute(check_identifier: :test_check, retry_count: 1) + }.not_to change { ProblemCheckTracker.where("blips > ?", 0).count } + end + it "schedules a retry" do expect_enqueued_with( job: :problem_check, @@ -94,6 +103,7 @@ RSpec.describe Jobs::ProblemCheck do before do ::ProblemCheck::TestCheck = Class.new(::ProblemCheck) do + self.perform_every = 30.minutes self.max_retries = 1 def call @@ -102,9 +112,15 @@ RSpec.describe Jobs::ProblemCheck do end end + it "updates the problem check tracker" do + expect { + described_class.new.execute(check_identifier: :test_check, retry_count: 1) + }.to change { ProblemCheckTracker.where("blips > ?", 0).count }.by(1) + 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) + described_class.new.execute(check_identifier: :test_check, retry_count: 1) end end end diff --git a/spec/jobs/problem_checks_spec.rb b/spec/jobs/problem_checks_spec.rb index 314e1968292..40c42b4fdfa 100644 --- a/spec/jobs/problem_checks_spec.rb +++ b/spec/jobs/problem_checks_spec.rb @@ -20,9 +20,43 @@ RSpec.describe Jobs::ProblemChecks do ProblemCheck.send(:remove_const, "NonScheduledCheck") end - it "schedules the individual scheduled checks" do - expect_enqueued_with(job: :problem_check, args: { check_identifier: "scheduled_check" }) do - described_class.new.execute([]) + context "when the tracker determines the check is ready to run" do + before do + ProblemCheckTracker.create!(identifier: "scheduled_check", next_run_at: 5.minutes.ago) + 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 + end + + context "when the tracker determines the check shouldn't run yet" do + before do + ProblemCheckTracker.create!(identifier: "scheduled_check", next_run_at: 5.minutes.from_now) + end + + it "does not schedule any check" do + expect_not_enqueued_with( + job: :problem_check, + args: { + check_identifier: "scheduled_check", + }, + ) { described_class.new.execute([]) } + end + end + + context "when dealing with a non-scheduled check" do + before { ProblemCheckTracker.create!(identifier: "non_scheduled_check", next_run_at: nil) } + + it "does not schedule any check" do + expect_not_enqueued_with( + job: :problem_check, + args: { + check_identifier: "non_scheduled_check", + }, + ) { described_class.new.execute([]) } end end @@ -34,13 +68,4 @@ RSpec.describe Jobs::ProblemChecks do }, ) { described_class.new.execute([]) } end - - 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 diff --git a/spec/models/problem_check_tracker_spec.rb b/spec/models/problem_check_tracker_spec.rb new file mode 100644 index 00000000000..6d0b3a9b800 --- /dev/null +++ b/spec/models/problem_check_tracker_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +RSpec.describe ProblemCheckTracker do + describe "validations" do + let(:record) { described_class.new(identifier: "twitter_login") } + + it { expect(record).to validate_presence_of(:identifier) } + it { expect(record).to validate_uniqueness_of(:identifier) } + + it { expect(record).to validate_numericality_of(:blips).is_greater_than_or_equal_to(0) } + end + + describe ".[]" do + before { Fabricate(:problem_check_tracker, identifier: "twitter_login") } + + context "when the problem check tracker already exists" do + it { expect(described_class[:twitter_login]).not_to be_new_record } + end + + context "when the problem check tracker doesn't exist yet" do + it { expect(described_class[:facebook_login]).to be_previously_new_record } + end + end + + describe "#ready_to_run?" do + let(:problem_tracker) { described_class.new(next_run_at:) } + + context "when the next run timestamp is not set" do + let(:next_run_at) { nil } + + it { expect(problem_tracker).to be_ready_to_run } + end + + context "when the next run timestamp is in the past" do + let(:next_run_at) { 5.minutes.ago } + + it { expect(problem_tracker).to be_ready_to_run } + end + + context "when the next run timestamp is in the future" do + let(:next_run_at) { 5.minutes.from_now } + + it { expect(problem_tracker).not_to be_ready_to_run } + end + end + + describe "#problem!" do + let(:problem_tracker) do + Fabricate(:problem_check_tracker, identifier: "twitter_login", **original_attributes) + end + + let(:original_attributes) do + { + blips: 0, + last_problem_at: 1.week.ago, + last_success_at: 24.hours.ago, + last_run_at: 24.hours.ago, + next_run_at: nil, + } + end + + let(:updated_attributes) { { blips: 1 } } + + it do + freeze_time + + expect { problem_tracker.problem!(next_run_at: 24.hours.from_now) }.to change { + problem_tracker.attributes + }.to(hash_including(updated_attributes)) + end + end + + describe "#no_problem!" do + let(:problem_tracker) do + Fabricate(:problem_check_tracker, identifier: "twitter_login", **original_attributes) + end + + let(:original_attributes) do + { + blips: 0, + last_problem_at: 1.week.ago, + last_success_at: Time.current, + last_run_at: 24.hours.ago, + next_run_at: nil, + } + end + + let(:updated_attributes) { { blips: 0 } } + + it do + freeze_time + + expect { problem_tracker.no_problem!(next_run_at: 24.hours.from_now) }.to change { + problem_tracker.attributes + }.to(hash_including(updated_attributes)) + end + end +end