diff --git a/.changesets/fix-no-logs-appearing-from-capistrano-tasks.md b/.changesets/fix-no-logs-appearing-from-capistrano-tasks.md new file mode 100644 index 000000000..a625fa672 --- /dev/null +++ b/.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. diff --git a/lib/appsignal.rb b/lib/appsignal.rb index 6bf666447..f212ec2ff 100644 --- a/lib/appsignal.rb +++ b/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 diff --git a/lib/appsignal/config.rb b/lib/appsignal/config.rb index bfc4ffd88..368a97f87 100644 --- a/lib/appsignal/config.rb +++ b/lib/appsignal/config.rb @@ -208,8 +208,6 @@ 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. # @@ -217,9 +215,6 @@ def self.determine_root_path # @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 diff --git a/lib/appsignal/integrations/capistrano/appsignal.cap b/lib/appsignal/integrations/capistrano/appsignal.cap index 7e4102e17..d12c776ef 100644 --- a/lib/appsignal/integrations/capistrano/appsignal.cap +++ b/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 = { diff --git a/lib/appsignal/integrations/capistrano/capistrano_2_tasks.rb b/lib/appsignal/integrations/capistrano/capistrano_2_tasks.rb index ea7b6dd7e..1c11e23a1 100644 --- a/lib/appsignal/integrations/capistrano/capistrano_2_tasks.rb +++ b/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 = { diff --git a/lib/appsignal/transmitter.rb b/lib/appsignal/transmitter.rb index 252507e9a..8523d4734 100644 --- a/lib/appsignal/transmitter.rb +++ b/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 diff --git a/spec/lib/appsignal/capistrano2_spec.rb b/spec/lib/appsignal/capistrano2_spec.rb index 8221e9109..ae8f6f172 100644 --- a/spec/lib/appsignal/capistrano2_spec.rb +++ b/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 diff --git a/spec/lib/appsignal/capistrano3_spec.rb b/spec/lib/appsignal/capistrano3_spec.rb index d46d2ed28..54a68eb5e 100644 --- a/spec/lib/appsignal/capistrano3_spec.rb +++ b/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 diff --git a/spec/lib/appsignal/transmitter_spec.rb b/spec/lib/appsignal/transmitter_spec.rb index 91067e521..49bf74611 100644 --- a/spec/lib/appsignal/transmitter_spec.rb +++ b/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 } diff --git a/spec/support/helpers/config_helpers.rb b/spec/support/helpers/config_helpers.rb index 74d388949..98b66ba6d 100644 --- a/spec/support/helpers/config_helpers.rb +++ b/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