Merge pull request #143 from Shopify/start_backend

Add ability to switch backends using start/stop APIs

Gemfile

@@ -6,9 +6,10 @@ gemspec
 # Specify the same dependency sources as the application Gemfile
 gem("activesupport", "~> 5.2")
 gem("railties", "~> 5.2")
-gem("vernier", "~> 0.7.0") if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("3.2.1")
+gem("vernier", "~> 1.3.1") if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("3.2.1")
 
 gem("google-cloud-storage", "~> 1.21")
 gem("rubocop", require: false)
 gem("rubocop-shopify", require: false)
 gem("rubocop-performance", require: false)
+gem("debug")

Gemfile.lock

@@ -33,6 +33,9 @@ GEM
     builder (3.2.4)
     concurrent-ruby (1.1.6)
     crass (1.0.6)
+    debug (1.9.2)
+      irb (~> 1.10)
+      reline (>= 0.3.8)
     declarative (0.0.20)
     digest-crc (0.5.1)
     erubi (1.9.0)
@@ -103,6 +106,10 @@ GEM
     httpclient (2.8.3)
     i18n (1.8.2)
       concurrent-ruby (~> 1.0)
+    io-console (0.7.2)
+    irb (1.14.1)
+      rdoc (>= 4.0.0)
+      reline (>= 0.4.2)
     json (2.7.2)
     jwt (2.3.0)
     language_server-protocol (3.17.0.3)
@@ -126,6 +133,8 @@ GEM
     parser (3.3.3.0)
       ast (~> 2.4.1)
       racc
+    psych (5.2.0)
+      stringio
     public_suffix (4.0.6)
     racc (1.8.0)
     rack (2.2.9)
@@ -145,7 +154,11 @@ GEM
       thor (>= 0.19.0, < 2.0)
     rainbow (3.1.1)
     rake (13.0.1)
+    rdoc (6.8.1)
+      psych (>= 4.0.0)
     regexp_parser (2.9.2)
+    reline (0.5.11)
+      io-console (~> 0.5)
     representable (3.1.1)
       declarative (< 0.1.0)
       trailblazer-option (>= 0.1.1, < 0.2.0)
@@ -178,6 +191,7 @@ GEM
       jwt (>= 1.5, < 3.0)
       multi_json (~> 1.10)
     stackprof (0.2.26)
+    stringio (3.1.2)
     strscan (3.1.0)
     thor (1.0.1)
     thread_safe (0.3.6)
@@ -186,7 +200,7 @@ GEM
       thread_safe (~> 0.1)
     uber (0.1.0)
     unicode-display_width (2.5.0)
-    vernier (0.7.0)
+    vernier (1.3.1)
     webrick (1.7.0)
 
 PLATFORMS
@@ -196,6 +210,7 @@ DEPENDENCIES
   activesupport (~> 5.2)
   app_profiler!
   bundler
+  debug
   google-cloud-storage (~> 1.21)
   minitest
   minitest-stub-const (= 0.6)
@@ -205,7 +220,7 @@ DEPENDENCIES
   rubocop
   rubocop-performance
   rubocop-shopify
-  vernier (~> 0.7.0)
+  vernier (~> 1.3.1)
 
 BUNDLED WITH
    2.4.18

lib/app_profiler.rb

@@ -76,50 +76,46 @@ def deprecator # :nodoc:
     end
 
     def run(*args, backend: nil, **kwargs, &block)
-      orig_backend = self.backend
-      begin
-        self.backend = backend if backend
-        profiler.run(*args, **kwargs, &block)
-      rescue BackendError => e
-        logger.error(
-          "[AppProfiler.run] exception #{e} configuring backend #{backend}: #{e.message}",
-        )
-        yield
+      if backend
+        original_backend = self.backend
+        self.backend = backend
       end
+      profiler.run(*args, **kwargs, &block)
+    rescue BackendError => e
+      logger.error(
+        "[AppProfiler.run] exception #{e} configuring backend #{backend}: #{e.message}",
+      )
+      yield
     ensure
-      AppProfiler.backend = orig_backend
+      self.backend = original_backend if backend
     end
 
-    def start(*args)
-      profiler.start(*args)
+    def start(*args, backend: nil, **kwargs)
+      self.backend = backend if backend
+      profiler.start(*args, **kwargs)
     end
 
     def stop
       profiler.stop
-      profiler.results
+      profiler.results.tap { clear }
     end
 
     def running?
-      @backend&.running?
+      @profiler&.running?
     end
 
     def profiler
-      backend
-      @backend ||= @profiler_backend.new
+      @profiler ||= profiler_backend.new
     end
 
     def backend=(new_backend)
-      return if new_backend == backend
-
-      new_profiler_backend = backend_for(new_backend)
+      return if (new_profiler_backend = backend_for(new_backend)) == profiler_backend
 
       if running?
         raise BackendError,
           "cannot change backend to #{new_backend} while #{backend} backend is running"
       end
 
-      return if @profiler_backend == new_profiler_backend
-
       clear
       @profiler_backend = new_profiler_backend
     end
@@ -156,22 +152,21 @@ def profile_sampler_enabled
 
     def backend_for(backend_name)
       if vernier_supported? &&
-          backend_name == AppProfiler::Backend::VernierBackend.name
+          backend_name&.to_sym == AppProfiler::Backend::VernierBackend.name
         AppProfiler::Backend::VernierBackend
-      elsif backend_name == AppProfiler::Backend::StackprofBackend.name
+      elsif backend_name&.to_sym == AppProfiler::Backend::StackprofBackend.name
         AppProfiler::Backend::StackprofBackend
       else
-        raise BackendError, "unknown backend #{backend_name}"
+        raise BackendError, "unknown backend #{backend_name.inspect}"
       end
     end
 
     def backend
-      @profiler_backend ||= Backend::StackprofBackend
-      @profiler_backend.name
+      profiler_backend.name
     end
 
     def vernier_supported?
-      defined?(AppProfiler::Backend::VernierBackend.name)
+      RUBY_VERSION >= "3.2.1"
     end
 
     def profile_header=(profile_header)
@@ -234,9 +229,14 @@ def viewer=(viewer)
 
     private
 
+    def profiler_backend
+      @profiler_backend ||= Backend::StackprofBackend
+    end
+
     def clear
-      @backend.stop if @backend&.running?
-      @backend = nil
+      profiler.stop if running?
+      @profiler = nil
+      @profiler_backend = nil
     end
   end
 

lib/app_profiler/railtie.rb

@@ -45,7 +45,7 @@ class Railtie < Rails::Railtie
       AppProfiler.profile_enqueue_success = app.config.app_profiler.profile_enqueue_success
       AppProfiler.profile_enqueue_failure = app.config.app_profiler.profile_enqueue_failure
       AppProfiler.after_process_queue = app.config.app_profiler.after_process_queue
-      AppProfiler.backend = app.config.app_profiler.profiler_backend || :stackprof
+      AppProfiler.backend = app.config.app_profiler.profiler_backend || :stackprof unless AppProfiler.running?
       AppProfiler.forward_metadata_on_upload = app.config.app_profiler.forward_metadata_on_upload || false
       AppProfiler.profile_sampler_enabled = app.config.app_profiler.profile_sampler_enabled || false
       AppProfiler.profile_sampler_config = app.config.app_profiler.profile_sampler_config ||

test/app_profiler/backend/stackprof_backend_test.rb

@@ -6,9 +6,14 @@ module AppProfiler
   module Backend
     class StackprofBackendTest < TestCase
       def setup
+        @original_backend = AppProfiler.backend
         AppProfiler.backend = :stackprof
       end
 
+      def teardown
+        AppProfiler.backend = @original_backend
+      end
+
       test ".run prints error when failed" do
         AppProfiler.logger.expects(:info).with { |value| value =~ /failed to start the profiler/ }
         profile = AppProfiler.run(mode: :unsupported) do

test/app_profiler/backend/vernier_backend_test.rb

@@ -8,12 +8,12 @@ module AppProfiler
   module Backend
     class VernierBackendTest < TestCase
       def setup
-        @orig_backend = AppProfiler.backend
+        @original_backend = AppProfiler.backend
         AppProfiler.backend = :vernier
       end
 
       def teardown
-        AppProfiler.backend = @orig_backend
+        AppProfiler.backend = @original_backend
       end
 
       test ".run prints error when failed" do
@@ -60,7 +60,7 @@ def teardown
 
         assert_instance_of(AppProfiler::VernierProfile, profile)
         assert_equal(:wall, profile[:meta][:mode])
-        # assert_equal(1000, profile[:interval]) # TODO https://github.com/jhawthorn/vernier/issues/30
+        assert_equal(1, profile[:meta][:interval])
       end
 
       test ".run assigns metadata to profiles" do
@@ -163,12 +163,11 @@ def teardown
       end
 
       test ".stop" do
-        AppProfiler.start
-        AppProfiler::Backend::VernierBackend.any_instance.expects(:stop)
-        AppProfiler.stop
-      ensure
-        AppProfiler::Backend::VernierBackend.any_instance.unstub(:stop)
-        AppProfiler.stop
+        Vernier::Collector.any_instance.expects(:start)
+        Vernier::Collector.any_instance.expects(:stop)
+
+        AppProfiler.profiler.start
+        AppProfiler.profiler.stop
       end
 
       test ".results prints error when failed" do

test/app_profiler/backend_test.rb

@@ -40,7 +40,7 @@ class BackendTest < TestCase
       AppProfiler.backend = orig_backend
     end
 
-    test ".backend_for= provides the backend class given a string" do
+    test ".backend_for provides the backend class given a string" do
       assert_equal(
         AppProfiler::Backend::StackprofBackend,
         AppProfiler.backend_for(AppProfiler::Backend::StackprofBackend.name),
@@ -53,7 +53,7 @@ class BackendTest < TestCase
       )
     end
 
-    test ".backend_for= raises if an unknown backend is requested" do
+    test ".backend_for raises if an unknown backend is requested" do
       assert_raises(BackendError) { AppProfiler.backend_for("not a real backend") }
     end