-
Notifications
You must be signed in to change notification settings - Fork 375
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
[PROF-7145] Remove support for profiling Ruby 2.2 #2592
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
ext/ddtrace_profiling_native_extension/native_extension_helpers.rb
Outdated
Show resolved
Hide resolved
@@ -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') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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! |
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
The warning message shown to customers is:
(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?: