appsignal-ruby icon indicating copy to clipboard operation
appsignal-ruby copied to clipboard

Only log the unwritable log directory message once

Open tombruijn opened this issue 3 years ago • 1 comments

When the configured or default log directory is not writable the following message can be printed multiple times, whenever the Appsignal::Config#log_file_path method is called:

appsignal: Unable to log to '/Users/tombruijn/appsignal/agent/integrations/appsignal-ruby/log'. Logging to '/private/tmp' instead. Please check the permissions for the application's (log) directory.
appsignal: Unable to log to '/Users/tombruijn/appsignal/agent/integrations/appsignal-ruby/log'. Logging to '/private/tmp' instead. Please check the permissions for the application's (log) directory.
appsignal: Unable to log to '/Users/tombruijn/appsignal/agent/integrations/appsignal-ruby/log'. Logging to '/private/tmp' instead. Please check the permissions for the application's (log) directory.
appsignal: Unable to log to '/Users/tombruijn/appsignal/agent/integrations/appsignal-ruby/log'. Logging to '/private/tmp' instead. Please check the permissions for the application's (log) directory.

We should only print this message once.

tombruijn avatar Dec 01 '21 09:12 tombruijn

I tried this before and ran into some issues with the test suite, to assert this message is only printed once. My work so far:

diff --git lib/appsignal/config.rb lib/appsignal/config.rb
index 6c5a7fee..1d3fa9e8 100644
--- lib/appsignal/config.rb
+++ lib/appsignal/config.rb
@@ -240,9 +240,12 @@ module Appsignal
     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")
       if path && File.writable?(path)
-        return File.join(File.realpath(path), "appsignal.log")
+        @log_file_path = File.join(File.realpath(path), "appsignal.log")
+        return @log_file_path
       end
 
       system_tmp_dir = self.class.system_tmp_dir
@@ -250,7 +253,7 @@ module Appsignal
         $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")
+        @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 "\
diff --git spec/lib/appsignal/config_spec.rb spec/lib/appsignal/config_spec.rb
index 8eee2bb2..5b711050 100644
--- spec/lib/appsignal/config_spec.rb
+++ spec/lib/appsignal/config_spec.rb
@@ -581,7 +581,10 @@ describe Appsignal::Config do
     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") }
@@ -589,11 +592,11 @@ describe Appsignal::Config do
       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
@@ -607,13 +610,17 @@ describe Appsignal::Config do
         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
-          expect(output).to include "appsignal: Unable to log to '#{log_path}'. "\
+        it "prints a warning once" do
+          capture_stdout(out_stream) do
+            config.log_file_path
+            config.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
 
@@ -621,13 +628,22 @@ describe Appsignal::Config 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
-          expect(output).to include "appsignal: Unable to log to '#{log_path}' "\
-            "or the '#{system_tmp_dir}' fallback."
+        it "prints a warning once", :only do
+          puts "\n\n\n"
+          capture_stdout(out_stream) do
+            config.log_file_path
+            config.log_file_path
+          end
+          puts output
+          message = "appsignal: Unable to log to '#{log_path}'. "\
+            "Logging to '#{system_tmp_dir}' instead."
+          puts "=" * 80
+          puts message
+          puts "-" * 80
+          expect(output.scan(message).count).to eq(1)
         end
       end
     end
@@ -643,11 +659,11 @@ describe Appsignal::Config do
 
       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
@@ -707,7 +723,7 @@ describe Appsignal::Config do
           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

tombruijn avatar Dec 01 '21 09:12 tombruijn