Skip to content

Commit

Permalink
[PROF-10015] Fix Phusion Passenger detection when not in Gemfile/`g…
Browse files Browse the repository at this point in the history
…ems.rb`

**What does this PR do?**

In #2978 because of an
incompatibility with Phusion Passenger, we added autodetection code to
enable the "no signals" workaround when this web server is in use.

This code was later extended in
#3280 once an upstream fix
was released, so that the "no signals" workaround was only applied on
old Passenger versions.

But in #3721 we got a
report that our version detection code did not work correctly in setups
where Passenger was not installed via `Gemfile`/`gems.rb`.

This PR fixes the version detection in these situations.

**Motivation:**

Make sure the "no signals" workaround does not get enabled in setups
where it does not need to be.

**Additional Notes:**

N/A

**How to test the change?**

I've added test coverage for this issue.

Additionally, here's how to manually install Passenger and validate
that the "no signals" workaround does not get enabled:

```
$ cat Gemfile
source 'https://rubygems.org'

gem 'datadog', git: 'https://github.com/DataDog/dd-trace-rb.git', branch: 'ivoanjo/prof-10015-fix-passenger-detection'
gem 'rack'
$ cat config.ru
require 'datadog/profiling/preload'

app = ->(env) {
  puts "Running on passenger #{PhusionPassenger::VERSION_STRING.inspect}"
  [200, {}, ["Hello, World!"]]
}
run app

$ docker run --network=host -ti -v `pwd`:/working ruby:3.3 /bin/bash
$ cd working/
$ apt update && apt-get install -y dirmngr gnupg apt-transport-https ca-certificates curl
$ curl https://oss-binaries.phusionpassenger.com/auto-software-signing-gpg-key.txt | gpg --dearmor | tee /etc/apt/trusted.gpg.d/phusion.gpg >/dev/null
$ sh -c 'echo deb https://oss-binaries.phusionpassenger.com/apt/passenger bookworm main > /etc/apt/sources.list.d/passenger.list'
$ apt update && apt-get install -y nginx libnginx-mod-http-passenger
$ bundle install
$ DD_PROFILING_ENABLED=true /usr/bin/passenger start
```

in master this outputs the warning message

> WARN -- datadog: [datadog] 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.

With this fix, this message is not shown, confirming the "no signals"
workaround is not enabled.
  • Loading branch information
ivoanjo committed Jul 2, 2024
1 parent 240dcbc commit 9f88ed1
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 1 deletion.
6 changes: 5 additions & 1 deletion lib/datadog/profiling/component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -397,8 +397,12 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:)

# See https://github.com/datadog/dd-trace-rb/issues/2976 for details.
private_class_method def self.incompatible_passenger_version?
first_compatible_version = Gem::Version.new('6.0.19')

if Gem.loaded_specs['passenger']
Gem.loaded_specs['passenger'].version < Gem::Version.new('6.0.19')
Gem.loaded_specs['passenger'].version < first_compatible_version
elsif defined?(PhusionPassenger::VERSION_STRING)
Gem::Version.new(PhusionPassenger::VERSION_STRING) < first_compatible_version
else
true
end
Expand Down
23 changes: 23 additions & 0 deletions spec/datadog/profiling/component_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,29 @@
end
end

context 'when passenger gem is not available, but PhusionPassenger::VERSION_STRING is available' do
context 'on passenger >= 6.0.19' do
before { stub_const('PhusionPassenger::VERSION_STRING', '6.0.19') }

it { is_expected.to be false }
end

context 'on passenger < 6.0.19' do
before do
stub_const('PhusionPassenger::VERSION_STRING', '6.0.18')
allow(Datadog.logger).to receive(:warn)
end

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', passenger: nil, mysql2: nil, rugged: nil)

Expand Down
1 change: 1 addition & 0 deletions vendor/rbs/passenger/0/passenger.rbs
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
module PhusionPassenger
VERSION_STRING: ::String
end

0 comments on commit 9f88ed1

Please sign in to comment.