Merge pull request #1306 from appsignal/refactor-logger

Fix internal logger in Capistrano tasks and remove Config logger instance

.changesets/fix-no-logs-appearing-from-capistrano-tasks.md

@@ -0,0 +1,6 @@
+---
+bump: patch
+type: fix
+---
+
+Fix no AppSignal internal logs being logged from Capistrano tasks.

lib/appsignal.rb

@@ -244,8 +244,7 @@ def configure(env = nil, root_path: nil)
       else
         @config = Config.new(
           root_path || Config.determine_root_path,
-          Config.determine_env(env),
-          Appsignal.internal_logger
+          Config.determine_env(env)
         )
       end
 

lib/appsignal/config.rb

@@ -208,18 +208,13 @@ def self.determine_root_path
     attr_accessor :root_path, :env, :config_hash
     attr_reader :system_config, :loaders_config, :initial_config, :file_config,
       :env_config, :override_config, :dsl_config
-    # @api private
-    attr_accessor :logger
 
     # Initialize a new configuration object for AppSignal.
     #
     # @param root_path [String] Root path of the app.
     # @param env [String] The environment to load when AppSignal is started. It
     #   will look for an environment with this name in the `config/appsignal.yml`
     #   config file.
-    # @param logger [Logger] The logger to use for the AppSignal gem. This is
-    #   used by the configuration class only. Default:
-    #   {Appsignal.internal_logger}. See also {Appsignal.start}.
     #
     # @api private
     # @see https://docs.appsignal.com/ruby/configuration/
@@ -230,13 +225,11 @@ def self.determine_root_path
     #   How to integrate AppSignal manually
     def initialize(
       root_path,
-      env,
-      logger = Appsignal.internal_logger
+      env
     )
       @root_path = root_path
       @config_file_error = false
       @config_file = config_file
-      @logger = logger
       @valid = false
 
       @env = env.to_s
@@ -426,7 +419,7 @@ def validate
       push_api_key = config_hash[:push_api_key] || ""
       if push_api_key.strip.empty?
         @valid = false
-        @logger.error "Push API key not set after loading config"
+        logger.error "Push API key not set after loading config"
       else
         @valid = true
       end
@@ -445,6 +438,10 @@ def freeze
 
     private
 
+    def logger
+      Appsignal.internal_logger
+    end
+
     def config_file
       @config_file ||=
         root_path.nil? ? nil : File.join(root_path, "config", "appsignal.yml")
@@ -546,7 +543,7 @@ def determine_overrides
 
     def merge(new_config)
       new_config.each do |key, value|
-        @logger.debug("Config key '#{key}' is being overwritten") unless config_hash[key].nil?
+        logger.debug("Config key '#{key}' is being overwritten") unless config_hash[key].nil?
         config_hash[key] = value
       end
     end

lib/appsignal/integrations/capistrano/appsignal.cap

@@ -10,12 +10,12 @@ namespace :appsignal do
 
     appsignal_config = Appsignal::Config.new(
       Dir.pwd,
-      appsignal_env,
-      Logger.new(StringIO.new)
+      appsignal_env
     ).tap do |c|
       c.merge_dsl_options(fetch(:appsignal_config, {}))
       c.validate
     end
+    Appsignal._start_logger
 
     if appsignal_config&.active?
       marker_data = {

lib/appsignal/integrations/capistrano/capistrano_2_tasks.rb

@@ -18,12 +18,12 @@ def self.tasks(config)
 
               appsignal_config = Appsignal::Config.new(
                 ENV.fetch("PWD", nil),
-                env,
-                Appsignal::Utils::IntegrationLogger.new(StringIO.new)
+                env
               ).tap do |c|
                 c.merge_dsl_options(fetch(:appsignal_config, {}))
                 c.validate
               end
+              Appsignal._start_logger
 
               if appsignal_config&.active?
                 marker_data = {

lib/appsignal/transmitter.rb

@@ -107,7 +107,7 @@ def http_client
           if ca_file && File.exist?(ca_file) && File.readable?(ca_file)
             http.ca_file = ca_file
           else
-            config.logger.warn "Ignoring non-existing or unreadable " \
+            Appsignal.internal_logger.warn "Ignoring non-existing or unreadable " \
               "`ca_file_path`: #{ca_file}"
           end
         end

spec/lib/appsignal/capistrano2_spec.rb

@@ -7,6 +7,8 @@
     let(:out_stream) { std_stream }
     let(:output) { out_stream.read }
     let(:config) { build_config }
+    let(:log_stream) { StringIO.new }
+    let(:logs) { log_contents(log_stream) }
     let(:capistrano_config) do
       Capistrano::Configuration.new.tap do |c|
         c.set(:rails_env, "production")
@@ -17,7 +19,10 @@
         c.dry_run = false
       end
     end
-    before { Appsignal::Integrations::Capistrano.tasks(capistrano_config) }
+    before do
+      Appsignal.internal_logger = test_logger(log_stream)
+      Appsignal::Integrations::Capistrano.tasks(capistrano_config)
+    end
 
     def run
       capture_stdout(out_stream) do
@@ -125,6 +130,7 @@ def run
               run
               expect(output).to include \
                 "Not notifying of deploy, config is not active for environment: production"
+              expect(logs).to contains_log(:error, "Push API key not set after loading config")
             end
           end
         end

spec/lib/appsignal/capistrano3_spec.rb

@@ -10,12 +10,12 @@
     let(:config) { build_config(:env => env, :options => options) }
     let(:out_stream) { std_stream }
     let(:output) { out_stream.read }
-    let(:logger) { Logger.new(out_stream) }
+    let(:log_stream) { StringIO.new }
+    let(:logs) { log_contents(log_stream) }
     let!(:capistrano_config) do
       Capistrano::Configuration.reset!
       Capistrano::Configuration.env.tap do |c|
         c.set(:log_level, :error)
-        c.set(:logger, logger)
         c.set(:rails_env, "production")
         c.set(:repository, "main")
         c.set(:deploy_to, "/home/username/app")
@@ -29,7 +29,10 @@
         :user => "batman"
       }
     end
-    before { Rake::Task["appsignal:deploy"].reenable }
+    before do
+      Appsignal.internal_logger = test_logger(log_stream)
+      Rake::Task["appsignal:deploy"].reenable
+    end
 
     def run
       capture_std_streams(out_stream, out_stream) do
@@ -141,6 +144,7 @@ def run
               run
               expect(output).to include \
                 "Not notifying of deploy, config is not active for environment: production"
+              expect(logs).to contains_log(:error, "Push API key not set after loading config")
             end
           end
         end

spec/lib/appsignal/transmitter_spec.rb

@@ -1,14 +1,15 @@
 describe Appsignal::Transmitter do
   let(:options) { {} }
   let(:config) do
-    build_config(
-      :options => { :hostname => "app1.local" }.merge(options),
-      :logger => Logger.new(log)
-    )
+    build_config(:options => { :hostname => "app1.local" }.merge(options))
   end
   let(:base_uri) { "action" }
-  let(:log) { StringIO.new }
   let(:instance) { Appsignal::Transmitter.new(base_uri, config) }
+  let(:log_stream) { StringIO.new }
+  let(:logs) { log_contents(log_stream) }
+  before do
+    Appsignal.internal_logger = test_logger(log_stream)
+  end
 
   describe "#uri" do
     let(:uri) { instance.uri }
@@ -98,7 +99,7 @@
         it "ignores the config and logs a warning" do
           expect(response).to be_kind_of(Net::HTTPResponse)
           expect(response.code).to eq "200"
-          expect(log.string).to_not include "Ignoring non-existing or unreadable " \
+          expect(logs).to_not include "Ignoring non-existing or unreadable " \
             "`ca_file_path`: #{config[:ca_file_path]}"
         end
       end
@@ -109,8 +110,11 @@
         it "ignores the config and logs a warning" do
           expect(response).to be_kind_of(Net::HTTPResponse)
           expect(response.code).to eq "200"
-          expect(log.string).to include "Ignoring non-existing or unreadable " \
-            "`ca_file_path`: #{config[:ca_file_path]}"
+          expect(logs).to contains_log(
+            :warn,
+            "Ignoring non-existing or unreadable " \
+              "`ca_file_path`: #{config[:ca_file_path]}"
+          )
         end
       end
 
@@ -124,8 +128,11 @@
         it "ignores the config and logs a warning" do
           expect(response).to be_kind_of(Net::HTTPResponse)
           expect(response.code).to eq "200"
-          expect(log.string).to include "Ignoring non-existing or unreadable " \
-            "`ca_file_path`: #{config[:ca_file_path]}"
+          expect(logs).to contains_log(
+            :warn,
+            "Ignoring non-existing or unreadable " \
+              "`ca_file_path`: #{config[:ca_file_path]}"
+          )
         end
 
         after { File.delete file }

spec/support/helpers/config_helpers.rb

@@ -16,13 +16,11 @@ def rails_project_fixture_path
   def build_config(
     root_path: project_fixture_path,
     env: "production",
-    options: {},
-    logger: Appsignal.internal_logger
+    options: {}
   )
     Appsignal::Config.new(
       root_path,
-      env,
-      logger
+      env
     ).tap do |c|
       c.merge_dsl_options(options) if options.any?
       c.validate