Merge pull request #2702 from DataDog/ivoanjo/prof-7360-cpu-profiling…

…-2.0-ga

[PROF-7360] Enable CPU Profiling 2.0 by default

benchmarks/profiler_sample_loop.rb

@@ -21,6 +21,7 @@ def create_profiler
       c.profiling.enabled = true
       c.profiling.exporter.transport = MockProfilerTransport.new
       c.tracing.transport_options = proc { |t| t.adapter :test }
+      c.profiling.advanced.force_enable_legacy_profiler = true
     end
 
     # Stop background threads

ext/ddtrace_profiling_native_extension/private_vm_api_access.c

@@ -19,6 +19,8 @@
   #pragma GCC diagnostic push
   #pragma GCC diagnostic ignored "-Wunused-parameter"
   #pragma GCC diagnostic ignored "-Wattributes"
+  #pragma GCC diagnostic ignored "-Wpragmas"
+  #pragma GCC diagnostic ignored "-Wexpansion-to-defined"
     #include <vm_core.h>
   #pragma GCC diagnostic pop
   #include <iseq.h>
@@ -651,6 +653,7 @@ check_method_entry(VALUE obj, int can_be_svar)
         if (can_be_svar) {
             return check_method_entry(((struct vm_svar *)obj)->cref_or_me, FALSE);
         }
+        // fallthrough
       default:
 #if VM_CHECK_MODE > 0
         rb_bug("check_method_entry: svar should not be there:");

integration/apps/rack/spec/integration/basic_spec.rb

@@ -14,7 +14,18 @@
   let(:expected_profiler_available) { RUBY_VERSION >= '2.3' }
 
   let(:expected_profiler_threads) do
-    contain_exactly('Datadog::Profiling::Collectors::OldStack', 'Datadog::Profiling::Scheduler') unless RUBY_VERSION < '2.3'
+    if RUBY_VERSION >= '2.6.'
+      contain_exactly(
+        'Datadog::Profiling::Collectors::IdleSamplingHelper',
+        'Datadog::Profiling::Collectors::CpuAndWallTimeWorker',
+        'Datadog::Profiling::Scheduler',
+      )
+    elsif RUBY_VERSION >= '2.3'
+      contain_exactly(
+        'Datadog::Profiling::Collectors::OldStack',
+        'Datadog::Profiling::Scheduler',
+      )
+    end
   end
 
   context 'component checks' do

integration/apps/sinatra2-classic/spec/integration/basic_spec.rb

@@ -14,7 +14,18 @@
   let(:expected_profiler_available) { RUBY_VERSION >= '2.3' }
 
   let(:expected_profiler_threads) do
-    contain_exactly('Datadog::Profiling::Collectors::OldStack', 'Datadog::Profiling::Scheduler') unless RUBY_VERSION < '2.3'
+    if RUBY_VERSION >= '2.6.'
+      contain_exactly(
+        'Datadog::Profiling::Collectors::IdleSamplingHelper',
+        'Datadog::Profiling::Collectors::CpuAndWallTimeWorker',
+        'Datadog::Profiling::Scheduler',
+      )
+    elsif RUBY_VERSION >= '2.3'
+      contain_exactly(
+        'Datadog::Profiling::Collectors::OldStack',
+        'Datadog::Profiling::Scheduler',
+      )
+    end
   end
 
   context 'component checks' do

integration/apps/sinatra2-modular/spec/integration/basic_spec.rb

@@ -14,7 +14,18 @@
   let(:expected_profiler_available) { RUBY_VERSION >= '2.3' }
 
   let(:expected_profiler_threads) do
-    contain_exactly('Datadog::Profiling::Collectors::OldStack', 'Datadog::Profiling::Scheduler') unless RUBY_VERSION < '2.3'
+    if RUBY_VERSION >= '2.6.'
+      contain_exactly(
+        'Datadog::Profiling::Collectors::IdleSamplingHelper',
+        'Datadog::Profiling::Collectors::CpuAndWallTimeWorker',
+        'Datadog::Profiling::Scheduler',
+      )
+    elsif RUBY_VERSION >= '2.3'
+      contain_exactly(
+        'Datadog::Profiling::Collectors::OldStack',
+        'Datadog::Profiling::Scheduler',
+      )
+    end
   end
 
   context 'component checks' do

lib/datadog/core/configuration/settings.rb

@@ -202,10 +202,10 @@ def initialize(*_)
           # @public_api
           settings :advanced do
             # This should never be reduced, as it can cause the resulting profiles to become biased.
-            # The current default should be enough for most services, allowing 16 threads to be sampled around 30 times
+            # The default should be enough for most services, allowing 16 threads to be sampled around 30 times
             # per second for a 60 second period.
             #
-            # This setting is ignored when CPU Profiling 2.0 is in use.
+            # @deprecated This setting is ignored when CPU Profiling 2.0 is in use.
             option :max_events, default: 32768
 
             # Controls the maximum number of frames for each thread sampled. Can be tuned to avoid omitted frames in the
@@ -236,7 +236,7 @@ def initialize(*_)
             # grouping and categorization of stack traces.
             option :code_provenance_enabled, default: true
 
-            # No longer does anything, and will be removed on dd-trace-rb 2.0.
+            # @deprecated No longer does anything, and will be removed on dd-trace-rb 2.0.
             #
             # This was added as a temporary support option in case of issues with the new `Profiling::HttpTransport` class
             # but we're now confident it's working nicely so we've removed the old code path.
@@ -252,28 +252,39 @@ def initialize(*_)
             # Forces enabling the new CPU Profiling 2.0 profiler (see ddtrace release notes for more details).
             #
             # Note that setting this to "false" (or not setting it) will not prevent the new profiler from
-            # being automatically used in the future.
-            # This option will be deprecated for removal once the new profiler gets enabled by default for all customers.
+            # being automatically used.
+            # This option will be deprecated for removal once the legacy profiler is removed.
             #
             # @default `DD_PROFILING_FORCE_ENABLE_NEW` environment variable, otherwise `false`
             option :force_enable_new_profiler do |o|
               o.default { env_to_bool('DD_PROFILING_FORCE_ENABLE_NEW', false) }
               o.lazy
             end
 
+            # Forces enabling the *legacy* (non-CPU Profiling 2.0 profiler) even when it would otherwise NOT be enabled.
+            #
+            # Temporarily added to ease migration to the new CPU Profiling 2.0 profiler, and will be removed soon.
+            # Do not use unless instructed to by support.
+            # This option will be deprecated for removal once the legacy profiler is removed.
+            #
+            # @default `DD_PROFILING_FORCE_ENABLE_LEGACY` environment variable, otherwise `false`
+            option :force_enable_legacy_profiler do |o|
+              o.default { env_to_bool('DD_PROFILING_FORCE_ENABLE_LEGACY', false) }
+              o.lazy
+            end
+
             # Forces enabling of profiling of time/resources spent in Garbage Collection.
             #
             # Note that setting this to "false" (or not setting it) will not prevent the feature from being
             # being automatically enabled in the future.
             #
-            # This toggle was added because, although this feature is safe and enabled by default on Ruby 2.x,
-            # on Ruby 3.x it can break in applications that make use of Ractors due to two Ruby VM bugs:
-            # https://bugs.ruby-lang.org/issues/19112 AND https://bugs.ruby-lang.org/issues/18464.
-            #
-            # If you use Ruby 3.x and your application does not use Ractors (or if your Ruby has been patched), the
-            # feature is fully safe to enable and this toggle can be used to do so.
-            #
-            # Furthermore, currently this feature can add a lot of overhead for GC-heavy workloads.
+            # This feature defaults to off for two reasons:
+            # 1. Currently this feature can add a lot of overhead for GC-heavy workloads.
+            # 2. Although this feature is safe on Ruby 2.x, on Ruby 3.x it can break in applications that make use of
+            #    Ractors due to two Ruby VM bugs:
+            #    https://bugs.ruby-lang.org/issues/19112 AND https://bugs.ruby-lang.org/issues/18464.
+            #    If you use Ruby 3.x and your application does not use Ractors (or if your Ruby has been patched), the
+            #    feature is fully safe to enable and this toggle can be used to do so.
             #
             # We expect the once the above issues are overcome, we'll automatically enable the feature on fixed Ruby
             # versions.
@@ -292,6 +303,8 @@ def initialize(*_)
             #
             # If you use Ruby 3.x and your application does not use Ractors (or if your Ruby has been patched), the
             # feature is fully safe to enable and this toggle can be used to do so.
+            #
+            # @default `true` on Ruby 2.x, `false` on Ruby 3.x
             option :allocation_counting_enabled, default: RUBY_VERSION.start_with?('2.')
           end
 

lib/datadog/profiling/component.rb

@@ -64,7 +64,7 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:)
 
         # NOTE: Please update the Initialization section of ProfilingDevelopment.md with any changes to this method
 
-        if settings.profiling.advanced.force_enable_new_profiler
+        if enable_new_profiler?(settings)
           print_new_profiler_warnings
 
           recorder = Datadog::Profiling::StackRecorder.new(
@@ -142,21 +142,43 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:)
       end
 
       private_class_method def self.print_new_profiler_warnings
-        if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.6')
+        return if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.6')
+
+        # For more details on the issue, see the "BIG Issue" comment on `gvl_owner` function in
+        # `private_vm_api_access.c`.
+        Datadog.logger.warn(
+          'The new CPU Profiling 2.0 profiler has been force-enabled on a legacy Ruby version (< 2.6). This is not ' \
+          'recommended in production environments, as due to limitations in Ruby APIs, we suspect it may lead to crashes ' \
+          'in very rare situations. Please report any issues you run into to Datadog support or ' \
+          'via <https://github.com/datadog/dd-trace-rb/issues/new>!'
+        )
+      end
+
+      private_class_method def self.enable_new_profiler?(settings)
+        if settings.profiling.advanced.force_enable_legacy_profiler
           Datadog.logger.warn(
-            'New Ruby profiler has been force-enabled. This is a beta feature. Please report any issues ' \
-            'you run into to Datadog support or via <https://github.com/datadog/dd-trace-rb/issues/new>!'
+            'Legacy profiler has been force-enabled via configuration. Do not use unless instructed to by support.'
           )
-        else
-          # For more details on the issue, see the "BIG Issue" comment on `gvl_owner` function in
-          # `private_vm_api_access.c`.
+          return false
+        end
+
+        return true if settings.profiling.advanced.force_enable_new_profiler
+
+        return false if RUBY_VERSION.start_with?('2.3.', '2.4.', '2.5.')
+
+        if Gem.loaded_specs['mysql2']
           Datadog.logger.warn(
-            'New Ruby profiler has been force-enabled on a legacy Ruby version (< 2.6). This is not recommended in ' \
-            'production environments, as due to limitations in Ruby APIs, we suspect it may lead to crashes in very ' \
-            'rare situations. Please report any issues you run into to Datadog support or ' \
-            'via <https://github.com/datadog/dd-trace-rb/issues/new>!'
+            'Falling back to legacy profiler because mysql2 gem is installed. Older versions of libmysqlclient (the C ' \
+            'library used by the mysql2 gem) have a bug in their signal handling code that the new profiler can trigger. ' \
+            'This bug (https://bugs.mysql.com/bug.php?id=83109) is fixed in libmysqlclient versions 8.0.0 and above. ' \
+            'If your Linux distribution provides a modern libmysqlclient, you can force-enable the new CPU Profiling 2.0 ' \
+            'profiler by using the `DD_PROFILING_FORCE_ENABLE_NEW` or `c.profiling.advanced.force_enable_new_profiler` ' \
+            'settings.'
           )
+          return false
         end
+
+        true
       end
     end
   end

lib/datadog/profiling/tasks/setup.rb

@@ -11,7 +11,6 @@ class Setup
         def run
           ACTIVATE_EXTENSIONS_ONLY_ONCE.run do
             begin
-              check_if_cpu_time_profiling_is_supported
               activate_forking_extensions
               setup_at_fork_hooks
             rescue StandardError, ScriptError => e
@@ -38,17 +37,6 @@ def activate_forking_extensions
           end
         end
 
-        def check_if_cpu_time_profiling_is_supported
-          unsupported = cpu_time_profiling_unsupported_reason
-
-          if unsupported
-            Datadog.logger.info do
-              'CPU time profiling skipped because native CPU time is not supported: ' \
-              "#{unsupported}. Profiles containing 'Wall time' data will still be reported."
-            end
-          end
-        end
-
         def setup_at_fork_hooks
           if Process.respond_to?(:at_fork)
             Process.at_fork(:child) do
@@ -64,20 +52,6 @@ def setup_at_fork_hooks
             end
           end
         end
-
-        def cpu_time_profiling_unsupported_reason
-          # NOTE: Only the first matching reason is returned, so try to keep a nice order on reasons
-
-          if RUBY_ENGINE == 'jruby'
-            'JRuby is not supported'
-          elsif RUBY_PLATFORM.include?('darwin')
-            'Feature requires Linux; macOS is not supported'
-          elsif RUBY_PLATFORM =~ /(mswin|mingw)/
-            'Feature requires Linux; Windows is not supported'
-          elsif !RUBY_PLATFORM.include?('linux')
-            "Feature requires Linux; #{RUBY_PLATFORM} is not supported"
-          end
-        end
       end
     end
   end

sig/datadog/profiling/component.rbs

@@ -28,6 +28,8 @@ module Datadog
       def self.enable_gc_profiling?: (untyped settings) -> bool
 
       def self.print_new_profiler_warnings: () -> void
+
+      def self.enable_new_profiler?: (untyped settings) -> bool
     end
   end
 end

spec/datadog/core/configuration/settings_spec.rb

@@ -460,6 +460,41 @@
         end
       end
 
+      describe '#force_enable_legacy_profiler' do
+        subject(:force_enable_legacy_profiler) { settings.profiling.advanced.force_enable_legacy_profiler }
+
+        context 'when DD_PROFILING_FORCE_ENABLE_LEGACY' do
+          around do |example|
+            ClimateControl.modify('DD_PROFILING_FORCE_ENABLE_LEGACY' => environment) do
+              example.run
+            end
+          end
+
+          context 'is not defined' do
+            let(:environment) { nil }
+
+            it { is_expected.to be false }
+          end
+
+          { 'true' => true, 'false' => false }.each do |string, value|
+            context "is defined as #{string}" do
+              let(:environment) { string }
+
+              it { is_expected.to be value }
+            end
+          end
+        end
+      end
+
+      describe '#force_enable_legacy_profiler=' do
+        it 'updates the #force_enable_legacy_profiler setting' do
+          expect { settings.profiling.advanced.force_enable_legacy_profiler = true }
+            .to change { settings.profiling.advanced.force_enable_legacy_profiler }
+            .from(false)
+            .to(true)
+        end
+      end
+
       describe '#force_enable_gc_profiling' do
         subject(:force_enable_gc_profiling) { settings.profiling.advanced.force_enable_gc_profiling }