From 1fb25ddb76f8d305d1a79c7e394e020a45a60424 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Wed, 26 Jan 2022 16:53:58 -0800 Subject: [PATCH] Hide public profiler instance --- lib/datadog/core/configuration.rb | 3 +- lib/datadog/profiling.rb | 12 +++++ lib/datadog/profiling/preload.rb | 2 +- lib/datadog/profiling/tasks/setup.rb | 2 +- lib/ddtrace/auto_instrument.rb | 2 +- spec/datadog/core/configuration_spec.rb | 2 +- spec/datadog/profiling/preload_spec.rb | 13 +---- spec/datadog/profiling/tasks/setup_spec.rb | 9 ++-- spec/{ddtrace => datadog}/profiling_spec.rb | 58 +++++++++++++++++++++ spec/ddtrace/auto_instrument_spec.rb | 17 +----- 10 files changed, 80 insertions(+), 40 deletions(-) rename spec/{ddtrace => datadog}/profiling_spec.rb (81%) diff --git a/lib/datadog/core/configuration.rb b/lib/datadog/core/configuration.rb index a5a676c9798..be1db7b69ca 100644 --- a/lib/datadog/core/configuration.rb +++ b/lib/datadog/core/configuration.rb @@ -153,8 +153,7 @@ def configuration_for(target, option = nil) def_delegators \ :components, - :health_metrics, - :profiler + :health_metrics def logger # avoid initializing components if they didn't already exist diff --git a/lib/datadog/profiling.rb b/lib/datadog/profiling.rb index ac3a3448a74..6b3c22911ef 100644 --- a/lib/datadog/profiling.rb +++ b/lib/datadog/profiling.rb @@ -82,6 +82,18 @@ def self.configuration ) end + # Starts the profiler, if the profiler is supported by in + # this runtime environment and if the profiler has been enabled + # in configuration. + # + # @return [Boolean] `true` if the profiler has successfully started, otherwise `false`. + # @public_api + def self.start_if_enabled + # Getting the profiler instance triggers start as a side-effect; + # otherwise we get nil + !!Datadog.send(:components).profiler + end + private_class_method def self.ruby_engine_unsupported? 'JRuby is not supported' if RUBY_ENGINE == 'jruby' end diff --git a/lib/datadog/profiling/preload.rb b/lib/datadog/profiling/preload.rb index 61cd81a7e6f..99dbbfefdf8 100644 --- a/lib/datadog/profiling/preload.rb +++ b/lib/datadog/profiling/preload.rb @@ -1,4 +1,4 @@ # typed: strict require 'ddtrace' -Datadog.profiler.start if Datadog.profiler +Datadog::Profiling.start_if_enabled diff --git a/lib/datadog/profiling/tasks/setup.rb b/lib/datadog/profiling/tasks/setup.rb index be8379028ce..72ac7802900 100644 --- a/lib/datadog/profiling/tasks/setup.rb +++ b/lib/datadog/profiling/tasks/setup.rb @@ -61,7 +61,7 @@ def setup_at_fork_hooks Thread.current.send(:update_native_ids) if Thread.current.respond_to?(:update_native_ids, true) # Restart profiler, if enabled - Datadog.profiler.start if Datadog.profiler + Datadog::Profiling.start_if_enabled rescue StandardError => e Datadog.logger.warn do "Error during post-fork hooks. Cause: #{e.message} Location: #{Array(e.backtrace).first}" diff --git a/lib/ddtrace/auto_instrument.rb b/lib/ddtrace/auto_instrument.rb index 09d1374dacd..3731c58c6c1 100644 --- a/lib/ddtrace/auto_instrument.rb +++ b/lib/ddtrace/auto_instrument.rb @@ -5,4 +5,4 @@ require 'ddtrace' Datadog.add_auto_instrument -Datadog.profiler.start if Datadog.profiler +Datadog::Profiling.start_if_enabled diff --git a/spec/datadog/core/configuration_spec.rb b/spec/datadog/core/configuration_spec.rb index 6a48e374233..ec963d8d981 100644 --- a/spec/datadog/core/configuration_spec.rb +++ b/spec/datadog/core/configuration_spec.rb @@ -325,7 +325,7 @@ it 'starts the profiler' do configure - expect(test_class.profiler).to have_received(:start) + expect(test_class.send(:components).profiler).to have_received(:start) end end end diff --git a/spec/datadog/profiling/preload_spec.rb b/spec/datadog/profiling/preload_spec.rb index d84448a1f43..9759bcb26c9 100644 --- a/spec/datadog/profiling/preload_spec.rb +++ b/spec/datadog/profiling/preload_spec.rb @@ -6,19 +6,8 @@ subject(:preload) { load 'datadog/profiling/preload.rb' } it 'starts the profiler' do - profiler = instance_double(Datadog::Profiling::Profiler) - - expect(Datadog).to receive(:profiler).and_return(profiler).at_least(:once) - expect(profiler).to receive(:start) + expect(Datadog::Profiling).to receive(:start_if_enabled) preload end - - context 'when the profiler is not available' do - it 'does not raise any error' do - expect(Datadog).to receive(:profiler).and_return(nil) - - preload - end - end end diff --git a/spec/datadog/profiling/tasks/setup_spec.rb b/spec/datadog/profiling/tasks/setup_spec.rb index 4e34b99b803..0c0b228162e 100644 --- a/spec/datadog/profiling/tasks/setup_spec.rb +++ b/spec/datadog/profiling/tasks/setup_spec.rb @@ -151,7 +151,7 @@ context 'when Process#at_fork is available' do before do allow(Process).to receive(:respond_to?).with(:at_fork).and_return(true) - allow(Datadog).to receive(:profiler) + allow(Datadog::Profiling).to receive(:start_if_enabled) without_partial_double_verification do allow(Process).to receive(:at_fork) @@ -173,17 +173,14 @@ end it 'sets up an at_fork hook that restarts the profiler' do - profiler = instance_double(Datadog::Profiling::Profiler) - - expect(Datadog).to receive(:profiler).and_return(profiler).at_least(:once) - expect(profiler).to receive(:start) + expect(Datadog::Profiling).to receive(:start_if_enabled) at_fork_hook.call end context 'when there is an issue starting the profiler' do before do - expect(Datadog).to receive(:profiler).and_raise('Dummy exception') + expect(Datadog::Profiling).to receive(:start_if_enabled).and_raise('Dummy exception') allow(Datadog.logger).to receive(:warn) # Silence logging during tests end diff --git a/spec/ddtrace/profiling_spec.rb b/spec/datadog/profiling_spec.rb similarity index 81% rename from spec/ddtrace/profiling_spec.rb rename to spec/datadog/profiling_spec.rb index 63e842eaecf..80c5427899a 100644 --- a/spec/ddtrace/profiling_spec.rb +++ b/spec/datadog/profiling_spec.rb @@ -5,6 +5,64 @@ RSpec.describe Datadog::Profiling do extend ConfigurationHelpers + describe '.configure' do + subject(:configure) { described_class.configure(&block) } + let(:block) { proc {} } + + it 'delegates to the global configuration' do + expect(Datadog).to receive(:internal_configure) { |&b| expect(b).to be_a_kind_of(Proc) } + configure + end + + context 'validation' do + it 'wraps the configuration object with a proxy' do + described_class.configure do |c| + expect(c).to be_a_kind_of(Datadog::Core::Configuration::ValidationProxy::Profiling) + end + end + + it 'allows profiling options' do + described_class.configure do |c| + expect(c).to respond_to(:profiling) + end + end + + it 'raises errors for non-profiling options' do + described_class.configure do |c| + expect(c).to_not respond_to(:tracer) + end + end + end + end + + describe '.configuration' do + subject(:configuration) { described_class.configuration } + it 'returns the global configuration' do + expect(configuration) + .to be_a_kind_of(Datadog::Core::Configuration::ValidationProxy::Profiling) + + expect(configuration.send(:settings)).to eq(Datadog.send(:internal_configuration)) + end + end + + describe '.start_if_enabled' do + subject(:start_if_enabled) { described_class.start_if_enabled } + + before do + allow(Datadog.send(:components)).to receive(:profiler).and_return(result) + end + + context 'with the profiler instance available' do + let(:result) { double } + it { is_expected.to be(true) } + end + + context 'with the profiler instance not available' do + let(:result) { nil } + it { is_expected.to be(false) } + end + end + describe '::supported?' do subject(:supported?) { described_class.supported? } diff --git a/spec/ddtrace/auto_instrument_spec.rb b/spec/ddtrace/auto_instrument_spec.rb index 5089084614b..f51d5dcbef4 100644 --- a/spec/ddtrace/auto_instrument_spec.rb +++ b/spec/ddtrace/auto_instrument_spec.rb @@ -112,22 +112,7 @@ def migrate_db end it 'starts the profiler' do - skip 'Profiling is not supported on JRuby' if PlatformHelpers.jruby? - skip 'Profiling is not supported on TruffleRuby' if PlatformHelpers.truffleruby? - - profiler = instance_double('Datadog::Profiling::Profiler') - - expect(Datadog).to receive(:profiler).and_return(profiler).at_least(:once) - expect(profiler).to receive(:start) - + expect(Datadog::Profiling).to receive(:start_if_enabled) auto_instrument end - - context 'when the profiler is not available' do - it 'does not raise any error' do - expect(Datadog).to receive(:profiler).and_return(nil) - - auto_instrument - end - end end