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

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

Merged
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
…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.

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
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.

Signal-based profiling produces 502 Bad Gateway response from Passenger
3 participants