Enable GC profiling when it's enabled in an app (#879)

When the app calls `GC::Profiler.enable` we will now automatically collect the Garbage Collection time for the Ruby VM magic dashboard. We check if an app has enabled the GC profiler by calling `GC::Profiler.enabled?`. This operation is only checking a boolean value in Ruby, which is also much faster the previous implementation of checking the now removed `enable_gc_instrumentation` config option. Previously we had the undocumented `enable_gc_instrumentation` config option, which called `GC::Profiler.enable`. We removed this in PR #876. We want to give developers more flexibility and allow to enable and disable it at will. This way they can choose to only profile certain parts of their app without having it enabled on app start. Part of #868
appsignal · Aug 10, 2022 · af7e666 · af7e666
1 parent c9ed88d
commit af7e666
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 7 deletions.
diff --git a/.changesets/listen-if-garbage-collection-is-enabled.md b/.changesets/listen-if-garbage-collection-is-enabled.md
@@ -0,0 +1,6 @@
+---
+bump: "patch"
+type: "change"
+---
+
+Listen if the Ruby Garbage Collection profiler is enabled and collect how long the GC is running for the Ruby VM magic dashboard. An app will need to call `GC::Profiler.enable` to enable the GC profiler. Do not enable this in production environments, or at least not for long, because this can negatively impact performance of apps.
diff --git a/lib/appsignal/garbage_collection.rb b/lib/appsignal/garbage_collection.rb
@@ -5,19 +5,34 @@ module Appsignal
   module GarbageCollection
     # Return the GC profiler wrapper.
     #
-    # Temporarily always returns the {NilProfiler}.
+    # Returns {Profiler} if the Ruby Garbage Collection profiler is enabled.
+    # This is checked by calling `GC::Profiler.enabled?`.
     #
     # GC profiling is disabled by default due to the overhead it causes. Do not
     # enable this in production for long periods of time.
     def self.profiler
-      @profiler ||= NilProfiler.new
+      # Cached instances so it doesn't create a new object every time this
+      # method is called. Especially necessary for the {Profiler} because a new
+      # instance will have a new internal time counter.
+      @real_profiler ||= Profiler.new
+      @nil_profiler ||= NilProfiler.new
+
+      enabled? ? @real_profiler : @nil_profiler
+    end
+
+    # Check if Garbage Collection is enabled at the moment.
+    #
+    # @return [Boolean]
+    def self.enabled?
+      GC::Profiler.enabled?
     end
 
-    # Unset the currently cached profiler
+    # Unset the currently cached profilers.
     #
     # @return [void]
     def self.clear_profiler!
-      @profiler = nil
+      @real_profiler = nil
+      @nil_profiler = nil
     end
 
     # A wrapper around Ruby's `GC::Profiler` that tracks garbage collection

diff --git a/lib/appsignal/probes/mri.rb b/lib/appsignal/probes/mri.rb
@@ -31,7 +31,7 @@ def call
         )
 
         set_gauge("thread_count", Thread.list.size)
-        if GC::Profiler.enabled?
+        if Appsignal::GarbageCollection.enabled?
           gauge_delta(:gc_time, @gc_profiler.total_time) do |gc_time|
             set_gauge("gc_time", gc_time) if gc_time > 0
           end

diff --git a/spec/lib/appsignal/garbage_collection_spec.rb b/spec/lib/appsignal/garbage_collection_spec.rb
@@ -12,8 +12,11 @@
     end
 
     context "when GC profiling is enabled" do
+      before { GC::Profiler.enable }
+      after { GC::Profiler.disable }
+
       it "returns the Profiler" do
-        expect(described_class.profiler).to be_a(Appsignal::GarbageCollection::NilProfiler)
+        expect(described_class.profiler).to be_a(Appsignal::GarbageCollection::Profiler)
       end
     end
   end

diff --git a/spec/lib/appsignal/probes/mri_spec.rb b/spec/lib/appsignal/probes/mri_spec.rb
@@ -81,7 +81,7 @@ def set_gauge(*args) # rubocop:disable Naming/AccessorMethodName
           expect(metrics).to_not include("gc_time")
         end
 
-        it "does not report a gc_time metric while disable" do
+        it "does not report a gc_time metric while temporarily disabled" do
           # While enabled
           allow(GC::Profiler).to receive(:enabled?).and_return(true)
           expect(gc_profiler_mock).to receive(:total_time).and_return(10, 15)