Skip to content

Commit

Permalink
[PROF-8667] Split profiling tests into ractor and non-ractor suites. (#…
Browse files Browse the repository at this point in the history
…3320)

Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>
  • Loading branch information
AlexJF and ivoanjo committed Dec 13, 2023
1 parent 9dc80c8 commit 14b40d4
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 38 deletions.
50 changes: 43 additions & 7 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ TEST_METADATA = {
'appsec:main' => {
'' => '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ✅ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ jruby'
},
'profiling:main' => {
'' => '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ✅ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ jruby'
},
'profiling:ractors' => {
'' => '❌ 2.1 / ❌ 2.2 / ❌ 2.3 / ❌ 2.4 / ❌ 2.5 / ❌ 2.6 / ❌ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ jruby'
},
'contrib' => {
'' => '✅ 2.1 / ✅ 2.2 / ✅ 2.3 / ✅ 2.4 / ✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ jruby'
},
Expand Down Expand Up @@ -323,20 +329,16 @@ desc 'Run RSpec'
namespace :spec do
task all: [:main, :benchmark,
:rails, :railsredis, :railsredis_activesupport, :railsactivejob,
:elasticsearch, :http, :redis, :sidekiq, :sinatra, :hanami, :hanami_autoinstrument]
:elasticsearch, :http, :redis, :sidekiq, :sinatra, :hanami, :hanami_autoinstrument,
:profiling]

desc '' # "Explicitly hiding from `rake -T`"
RSpec::Core::RakeTask.new(:main) do |t, args|
t.pattern = 'spec/**/*_spec.rb'
t.exclude_pattern = 'spec/**/{contrib,benchmark,redis,opentracer,auto_instrument,opentelemetry}/**/*_spec.rb,'\
t.exclude_pattern = 'spec/**/{contrib,benchmark,redis,opentracer,auto_instrument,opentelemetry,profiling}/**/*_spec.rb,'\
' spec/**/{auto_instrument,opentelemetry}_spec.rb'
t.rspec_opts = args.to_a.join(' ')
end
if RUBY_ENGINE == 'ruby' && OS.linux? && Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.3.0')
# "bundle exec rake compile" currently only works on MRI Ruby on Linux
Rake::Task[:main].enhance([:clean])
Rake::Task[:main].enhance([:compile])
end

RSpec::Core::RakeTask.new(:benchmark) do |t, args|
t.pattern = 'spec/ddtrace/benchmark/**/*_spec.rb'
Expand Down Expand Up @@ -535,6 +537,40 @@ namespace :spec do
end

task appsec: [:'appsec:all']

namespace :profiling do
task all: [:main, :ractors]

task :compile_native_extensions do
# "bundle exec rake compile" currently only works on MRI Ruby on Linux
if RUBY_ENGINE == 'ruby' && OS.linux? && Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.3.0')
Rake::Task[:clean].invoke
Rake::Task[:compile].invoke
end
end

# Datadog Profiling main specs without Ractor creation
# NOTE: Ractor creation will transition the entire Ruby VM into multi-ractor mode. This cannot be undone
# and, as such, may introduce side-effects between tests and make them flaky depending on order of
# execution. By splitting in two separate suites, the side-effect impact should be mitigated as
# the non-ractor VM will never trigger the transition into multi-ractor mode.
desc '' # "Explicitly hiding from `rake -T`"
RSpec::Core::RakeTask.new(:main) do |t, args|
t.pattern = 'spec/datadog/profiling/**/*_spec.rb,spec/datadog/profiling_spec.rb'
t.rspec_opts = [*args.to_a, '-t ~ractors'].join(' ')
end

desc '' # "Explicitly hiding from `rake -T`"
RSpec::Core::RakeTask.new(:ractors) do |t, args|
t.pattern = 'spec/datadog/profiling/**/*_spec.rb'
t.rspec_opts = [*args.to_a, '-t ractors'].join(' ')
end

# Make sure each profiling test suite has a dependency on compiled native extensions
Rake::Task[:all].prerequisite_tasks.each { |t| t.enhance([:compile_native_extensions]) }
end

task profiling: [:'profiling:all']
end

if defined?(RuboCop::RakeTask)
Expand Down
15 changes: 10 additions & 5 deletions spec/datadog/core/configuration/components_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1070,18 +1070,23 @@
end

context 'is enabled' do
before do
skip 'Profiling not supported.' unless Datadog::Profiling.supported?
# Using a generic double rather than instance_double since if profiling is not supported by the
# current CI runner we won't even load the Datadog::Profiling::Profiler class.
let(:profiler) { instance_double('Datadog::Profiling::Profiler') }

before do
allow(settings.profiling)
.to receive(:enabled)
.and_return(true)
allow(profiler_setup_task).to receive(:run)
expect(Datadog::Profiling::Component).to receive(:build_profiler_component).with(
settings: settings,
agent_settings: agent_settings,
optional_tracer: anything,
).and_return(profiler)
end

it do
expect(components.profiler)
.to receive(:start)
expect(profiler).to receive(:start)

startup!
end
Expand Down
24 changes: 0 additions & 24 deletions spec/datadog/core/configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -316,30 +316,6 @@
end
end

context 'when the profiler' do
context 'is not changed' do
before { skip_if_profiling_not_supported(self) }

context 'and profiling is enabled' do
before do
allow(test_class.configuration.profiling)
.to receive(:enabled)
.and_return(true)

allow_any_instance_of(Datadog::Profiling::Profiler)
.to receive(:start)
allow_any_instance_of(Datadog::Profiling::Tasks::Setup)
.to receive(:run)
end

it 'starts the profiler' do
configure
expect(test_class.send(:components).profiler).to have_received(:start)
end
end
end
end

context 'when reconfigured multiple times' do
context 'with runtime metrics active' do
before do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@
end
end

context 'when called from a background ractor' do
context 'when called from a background ractor', :ractors => true do
# Even though we're not testing it explicitly, the GC profiling hooks can sometimes be called when running these
# specs. Unfortunately, there's a VM crash in that case as well -- https://bugs.ruby-lang.org/issues/18464 --
# so this must be disabled when interacting with Ractors.
Expand Down
2 changes: 1 addition & 1 deletion spec/datadog/profiling/native_extension_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
it { is_expected.to be true }
end

context 'on a background Ractor' do
context 'on a background Ractor', :ractors => true do
# @ivoanjo: When we initially added this test, our test suite kept deadlocking in CI in a later test (not on
# this one).
#
Expand Down

0 comments on commit 14b40d4

Please sign in to comment.