Deprecate Appsignal.start_logger (#1119)

appsignal · Jun 26, 2024 · 1f648ab · 1f648ab
1 parent 850ab9a
commit 1f648ab
Show file tree

Hide file tree

Showing 12 changed files with 71 additions and 43 deletions.
diff --git a/.changesets/deprecate-appsignal-start_logger.md b/.changesets/deprecate-appsignal-start_logger.md
@@ -0,0 +1,6 @@
+---
+bump: patch
+type: deprecate
+---
+
+Deprecate the `Appsignal.start_logger` method. Remove this method call from apps if it is present. Calling `Appsignal.start` will now initialize the logger.
diff --git a/lib/appsignal.rb b/lib/appsignal.rb
@@ -62,7 +62,7 @@ class << self
     #     defaults are pointed to this attribute.
     #   @api private
     #   @return [Logger]
-    #   @see start_logger
+    #   @see start
     attr_writer :internal_logger
 
     # @api private
@@ -84,8 +84,6 @@ def testing?
     # AppSignal](https://docs.appsignal.com/ruby/instrumentation/integrating-appsignal.html)
     # guide.
     #
-    # To start the logger see {.start_logger}.
-    #
     # @example
     #   Appsignal.start
     #
@@ -108,8 +106,9 @@ def start
         ENV["APPSIGNAL_APP_ENV"] || ENV["RAILS_ENV"] || ENV.fetch("RACK_ENV", nil)
       )
 
+      _start_logger
+
       if config.valid?
-        internal_logger.level = config.log_level
         if config.active?
           internal_logger.info "Starting AppSignal #{Appsignal::VERSION} " \
             "(#{$PROGRAM_NAME}, Ruby #{RUBY_VERSION}, #{RUBY_PLATFORM})"
@@ -160,7 +159,7 @@ def stop(called_by = nil)
     def forked
       return unless active?
 
-      Appsignal.start_logger
+      Appsignal._start_logger
       internal_logger.debug("Forked process, resubscribing and restarting extension")
       Appsignal::Extension.start
     end
@@ -170,9 +169,9 @@ def get_server_state(key)
     end
 
     # In memory internal logger used before any internal logger is started with
-    # {.start_logger}.
+    # {._start_logger}.
     #
-    # The contents of this logger are flushed to the logger in {.start_logger}.
+    # The contents of this logger are flushed to the logger in {._start_logger}.
     #
     # @api private
     # @return [StringIO]
@@ -201,14 +200,26 @@ def log_formatter(prefix = nil)
       end
     end
 
+    # @deprecated Only {.start} has to be called.
+    # @return [void]
+    # @since 0.7.0
+    def start_logger
+      callers = caller
+      Appsignal::Utils::StdoutAndLoggerMessage.warning \
+        "Callng 'Appsignal.start_logger' is deprecated. " \
+          "The logger will be started when calling 'Appsignal.start'. " \
+          "Remove the 'Appsignal.start_logger' call in the following file to " \
+          "remove this message.\n#{callers.first}"
+    end
+
     # Start the AppSignal internal logger.
     #
     # Sets the log level and sets the logger. Uses a file-based logger or the
     # STDOUT-based logger. See the `:log` configuration option.
     #
+    # @api private
     # @return [void]
-    # @since 0.7.0
-    def start_logger
+    def _start_logger
       if config && config[:log] == "file" && config.log_file_path
         start_internal_file_logger(config.log_file_path)
       else

diff --git a/lib/appsignal/cli/diagnose.rb b/lib/appsignal/cli/diagnose.rb
@@ -200,7 +200,7 @@ def configure_appsignal(options)
             initial_config
           )
           Appsignal.config.write_to_environment
-          Appsignal.start_logger
+          Appsignal._start_logger
           Appsignal.internal_logger.info("Starting AppSignal diagnose")
         end
 

diff --git a/lib/appsignal/config.rb b/lib/appsignal/config.rb
@@ -233,7 +233,7 @@ class Config
     #   variables config.
     # @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_logger}.
+    #   {Appsignal.internal_logger}. See also {Appsignal.start}.
     # @param config_file [String] Custom config file location. Default
     #   `config/appsignal.yml`.
     #

diff --git a/lib/appsignal/integrations/hanami.rb b/lib/appsignal/integrations/hanami.rb
@@ -16,7 +16,6 @@ def self.init
           hanami_app_config.env
         )
 
-        Appsignal.start_logger
         Appsignal.start
 
         return unless Appsignal.active?

diff --git a/lib/appsignal/integrations/padrino.rb b/lib/appsignal/integrations/padrino.rb
@@ -12,7 +12,6 @@ def self.init
         root = Padrino.mounted_root
         Appsignal.config = Appsignal::Config.new(root, Padrino.env)
 
-        Appsignal.start_logger
         Appsignal.start
       end
     end

diff --git a/lib/appsignal/integrations/railtie.rb b/lib/appsignal/integrations/railtie.rb
@@ -26,9 +26,6 @@ def self.initialize_appsignal(app)
           :log_path => Rails.root.join("log")
         )
 
-        # Start logger
-        Appsignal.start_logger
-
         app.middleware.insert(
           0,
           ::Rack::Events,

diff --git a/lib/appsignal/integrations/sinatra.rb b/lib/appsignal/integrations/sinatra.rb
@@ -12,7 +12,6 @@
     app_settings.environment
   )
 
-  Appsignal.start_logger
   Appsignal.start
 end
 

diff --git a/spec/lib/appsignal/integrations/hanami_spec.rb b/spec/lib/appsignal/integrations/hanami_spec.rb
@@ -19,7 +19,6 @@ def uninstall_hanami_middleware
     describe Appsignal::Integrations::HanamiPlugin do
       it "starts AppSignal on init" do
         expect(Appsignal).to receive(:start)
-        expect(Appsignal).to receive(:start_logger)
         Appsignal::Integrations::HanamiPlugin.init
       end
 

diff --git a/spec/lib/appsignal/integrations/padrino_spec.rb b/spec/lib/appsignal/integrations/padrino_spec.rb
@@ -5,18 +5,13 @@
     before do
       allow(Appsignal).to receive(:active?).and_return(true)
       allow(Appsignal).to receive(:start).and_return(true)
-      allow(Appsignal).to receive(:start_logger).and_return(true)
     end
 
     describe Appsignal::Integrations::PadrinoPlugin do
       it "starts AppSignal on init" do
         expect(Appsignal).to receive(:start)
       end
 
-      it "starts the logger on init" do
-        expect(Appsignal).to receive(:start_logger)
-      end
-
       context "when not active" do
         before { allow(Appsignal).to receive(:active?).and_return(false) }
 

diff --git a/spec/lib/appsignal/integrations/sinatra_spec.rb b/spec/lib/appsignal/integrations/sinatra_spec.rb
@@ -24,7 +24,6 @@ def uninstall_sinatra_integration
       it "does not start AppSignal again" do
         expect(Appsignal::Config).to_not receive(:new)
         expect(Appsignal).to_not receive(:start)
-        expect(Appsignal).to_not receive(:start_logger)
         install_sinatra_integration
       end
 

diff --git a/spec/lib/appsignal_spec.rb b/spec/lib/appsignal_spec.rb
@@ -21,21 +21,30 @@
 
   describe ".start" do
     context "with no config set beforehand" do
-      it "should do nothing when config is not set and there is no valid config in the env" do
-        expect(Appsignal.internal_logger).to receive(:error)
-          .with("Push API key not set after loading config").once
-        expect(Appsignal.internal_logger).to receive(:error)
-          .with("Not starting, no valid config for this environment").once
+      let(:stdout_stream) { std_stream }
+      let(:stdout) { stdout_stream.read }
+      let(:stderr_stream) { std_stream }
+      let(:stderr) { stderr_stream.read }
+      before { ENV["APPSIGNAL_LOG"] = "stdout" }
+
+      it "does nothing when config is not set and there is no valid config in the env" do
         expect(Appsignal::Extension).to_not receive(:start)
-        Appsignal.start
+        capture_std_streams(stdout_stream, stderr_stream) { Appsignal.start }
+
+        expect(stdout).to contains_log(
+          :error,
+          "appsignal: Not starting, no valid config for this environment"
+        )
       end
 
       it "should create a config from the env" do
         ENV["APPSIGNAL_PUSH_API_KEY"] = "something"
         expect(Appsignal::Extension).to receive(:start)
-        expect(Appsignal.internal_logger).not_to receive(:error)
-        silence { Appsignal.start }
+        capture_std_streams(stdout_stream, stderr_stream) { Appsignal.start }
+
         expect(Appsignal.config[:push_api_key]).to eq("something")
+        expect(stderr).to_not include("[ERROR]")
+        expect(stdout).to_not include("[ERROR]")
       end
     end
 
@@ -129,7 +138,7 @@
 
   describe ".forked" do
     context "when not active" do
-      it "should should do nothing" do
+      it "does nothing" do
         expect(Appsignal::Extension).to_not receive(:start)
 
         Appsignal.forked
@@ -141,8 +150,8 @@
         Appsignal.config = project_fixture_config
       end
 
-      it "should resubscribe and start the extension" do
-        expect(Appsignal).to receive(:start_logger)
+      it "starts the logger and extension" do
+        expect(Appsignal).to receive(:_start_logger)
         expect(Appsignal::Extension).to receive(:start)
 
         Appsignal.forked
@@ -228,7 +237,6 @@
       before do
         Appsignal.config = project_fixture_config("not_active")
         Appsignal.start
-        Appsignal.start_logger
         Appsignal.internal_logger = test_logger(log_stream)
       end
       after { Appsignal.internal_logger = nil }
@@ -314,7 +322,6 @@
     before do
       Appsignal.config = project_fixture_config
       Appsignal.start
-      Appsignal.start_logger
       Appsignal.internal_logger = test_logger(log_stream)
     end
     after { Appsignal.internal_logger = nil }
@@ -1123,6 +1130,23 @@
   end
 
   describe ".start_logger" do
+    let(:stderr_stream) { std_stream }
+    let(:stderr) { stderr_stream.read }
+    let(:log_stream) { std_stream }
+    let(:log) { log_contents(log_stream) }
+
+    it "prints and logs a deprecation warning" do
+      use_logger_with(log_stream) do
+        capture_std_streams(std_stream, stderr_stream) do
+          Appsignal.start_logger
+        end
+      end
+      expect(stderr).to include("appsignal WARNING: Callng 'Appsignal.start_logger' is deprecated.")
+      expect(log).to contains_log(:warn, "Callng 'Appsignal.start_logger' is deprecated.")
+    end
+  end
+
+  describe "._start_logger" do
     let(:out_stream) { std_stream }
     let(:output) { out_stream.read }
     let(:log_path) { File.join(tmp_dir, "log") }
@@ -1154,7 +1178,7 @@ def initialize_config
         before do
           capture_stdout(out_stream) do
             initialize_config
-            Appsignal.start_logger
+            Appsignal._start_logger
             Appsignal.internal_logger.error("Log to file")
           end
           expect(Appsignal.internal_logger).to be_a(Appsignal::Utils::IntegrationLogger)
@@ -1182,7 +1206,7 @@ def initialize_config
 
           capture_stdout(out_stream) do
             initialize_config
-            Appsignal.start_logger
+            Appsignal._start_logger
             Appsignal.internal_logger.error("Log to not writable log file")
             expect(Appsignal.internal_logger).to be_a(Appsignal::Utils::IntegrationLogger)
           end
@@ -1216,7 +1240,7 @@ def initialize_config
 
         capture_stdout(out_stream) do
           initialize_config
-          Appsignal.start_logger
+          Appsignal._start_logger
           Appsignal.internal_logger.error("Log to not writable log path")
         end
         expect(Appsignal.internal_logger).to be_a(Appsignal::Utils::IntegrationLogger)
@@ -1245,7 +1269,7 @@ def initialize_config
       before do
         capture_stdout(out_stream) do
           initialize_config
-          Appsignal.start_logger
+          Appsignal._start_logger
           Appsignal.internal_logger.error("Log to stdout")
         end
         expect(Appsignal.internal_logger).to be_a(Appsignal::Utils::IntegrationLogger)
@@ -1272,7 +1296,7 @@ def initialize_config
         before do
           Appsignal.config = nil
           capture_stdout(out_stream) do
-            Appsignal.start_logger
+            Appsignal._start_logger
           end
         end
 
@@ -1287,7 +1311,7 @@ def initialize_config
             capture_stdout(out_stream) do
               initialize_config
               Appsignal.config[:log_level] = "debug"
-              Appsignal.start_logger
+              Appsignal._start_logger
             end
           end
-Original file line number
+Diff line change
@@ Expand Up / @@ -12,7 +12,6 @@ @@
         app_settings.environment
       )
-      Appsignal.start_logger
       Appsignal.start
     end
@@ Expand Down @@