Skip to content

Commit

Permalink
Fix profiler wrongly detecting that google-protobuf is installed
Browse files Browse the repository at this point in the history
**What does this PR do?**:

The `google-protobuf` gem is a dependency needed by the profiler
(that we plan on removing when we fully move to libdatadog), but that
is not a dependency of the `ddtrace` gem.

Because it's not a dependency of `ddtrace`, we need to gracefully handle
the case where this gem is missing. Thus, before requiring any files
from this gem, the profiler uses rubygems to check if the gem is
available.

But, in #1506, a customer asked us to allow skipping this API call
to rubygems, since it didn't work on their setup. Instead, we added
an extra check -- if the `::Google::Protobuf` module is available,
then we assume protobuf is installed.

**Very very unfortunately**, there's actually another gem, called
`protobuf` (or `protobuf-cucumber`, its fork) that is not compatible
with `ddtrace` BUT STILL defines `::Google::Protobuf`.

This is _horrifying_ -- having two different gems that define different
versions of a module with the same name.

But in this very bizarre situation, the `google-protobuf` detection
code got confused, and one internal customer reported that he saw
the following error on their logs:

> [DDTRACE] Error while loading google-protobuf gem. Cause: 'LoadError
> cannot load such file -- google/protobuf' Location:
> 'lib/datadog/profiling.rb:106:in `require''. This can happen when
> google-protobuf is missing its native components. [...]
```

To fix this, I've tweaked the `google-protobuf` detection to also
check for the `::Protobuf` module. This top-level module is never
defined by `google-protobuf`, and we can use it to distinguish
between the two gems.

**Motivation**:

Avoid misleading error message when loading `ddtrace` with
`protobuf`/`protobuf-cucumber` installed.

**Additional Notes**:

I'm looking forward to dropping this whole `google-protobuf` code
once the new profiler becomes the default for all customers.

**How to test the change?**:

The following trivial Ruby app is able to trigger the issue.

* gems.rb:

```ruby
source 'http://rubygems.org'
gem 'protobuf-cucumber'
gem 'ddtrace'
```

* example.rb:

```ruby
require 'protobuf'
require 'ddtrace'
puts "Works!"
```
  • Loading branch information
ivoanjo committed Feb 2, 2023
1 parent 2f31cd8 commit 47340ba
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 2 deletions.
8 changes: 6 additions & 2 deletions lib/datadog/profiling.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ def self.start_if_enabled
# NOTE: On environments where protobuf is already loaded, we skip the check. This allows us to support environments
# where no Gem.loaded_version is NOT available but customers are able to load protobuf; see for instance
# https://github.com/teamcapybara/capybara/commit/caf3bcd7664f4f2691d0ca9ef3be9a2a954fecfb
if !defined?(::Google::Protobuf) && Gem.loaded_specs['google-protobuf'].nil?
if !protobuf_already_loaded? && Gem.loaded_specs['google-protobuf'].nil?
"Missing google-protobuf dependency; please add `gem 'google-protobuf', '~> 3.0'` to your Gemfile or gems.rb file"
end
end
Expand All @@ -74,12 +74,16 @@ def self.start_if_enabled
# See above for why we skip the check when protobuf is already loaded; note that when protobuf was already loaded
# we skip the version check to avoid the call to Gem.loaded_specs. Unfortunately, protobuf does not seem to
# expose the gem version constant elsewhere, so in that setup we are not able to check the version.
if !defined?(::Google::Protobuf) && Gem.loaded_specs['google-protobuf'].version < GOOGLE_PROTOBUF_MINIMUM_VERSION
if !protobuf_already_loaded? && Gem.loaded_specs['google-protobuf'].version < GOOGLE_PROTOBUF_MINIMUM_VERSION
'Your google-protobuf is too old; ensure that you have google-protobuf >= 3.0 by ' \
"adding `gem 'google-protobuf', '~> 3.0'` to your Gemfile or gems.rb file"
end
end

private_class_method def self.protobuf_already_loaded?
defined?(::Google::Protobuf) && !defined?(::Protobuf)
end

private_class_method def self.protobuf_failed_to_load?
unless protobuf_loaded_successfully?
'There was an error loading the google-protobuf library; see previous warning message for details'
Expand Down
10 changes: 10 additions & 0 deletions spec/datadog/profiling_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,16 @@
end

it { is_expected.to be nil }

context "but it's the protobuf/cucumber-protobuf gem instead of google-protobuf" do
include_context 'loaded gems', :'google-protobuf' => nil

before do
stub_const('::Protobuf', Module.new)
end

it { is_expected.to include 'Missing google-protobuf' }
end
end
end
end
Expand Down

0 comments on commit 47340ba

Please sign in to comment.