Skip to content

Commit

Permalink
Allow profiler to run without "no signals" workaround on passenger 6.…
Browse files Browse the repository at this point in the history
…0.19+

**What does this PR do?**

This PR is the last step to fully fixing the issue reported in #2976.

In that issue, we discovered that he passenger web server had an
incompatibility with the profiler. At the time, we added code to
detect when passenger was in use and apply the "no signals" workaround
to avoid this issue out-of-the-box.

Upstream passenger has since accepted our PR to fix the issue, allowing
us in this PR to change our detection logic to:

a) Not enable the "no signals" workaround on passenger 6.0.19+
b) Provide an actionable error message to customers on older passenger
   versions to tell them that the best option is to upgrade

**Motivation:**

Improve the out-of-the-box experience of profiler users that use the
passenger web server.

**Additional Notes:**

N/A

**How to test the change?**

I used the following `config.ru`:

```ruby
def self.fib(n)
  n <= 1 ? n : fib(n-1) + fib(n-2)
end

app = ->(env) {
  puts " ** Got request!"

  [200, {}, ["Hello, World! #{fib(30)}"]]
}
run app
```

and `Gemfile`:

```ruby
source 'https://rubygems.org'

gem 'ddtrace', path: 'path/to/local/repo'
 #gem 'passenger', '= 6.0.18'
gem 'passenger', '= 6.0.19'
gem 'puma'
```

to validate that the passenger versions are correctly detected +
request processing is not impacted on either.

Fixes #2976
  • Loading branch information
ivoanjo committed Nov 24, 2023
1 parent adc07c4 commit 80d5b61
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 7 deletions.
18 changes: 13 additions & 5 deletions lib/datadog/profiling/component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -171,12 +171,11 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:)
return true
end

if defined?(::PhusionPassenger)
if (defined?(::PhusionPassenger) || Gem.loaded_specs['passenger']) && incompatible_passenger_version?
Datadog.logger.warn(
'Enabling the profiling "no signals" workaround because the passenger web server is in use. ' \
'This is needed because passenger is currently incompatible with the normal working mode ' \
'of the profiler, as detailed in <https://github.com/DataDog/dd-trace-rb/issues/2976>. ' \
'Profiling data will have lower quality.'
'Enabling the profiling "no signals" workaround because an incompatible version of the passenger gem is ' \
'installed. Profiling data will have lower quality.' \
'To fix this, upgrade the passenger gem to version 6.0.19 or above.'
)
return true
end
Expand Down Expand Up @@ -237,6 +236,15 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:)
true
end
end

# See https://github.com/datadog/dd-trace-rb/issues/2976 for details.
private_class_method def self.incompatible_passenger_version?
if Gem.loaded_specs['passenger']
Gem.loaded_specs['passenger'].version < Gem::Version.new('6.0.19')
else
true
end
end
end
end
end
1 change: 1 addition & 0 deletions sig/datadog/profiling/component.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ module Datadog
def self.no_signals_workaround_enabled?: (untyped settings) -> bool

def self.incompatible_libmysqlclient_version?: (untyped settings) -> bool
def self.incompatible_passenger_version?: () -> bool
end
end
end
28 changes: 26 additions & 2 deletions spec/datadog/profiling/component_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,9 @@
end
end

context 'when running inside the passenger web server' do
context 'when running inside the passenger web server, even when gem is not available' do
include_context('loaded gems', passenger: nil, rugged: nil, mysql2: nil)

before do
stub_const('::PhusionPassenger', Module.new)
allow(Datadog.logger).to receive(:warn)
Expand All @@ -495,8 +497,30 @@
end
end

context 'when passenger gem is available' do
context 'on passenger >= 6.0.19' do
include_context('loaded gems', passenger: Gem::Version.new('6.0.19'), rugged: nil, mysql2: nil)

it { is_expected.to be false }
end

context 'on passenger < 6.0.19' do
include_context('loaded gems', passenger: Gem::Version.new('6.0.18'), rugged: nil, mysql2: nil)

before { allow(Datadog.logger).to receive(:warn) }

it { is_expected.to be true }

it 'logs a warning message mentioning that the no signals workaround is going to be used' do
expect(Datadog.logger).to receive(:warn).with(/Enabling the profiling "no signals" workaround/)

no_signals_workaround_enabled?
end
end
end

context 'when mysql2 / rugged gems + passenger are not available' do
include_context('loaded gems', mysql2: nil, rugged: nil)
include_context('loaded gems', passenger: nil, mysql2: nil, rugged: nil)

it { is_expected.to be false }
end
Expand Down

0 comments on commit 80d5b61

Please sign in to comment.