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

Skip Gem.loaded_specs calls if protobuf is already loaded #1506

Merged
merged 1 commit into from
May 19, 2021

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented May 19, 2021

On environments where protobuf is already loaded, we skip any interaction with Gem.loaded_specs. This allows us to support customers that don't load gems using rubygems but still have all the needed dependencies.

This is similar to
teamcapybara/capybara@caf3bcd

The change is quite small so I think it's worth it.

On environments where protobuf is already loaded, we skip any
interaction with `Gem.loaded_specs`. This allows us to support
customers that don't load gems using rubygems but still have all the
needed dependencies.

This is similar to
teamcapybara/capybara@caf3bcd
@ivoanjo ivoanjo requested a review from a team May 19, 2021 08:19
@ivoanjo ivoanjo merged commit 8434b6c into master May 19, 2021
@ivoanjo ivoanjo deleted the skip-checks-if-protobuf-already-loaded branch May 19, 2021 08:45
@github-actions github-actions bot added this to the 0.50.0 milestone May 19, 2021
ivoanjo added a commit that referenced this pull request Feb 2, 2023
**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!"
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants