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

"Incompatible version of the passenger gem" even when Passenger has the right version #3721

Closed
23tux opened this issue Jun 18, 2024 · 7 comments
Assignees
Labels
bug Involves a bug community Was opened by a community member profiling Involves Datadog profiling
Milestone

Comments

@23tux
Copy link

23tux commented Jun 18, 2024

Current behaviour
We have passenger 6.0.22 installed in our Dockerfile on Ruby 3.3.3-bullseye via

apt-get install -y nginx libnginx-mod-http-passenger

We don't have the passenger gem itself in our Gemfile. The PhusionPassenger constant get's injected by the nginx module.

At startup ddtrace complains with this warning:

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

Expected behaviour
ddtrace should look at the PhusionPassenger::VERSION_STRING constant instead of Gem.loaded_specs['passenger'].version:

private_class_method def self.incompatible_passenger_version?
if Gem.loaded_specs['passenger']
Gem.loaded_specs['passenger'].version < Gem::Version.new('6.0.19')
else
true
end
end

So maybe these lines could be

      private_class_method def self.incompatible_passenger_version?
        if defined?(PhusionPassenger)
          PhusionPassenger::VERSION_STRING < Gem::Version.new('6.0.19')
        else
          true
        end
      end

Steps to reproduce

  • Remove passenger gem from Gemfile if present
  • Install Passenger in nginx via libnginx-mod-http-passenger
  • Start the rails server in passenger, then you should see the warning

Environment

  • datadog version: ddtrace 1.23.1
  • Ruby version: 3.3.3
  • Operating system: Debian bullseye
  • Relevant library versions:
@23tux 23tux added bug Involves a bug community Was opened by a community member labels Jun 18, 2024
@ivoanjo ivoanjo self-assigned this Jun 18, 2024
@ivoanjo
Copy link
Member

ivoanjo commented Jun 18, 2024

Hey! Thanks for getting in touch. Will definitely fix :)

In the meanwhile, it's possible to use DD_PROFILING_NO_SIGNALS_WORKAROUND_ENABLED=false or via code:

Datadog.configure do |c|
  c.profiling.advanced.no_signals_workaround_enabled = false
end

To override the incorrect check!

@ivoanjo ivoanjo added the profiling Involves Datadog profiling label Jun 18, 2024
@23tux
Copy link
Author

23tux commented Jun 20, 2024

Thanks for the quick response, I'll try disable the config and let you know if it worked!

@ivoanjo
Copy link
Member

ivoanjo commented Jun 20, 2024

Oops! I've updated my comment above -- I had a bit too much copy pasta in the code example and I had set it to = true instead of = false. (So make sure to actually set it to false and not true)

@23tux
Copy link
Author

23tux commented Jun 20, 2024

too late 😅 Gonna switch it to false. This double negation stuff is always confusing

@23tux
Copy link
Author

23tux commented Jun 20, 2024

I changed it to false, but I still get this warning

WARN -- ddtrace: [ddtrace] Profiling "no signals" workaround disabled via configuration

I'm not sure what to do here, do you have an idea?

@ivoanjo
Copy link
Member

ivoanjo commented Jun 20, 2024

I'm not sure what to do here, do you have an idea?

That warning confirms that it was disabled successfully, so you're getting the full data quality from the profiler, but I guess the aim was also to get rid of the warning, and I kinda just directed you to replace one warning with the other 😅.

For now you'll have to live with this warning -- I'll open the PR with the fix ASAP so you'll be able to remove the manual disable and have no warnings :)

@23tux
Copy link
Author

23tux commented Jun 20, 2024

Yes, getting rid of the warning would be nice. But anyway, thanks for the quick response, and the main goal is to get full data quality. Then I'll have an eye on the releases and update once the PR is merged. Thanks again!

@23tux 23tux closed this as completed Jun 20, 2024
ivoanjo added a commit that referenced this issue 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 issue 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 ivoanjo added this to the 2.2.0 milestone Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug community Was opened by a community member profiling Involves Datadog profiling
Projects
None yet
Development

No branches or pull requests

2 participants