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-10015] Fix Phusion Passenger detection when not in Gemfile/gems.rb #3750

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jul 2, 2024

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.

…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.
@ivoanjo ivoanjo requested review from a team as code owners July 2, 2024 11:45
@github-actions github-actions bot added the profiling Involves Datadog profiling label Jul 2, 2024
@ivoanjo ivoanjo merged commit 7611172 into master Jul 2, 2024
166 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/prof-10015-fix-passenger-detection branch July 2, 2024 12:43
@github-actions github-actions bot added this to the 2.2.0 milestone Jul 2, 2024
@TonyCTHsu TonyCTHsu mentioned this pull request Jul 11, 2024
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.

2 participants