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

Create helper to access Bundler.rubygems specs #2906

Merged
merged 7 commits into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

## dev

Version <dev> updates View Componment instrumentation to use a default metric name when one is unavaliable and resolves a bug in rdkafka instrumentation when using the karafka-rdkafka gem.
Version <dev> updates View Componment instrumentation to use a default metric name when one is unavaliable, resolves a bug in rdkafka instrumentation when using the karafka-rdkafka gem and fixes a bug with Grape instrumentation.

- **Feature: Use default `View/component` metric name for unidentified View Components**

Expand All @@ -12,6 +12,9 @@ Version <dev> updates View Componment instrumentation to use a default metric na

Due to version differences between the rdkafka gem and karafka-rdkafka gem, the agent could encounter an error when it tried to install rdkafka instrumentation. This has now been resolved. Thank you to @krisdigital for bringing this issue to our attention. [PR#2880](https://github.com/newrelic/newrelic-ruby-agent/pull/2880)

- **Bugfix: Stop calling deprecated all_specs method to check for the presence of newrelic-grape**

In 9.14.0, we released a fix for calls to the deprecated `Bundler.rubygems.all_specs`, but the fix fell short for the agent's Grape instrumentation and deprecation warnings could still be raised. The condition has been simplified and deprecation warnings should no longer be raised. Thank you, [@excelsior](https://github.com/excelsior) for bringing this to our attention. [Issue#](https://github.com/newrelic/newrelic-ruby-agent/issues/2885) [PR#2906](https://github.com/newrelic/newrelic-ruby-agent/pull/2906)

## v9.14.0

Expand Down
4 changes: 1 addition & 3 deletions lib/new_relic/agent/instrumentation/grape.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@

depends_on do
begin
if defined?(Bundler) &&
((Bundler.rubygems.respond_to?(:installed_specs) && Bundler.rubygems.installed_specs.map(&:name).include?('newrelic-grape')) ||
Bundler.rubygems.all_specs.map(&:name).include?('newrelic-grape'))
if NewRelic::Helper.rubygems_specs.map(&:name).include?('newrelic-grape')
NewRelic::Agent.logger.info('Not installing New Relic supported Grape instrumentation because the third party newrelic-grape gem is present')
false
else
Expand Down
6 changes: 1 addition & 5 deletions lib/new_relic/control/frameworks/rails4.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,7 @@ class Control
module Frameworks
class Rails4 < NewRelic::Control::Frameworks::Rails3
def rails_gem_list
if Bundler.rubygems.respond_to?(:installed_specs)
Bundler.rubygems.installed_specs.map { |gem| "#{gem.name} (#{gem.version})" }
else
Bundler.rubygems.all_specs.map { |gem| "#{gem.name} (#{gem.version})" }
end
NewRelic::Helper.rubygems_specs.map { |gem| "#{gem.name} (#{gem.version})" }
end

def append_plugin_list
Expand Down
6 changes: 1 addition & 5 deletions lib/new_relic/environment_report.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,7 @@ def self.registered_reporters=(logic)
####################################
report_on('Gems') do
begin
if Bundler.rubygems.respond_to?(:installed_specs)
Bundler.rubygems.installed_specs.map { |gem| "#{gem.name}(#{gem.version})" }
else
Bundler.rubygems.all_specs.map { |gem| "#{gem.name}(#{gem.version})" }
end
NewRelic::Helper.rubygems_specs.map { |gem| "#{gem.name}(#{gem.version})" }
rescue
# There are certain rubygem, bundler, rails combinations (e.g. gem
# 1.6.2, rails 2.3, bundler 1.2.3) where the code above throws an error
Expand Down
15 changes: 15 additions & 0 deletions lib/new_relic/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,20 @@ def executable_in_path?(executable)
File.exist?(executable_path) && File.file?(executable_path) && File.executable?(executable_path)
end
end

# Bundler version 2.5.12 deprecated all_specs and added installed_specs.
# To support newer Bundler versions, try to use installed_specs first,
# then fall back to all_specs.
# All callers expect this to be an array, so return an array if Bundler isn't defined
# @api private
def rubygems_specs
return [] unless defined?(Bundler)

if Bundler.rubygems.respond_to?(:installed_specs)
Bundler.rubygems.installed_specs
else
Bundler.rubygems.all_specs
end
end
end
end
6 changes: 1 addition & 5 deletions lib/new_relic/language_support.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,7 @@ def snakeize(string)
def bundled_gem?(gem_name)
return false unless defined?(Bundler)

if Bundler.rubygems.respond_to?(:installed_specs)
Bundler.rubygems.installed_specs.map(&:name).include?(gem_name)
else
Bundler.rubygems.all_specs.map(&:name).include?(gem_name)
end
NewRelic::Helper.rubygems_specs.map(&:name).include?(gem_name)
rescue => e
::NewRelic::Agent.logger.info("Could not determine if third party #{gem_name} gem is installed", e)
false
Expand Down
24 changes: 24 additions & 0 deletions test/new_relic/helper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,4 +77,28 @@ def test_run_command_sad_exception
NewRelic::Helper.run_command('executable that existed at detection time but is not there now')
end
end

#
# rubygems_specs
#
def test_rubygems_specs_returns_empty_array_without_bundler
stub(:defined?, nil, ['Bundler']) do
result = NewRelic::Helper.rubygems_specs

assert_instance_of Array, result
assert_empty Array, result
end
end

def test_rubygems_specs_works_with_all_specs_when_installed_specs_is_absent
Bundler.rubygems.stub(:respond_to?, nil) do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why, but Rubies 2.6 and below don't like the third argument for a stub in this scenario. However, they're fine with it in the test above. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that the older Rubies are given an older Minitest. Wanting to use 3 arguments was the primary reason for the introduction of our skip_unless_minitest5_or_above helper.

assert_equal Bundler.rubygems.all_specs, NewRelic::Helper.rubygems_specs
end
end

def test_rubygems_specs_works_with_installed_specs
skip 'running a version of Bundler that has not defined installed_specs' unless Bundler.rubygems.respond_to?(:installed_specs)

assert_equal Bundler.rubygems.installed_specs, NewRelic::Helper.rubygems_specs
end
end
Loading