Skip to content

Commit

Permalink
Merge pull request #1306 from appsignal/refactor-logger
Browse files Browse the repository at this point in the history
Fix internal logger in Capistrano tasks and remove Config logger instance
  • Loading branch information
tombruijn authored Sep 27, 2024
2 parents 3de0070 + 6ae1c36 commit d81bbdf
Show file tree
Hide file tree
Showing 10 changed files with 52 additions and 35 deletions.
6 changes: 6 additions & 0 deletions .changesets/fix-no-logs-appearing-from-capistrano-tasks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
bump: patch
type: fix
---

Fix no AppSignal internal logs being logged from Capistrano tasks.
3 changes: 1 addition & 2 deletions lib/appsignal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
17 changes: 7 additions & 10 deletions lib/appsignal/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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/
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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")
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions lib/appsignal/integrations/capistrano/appsignal.cap
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
4 changes: 2 additions & 2 deletions lib/appsignal/integrations/capistrano/capistrano_2_tasks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
2 changes: 1 addition & 1 deletion lib/appsignal/transmitter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 7 additions & 1 deletion spec/lib/appsignal/capistrano2_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
10 changes: 7 additions & 3 deletions spec/lib/appsignal/capistrano3_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
27 changes: 17 additions & 10 deletions spec/lib/appsignal/transmitter_spec.rb
Original file line number Diff line number Diff line change
@@ -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 }
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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 }
Expand Down
6 changes: 2 additions & 4 deletions spec/support/helpers/config_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit d81bbdf

Please sign in to comment.