Merge pull request #4406 from DataDog/ivoanjo/prof-11385-gvl-profiling-ga

[PROF-11385] Enable GVL profiling by default on Ruby 3.2+

Author: ivoanjo

.gitlab/benchmarks.yml

@@ -104,14 +104,6 @@ only-profiling-heap:
     DD_PROFILING_EXPERIMENTAL_HEAP_ENABLED: "true"
     ADD_TO_GEMFILE: "gem 'datadog', github: 'datadog/dd-trace-rb', ref: '$CI_COMMIT_SHA'"
 
-only-profiling-gvl:
-  extends: .benchmarks
-  variables:
-    DD_BENCHMARKS_CONFIGURATION: only-profiling
-    DD_PROFILING_ENABLED: "true"
-    DD_PROFILING_PREVIEW_GVL_ENABLED: "true"
-    ADD_TO_GEMFILE: "gem 'datadog', github: 'datadog/dd-trace-rb', ref: '$CI_COMMIT_SHA'"
-
 profiling-and-tracing:
   extends: .benchmarks
   variables:

lib/datadog/core/configuration/settings.rb

@@ -463,15 +463,31 @@ def initialize(*_)
               end
             end
 
-            # Enables GVL profiling. This will show when threads are waiting for GVL in the timeline view.
-            #
-            # This is a preview feature and disabled by default. It requires Ruby 3.2+.
-            #
-            # @default `DD_PROFILING_PREVIEW_GVL_ENABLED` environment variable as a boolean, otherwise `false`
+            # @deprecated Use {:gvl_enabled} instead.
             option :preview_gvl_enabled do |o|
               o.type :bool
-              o.env 'DD_PROFILING_PREVIEW_GVL_ENABLED'
               o.default false
+              o.after_set do |_, _, precedence|
+                unless precedence == Datadog::Core::Configuration::Option::Precedence::DEFAULT
+                  Datadog.logger.warn(
+                    'The profiling.advanced.preview_gvl_enabled setting has been deprecated for removal and ' \
+                    'no longer does anything. Please remove it from your Datadog.configure block. ' \
+                    'GVL profiling is now controlled by the profiling.advanced.gvl_enabled setting instead.'
+                  )
+                end
+              end
+            end
+
+            # Controls GVL profiling. This will show when threads are waiting for GVL in the timeline view.
+            #
+            # This feature requires Ruby 3.2+.
+            #
+            # @default `DD_PROFILING_GVL_ENABLED` environment variable as a boolean, otherwise `true`
+            option :gvl_enabled do |o|
+              o.type :bool
+              o.deprecated_env 'DD_PROFILING_PREVIEW_GVL_ENABLED'
+              o.env 'DD_PROFILING_GVL_ENABLED'
+              o.default true
             end
 
             # Controls the smallest time period the profiler will report a thread waiting for the GVL.

lib/datadog/profiling/component.rb

@@ -432,17 +432,11 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:,
       end
 
       private_class_method def self.enable_gvl_profiling?(settings, logger)
-        if RUBY_VERSION < "3.2"
-          if settings.profiling.advanced.preview_gvl_enabled
-            logger.warn("GVL profiling is currently not supported in Ruby < 3.2 and will not be enabled.")
-          end
-
-          return false
-        end
+        return false if RUBY_VERSION < "3.2"
 
         # GVL profiling only makes sense in the context of timeline. We could emit a warning here, but not sure how
         # useful it is -- if a customer disables timeline, there's nowhere to look for GVL profiling anyway!
-        settings.profiling.advanced.timeline_enabled && settings.profiling.advanced.preview_gvl_enabled
+        settings.profiling.advanced.timeline_enabled && settings.profiling.advanced.gvl_enabled
       end
     end
   end

spec/datadog/core/configuration/settings_spec.rb

@@ -688,18 +688,50 @@
         end
       end
 
-      describe '#preview_gvl_enabled' do
-        subject(:preview_gvl_enabled) { settings.profiling.advanced.preview_gvl_enabled }
+      describe '#preview_gvl_enabled=' do
+        it 'logs a warning informing customers this no longer does anything' do
+          expect(Datadog.logger).to receive(:warn).with(/no longer does anything/)
 
-        it_behaves_like 'a binary setting with', env_variable: 'DD_PROFILING_PREVIEW_GVL_ENABLED', default: false
+          settings.profiling.advanced.preview_gvl_enabled = true
+        end
       end
 
-      describe '#preview_gvl_enabled=' do
-        it 'updates the #preview_gvl_enabled setting' do
-          expect { settings.profiling.advanced.preview_gvl_enabled = true }
-            .to change { settings.profiling.advanced.preview_gvl_enabled }
-            .from(false)
-            .to(true)
+      describe '#gvl_enabled' do
+        subject(:gvl_enabled) { settings.profiling.advanced.gvl_enabled }
+
+        it_behaves_like 'a binary setting with', env_variable: 'DD_PROFILING_GVL_ENABLED', default: true
+
+        context 'when DD_PROFILING_PREVIEW_GVL_ENABLED' do
+          around do |example|
+            ClimateControl.modify('DD_PROFILING_PREVIEW_GVL_ENABLED' => environment) do
+              example.run
+            end
+          end
+
+          context 'is not defined' do
+            let(:environment) { nil }
+
+            it { is_expected.to be true }
+          end
+
+          [true, false].each do |value|
+            context "is defined as #{value}" do
+              let(:environment) { value.to_s }
+
+              before { expect(Datadog::Core).to receive(:log_deprecation) }
+
+              it { is_expected.to be value }
+            end
+          end
+        end
+      end
+
+      describe '#gvl_enabled=' do
+        it 'updates the #gvl_enabled setting' do
+          expect { settings.profiling.advanced.gvl_enabled = false }
+            .to change { settings.profiling.advanced.gvl_enabled }
+            .from(true)
+            .to(false)
         end
       end
 

spec/datadog/profiling/component_spec.rb

@@ -636,7 +636,7 @@
 
       context "when GVL profiling is requested" do
         before do
-          settings.profiling.advanced.preview_gvl_enabled = true
+          settings.profiling.advanced.gvl_enabled = true
           # This triggers a warning in some Rubies so it's easier for testing to disable it
           settings.profiling.advanced.gc_enabled = false
         end
@@ -645,19 +645,11 @@
           before { skip "Behavior does not apply to current Ruby version" if RUBY_VERSION >= "3.2." }
 
           it "does not enable GVL profiling" do
-            allow(logger).to receive(:warn)
-
             expect(Datadog::Profiling::Collectors::CpuAndWallTimeWorker)
               .to receive(:new).with(hash_including(gvl_profiling_enabled: false))
 
             build_profiler_component
           end
-
-          it "logs a warning" do
-            expect(logger).to receive(:warn).with(/GVL profiling is currently not supported/)
-
-            build_profiler_component
-          end
         end
 
         context "on Ruby >= 3.2" do
@@ -686,6 +678,23 @@
           end
         end
       end
+
+      context "when GVL profiling is disabled" do
+        before do
+          settings.profiling.advanced.gvl_enabled = false
+        end
+
+        context "on Ruby >= 3.2" do
+          before { skip "On Ruby < 3.2 you can't enable the feature, it's always disabled" if RUBY_VERSION < "3.2." }
+
+          it "disables GVL profiling" do
+            expect(Datadog::Profiling::Collectors::CpuAndWallTimeWorker)
+              .to receive(:new).with(hash_including(gvl_profiling_enabled: false))
+
+            build_profiler_component
+          end
+        end
+      end
     end
   end