Automatically enable profiler "no signals" workaround for passenger web server #2978
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**What does this PR do?**: This PR adds one more case where we automatically need to apply the profiler "no signals" workaround (added in #2873): when the Ruby app is running inside the passenger web server. **Motivation**: This makes sure the profiler does not cause issues out-of-the-box for customers running passenger. **Additional Notes**: Passenger detection was inspired on https://stackoverflow.com/a/12246652/4432562 . This was reported in #2976 and I suspect at least one other customer has ran into this issue. Separate from this PR, I plan to improve our documentation, as well as try to get in touch with the passenger folks to get a fix upstream. I'm tracking that work in the mentioned issue. I'm also considering updating the integration apps to include passenger, but I wanted to get in the fix asap, and will circle back to that one in a separate PR if I can get it to work sanely. **How to test the change?**: Running the test-case submitted for #2976: https://github.com/elliterate/ddtrace-passenger-example can be used to validate that the profiler auto-enables the no signals workaround.
ivoanjo
changed the title
marcotc
approved these changes
Jul 18, 2023
ivoanjo
added a commit
that referenced
this pull request
Jul 25, 2023
**What does this PR do?**: This PR adds one more webserver to the rack integration testing app: Phusion Passenger. **Motivation**: In #2976 a customer reported an issue with profiling apps running on passenger. I'm adding passenger to our test suite to avoid any future regressions. **Additional Notes**: I've actually tested that the customer issue does show up on our integration app if I revert the changes from #2978. **How to test the change?**: Validate that the rack CI app is running with passenger as well.
Merged
ivoanjo
added a commit
that referenced
this pull request
Jul 2, 2024
…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
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
This PR adds one more case where we automatically need to apply the profiler "no signals" workaround (added in #2873): when the Ruby app is running inside the passenger web server.
Motivation:
This makes sure the profiler does not cause issues out-of-the-box for customers running passenger.
Additional Notes:
Passenger detection was inspired on https://stackoverflow.com/a/12246652/4432562 .
This was reported in #2976 and
I suspect(now confirmed) at least one other customer has ran into this issue.Separate from this PR, I plan to improve our documentation, as well as try to get in touch with the passenger folks to get a fix upstream. I'm tracking that work in the mentioned issue.
I'm also considering updating the integration apps to include passenger, but I wanted to get in the fix asap, and will circle back to that one in a separate PR if I can get it to work sanely.
How to test the change?:
Running the test-case submitted for #2976: https://github.com/elliterate/ddtrace-passenger-example can be used to validate that the profiler auto-enables the no signals workaround.