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.

Sign up for GitHub

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

Jump to bottom

Allow profiler to run without "no signals" workaround on passenger 6.0.19+ #3280

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Allow profiler to run without "no signals" workaround on passenger 6.0.19+ #3280

merged 1 commit into from
Nov 27, 2023

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Nov 24, 2023

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:

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:

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.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Fixes #2976

@ivoanjo ivoanjo requested a review from a team as a code owner November 24, 2023 12:24
@github-actions github-actions bot added the profiling Involves Datadog profiling label Nov 24, 2023
@ivoanjo
Allow profiler to run without "no signals" workaround on passenger 6.…
80d5b61 
…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
@ivoanjo ivoanjo force-pushed the ivoanjo/passenger-detection-improvements branch from 82b7a83 to 80d5b61 Compare November 24, 2023 12:27
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b4c9019) 98.23% compared to head (80d5b61) 98.22%.
Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3280      +/-   ##
==========================================
- Coverage   98.23%   98.22%   -0.01%     
==========================================
  Files        1253     1253              
  Lines       72393    72409      +16     
  Branches     3393     3395       +2     
==========================================
+ Hits        71112    71127      +15     
- Misses       1281     1282       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AlexJF
AlexJF approved these changes Nov 24, 2023
View reviewed changes
Copy link
Contributor

@AlexJF AlexJF left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ivoanjo ivoanjo merged commit 50501bb into master Nov 27, 2023
217 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/passenger-detection-improvements branch November 27, 2023 11:45
@github-actions github-actions bot added this to the 1.18.0 milestone Nov 27, 2023
@TonyCTHsu TonyCTHsu mentioned this pull request Dec 7, 2023
Merged
ivoanjo added a commit that referenced this pull request Jul 2, 2024
@ivoanjo
[PROF-10015] Fix Phusion Passenger detection when not in Gemfile/`g…
5552215 
…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 added a commit that referenced this pull request Jul 2, 2024
@ivoanjo
[PROF-10015] Fix Phusion Passenger detection when not in Gemfile/`g…
9f88ed1 
…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 mentioned this pull request Jul 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexJF AlexJF AlexJF approved these changes

Assignees
No one assigned
Labels
profiling Involves Datadog profiling
Projects
None yet
Milestone
1.18.0
Development

Successfully merging this pull request may close these issues.

Signal-based profiling produces 502 Bad Gateway response from Passenger
3 participants
@ivoanjo @codecov-commenter @AlexJF