Skip to content

Commit

Permalink
Log log_file_path warning only once (#1064)
Browse files Browse the repository at this point in the history
Avoid printing the warning more than once if `log_file_path` is called
more than once. Only print the warning once to avoid confusion.

Fixes #776
  • Loading branch information
tombruijn authored Apr 26, 2024
1 parent a2f4b31 commit 226a8f5
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 14 deletions.
6 changes: 6 additions & 0 deletions .changesets/print-log-path-warning-only-once.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
bump: "patch"
type: "change"
---

If the gem can't find a valid log path in the app's `log/` directory, it will no longer print the warning more than once.
17 changes: 13 additions & 4 deletions lib/appsignal/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -320,20 +320,29 @@ def log_level
end

def log_file_path
return @log_file_path if defined? @log_file_path

path = config_hash[:log_path] || (root_path && File.join(root_path, "log"))
return File.join(File.realpath(path), "appsignal.log") if path && File.writable?(path)
if path && File.writable?(path)
@log_file_path = File.join(File.realpath(path), "appsignal.log")
return @log_file_path
end

system_tmp_dir = self.class.system_tmp_dir
if File.writable? system_tmp_dir
$stdout.puts "appsignal: Unable to log to '#{path}'. Logging to " \
"'#{system_tmp_dir}' instead. Please check the " \
"permissions for the application's (log) directory."
File.join(system_tmp_dir, "appsignal.log")
"'#{system_tmp_dir}' instead. " \
"Please check the permissions for the application's (log) " \
"directory."
@log_file_path = File.join(system_tmp_dir, "appsignal.log")
else
$stdout.puts "appsignal: Unable to log to '#{path}' or the " \
"'#{system_tmp_dir}' fallback. Please check the permissions " \
"for the application's (log) directory."
@log_file_path = nil
end

@log_file_path
end

def valid?
Expand Down
42 changes: 32 additions & 10 deletions spec/lib/appsignal/config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -757,19 +757,22 @@
let(:out_stream) { std_stream }
let(:output) { out_stream.read }
let(:config) { project_fixture_config("production", :log_path => log_path) }
subject { capture_stdout(out_stream) { config.log_file_path } }

def log_file_path
capture_stdout(out_stream) { config.log_file_path }
end

context "when path is writable" do
let(:log_path) { File.join(tmp_dir, "writable-path") }
before { FileUtils.mkdir_p(log_path, :mode => 0o755) }
after { FileUtils.rm_rf(log_path) }

it "returns log file path" do
expect(subject).to eq File.join(log_path, "appsignal.log")
expect(log_file_path).to eq File.join(log_path, "appsignal.log")
end

it "prints no warning" do
subject
log_file_path
expect(output).to be_empty
end
end
Expand All @@ -783,28 +786,47 @@
before { FileUtils.chmod(0o777, system_tmp_dir) }

it "returns returns the tmp location" do
expect(subject).to eq(File.join(system_tmp_dir, "appsignal.log"))
expect(log_file_path).to eq(File.join(system_tmp_dir, "appsignal.log"))
end

it "prints a warning" do
subject
log_file_path
expect(output).to include "appsignal: Unable to log to '#{log_path}'. " \
"Logging to '#{system_tmp_dir}' instead."
end

it "prints a warning once" do
capture_stdout(out_stream) do
log_file_path
log_file_path
end
message = "appsignal: Unable to log to '#{log_path}'. " \
"Logging to '#{system_tmp_dir}' instead."
expect(output.scan(message).count).to eq(1)
end
end

context "when the /tmp fallback path is not writable" do
before { FileUtils.chmod(0o555, system_tmp_dir) }

it "returns nil" do
expect(subject).to be_nil
expect(log_file_path).to be_nil
end

it "prints a warning" do
subject
log_file_path
expect(output).to include "appsignal: Unable to log to '#{log_path}' " \
"or the '#{system_tmp_dir}' fallback."
end

it "prints a warning once" do
capture_stdout(out_stream) do
log_file_path
log_file_path
end
message = "appsignal: Unable to log to '#{log_path}' or the '#{system_tmp_dir}' fallback."
expect(output.scan(message).count).to eq(1)
end
end
end

Expand All @@ -819,11 +841,11 @@

context "when root_path is set" do
it "returns returns the project log location" do
expect(subject).to eq File.join(config.root_path, "log/appsignal.log")
expect(log_file_path).to eq File.join(config.root_path, "log/appsignal.log")
end

it "prints no warning" do
subject
log_file_path
expect(output).to be_empty
end
end
Expand Down Expand Up @@ -883,7 +905,7 @@
end

it "returns real path of log path" do
expect(subject).to eq(File.join(real_path, "appsignal.log"))
expect(log_file_path).to eq(File.join(real_path, "appsignal.log"))
end
end
end
Expand Down

0 comments on commit 226a8f5

Please sign in to comment.