From 0b8012e85fb8d8e600ee6beed1f6d040369bb227 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 19 Feb 2025 09:27:22 +0000 Subject: [PATCH] [PROF-11385] Enable GVL profiling by default on Ruby 3.2+ **What does this PR do?** This PR graduates the GVL profiling feature from preview to GA, and turns it on by default. This means the old `preview_gvl_enabled` setting and its corresponding `DD_PROFILING_PREVIEW_GVL_ENABLED` environment variable are now deprecated. **Motivation:** Roll out this cool feature to customers ;) **Change log entry** Yes. Enable GVL profiling by default on Ruby 3.2+ Also, here's a snippet for the release highlights: ```markdown ### GVL Profiling is now enabled by default on Ruby 3.2+ GVL profiling means the profiler gathers information from threads waiting to acquire the Ruby "Global VM Lock" (GVL). This waiting can be a big a source of latency for Ruby applications: a thread "Waiting on the GVL" is a thread that's ready to make progress, but can't start because Ruby is busy doing something else. For more details on why GVL profiling is relevant, check out [How the Ruby Global VM Lock impacts app performance](https://www.youtube.com/watch?v=MmKhsvvyiCw) and https://github.com/DataDog/dd-trace-rb/pull/3929. ``` **Additional Notes:** N/A **How to test the change?** This change includes test coverage. --- .gitlab/benchmarks.yml | 8 --- lib/datadog/core/configuration/settings.rb | 28 ++++++++--- lib/datadog/profiling/component.rb | 10 +--- .../core/configuration/settings_spec.rb | 50 +++++++++++++++---- spec/datadog/profiling/component_spec.rb | 27 ++++++---- 5 files changed, 83 insertions(+), 40 deletions(-) diff --git a/.gitlab/benchmarks.yml b/.gitlab/benchmarks.yml index 2ced6f01622..9ca5d641dab 100644 --- a/.gitlab/benchmarks.yml +++ b/.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: diff --git a/lib/datadog/core/configuration/settings.rb b/lib/datadog/core/configuration/settings.rb index 5b873c0c979..9ec489fc3f5 100644 --- a/lib/datadog/core/configuration/settings.rb +++ b/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. diff --git a/lib/datadog/profiling/component.rb b/lib/datadog/profiling/component.rb index 666aefce061..83d737a3b71 100644 --- a/lib/datadog/profiling/component.rb +++ b/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 diff --git a/spec/datadog/core/configuration/settings_spec.rb b/spec/datadog/core/configuration/settings_spec.rb index beebf143951..99b8e34e4f5 100644 --- a/spec/datadog/core/configuration/settings_spec.rb +++ b/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 diff --git a/spec/datadog/profiling/component_spec.rb b/spec/datadog/profiling/component_spec.rb index ab314976d1f..3d80591366f 100644 --- a/spec/datadog/profiling/component_spec.rb +++ b/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