From f653915f86af0f3ba6e700789214b4c3d490d7f8 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 1 Feb 2023 11:03:32 +0000 Subject: [PATCH 1/9] Disable use of profiling in Ruby 2.2 This is the first part of the work to drop support for Ruby 2.2. The support will be removed in the following commits, but from this commit on, we will no longer build, try to use, or test profiling on Ruby 2.2. --- Rakefile | 2 +- ddtrace.gemspec | 1 + .../native_extension_helpers.rb | 8 ++++---- integration/apps/rack/spec/integration/basic_spec.rb | 3 +-- .../sinatra2-classic/spec/integration/basic_spec.rb | 3 +-- .../sinatra2-modular/spec/integration/basic_spec.rb | 3 +-- .../build_ddtrace_profiling_native_extension.rb | 4 ++-- .../profiling/native_extension_helpers_spec.rb | 12 +++++++++--- spec/datadog/profiling/spec_helper.rb | 2 +- 9 files changed, 21 insertions(+), 17 deletions(-) diff --git a/Rakefile b/Rakefile index df3a8133036..eff2da8186c 100644 --- a/Rakefile +++ b/Rakefile @@ -21,7 +21,7 @@ namespace :spec do ' 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.2.0') + 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]) diff --git a/ddtrace.gemspec b/ddtrace.gemspec index 3bba5150613..c61bb3f5ec2 100644 --- a/ddtrace.gemspec +++ b/ddtrace.gemspec @@ -41,6 +41,7 @@ Gem::Specification.new do |spec| ]] .select { |fn| File.file?(fn) } # We don't want directories, only files .reject { |fn| fn.end_with?('.so', '.bundle') } # Exclude local profiler binary artifacts + .reject { |fn| fn.end_with?('skipped_reason.txt') } # Generated by profiler; should never be distributed spec.executables = ['ddtracerb'] spec.require_paths = ['lib'] diff --git a/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb b/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb index bed99b34179..11a23c90d97 100644 --- a/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb +++ b/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb @@ -88,7 +88,7 @@ def self.unsupported_reason on_macos? || on_unknown_os? || not_on_amd64_or_arm64? || - on_ruby_2_1? || + on_ruby_2_1_or_2_2? || expected_to_use_mjit_but_mjit_is_disabled? || libdatadog_not_available? || libdatadog_not_usable? @@ -260,13 +260,13 @@ def self.pkg_config_missing?(command: $PKGCONFIG) # rubocop:disable Style/Global architecture_not_supported unless RUBY_PLATFORM.start_with?('x86_64', 'aarch64', 'arm64') end - private_class_method def self.on_ruby_2_1? + private_class_method def self.on_ruby_2_1_or_2_2? ruby_version_not_supported = explain_issue( - 'the profiler only supports Ruby 2.2 or newer.', + 'the profiler only supports Ruby 2.3 or newer.', suggested: UPGRADE_RUBY, ) - ruby_version_not_supported if RUBY_VERSION.start_with?('2.1.') + ruby_version_not_supported if RUBY_VERSION.start_with?('2.1.', '2.2.') end # On some Rubies, we require the mjit header to be present. If Ruby was installed without MJIT support, we also skip diff --git a/integration/apps/rack/spec/integration/basic_spec.rb b/integration/apps/rack/spec/integration/basic_spec.rb index ffc57de99e7..ef3b9fb1bde 100644 --- a/integration/apps/rack/spec/integration/basic_spec.rb +++ b/integration/apps/rack/spec/integration/basic_spec.rb @@ -11,10 +11,9 @@ it { is_expected.to be_a_kind_of(Net::HTTPOK) } end - let(:expected_profiler_available) { RUBY_VERSION >= '2.2' } + let(:expected_profiler_available) { RUBY_VERSION >= '2.3' } let(:expected_profiler_threads) do - # NOTE: Threads can't be named on Ruby 2.2 contain_exactly('Datadog::Profiling::Collectors::OldStack', 'Datadog::Profiling::Scheduler') unless RUBY_VERSION < '2.3' end diff --git a/integration/apps/sinatra2-classic/spec/integration/basic_spec.rb b/integration/apps/sinatra2-classic/spec/integration/basic_spec.rb index 1de15eb55c9..03e05e0a0d6 100644 --- a/integration/apps/sinatra2-classic/spec/integration/basic_spec.rb +++ b/integration/apps/sinatra2-classic/spec/integration/basic_spec.rb @@ -11,10 +11,9 @@ it { is_expected.to be_a_kind_of(Net::HTTPOK) } end - let(:expected_profiler_available) { RUBY_VERSION >= '2.2' } + let(:expected_profiler_available) { RUBY_VERSION >= '2.3' } let(:expected_profiler_threads) do - # NOTE: Threads can't be named on Ruby 2.2 contain_exactly('Datadog::Profiling::Collectors::OldStack', 'Datadog::Profiling::Scheduler') unless RUBY_VERSION < '2.3' end diff --git a/integration/apps/sinatra2-modular/spec/integration/basic_spec.rb b/integration/apps/sinatra2-modular/spec/integration/basic_spec.rb index 1de15eb55c9..03e05e0a0d6 100644 --- a/integration/apps/sinatra2-modular/spec/integration/basic_spec.rb +++ b/integration/apps/sinatra2-modular/spec/integration/basic_spec.rb @@ -11,10 +11,9 @@ it { is_expected.to be_a_kind_of(Net::HTTPOK) } end - let(:expected_profiler_available) { RUBY_VERSION >= '2.2' } + let(:expected_profiler_available) { RUBY_VERSION >= '2.3' } let(:expected_profiler_threads) do - # NOTE: Threads can't be named on Ruby 2.2 contain_exactly('Datadog::Profiling::Collectors::OldStack', 'Datadog::Profiling::Scheduler') unless RUBY_VERSION < '2.3' end diff --git a/integration/images/include/build_ddtrace_profiling_native_extension.rb b/integration/images/include/build_ddtrace_profiling_native_extension.rb index 875d8222054..0dfcc711fb4 100755 --- a/integration/images/include/build_ddtrace_profiling_native_extension.rb +++ b/integration/images/include/build_ddtrace_profiling_native_extension.rb @@ -1,8 +1,8 @@ #!/usr/bin/env ruby if local_gem_path = ENV['DD_DEMO_ENV_GEM_LOCAL_DDTRACE'] - if RUBY_VERSION.start_with?('2.1.') - puts "\n== Skipping build of profiler native extension on Ruby 2.1 ==" + if (RUBY_VERSION.start_with?('2.1.') || RUBY_VERSION.start_with?('2.2.')) + puts "\n== Skipping build of profiler native extension on Ruby 2.1/2.2 ==" else puts "\n== Building profiler native extension ==" success = diff --git a/spec/datadog/profiling/native_extension_helpers_spec.rb b/spec/datadog/profiling/native_extension_helpers_spec.rb index d3c503e6884..3104f8c11b5 100644 --- a/spec/datadog/profiling/native_extension_helpers_spec.rb +++ b/spec/datadog/profiling/native_extension_helpers_spec.rb @@ -129,8 +129,8 @@ end shared_examples 'supported ruby validation' do - context 'when not on Ruby 2.1' do - before { stub_const('RUBY_VERSION', '2.2.0') } + context 'when not on Ruby 2.1 or 2.2' do + before { stub_const('RUBY_VERSION', '2.3.0') } shared_examples 'libdatadog available' do context 'when libdatadog fails to activate' do @@ -191,7 +191,13 @@ context 'when on Ruby 2.1' do before { stub_const('RUBY_VERSION', '2.1.10') } - it { is_expected.to include 'profiler only supports Ruby 2.2 or newer' } + it { is_expected.to include 'profiler only supports Ruby 2.3 or newer' } + end + + context 'when on Ruby 2.2' do + before { stub_const('RUBY_VERSION', '2.2.10') } + + it { is_expected.to include 'profiler only supports Ruby 2.3 or newer' } end end diff --git a/spec/datadog/profiling/spec_helper.rb b/spec/datadog/profiling/spec_helper.rb index 8b24c5ad19e..d2bfea17863 100644 --- a/spec/datadog/profiling/spec_helper.rb +++ b/spec/datadog/profiling/spec_helper.rb @@ -34,7 +34,7 @@ def build_stack_sample( def skip_if_profiling_not_supported(testcase) testcase.skip('Profiling is not supported on JRuby') if PlatformHelpers.jruby? testcase.skip('Profiling is not supported on TruffleRuby') if PlatformHelpers.truffleruby? - testcase.skip('Profiling is not supported on Ruby 2.1') if RUBY_VERSION.start_with?('2.1.') + testcase.skip('Profiling is not supported on Ruby 2.1/2.2') if RUBY_VERSION.start_with?('2.1.', '2.2.') # Profiling is not officially supported on macOS due to missing libdatadog binaries, # but it's still useful to allow it to be enabled for development. From 0482d73c24888624373e06e9dba8acf20fc95e01 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 1 Feb 2023 11:07:51 +0000 Subject: [PATCH 2/9] Minor comment cleanups, now that we've dropped profiling Ruby 2.2 --- ext/ddtrace_profiling_native_extension/extconf.rb | 2 +- ext/ddtrace_profiling_native_extension/ruby_helpers.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/ddtrace_profiling_native_extension/extconf.rb b/ext/ddtrace_profiling_native_extension/extconf.rb index 2e990c9256a..9757661b89a 100644 --- a/ext/ddtrace_profiling_native_extension/extconf.rb +++ b/ext/ddtrace_profiling_native_extension/extconf.rb @@ -125,7 +125,7 @@ def add_compiler_flag(flag) # have_library 'pthread' # have_func 'pthread_getcpuclockid' # ``` - # but it broke the build on Windows and on older Ruby versions (2.2) + # but a) it broke the build on Windows, b) on older Ruby versions (2.2 and below) and c) It's slower to build # so instead we just assume that we have the function we need on Linux, and nowhere else $defs << '-DHAVE_PTHREAD_GETCPUCLOCKID' end diff --git a/ext/ddtrace_profiling_native_extension/ruby_helpers.h b/ext/ddtrace_profiling_native_extension/ruby_helpers.h index 8d2f17683b7..84889fb83dd 100644 --- a/ext/ddtrace_profiling_native_extension/ruby_helpers.h +++ b/ext/ddtrace_profiling_native_extension/ruby_helpers.h @@ -12,7 +12,7 @@ static inline VALUE process_pending_interruptions(DDTRACE_UNUSED VALUE _) { return Qnil; } -// RB_UNLIKELY is not supported on Ruby 2.2 and 2.3 +// RB_UNLIKELY is not supported on Ruby 2.3 #ifndef RB_UNLIKELY #define RB_UNLIKELY(x) x #endif From b751833edafe59707e2688b4e95d93b4a66970af Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 1 Feb 2023 11:10:29 +0000 Subject: [PATCH 3/9] Simplify thread naming, now that profiling doesn't support Ruby 2.2 --- ext/ddtrace_profiling_native_extension/extconf.rb | 2 -- .../private_vm_api_access.c | 12 +----------- .../profiling/collectors/cpu_and_wall_time_worker.rb | 2 +- .../profiling/collectors/idle_sampling_helper.rb | 2 +- 4 files changed, 3 insertions(+), 15 deletions(-) diff --git a/ext/ddtrace_profiling_native_extension/extconf.rb b/ext/ddtrace_profiling_native_extension/extconf.rb index 9757661b89a..4c075d7ae86 100644 --- a/ext/ddtrace_profiling_native_extension/extconf.rb +++ b/ext/ddtrace_profiling_native_extension/extconf.rb @@ -168,8 +168,6 @@ def add_compiler_flag(flag) $defs << '-DNO_RB_TIME_TIMESPEC_NEW' # ...the VM changed enough that we need an alternative legacy rb_profile_frames $defs << '-DUSE_LEGACY_RB_PROFILE_FRAMES' - # ... you couldn't name threads - $defs << '-DNO_THREAD_NAMES' end # If we got here, libdatadog is available and loaded diff --git a/ext/ddtrace_profiling_native_extension/private_vm_api_access.c b/ext/ddtrace_profiling_native_extension/private_vm_api_access.c index 56e4033dbce..c055ce0dd5c 100644 --- a/ext/ddtrace_profiling_native_extension/private_vm_api_access.c +++ b/ext/ddtrace_profiling_native_extension/private_vm_api_access.c @@ -247,19 +247,9 @@ bool is_thread_alive(VALUE thread) { return thread_struct_from_object(thread)->status != THREAD_KILLED; } -// `thread` gets used on all Rubies except 2.2 -// To avoid getting false "unused argument" warnings in setups where it's not used, we need to do this weird dance -// with diagnostic stuff. See https://nelkinda.com/blog/suppress-warnings-in-gcc-and-clang/#d11e364 for details. -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Wunused-parameter" VALUE thread_name_for(VALUE thread) { - #ifdef NO_THREAD_NAMES - return Qnil; - #else - return thread_struct_from_object(thread)->name; - #endif + return thread_struct_from_object(thread)->name; } -#pragma GCC diagnostic pop // ----------------------------------------------------------------------------- // The sources below are modified versions of code extracted from the Ruby project. diff --git a/lib/datadog/profiling/collectors/cpu_and_wall_time_worker.rb b/lib/datadog/profiling/collectors/cpu_and_wall_time_worker.rb index 16ee8d34044..e6f717ea78c 100644 --- a/lib/datadog/profiling/collectors/cpu_and_wall_time_worker.rb +++ b/lib/datadog/profiling/collectors/cpu_and_wall_time_worker.rb @@ -40,7 +40,7 @@ def start @worker_thread = Thread.new do begin - Thread.current.name = self.class.name unless Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.3') + Thread.current.name = self.class.name self.class._native_sampling_loop(self) diff --git a/lib/datadog/profiling/collectors/idle_sampling_helper.rb b/lib/datadog/profiling/collectors/idle_sampling_helper.rb index 0d9847b241a..8a20bba7356 100644 --- a/lib/datadog/profiling/collectors/idle_sampling_helper.rb +++ b/lib/datadog/profiling/collectors/idle_sampling_helper.rb @@ -31,7 +31,7 @@ def start @worker_thread = Thread.new do begin - Thread.current.name = self.class.name unless Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.3') + Thread.current.name = self.class.name self.class._native_idle_sampling_loop(self) From 099cd20d0011cadbc0f01ec7185b66d9e71797fd Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 1 Feb 2023 11:13:39 +0000 Subject: [PATCH 4/9] Remove fallback for Ruby 2.2 not having `rb_time_timespec_new` --- ext/ddtrace_profiling_native_extension/extconf.rb | 2 -- .../stack_recorder.c | 13 +++---------- lib/datadog/profiling/stack_recorder.rb | 5 ----- 3 files changed, 3 insertions(+), 17 deletions(-) diff --git a/ext/ddtrace_profiling_native_extension/extconf.rb b/ext/ddtrace_profiling_native_extension/extconf.rb index 4c075d7ae86..e922e927d93 100644 --- a/ext/ddtrace_profiling_native_extension/extconf.rb +++ b/ext/ddtrace_profiling_native_extension/extconf.rb @@ -164,8 +164,6 @@ def add_compiler_flag(flag) # For REALLY OLD Rubies... if RUBY_VERSION < '2.3' - # ...there was no rb_time_timespec_new function - $defs << '-DNO_RB_TIME_TIMESPEC_NEW' # ...the VM changed enough that we need an alternative legacy rb_profile_frames $defs << '-DUSE_LEGACY_RB_PROFILE_FRAMES' end diff --git a/ext/ddtrace_profiling_native_extension/stack_recorder.c b/ext/ddtrace_profiling_native_extension/stack_recorder.c index c072c4e6118..4e9adbabebc 100644 --- a/ext/ddtrace_profiling_native_extension/stack_recorder.c +++ b/ext/ddtrace_profiling_native_extension/stack_recorder.c @@ -128,8 +128,6 @@ static VALUE ok_symbol = Qnil; // :ok in Ruby static VALUE error_symbol = Qnil; // :error in Ruby -static ID ruby_time_from_id; // id of :ruby_time_from in Ruby - static VALUE stack_recorder_class = Qnil; // Contains native state for each instance @@ -204,7 +202,6 @@ void stack_recorder_init(VALUE profiling_module) { ok_symbol = ID2SYM(rb_intern_const("ok")); error_symbol = ID2SYM(rb_intern_const("error")); - ruby_time_from_id = rb_intern_const("ruby_time_from"); } // This structure is used to define a Ruby object that stores a pointer to a ddog_prof_Profile instance @@ -309,13 +306,9 @@ static VALUE _native_serialize(DDTRACE_UNUSED VALUE _self, VALUE recorder_instan } static VALUE ruby_time_from(ddog_Timespec ddprof_time) { - #ifndef NO_RB_TIME_TIMESPEC_NEW // Modern Rubies - const int utc = INT_MAX - 1; // From Ruby sources - struct timespec time = {.tv_sec = ddprof_time.seconds, .tv_nsec = ddprof_time.nanoseconds}; - return rb_time_timespec_new(&time, utc); - #else // Ruby < 2.3 - return rb_funcall(stack_recorder_class, ruby_time_from_id, 2, LONG2NUM(ddprof_time.seconds), UINT2NUM(ddprof_time.nanoseconds)); - #endif + const int utc = INT_MAX - 1; // From Ruby sources + struct timespec time = {.tv_sec = ddprof_time.seconds, .tv_nsec = ddprof_time.nanoseconds}; + return rb_time_timespec_new(&time, utc); } void record_sample(VALUE recorder_instance, ddog_prof_Sample sample) { diff --git a/lib/datadog/profiling/stack_recorder.rb b/lib/datadog/profiling/stack_recorder.rb index ff94a7c66b1..6a2d282fb78 100644 --- a/lib/datadog/profiling/stack_recorder.rb +++ b/lib/datadog/profiling/stack_recorder.rb @@ -66,11 +66,6 @@ def clear end end - # Used only for Ruby 2.2 which doesn't have the native `rb_time_timespec_new` API; called from native code - def self.ruby_time_from(timespec_seconds, timespec_nanoseconds) - Time.at(0).utc + timespec_seconds + (timespec_nanoseconds.to_r / 1_000_000_000) - end - def reset_after_fork self.class._native_reset_after_fork(self) end From 69e2bab41d603c6fecbe9c17c32d3a93ab164d50 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 1 Feb 2023 11:19:21 +0000 Subject: [PATCH 5/9] Remove custom rb_profile_frames used for Ruby 2.2 --- .../collectors_stack.c | 11 -- .../extconf.rb | 6 -- .../private_vm_api_access.c | 101 ------------------ 3 files changed, 118 deletions(-) diff --git a/ext/ddtrace_profiling_native_extension/collectors_stack.c b/ext/ddtrace_profiling_native_extension/collectors_stack.c index 85f60f4de03..505442d6fc5 100644 --- a/ext/ddtrace_profiling_native_extension/collectors_stack.c +++ b/ext/ddtrace_profiling_native_extension/collectors_stack.c @@ -238,18 +238,7 @@ static void sample_thread_internal( filename = rb_profile_frame_path(buffer->stack_buffer[i]); line = buffer->lines_buffer[i]; } else { - // **IMPORTANT**: Be very careful when calling any `rb_profile_frame_...` API with a non-Ruby frame, as legacy - // Rubies may assume that what's in a buffer will lead to a Ruby frame. - // - // In particular for Ruby 2.2 the buffer contains a Ruby string (see the notes on our custom - // rb_profile_frames for Ruby 2.2) and CALLING **ANY** OF THOSE APIs ON IT WILL CAUSE INSTANT VM CRASHES - -#ifndef USE_LEGACY_RB_PROFILE_FRAMES // Modern Rubies name = ddtrace_rb_profile_frame_method_name(buffer->stack_buffer[i]); -#else // Ruby < 2.3 - name = buffer->stack_buffer[i]; -#endif - filename = NIL_P(last_ruby_frame) ? Qnil : rb_profile_frame_path(last_ruby_frame); line = last_ruby_line; } diff --git a/ext/ddtrace_profiling_native_extension/extconf.rb b/ext/ddtrace_profiling_native_extension/extconf.rb index e922e927d93..b698b4d991d 100644 --- a/ext/ddtrace_profiling_native_extension/extconf.rb +++ b/ext/ddtrace_profiling_native_extension/extconf.rb @@ -162,12 +162,6 @@ def add_compiler_flag(flag) $defs << '-DUSE_LEGACY_RB_VM_FRAME_METHOD_ENTRY' end -# For REALLY OLD Rubies... -if RUBY_VERSION < '2.3' - # ...the VM changed enough that we need an alternative legacy rb_profile_frames - $defs << '-DUSE_LEGACY_RB_PROFILE_FRAMES' -end - # If we got here, libdatadog is available and loaded ENV['PKG_CONFIG_PATH'] = "#{ENV['PKG_CONFIG_PATH']}:#{Libdatadog.pkgconfig_folder}" Logging.message(" [ddtrace] PKG_CONFIG_PATH set to #{ENV['PKG_CONFIG_PATH'].inspect}\n") diff --git a/ext/ddtrace_profiling_native_extension/private_vm_api_access.c b/ext/ddtrace_profiling_native_extension/private_vm_api_access.c index c055ce0dd5c..894d61a2e9c 100644 --- a/ext/ddtrace_profiling_native_extension/private_vm_api_access.c +++ b/ext/ddtrace_profiling_native_extension/private_vm_api_access.c @@ -280,8 +280,6 @@ VALUE thread_name_for(VALUE thread) { // OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF // SUCH DAMAGE. -#ifndef USE_LEGACY_RB_PROFILE_FRAMES // Modern Rubies - // Taken from upstream vm_core.h at commit 5f10bd634fb6ae8f74a4ea730176233b0ca96954 (March 2022, Ruby 3.2 trunk) // Copyright (C) 2004-2007 Koichi Sasada // to support our custom rb_profile_frames (see below) @@ -382,9 +380,6 @@ calc_lineno(const rb_iseq_t *iseq, const VALUE *pc) // was called from. // * Imported fix from https://github.com/ruby/ruby/pull/7116 to avoid sampling threads that are still being created // -// **IMPORTANT: WHEN CHANGING THIS FUNCTION, CONSIDER IF THE SAME CHANGE ALSO NEEDS TO BE MADE TO THE VARIANT FOR -// RUBY 2.2 AND BELOW WHICH IS ALSO PRESENT ON THIS FILE** -// // What is rb_profile_frames? // `rb_profile_frames` is a Ruby VM debug API added for use by profilers for sampling the stack trace of a Ruby thread. // Its main other user is the stackprof profiler: https://github.com/tmm1/stackprof . @@ -712,104 +707,8 @@ check_method_entry(VALUE obj, int can_be_svar) return check_method_entry(ep[-1], TRUE); } #endif // USE_LEGACY_RB_VM_FRAME_METHOD_ENTRY - #endif // RUBY_MJIT_HEADER -#else // USE_LEGACY_RB_PROFILE_FRAMES, Ruby < 2.3 - -// Taken from upstream vm_backtrace.c at commit bbda1a027475bf7ce5e1a9583a7b55d0be71c8fe (March 2018, ruby_2_2 branch) -// Copyright (C) 1993-2012 Yukihiro Matsumoto -// to support our custom rb_profile_frames (see below) -// Modifications: None -inline static int -calc_lineno(const rb_iseq_t *iseq, const VALUE *pc) -{ - return rb_iseq_line_no(iseq, pc - iseq->iseq_encoded); -} - -// Taken from upstream vm_backtrace.c at commit bbda1a027475bf7ce5e1a9583a7b55d0be71c8fe (March 2018, ruby_2_2 branch) -// Copyright (C) 1993-2012 Yukihiro Matsumoto -// Modifications: -// * Renamed rb_profile_frames => ddtrace_rb_profile_frames -// * Add thread argument -// * Add is_ruby_frame argument -// * Removed `if (lines)` tests -- require/assume that like `buff`, `lines` is always specified -// * Added support for getting the name from native methods by getting inspiration from `backtrace_each` in -// `vm_backtrace.c`. Note that unlike the `rb_profile_frames` for modern Rubies, this version actually returns the -// method name as as `VALUE` containing a Ruby string in the `buff`. -// * Skip dummy frame that shows up in main thread -// * Add `end_cfp == NULL` and `end_cfp <= cfp` safety checks. These are used in a bunch of places in -// `vm_backtrace.c` (`backtrace_each`, `backtrace_size`, `rb_ec_partial_backtrace_object`) but are conspicuously -// absent from `rb_profile_frames`. Oversight? -// * Check thread status and do not sample if thread has been killed. -// * Imported fix from https://github.com/ruby/ruby/pull/7116 to avoid sampling threads that are still being created -// -// The `rb_profile_frames` function changed quite a bit between Ruby 2.2 and 2.3. Since the change was quite complex -// I opted not to try to extend support to Ruby 2.2 using the same custom function, and instead I started -// anew from the Ruby 2.2 version of the function, applying some of the same fixes that we have for the modern version. -int ddtrace_rb_profile_frames(VALUE thread, int start, int limit, VALUE *buff, int *lines, bool* is_ruby_frame) -{ - // **IMPORTANT: THIS IS A CUSTOM RB_PROFILE_FRAMES JUST FOR RUBY 2.2; - // SEE ABOVE FOR THE FUNCTION THAT GETS USED FOR MODERN RUBIES** - - int i; - rb_thread_t *th = thread_struct_from_object(thread); - rb_control_frame_t *cfp = th->cfp, *end_cfp = RUBY_VM_END_CONTROL_FRAME(th); - - // This should not happen for ddtrace (it can only happen when a thread is still being created), but I've imported - // it from https://github.com/ruby/ruby/pull/7116 in a "just in case" kind of mindset. - if (cfp == NULL) return 0; - - // Avoid sampling dead threads - if (th->status == THREAD_KILLED) return 0; - - // `vm_backtrace.c` includes this check in several methods. This happens on newly-created threads, and may - // also (not entirely sure) happen on dead threads - if (end_cfp == NULL) return PLACEHOLDER_STACK_IN_NATIVE_CODE; - - // Fix: Skip dummy frame that shows up in main thread. - // - // According to a comment in `backtrace_each` (`vm_backtrace.c`), there's two dummy frames that we should ignore - // at the base of every thread's stack. - // (see https://github.com/ruby/ruby/blob/4bd38e8120f2fdfdd47a34211720e048502377f1/vm_backtrace.c#L890-L914 ) - // - // One is being pointed to by `RUBY_VM_END_CONTROL_FRAME(ec)`, and so we need to advance to the next one, and - // reaching it will be used as a condition to break out of the loop below. - // - // Note that in `backtrace_each` there's two calls to `RUBY_VM_NEXT_CONTROL_FRAME`, but the loop bounds there - // are computed in a different way, so the two calls really are equivalent to one here. - end_cfp = RUBY_VM_NEXT_CONTROL_FRAME(end_cfp); - - // See comment on `record_placeholder_stack_in_native_code` for a full explanation of what this means (and why we don't just return 0) - if (end_cfp <= cfp) return PLACEHOLDER_STACK_IN_NATIVE_CODE; - - for (i=0; iiseq && cfp->pc) { /* should be NORMAL_ISEQ */ - if (start > 0) { - start--; - continue; - } - - /* record frame info */ - buff[i] = cfp->iseq->self; - lines[i] = calc_lineno(cfp->iseq, cfp->pc); - is_ruby_frame[i] = true; - i++; - } else if (RUBYVM_CFUNC_FRAME_P(cfp)) { - ID mid = cfp->me->def ? cfp->me->def->original_id : cfp->me->called_id; - buff[i] = rb_id2str(mid); - lines[i] = 0; - is_ruby_frame[i] = false; - i++; - } - cfp = RUBY_VM_PREVIOUS_CONTROL_FRAME(cfp); - } - - return i; -} - -#endif // USE_LEGACY_RB_PROFILE_FRAMES - #ifndef NO_RACTORS // This API and definition are exported as a public symbol by the VM BUT the function header is not defined in any public header, so we // repeat it here to be able to use in our code. From 923ad4fdbfb01dd833442b63c0a415e63f4da52b Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 1 Feb 2023 11:36:53 +0000 Subject: [PATCH 6/9] Remove remaining Ruby 2.2 special casing from profiling code and specs --- .../build_ddtrace_profiling_native_extension.rb | 2 +- lib/datadog/profiling/collectors/old_stack.rb | 4 +--- .../profiling/collectors/cpu_and_wall_time_spec.rb | 8 -------- .../collectors/cpu_and_wall_time_worker_spec.rb | 4 ---- .../datadog/profiling/collectors/old_stack_spec.rb | 14 ++++---------- spec/datadog/profiling/ext/forking_spec.rb | 9 --------- spec/datadog/profiling/http_transport_spec.rb | 10 ---------- spec/datadog/profiling/native_extension_spec.rb | 4 ---- 8 files changed, 6 insertions(+), 49 deletions(-) diff --git a/integration/images/include/build_ddtrace_profiling_native_extension.rb b/integration/images/include/build_ddtrace_profiling_native_extension.rb index 0dfcc711fb4..8f6ba7493ce 100755 --- a/integration/images/include/build_ddtrace_profiling_native_extension.rb +++ b/integration/images/include/build_ddtrace_profiling_native_extension.rb @@ -1,7 +1,7 @@ #!/usr/bin/env ruby if local_gem_path = ENV['DD_DEMO_ENV_GEM_LOCAL_DDTRACE'] - if (RUBY_VERSION.start_with?('2.1.') || RUBY_VERSION.start_with?('2.2.')) + if RUBY_VERSION.start_with?('2.1.', '2.2.') puts "\n== Skipping build of profiler native extension on Ruby 2.1/2.2 ==" else puts "\n== Building profiler native extension ==" diff --git a/lib/datadog/profiling/collectors/old_stack.rb b/lib/datadog/profiling/collectors/old_stack.rb index bf944a2c262..89846b86a12 100644 --- a/lib/datadog/profiling/collectors/old_stack.rb +++ b/lib/datadog/profiling/collectors/old_stack.rb @@ -75,9 +75,7 @@ def initialize( # Cache this buffer, since it's pretty expensive to keep accessing it @stack_sample_event_recorder = recorder[Events::StackSample] # See below for details on why this is needed - @needs_process_waiter_workaround = - Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.3') && - Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.7') + @needs_process_waiter_workaround = Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.7') end def start diff --git a/spec/datadog/profiling/collectors/cpu_and_wall_time_spec.rb b/spec/datadog/profiling/collectors/cpu_and_wall_time_spec.rb index 6885ebe22ab..4c489647dab 100644 --- a/spec/datadog/profiling/collectors/cpu_and_wall_time_spec.rb +++ b/spec/datadog/profiling/collectors/cpu_and_wall_time_spec.rb @@ -93,8 +93,6 @@ def stats end it 'includes the thread names, if available' do - skip 'Thread names not available on Ruby 2.2' if RUBY_VERSION < '2.3' - t1.name = 'thread t1' t2.name = nil t3.name = 'thread t3' @@ -110,12 +108,6 @@ def stats expect(t3_sample).to include(labels: include(:'thread name' => 'thread t3')) end - it 'does not include thread names on Ruby 2.2' do - skip 'Testcase only applies to Ruby 2.2' if RUBY_VERSION >= '2.3' - - expect(samples.flat_map { |it| it.fetch(:labels).keys }).to_not include(':thread name') - end - it 'includes the wall-time elapsed between samples' do sample wall_time_at_first_sample = diff --git a/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb b/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb index b4115289c98..1b85376412f 100644 --- a/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb +++ b/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb @@ -37,8 +37,6 @@ end it 'creates a new thread' do - skip 'Spec not compatible with Ruby 2.2' if RUBY_VERSION.start_with?('2.2.') - start expect(Thread.list.map(&:name)).to include(described_class.name) @@ -430,8 +428,6 @@ it 'shuts down the background thread' do stop - skip 'Spec not compatible with Ruby 2.2' if RUBY_VERSION.start_with?('2.2.') - expect(Thread.list.map(&:name)).to_not include(described_class.name) end diff --git a/spec/datadog/profiling/collectors/old_stack_spec.rb b/spec/datadog/profiling/collectors/old_stack_spec.rb index d33125bce47..f5bcb9eadf3 100644 --- a/spec/datadog/profiling/collectors/old_stack_spec.rb +++ b/spec/datadog/profiling/collectors/old_stack_spec.rb @@ -736,22 +736,16 @@ describe 'the crash' do # Let's not get surprised if this shows up in other Ruby versions - it 'does not affect Ruby < 2.3 nor Ruby >= 2.7' do - unless Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.3') || - Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7') - skip 'Test case only applies to Ruby < 2.3 or Ruby >= 2.7' - end + it 'does not affect Ruby >= 2.7' do + skip('Test case only applies to Ruby >= 2.7') unless Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7') expect_in_fork do expect(process_waiter_thread.instance_variable_get(:@hello)).to be nil end end - it 'affects Ruby >= 2.3 and < 2.7' do - unless Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.3') && - Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.7') - skip 'Test case only applies to Ruby >= 2.3 and < 2.7' - end + it 'affects Ruby < 2.7' do + skip('Test case only applies to Ruby < 2.7') unless Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.7') expect_in_fork( fork_expectations: proc do |status:, stdout:, stderr:| diff --git a/spec/datadog/profiling/ext/forking_spec.rb b/spec/datadog/profiling/ext/forking_spec.rb index 40c92139d62..863d4f9998b 100644 --- a/spec/datadog/profiling/ext/forking_spec.rb +++ b/spec/datadog/profiling/ext/forking_spec.rb @@ -108,15 +108,6 @@ def fork(&block) fork_result end end - - before do - # TODO: This test breaks other tests when Forking#apply! runs first in Ruby < 2.3 - # Unclear whether its the setup from this test, or cleanup elsewhere (e.g. spec_helper.rb) - # Either way, #apply! causes callbacks not to work; Forking patch is - # not hooking in properly. See `fork_class.method(:fork).source_location` - # and `fork.class.ancestors` vs `fork.singleton_class.ancestors`. - skip 'Test is unstable for Ruby < 2.3' if Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.3') - end end shared_context 'at_fork callbacks' do diff --git a/spec/datadog/profiling/http_transport_spec.rb b/spec/datadog/profiling/http_transport_spec.rb index 1ce2eb5aab7..7bdccea180e 100644 --- a/spec/datadog/profiling/http_transport_spec.rb +++ b/spec/datadog/profiling/http_transport_spec.rb @@ -374,16 +374,6 @@ StartCallback: -> { init_signal.push(1) } ) server.listeners << unix_domain_socket - - if RUBY_VERSION.start_with?('2.2.') - # Workaround for webrick bug in Ruby 2.2. - # This `setup_shutdown_pipe` method was added in 2.2 but it had a bug when webrick - # was configured with `DoNotListen: true` and was never called, which led to failures as webrick requires and - # expects it to have been called. - # In Ruby 2.3 this was fixed and this method always gets called, even with `DoNotListen: true`. - server.send(:setup_shutdown_pipe) - end - server end let(:adapter) { Datadog::Transport::Ext::UnixSocket::ADAPTER } diff --git a/spec/datadog/profiling/native_extension_spec.rb b/spec/datadog/profiling/native_extension_spec.rb index a0d985f56ba..9b29f2fd4be 100644 --- a/spec/datadog/profiling/native_extension_spec.rb +++ b/spec/datadog/profiling/native_extension_spec.rb @@ -272,10 +272,6 @@ def wait_for_thread_to_die end describe 'correctness' do - before do - skip 'Ruby 2.2 does not expose ruby_thread_has_gvl_p so nothing to compare to' if RUBY_VERSION.start_with?('2.2.') - end - let(:ready_queue) { Queue.new } let(:background_thread) do Thread.new do From 4e95ff42b1ecaf1e615b670dbcfc4dc561f010a3 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 1 Feb 2023 11:40:15 +0000 Subject: [PATCH 7/9] [PROF-7145] Remove support for profiling Ruby 2.2 **What does this PR do?**: This PR removes support for profiling Ruby 2.2 using the Continuous Profiling product. Other products shipped in ddtrace (tracing, application security monitoring, CI, etc) are unaffected. This dropping of support does not impact our customer's ability to install the latest dd-trace-rb on Ruby 2.2. What it does is 1. Skip compilation of the profiling native extension on Ruby 2.2 2. Print a warning message when customers try to enable profiling on Ruby 2.2 (but does not otherwise block or break their application) The warning message shown to customers is: > W, [2023-02-01T11:42:57.022293 #410738] WARN -- ddtrace: [ddtrace] > Profiling was requested but is not supported, profiling disabled: > Your ddtrace installation is missing support for the Continuous > Profiler because the profiler only supports Ruby 2.3 or newer. > Upgrade to a modern Ruby to enable profiling for your app. Customers are welcome to continue using older versions of dd-trace-rb to profile their Ruby 2.2 (as well as 2.1) applications. (This PR is similar to #2140 where we dropped support for Ruby 2.1) **Motivation**: There is little customer interest on profiling Ruby 2.2, and supporting old Rubies also comes with an extra tax -- just look how much code and complexity is being removed by this PR. **Additional Notes**: (N/A) **How to test the change?**: * Validate that test suite still runs successfully on Ruby 2.2 * Validate that you get the above error message when trying to profile a Ruby 2.2 application --- docs/GettingStarted.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/GettingStarted.md b/docs/GettingStarted.md index 93112fa3d27..6aa17bc233e 100644 --- a/docs/GettingStarted.md +++ b/docs/GettingStarted.md @@ -126,7 +126,7 @@ To contribute, check out the [contribution guidelines][contribution docs] and [d | | | 2.5 | Full | Latest | | | | 2.4 | Full | Latest | | | | 2.3 | Full | Latest | -| | | 2.2 | Full | Latest | +| | | 2.2 | Full (except for Profiling) | Latest | | | | 2.1 | Full (except for Profiling) | Latest | | | | 2.0 | EOL since June 7th, 2021 | < 0.50.0 | | | | 1.9.3 | EOL since August 6th, 2020 | < 0.27.0 | From 6b1f5cefc6554740c0341146a7475400519bfe3c Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 1 Feb 2023 13:01:57 +0000 Subject: [PATCH 8/9] Avoid running Profiling::Ext::Forking specs on unsupported Rubies --- spec/datadog/profiling/ext/forking_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/datadog/profiling/ext/forking_spec.rb b/spec/datadog/profiling/ext/forking_spec.rb index 863d4f9998b..d3b83df9317 100644 --- a/spec/datadog/profiling/ext/forking_spec.rb +++ b/spec/datadog/profiling/ext/forking_spec.rb @@ -5,9 +5,9 @@ require 'datadog/profiling/ext/forking' RSpec.describe Datadog::Profiling::Ext::Forking do - describe '::apply!' do - before { skip_if_profiling_not_supported(self) } + before { skip_if_profiling_not_supported(self) } + describe '::apply!' do subject(:apply!) { described_class.apply! } let(:toplevel_receiver) { TOPLEVEL_BINDING.receiver } From ff8eeadd91cfb91d0b57504ac33b81da8d4f7e1c Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 1 Feb 2023 15:20:18 +0000 Subject: [PATCH 9/9] Improve naming of support helpers as suggested during review --- .../native_extension_helpers.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb b/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb index 11a23c90d97..b699cee1bb8 100644 --- a/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb +++ b/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb @@ -87,8 +87,8 @@ def self.unsupported_reason on_windows? || on_macos? || on_unknown_os? || - not_on_amd64_or_arm64? || - on_ruby_2_1_or_2_2? || + on_unsupported_cpu_arch? || + on_unsupported_ruby_version? || expected_to_use_mjit_but_mjit_is_disabled? || libdatadog_not_available? || libdatadog_not_usable? @@ -251,7 +251,7 @@ def self.pkg_config_missing?(command: $PKGCONFIG) # rubocop:disable Style/Global unknown_os_not_supported unless RUBY_PLATFORM.include?('darwin') || RUBY_PLATFORM.include?('linux') end - private_class_method def self.not_on_amd64_or_arm64? + private_class_method def self.on_unsupported_cpu_arch? architecture_not_supported = explain_issue( 'your CPU architecture is not supported by the Datadog Continuous Profiler.', suggested: GET_IN_TOUCH, @@ -260,7 +260,7 @@ def self.pkg_config_missing?(command: $PKGCONFIG) # rubocop:disable Style/Global architecture_not_supported unless RUBY_PLATFORM.start_with?('x86_64', 'aarch64', 'arm64') end - private_class_method def self.on_ruby_2_1_or_2_2? + private_class_method def self.on_unsupported_ruby_version? ruby_version_not_supported = explain_issue( 'the profiler only supports Ruby 2.3 or newer.', suggested: UPGRADE_RUBY,