From 815471da7634b6cdfd1f99244bafabd8a93a3bc8 Mon Sep 17 00:00:00 2001 From: Jeremy Lecour Date: Wed, 29 Aug 2018 12:14:34 +0200 Subject: [PATCH 1/4] CheckProcessor configuration is always passed It's better to redure coupling between these classes and Rails. It gives liberty to provide configuration and other parameters depending on the context. --- app/services/check_domain_processor.rb | 4 ---- app/services/check_processor.rb | 17 +++++------------ app/services/check_ssl_processor.rb | 4 ---- lib/tasks/checks.rake | 8 ++++++-- test/services/check_domain_processor_test.rb | 3 ++- test/services/check_processor_test.rb | 9 +++------ test/services/check_ssl_processor_test.rb | 3 ++- 7 files changed, 18 insertions(+), 30 deletions(-) diff --git a/app/services/check_domain_processor.rb b/app/services/check_domain_processor.rb index 8afc7d1..9c11112 100644 --- a/app/services/check_domain_processor.rb +++ b/app/services/check_domain_processor.rb @@ -6,10 +6,6 @@ class CheckDomainProcessor protected - def configuration_key - "checks_domain" - end - def resolvers %i[ resolve_last_run_failed diff --git a/app/services/check_processor.rb b/app/services/check_processor.rb index 37226b5..9b5a5f9 100644 --- a/app/services/check_processor.rb +++ b/app/services/check_processor.rb @@ -4,11 +4,13 @@ module CheckProcessor attr_reader :configuration - def initialize(configuration = nil) - @configuration = configuration || default_configuration + def initialize(configuration:, logger: NullLogger.new) + @logger = logger + @configuration = configuration end def sync_dates + @sync_started_at = Time.now resolvers.each do |resolver| public_send(resolver).find_each(batch_size: 100).each do |check| process(check) @@ -16,6 +18,7 @@ module CheckProcessor sleep configuration.interval end end + @sync_finished_at = Time.now end # :nocov: @@ -65,15 +68,5 @@ module CheckProcessor def process(_check) fail NotImplementedError, "#{self.class.name} did not implemented method #{__callee__}" end - - def configuration_key - fail NotImplementedError, "#{self.class.name} did not implemented method #{__callee__}" - end # :nocov: - - private - - def default_configuration - Rails.configuration.chexpire.fetch(configuration_key) - end end diff --git a/app/services/check_ssl_processor.rb b/app/services/check_ssl_processor.rb index bc75102..764ea98 100644 --- a/app/services/check_ssl_processor.rb +++ b/app/services/check_ssl_processor.rb @@ -6,10 +6,6 @@ class CheckSSLProcessor protected - def configuration_key - "checks_ssl" - end - def resolvers %i[ resolve_all diff --git a/lib/tasks/checks.rake b/lib/tasks/checks.rake index 9556f49..9eb409a 100644 --- a/lib/tasks/checks.rake +++ b/lib/tasks/checks.rake @@ -7,13 +7,17 @@ namespace :checks do desc "Refresh domains expiry dates" task domain: :environment do - process = CheckDomainProcessor.new + configuration = Rails.configuration.chexpire.fetch("checks_domain") + + process = CheckDomainProcessor.new(configuration: configuration) process.sync_dates end desc "Refresh SSL expiry dates" task ssl: :environment do - process = CheckSSLProcessor.new + configuration = Rails.configuration.chexpire.fetch("checks_ssl") + + process = CheckSSLProcessor.new(configuration: configuration) process.sync_dates end end diff --git a/test/services/check_domain_processor_test.rb b/test/services/check_domain_processor_test.rb index c9801e4..65281d3 100644 --- a/test/services/check_domain_processor_test.rb +++ b/test/services/check_domain_processor_test.rb @@ -5,7 +5,8 @@ require "test_helper" class CheckDomainProcessorTest < ActiveSupport::TestCase setup do - @processor = CheckDomainProcessor.new + configuration = Rails.configuration.chexpire.fetch("checks_domain") + @processor = CheckDomainProcessor.new(configuration: configuration) end test "process WhoisSyncJob for domain checks" do diff --git a/test/services/check_processor_test.rb b/test/services/check_processor_test.rb index 876f8d5..8f8694f 100644 --- a/test/services/check_processor_test.rb +++ b/test/services/check_processor_test.rb @@ -9,10 +9,6 @@ class CheckDummyProcessor base_scope end - def configuration_key - "checks_dummy" - end - def resolvers %i[ resolve_expire_short_term @@ -23,7 +19,8 @@ end class CheckProcessorTest < ActiveSupport::TestCase setup do - @processor = CheckDummyProcessor.new + configuration = Rails.configuration.chexpire.fetch("checks_dummy") + @processor = CheckDummyProcessor.new(configuration: configuration) end test "resolve_last_run_failed includes already and never succeeded" do @@ -119,7 +116,7 @@ class CheckProcessorTest < ActiveSupport::TestCase configuration.expect(:interval, 0.000001) end - processor = CheckDummyProcessor.new(configuration) + processor = CheckDummyProcessor.new(configuration: configuration) mock = Minitest::Mock.new assert_stub = lambda { |actual_time| diff --git a/test/services/check_ssl_processor_test.rb b/test/services/check_ssl_processor_test.rb index 8075382..51a71bf 100644 --- a/test/services/check_ssl_processor_test.rb +++ b/test/services/check_ssl_processor_test.rb @@ -5,7 +5,8 @@ require "test_helper" class CheckSSLProcessorTest < ActiveSupport::TestCase setup do - @processor = CheckSSLProcessor.new + configuration = Rails.configuration.chexpire.fetch("checks_ssl") + @processor = CheckSSLProcessor.new(configuration: configuration) end test "process SSLSyncJob for ssl checks" do From 5326dcc7aa2dc52fd8d95da89415a9253993b7a0 Mon Sep 17 00:00:00 2001 From: Jeremy Lecour Date: Wed, 29 Aug 2018 12:19:05 +0200 Subject: [PATCH 2/4] Add a logger to check processors Currently it is unused. --- Gemfile | 2 ++ Gemfile.lock | 5 +++++ lib/tasks/checks.rake | 8 ++++++-- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index 5230b7d..710b52f 100644 --- a/Gemfile +++ b/Gemfile @@ -48,6 +48,8 @@ gem 'octicons' gem 'kaminari' gem 'has_scope' +gem 'logging' + # Reduces boot times through caching; required in config/boot.rb gem 'bootsnap', '>= 1.1.0', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 3278ee9..9e7f69e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -166,6 +166,10 @@ GEM rb-fsevent (~> 0.9, >= 0.9.4) rb-inotify (~> 0.9, >= 0.9.7) ruby_dep (~> 1.2) + little-plugger (1.1.4) + logging (2.2.2) + little-plugger (~> 1.1) + multi_json (~> 1.10) loofah (2.2.2) crass (~> 1.0.2) nokogiri (>= 1.5.9) @@ -363,6 +367,7 @@ DEPENDENCIES launchy letter_opener_web listen (>= 3.0.5, < 3.2) + logging mysql2 (>= 0.4.4, < 0.6.0) naught octicons diff --git a/lib/tasks/checks.rake b/lib/tasks/checks.rake index 9eb409a..a4ffaf6 100644 --- a/lib/tasks/checks.rake +++ b/lib/tasks/checks.rake @@ -7,17 +7,21 @@ namespace :checks do desc "Refresh domains expiry dates" task domain: :environment do + logger = Logging.logger(STDOUT) + logger.level = :warn configuration = Rails.configuration.chexpire.fetch("checks_domain") - process = CheckDomainProcessor.new(configuration: configuration) + process = CheckDomainProcessor.new(logger: logger, configuration: configuration) process.sync_dates end desc "Refresh SSL expiry dates" task ssl: :environment do + logger = Logging.logger(STDOUT) + logger.level = :warn configuration = Rails.configuration.chexpire.fetch("checks_ssl") - process = CheckSSLProcessor.new(configuration: configuration) + process = CheckSSLProcessor.new(logger: logger, configuration: configuration) process.sync_dates end end From 1235845e545877d84cdc648006cdf173fbdb3395 Mon Sep 17 00:00:00 2001 From: Jeremy Lecour Date: Thu, 30 Aug 2018 17:50:38 +0200 Subject: [PATCH 3/4] Add a logger to CheckProcessor to log errors for each batch of checks --- app/services/check_processor.rb | 47 ++++++++++++++++++++++----- lib/tasks/checks.rake | 41 +++++++++++++++++------ test/services/check_processor_test.rb | 13 +++----- 3 files changed, 74 insertions(+), 27 deletions(-) diff --git a/app/services/check_processor.rb b/app/services/check_processor.rb index 9b5a5f9..1b77758 100644 --- a/app/services/check_processor.rb +++ b/app/services/check_processor.rb @@ -2,23 +2,54 @@ # License: GNU AGPL-3+ (see full text in LICENSE file) module CheckProcessor - attr_reader :configuration + attr_reader :configuration, :logger def initialize(configuration:, logger: NullLogger.new) + # Levels: debug info warn error fatal @logger = logger @configuration = configuration end - def sync_dates - @sync_started_at = Time.now - resolvers.each do |resolver| - public_send(resolver).find_each(batch_size: 100).each do |check| - process(check) + def sync_dates # rubocop:disable Metrics/AbcSize,Metrics/MethodLength + sync_started_at = Time.now + logger.info "#{self.class.name}: sync_dates has started" - sleep configuration.interval + resolvers.each do |resolver| + logger.info "#{self.class.name}: using resolver '#{resolver}'" + + public_send(resolver).find_in_batches(batch_size: 100) do |checks| + group_started_at = Time.now + + checks.each do |check| + logger.info "#{self.class.name}: processing check ##{check.id}" + process(check) + + logger.debug "#{self.class.name}: sleeping #{configuration.interval} seconds" + sleep configuration.interval + end + + group_finished_at = Time.now + + check_errors_scope(check_ids: checks.map(&:id), + after_date: group_started_at, + before_date: group_finished_at).includes(:check).each do |check_log| + message = "#{self.class.name}: check ##{check_log.check_id} for '#{check_log.check.domain}' failed (#{check_log.exit_status}) ; #{check_log.error.lines.first}" # rubocop:disable Metrics/LineLength + logger.error(message) + end end end - @sync_finished_at = Time.now + + sync_finished_at = Time.now + duration = (sync_finished_at - sync_started_at).to_i + logger.info "#{self.class.name}: sync_dates has finished (#{duration}s)" + end + + def check_errors_scope(check_ids:, before_date: nil, after_date: nil) + scope = CheckLog.failed.where("exit_status > 0").where(id: check_ids) + scope = scope.where("created_at <= ?", before_date) if before_date + scope = scope.where("created_at >= ?", after_date) if after_date + + scope end # :nocov: diff --git a/lib/tasks/checks.rake b/lib/tasks/checks.rake index a4ffaf6..7f0e6a6 100644 --- a/lib/tasks/checks.rake +++ b/lib/tasks/checks.rake @@ -1,27 +1,48 @@ # Copyright (C) 2018 Colin Darie , 2018 Evolix # License: GNU AGPL-3+ (see full text in LICENSE file) +require "null_logger" + namespace :checks do namespace :sync_dates do task all: [:domain, :ssl] + def stdout_logger(env={}) + verbose_mode = env.fetch("VERBOSE") { 0 }.to_i == 1 + quiet_mode = env.fetch("QUIET") { 0 }.to_i == 1 + silent_mode = env.fetch("SILENT") { 0 }.to_i == 1 + + if silent_mode + NullLogger.new + else + logger = Logging.logger(STDOUT) + logger.level = if quiet_mode + :error + elsif verbose_mode + :debug + else + :info + end + + logger + end + end + desc "Refresh domains expiry dates" task domain: :environment do - logger = Logging.logger(STDOUT) - logger.level = :warn - configuration = Rails.configuration.chexpire.fetch("checks_domain") - - process = CheckDomainProcessor.new(logger: logger, configuration: configuration) + process = CheckDomainProcessor.new( + logger: stdout_logger(ENV), + configuration: Rails.configuration.chexpire.fetch("checks_domain") + ) process.sync_dates end desc "Refresh SSL expiry dates" task ssl: :environment do - logger = Logging.logger(STDOUT) - logger.level = :warn - configuration = Rails.configuration.chexpire.fetch("checks_ssl") - - process = CheckSSLProcessor.new(logger: logger, configuration: configuration) + process = CheckSSLProcessor.new( + logger: stdout_logger(ENV), + configuration: Rails.configuration.chexpire.fetch("checks_ssl") + ) process.sync_dates end end diff --git a/test/services/check_processor_test.rb b/test/services/check_processor_test.rb index 8f8694f..da5b914 100644 --- a/test/services/check_processor_test.rb +++ b/test/services/check_processor_test.rb @@ -108,19 +108,15 @@ class CheckProcessorTest < ActiveSupport::TestCase test "#sync_dates respects the interval configuration between sends" do create_list(:check, 3, :expires_next_week) - configuration = Minitest::Mock.new - 2.times do configuration.expect(:long_term_interval, 300) end - configuration.expect(:long_term_frequency, 4) - - 3.times do - configuration.expect(:interval, 0.000001) - end + configuration = OpenStruct.new(long_term_interval: 300, + long_term_frequency: 4, + interval: 0.000001) processor = CheckDummyProcessor.new(configuration: configuration) mock = Minitest::Mock.new assert_stub = lambda { |actual_time| - assert_equal 0.000001, actual_time + assert_equal configuration.interval, actual_time mock } @@ -130,7 +126,6 @@ class CheckProcessorTest < ActiveSupport::TestCase end end - configuration.verify mock.verify end end From fab5732b329fd3a92779f89962876ce00dc0ac03 Mon Sep 17 00:00:00 2001 From: Jeremy Lecour Date: Thu, 30 Aug 2018 18:36:26 +0200 Subject: [PATCH 4/4] fix rubocop offenses --- lib/tasks/checks.rake | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/tasks/checks.rake b/lib/tasks/checks.rake index 7f0e6a6..4f0975c 100644 --- a/lib/tasks/checks.rake +++ b/lib/tasks/checks.rake @@ -7,13 +7,13 @@ namespace :checks do namespace :sync_dates do task all: [:domain, :ssl] - def stdout_logger(env={}) + def stdout_logger(env = {}) # rubocop:disable Metrics/MethodLength verbose_mode = env.fetch("VERBOSE") { 0 }.to_i == 1 quiet_mode = env.fetch("QUIET") { 0 }.to_i == 1 silent_mode = env.fetch("SILENT") { 0 }.to_i == 1 if silent_mode - NullLogger.new + NullLogger.new else logger = Logging.logger(STDOUT) logger.level = if quiet_mode @@ -32,7 +32,7 @@ namespace :checks do task domain: :environment do process = CheckDomainProcessor.new( logger: stdout_logger(ENV), - configuration: Rails.configuration.chexpire.fetch("checks_domain") + configuration: Rails.configuration.chexpire.fetch("checks_domain"), ) process.sync_dates end @@ -41,7 +41,7 @@ namespace :checks do task ssl: :environment do process = CheckSSLProcessor.new( logger: stdout_logger(ENV), - configuration: Rails.configuration.chexpire.fetch("checks_ssl") + configuration: Rails.configuration.chexpire.fetch("checks_ssl"), ) process.sync_dates end