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-7145] Remove support for profiling Ruby 2.2 #2592

Merged
merged 9 commits into from
Feb 1, 2023
2 changes: 1 addition & 1 deletion Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down
1 change: 1 addition & 0 deletions ddtrace.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Expand Down
2 changes: 1 addition & 1 deletion docs/GettingStarted.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
11 changes: 0 additions & 11 deletions ext/ddtrace_profiling_native_extension/collectors_stack.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
12 changes: 1 addition & 11 deletions ext/ddtrace_profiling_native_extension/extconf.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -162,16 +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'
# ...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'
# ... you couldn't name threads
$defs << '-DNO_THREAD_NAMES'
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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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? ||
ivoanjo marked this conversation as resolved.
Show resolved Hide resolved
expected_to_use_mjit_but_mjit_is_disabled? ||
libdatadog_not_available? ||
libdatadog_not_usable?
Expand Down Expand Up @@ -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
Expand Down
113 changes: 1 addition & 112 deletions ext/ddtrace_profiling_native_extension/private_vm_api_access.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -290,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)
Expand Down Expand Up @@ -392,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 .
Expand Down Expand Up @@ -722,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; i<limit && cfp != end_cfp;) {
if (cfp->iseq && 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.
Expand Down
2 changes: 1 addition & 1 deletion ext/ddtrace_profiling_native_extension/ruby_helpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 3 additions & 10 deletions ext/ddtrace_profiling_native_extension/stack_recorder.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 1 addition & 2 deletions integration/apps/rack/spec/integration/basic_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
@@ -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.', '2.2.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's another ad-hoc pattern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern shows up in the codebase 3 times:

Searching 2551 files for "RUBY_VERSION.start_with?('2.1.', '2.2.')" (case sensitive)

~/datadog/second-dd-trace-rb/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb:
  267            )
  268  
  269:           ruby_version_not_supported if RUBY_VERSION.start_with?('2.1.', '2.2.')
  270          end
  271  

~/datadog/second-dd-trace-rb/integration/images/include/build_ddtrace_profiling_native_extension.rb:
    2  
    3  if local_gem_path = ENV['DD_DEMO_ENV_GEM_LOCAL_DDTRACE']
    4:   if RUBY_VERSION.start_with?('2.1.', '2.2.')
    5      puts "\n== Skipping build of profiler native extension on Ruby 2.1/2.2 =="
    6    else

~/datadog/second-dd-trace-rb/spec/datadog/profiling/spec_helper.rb:
   35      testcase.skip('Profiling is not supported on JRuby') if PlatformHelpers.jruby?
   36      testcase.skip('Profiling is not supported on TruffleRuby') if PlatformHelpers.truffleruby?
   37:     testcase.skip('Profiling is not supported on Ruby 2.1/2.2') if RUBY_VERSION.start_with?('2.1.', '2.2.')
   38  
   39      # Profiling is not officially supported on macOS due to missing libdatadog binaries,

3 matches across 3 files

...but actually only shows up once in the code shipped to customers.

I'm somewhat hesitant in introducing additional complexity to avoid having duplication in test/scaffolding code, so I'm leaving towards keeping it as-is.

puts "\n== Skipping build of profiler native extension on Ruby 2.1/2.2 =="
else
puts "\n== Building profiler native extension =="
success =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we should have a helper or two to do these kind of checks instead of ad-hoc winging the checks each time.

This would also make such calls log-able/trackable/grep-able.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would usually say yes, but given that's additional work done just for the benefit of Ruby 2.1 and 2.2, in this very specific case I don't think it's work the extra effort.

Thread.current.name = self.class.name

self.class._native_sampling_loop(self)

Expand Down
2 changes: 1 addition & 1 deletion lib/datadog/profiling/collectors/idle_sampling_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
4 changes: 1 addition & 3 deletions lib/datadog/profiling/collectors/old_stack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 0 additions & 5 deletions lib/datadog/profiling/stack_recorder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 0 additions & 8 deletions spec/datadog/profiling/collectors/cpu_and_wall_time_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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 =
Expand Down
Loading