Fix Sinatra instrumentation causing boot loop (#1105)

I noticed in our test setup that when I added a Sinatra app to the Rails app, and loaded the Sinatra instrumentation as specified in our docs it would get stuck in a boot loop. This was caused by the two different configs overwriting each other every time. Prevent this issue by skipping loading the AppSignal config if it's already active.
appsignal · Jun 21, 2024 · 2478eb1 · 2478eb1
1 parent 7382afa
commit 2478eb1
Show file tree

Hide file tree

Showing 3 changed files with 73 additions and 40 deletions.
diff --git a/.changesets/don-t-start-for-sinatra-if-already-started.md b/.changesets/don-t-start-for-sinatra-if-already-started.md
@@ -0,0 +1,7 @@
+---
+bump: patch
+type: fix
+---
+
+Fix issue with AppSignal getting stuck in a boot loop when loading the Sinatra integration with: `require "appsignal/integrations/sinatra"`
+This could happen in nested applications, like a Sinatra app in a Rails app. It will now use the first config AppSignal starts with.
diff --git a/lib/appsignal/integrations/sinatra.rb b/lib/appsignal/integrations/sinatra.rb
@@ -5,13 +5,15 @@
 
 Appsignal.internal_logger.debug("Loading Sinatra (#{Sinatra::VERSION}) integration")
 
-app_settings = ::Sinatra::Application.settings
-Appsignal.config = Appsignal::Config.new(
-  app_settings.root || Dir.pwd,
-  app_settings.environment
-)
+unless Appsignal.active?
+  app_settings = ::Sinatra::Application.settings
+  Appsignal.config = Appsignal::Config.new(
+    app_settings.root || Dir.pwd,
+    app_settings.environment
+  )
 
-Appsignal.start_logger
-Appsignal.start
+  Appsignal.start_logger
+  Appsignal.start
+end
 
 ::Sinatra::Base.use(Appsignal::Rack::SinatraBaseInstrumentation) if Appsignal.active?
diff --git a/spec/lib/appsignal/integrations/sinatra_spec.rb b/spec/lib/appsignal/integrations/sinatra_spec.rb
@@ -13,59 +13,83 @@ def uninstall_sinatra_integration
   end
 
   describe "Sinatra integration" do
-    before { allow(Appsignal).to receive(:active?).and_return(true) }
+    before do
+      Appsignal.config = nil
+    end
     after { uninstall_sinatra_integration }
 
-    context "Appsignal.internal_logger" do
-      subject { Appsignal.internal_logger }
+    context "when active" do
+      before { allow(Appsignal).to receive(:active?).and_return(true) }
+
+      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
 
-      it "sets a logger" do
+      it "adds the instrumentation middleware to Sinatra::Base" do
         install_sinatra_integration
-        is_expected.to be_a Logger
+        expect(Sinatra::Base.middleware.to_a).to include(
+          [Appsignal::Rack::SinatraBaseInstrumentation, [], nil]
+        )
       end
     end
 
-    describe "middleware" do
-      context "when AppSignal is not active" do
-        before { allow(Appsignal).to receive(:active?).and_return(false) }
+    context "when not active" do
+      context "Appsignal.internal_logger" do
+        subject { Appsignal.internal_logger }
 
-        it "does not add the instrumentation middleware to Sinatra::Base" do
+        it "sets a logger" do
           install_sinatra_integration
-          expect(Sinatra::Base.middleware.to_a).to_not include(
-            [Appsignal::Rack::SinatraBaseInstrumentation, [], nil]
-          )
+          is_expected.to be_a Logger
         end
       end
 
-      context "when AppSignal is active" do
-        it "adds the instrumentation middleware to Sinatra::Base" do
-          install_sinatra_integration
-          expect(Sinatra::Base.middleware.to_a).to include(
-            [Appsignal::Rack::SinatraBaseInstrumentation, [], nil]
-          )
+      describe "middleware" do
+        context "when AppSignal is not active" do
+          it "does not add the instrumentation middleware to Sinatra::Base" do
+            install_sinatra_integration
+            expect(Sinatra::Base.middleware.to_a).to_not include(
+              [Appsignal::Rack::SinatraBaseInstrumentation, [], nil]
+            )
+          end
         end
-      end
-    end
-
-    describe "environment" do
-      subject { Appsignal.config.env }
 
-      context "without APPSIGNAL_APP_ENV" do
-        before { install_sinatra_integration }
+        context "when the new AppSignal config is active" do
+          it "adds the instrumentation middleware to Sinatra::Base" do
+            ENV["APPSIGNAL_APP_NAME"] = "My Sinatra app name"
+            ENV["APPSIGNAL_APP_ENV"] = "test"
+            ENV["APPSIGNAL_PUSH_API_KEY"] = "my-key"
 
-        it "uses the app environment" do
-          expect(subject).to eq("test")
+            install_sinatra_integration
+            expect(Sinatra::Base.middleware.to_a).to include(
+              [Appsignal::Rack::SinatraBaseInstrumentation, [], nil]
+            )
+          end
         end
       end
 
-      context "with APPSIGNAL_APP_ENV" do
-        before do
-          ENV["APPSIGNAL_APP_ENV"] = "env-staging"
-          install_sinatra_integration
+      describe "environment" do
+        subject { Appsignal.config.env }
+
+        context "without APPSIGNAL_APP_ENV" do
+          before { install_sinatra_integration }
+
+          it "uses the app environment" do
+            expect(subject).to eq("test")
+          end
         end
 
-        it "uses the environment variable" do
-          expect(subject).to eq("env-staging")
+        context "with APPSIGNAL_APP_ENV" do
+          before do
+            ENV["APPSIGNAL_APP_ENV"] = "env-staging"
+            install_sinatra_integration
+          end
+
+          it "uses the environment variable" do
+            expect(subject).to eq("env-staging")
+          end
         end
       end
     end