Merge branch '349592-log-port-conflicts' into 'master'

Rescue from and log errors in metrics exporters

See merge request gitlab-org/gitlab!77651

lib/gitlab/metrics/exporter/base_exporter.rb

@@ -11,28 +11,26 @@ module Gitlab
 
         attr_accessor :readiness_checks
 
-        def initialize(settings, **options)
+        def initialize(settings, log_enabled:, log_file:, **options)
           super(**options)
 
           @settings = settings
+
+          # log_enabled does not exist for all exporters
+          log_sink = log_enabled ? File.join(Rails.root, 'log', log_file) : File::NULL
+          @logger = WEBrick::Log.new(log_sink)
+          @logger.time_format = "[%Y-%m-%dT%H:%M:%S.%L%z]"
         end
 
         def enabled?
           settings.enabled
         end
 
-        def log_filename
-          raise NotImplementedError
-        end
-
         private
 
-        attr_reader :settings
+        attr_reader :settings, :logger
 
         def start_working
-          logger = WEBrick::Log.new(log_filename)
-          logger.time_format = "[%Y-%m-%dT%H:%M:%S.%L%z]"
-
           access_log = [
             [logger, WEBrick::AccessLog::COMBINED_LOG_FORMAT]
           ]
@@ -44,6 +42,9 @@ module Gitlab
           server.mount '/', Rack::Handler::WEBrick, rack_app
 
           true
+        rescue StandardError => e
+          logger.error(e)
+          false
         end
 
         def run_thread

lib/gitlab/metrics/exporter/sidekiq_exporter.rb

@@ -4,12 +4,11 @@ module Gitlab
   module Metrics
     module Exporter
       class SidekiqExporter < BaseExporter
-        def log_filename
-          if settings['log_enabled']
-            File.join(Rails.root, 'log', 'sidekiq_exporter.log')
-          else
-            File::NULL
-          end
+        def initialize(settings, **options)
+          super(settings,
+            log_enabled: settings['log_enabled'],
+            log_file: 'sidekiq_exporter.log',
+            **options)
         end
       end
     end

lib/gitlab/metrics/exporter/web_exporter.rb

@@ -26,8 +26,8 @@ module Gitlab
         attr_reader :running
 
         # This exporter is always run on master process
-        def initialize
-          super(Settings.monitoring.web_exporter)
+        def initialize(**options)
+          super(Settings.monitoring.web_exporter, log_enabled: true, log_file: 'web_exporter.log', **options)
 
           # DEPRECATED:
           # these `readiness_checks` are deprecated
@@ -39,10 +39,6 @@ module Gitlab
           ]
         end
 
-        def log_filename
-          File.join(Rails.root, 'log', 'web_exporter.log')
-        end
-
         def mark_as_not_running!
           @running = false
         end

spec/lib/gitlab/metrics/exporter/base_exporter_spec.rb

@@ -4,13 +4,8 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Metrics::Exporter::BaseExporter do
   let(:settings) { double('settings') }
-  let(:exporter) { described_class.new(settings) }
-  let(:log_filename) { File.join(Rails.root, 'log', 'sidekiq_exporter.log') }
-
-  before do
-    allow_any_instance_of(described_class).to receive(:log_filename).and_return(log_filename)
-    allow_any_instance_of(described_class).to receive(:settings).and_return(settings)
-  end
+  let(:log_enabled) { false }
+  let(:exporter) { described_class.new(settings, log_enabled: log_enabled, log_file: 'test_exporter.log') }
 
   describe 'when exporter is enabled' do
     before do
@@ -61,6 +56,38 @@ RSpec.describe Gitlab::Metrics::Exporter::BaseExporter do
 
             exporter.start.join
           end
+
+          context 'logging enabled' do
+            let(:log_enabled) { true }
+            let(:logger) { instance_double(WEBrick::Log) }
+
+            before do
+              allow(logger).to receive(:time_format=)
+              allow(logger).to receive(:info)
+            end
+
+            it 'configures a WEBrick logger with the given file' do
+              expect(WEBrick::Log).to receive(:new).with(end_with('test_exporter.log')).and_return(logger)
+
+              exporter
+            end
+
+            it 'logs any errors during startup' do
+              expect(::WEBrick::Log).to receive(:new).and_return(logger)
+              expect(::WEBrick::HTTPServer).to receive(:new).and_raise 'fail'
+              expect(logger).to receive(:error)
+
+              exporter.start
+            end
+          end
+
+          context 'logging disabled' do
+            it 'configures a WEBrick logger with the null device' do
+              expect(WEBrick::Log).to receive(:new).with(File::NULL).and_call_original
+
+              exporter
+            end
+          end
         end
 
         describe 'when thread is not alive' do

spec/lib/gitlab/metrics/exporter/sidekiq_exporter_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Metrics::Exporter::SidekiqExporter do
-  let(:exporter) { described_class.new(Settings.monitoring.sidekiq_exporter) }
-
-  after do
-    exporter.stop
-  end
-
-  context 'with valid config' do
-    before do
-      stub_config(
-        monitoring: {
-          sidekiq_exporter: {
-            enabled: true,
-            log_enabled: false,
-            port: 0,
-            address: '127.0.0.1'
-          }
-        }
-      )
-    end
-
-    it 'does start thread' do
-      expect(exporter.start).not_to be_nil
-    end
-
-    it 'does not enable logging by default' do
-      expect(exporter.log_filename).to eq(File::NULL)
-    end
-  end
-
-  context 'with logging enabled' do
-    before do
-      stub_config(
-        monitoring: {
-          sidekiq_exporter: {
-            enabled: true,
-            log_enabled: true,
-            port: 0,
-            address: '127.0.0.1'
-          }
-        }
-      )
-    end
-
-    it 'returns a valid log filename' do
-      expect(exporter.log_filename).to end_with('sidekiq_exporter.log')
-    end
-  end
-end