Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PROF-8667] Split profiling tests into ractor and non-ractor suites. #3320

Merged
merged 6 commits into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading