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.

Sign up for GitHub

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

Jump to bottom

[PROF-7145] Remove support for profiling Ruby 2.2 #2592

Merged
merged 9 commits into from
Feb 1, 2023
Merged

[PROF-7145] Remove support for profiling Ruby 2.2 #2592

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f653915
Disable use of profiling in Ruby 2.2
ivoanjo Feb 1, 2023
0482d73
Minor comment cleanups, now that we've dropped profiling Ruby 2.2
ivoanjo Feb 1, 2023
b751833
Simplify thread naming, now that profiling doesn't support Ruby 2.2
ivoanjo Feb 1, 2023
099cd20
Remove fallback for Ruby 2.2 not having `rb_time_timespec_new`
ivoanjo Feb 1, 2023
69e2bab
Remove custom rb_profile_frames used for Ruby 2.2
ivoanjo Feb 1, 2023
923ad4f
Remove remaining Ruby 2.2 special casing from profiling code and specs
ivoanjo Feb 1, 2023
4e95ff4
[PROF-7145] Remove support for profiling Ruby 2.2
ivoanjo Feb 1, 2023
6b1f5ce
Avoid running Profiling::Ext::Forking specs on unsupported Rubies
ivoanjo Feb 1, 2023
ff8eead
Improve naming of support helpers as suggested during review
ivoanjo Feb 1, 2023
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
2 changes: 1 addition & 1 deletion integration/images/include/build_ddtrace_profiling_native_extension.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.')
Copy link
Contributor

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 =="
Expand Down
4 changes: 1 addition & 3 deletions lib/datadog/profiling/collectors/old_stack.rb
View file
Open in desktop
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
8 changes: 0 additions & 8 deletions spec/datadog/profiling/collectors/cpu_and_wall_time_spec.rb
View file
Open in desktop
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
4 changes: 0 additions & 4 deletions spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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

Expand Down
14 changes: 4 additions & 10 deletions spec/datadog/profiling/collectors/old_stack_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:|
Expand Down
9 changes: 0 additions & 9 deletions spec/datadog/profiling/ext/forking_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 0 additions & 10 deletions spec/datadog/profiling/http_transport_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
4 changes: 0 additions & 4 deletions spec/datadog/profiling/native_extension_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down