Skip to content

Commit

Permalink
Hide public profiler instance
Browse files Browse the repository at this point in the history
  • Loading branch information
marcotc committed Jan 27, 2022
1 parent a948598 commit 8f6708a
Show file tree
Hide file tree
Showing 17 changed files with 90 additions and 50 deletions.
4 changes: 2 additions & 2 deletions benchmarks/profiler_sample_loop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ def create_profiler
end

# Stop background threads
Datadog.profiler.shutdown!
Datadog.shutdown!

# Call collection directly
@stack_collector = Datadog.profiler.collectors.first
@stack_collector = Datadog.send(:components).profiler.collectors.first
@recorder = @stack_collector.recorder
end

Expand Down
4 changes: 2 additions & 2 deletions benchmarks/profiler_submission.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ def create_profiler
end

# Stop background threads
Datadog.profiler.shutdown!
Datadog.shutdown!

# Call exporter directly
@exporter = Datadog.profiler.scheduler.exporters.first
@exporter = Datadog.send(:components).profiler.scheduler.exporters.first
@flush = Marshal.load(
Zlib::GzipReader.new(File.open(ENV['FLUSH_DUMP_FILE'] || 'benchmarks/data/profiler-submission-marshal.gz'))
)
Expand Down
4 changes: 2 additions & 2 deletions docs/ProfilingDevelopment.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ Components below live inside <../lib/datadog/profiling>:
When started via `ddtracerb exec` (together with `DD_PROFILING_ENABLED=true`), initialization goes through the following
flow:

1. <../lib/datadog/profiling/preload.rb> triggers the creation of the `Datadog.profiler` instance by calling the method
2. `Datadog.profiler` is handled by `Datadog::Configuration`, which triggers the configuration of `ddtrace` components
1. <../lib/datadog/profiling/preload.rb> triggers the creation of the profiler instance by calling the method `Datadog::Profiling.start_if_enabled`
2. The profiler instance is handled by `Datadog::Configuration`, which triggers the configuration of `ddtrace` components
in `#build_components`
3. Inside `Datadog::Components`, the `build_profiler` method triggers the execution of the `Tasks::Setup`
4. The `Setup` task activates our extensions
Expand Down
2 changes: 1 addition & 1 deletion integration/apps/rack/app/acme.rb
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ def check(request)
def detailed_check(request)
['200', { 'Content-Type' => 'application/json'}, [JSON.pretty_generate(
webserver_process: $PROGRAM_NAME,
profiler_available: !!Datadog.profiler,
profiler_available: Datadog::Profiling.start_if_enabled,
# NOTE: Threads can't be named on Ruby 2.1 and 2.2
profiler_threads: ((Thread.list.map(&:name).select { |it| it && it.include?('Profiling') }) unless RUBY_VERSION < '2.3')
)], "\n"]
Expand Down
2 changes: 1 addition & 1 deletion integration/apps/rack/app/resque_background_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def self.perform(key, value)
key: key,
value: value,
resque_process: $PROGRAM_NAME,
profiler_available: !!Datadog.profiler,
profiler_available: Datadog::Profiling.start_if_enabled,
# NOTE: Threads can't be named on Ruby 2.1 and 2.2
profiler_threads: ((Thread.list.map(&:name).select { |it| it && it.include?('Profiling') }) unless RUBY_VERSION < '2.3')
))
Expand Down
2 changes: 1 addition & 1 deletion integration/apps/rack/app/sidekiq_background_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def perform(key, value)
key: key,
value: value,
sidekiq_process: $PROGRAM_NAME,
profiler_available: !!Datadog.profiler,
profiler_available: Datadog::Profiling.start_if_enabled,
# NOTE: Threads can't be named on Ruby 2.1 and 2.2
profiler_threads: ((Thread.list.map(&:name).select { |it| it && it.include?('Profiling') }) unless RUBY_VERSION < '2.3')
))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def check
def detailed_check
render json: {
webserver_process: $PROGRAM_NAME,
profiler_available: !!Datadog.profiler,
profiler_available: Datadog::Profiling.start_if_enabled,
profiler_threads: Thread.list.map(&:name).select { |it| it && it.include?('Profiling') }
}
end
Expand Down
3 changes: 1 addition & 2 deletions lib/datadog/core/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions lib/datadog/profiling.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/datadog/profiling/preload.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# typed: strict
require 'ddtrace'

Datadog.profiler.start if Datadog.profiler
Datadog::Profiling.start_if_enabled
2 changes: 1 addition & 1 deletion lib/datadog/profiling/tasks/setup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
2 changes: 1 addition & 1 deletion lib/ddtrace/auto_instrument.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@
require 'ddtrace'

Datadog.add_auto_instrument
Datadog.profiler.start if Datadog.profiler
Datadog::Profiling.start_if_enabled
2 changes: 1 addition & 1 deletion spec/datadog/core/configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 1 addition & 12 deletions spec/datadog/profiling/preload_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
9 changes: 3 additions & 6 deletions spec/datadog/profiling/tasks/setup_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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

Expand Down
58 changes: 58 additions & 0 deletions spec/ddtrace/profiling_spec.rb → spec/datadog/profiling_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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? }

Expand Down
17 changes: 1 addition & 16 deletions spec/ddtrace/auto_instrument_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 8f6708a

Please sign in to comment.