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

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Feb 1, 2023

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.

(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

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.
**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
@ivoanjo ivoanjo requested a review from a team February 1, 2023 13:02
@github-actions github-actions bot added the profiling Involves Datadog profiling label Feb 1, 2023
Copy link
Contributor

@lloeki lloeki left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -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
Contributor

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.

@@ -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.

@codecov-commenter
Copy link

Codecov Report

Merging #2592 (6b1f5ce) into master (4a1050c) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2592      +/-   ##
==========================================
- Coverage   98.04%   98.04%   -0.01%     
==========================================
  Files        1143     1143              
  Lines       62031    62017      -14     
  Branches     2801     2789      -12     
==========================================
- Hits        60821    60807      -14     
  Misses       1210     1210              
Impacted Files Coverage Δ
lib/datadog/profiling/stack_recorder.rb 76.66% <ø> (-1.46%) ⬇️
...dog/profiling/collectors/cpu_and_wall_time_spec.rb 97.88% <ø> (-0.02%) ⬇️
...filing/collectors/cpu_and_wall_time_worker_spec.rb 94.71% <ø> (-0.04%) ⬇️
spec/datadog/profiling/http_transport_spec.rb 99.56% <ø> (+0.42%) ⬆️
spec/datadog/profiling/native_extension_spec.rb 98.72% <ø> (-0.02%) ⬇️
...iling_native_extension/native_extension_helpers.rb 96.73% <100.00%> (ø)
...g/profiling/collectors/cpu_and_wall_time_worker.rb 100.00% <100.00%> (ø)
...tadog/profiling/collectors/idle_sampling_helper.rb 93.54% <100.00%> (ø)
lib/datadog/profiling/collectors/old_stack.rb 100.00% <100.00%> (ø)
...pec/datadog/profiling/collectors/old_stack_spec.rb 97.47% <100.00%> (-0.03%) ⬇️
... and 7 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ivoanjo
Copy link
Member Author

ivoanjo commented Feb 1, 2023

Thanks Loic for the review!

One of the system tests is shown as failing, but we're investigating it separately, as it happens with master as well and we don't expect it to be related to this change.

Merging away!

@ivoanjo ivoanjo merged commit 9ec31e0 into master Feb 1, 2023
@ivoanjo ivoanjo deleted the ivoanjo/prof-7145-drop-support-profiling-ruby-2-2 branch February 1, 2023 17:09
@github-actions github-actions bot added this to the 1.10.0 milestone Feb 1, 2023
@TonyCTHsu TonyCTHsu mentioned this pull request Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants