improve usage of the 'hostname' executable, others

* Do not shell out directly with `%x()` to any executable. Instead, use
the new Open3 based `NewRelic::Helper.run_command` method.
* The new `NewRelic::Helper.run_command` should prevent any unwanted
error messages pertaining to binary execution from appearing in any
output or log unless the log level is set to 'debug'.
* When executing an external binary, make sure it exists first
* If the 'hostname' binary does not exist, have
`NewRelic::Agent::Hostname.get_fqdn` fall back to `get`, which uses
`Socket`.
* When using 'uname' for JRuby to determine the platform, use 'unknown'
if the 'uname' call fails
* Introduce new `NewRelic::CommandExcutableNotFoundError` and
`NewRelic::CommandRunFailedError` error classes to help with logic flow.
* Bring in 'minitest-stub-const' as a development dependency for
stubbing constants

resolves #697

Thank you to @metaskills for submitting #697 and pointing out that
'hostname' will not be available to AWS Lambda Ruby functions.

Thank you to @brcarp for suggesting the use of Open3 as an improvement
over `%x()` to handle output.

Thank you to both @metaskills and @brcarp for their continued input and
maintainer collaboration on issue #697

lib/new_relic/agent/hostname.rb

@@ -2,6 +2,7 @@
 # This file is distributed under New Relic's license terms.
 # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
 require 'socket'
+require 'new_relic/helper'
 
 module NewRelic
   module Agent
@@ -17,17 +18,27 @@ def self.get
         end
       end
 
-      # calling hostname with -f on some OS's (NetBSD, FreeBSD, Solaris)
-      # produces invalid option error, but doesn't raise exception.  Instead,
-      # we get back empty string.  So, solution here is to check for non-zero
-      # exit status and retry the command without the -f flag.
+      # Pass '-f' to the external executable 'hostname' to request the fully
+      # qualified domain name (fqdn). For implementations of 'hostname' that
+      # do not support '-f' (such as the one OpenBSD ships with), fall back
+      # to calling 'hostname' without the '-f'. If both ways of calling
+      # 'hostname' fail, or in a context where 'hostname' is not even
+      # available (within an AWS Lambda function, for example), call the
+      # 'get' method which uses Socket instead of an external executable.
       def self.get_fqdn
-        fqdn = %x(hostname -f 2>/dev/null).chomp!
-        fqdn = %x(hostname).chomp! unless $?.exitstatus.zero?
-        fqdn
-      rescue => e
-        NewRelic::Agent.logger.debug "Unable to determine fqdn #{e}"
-        nil
+        begin
+          NewRelic::Helper.run_command('hostname -f')
+        rescue NewRelic::CommandRunFailedError
+          NewRelic::Helper.run_command('hostname')
+        end
+      rescue NewRelic::CommandExecutableNotFoundError, NewRelic::CommandRunFailedError => e
+        message = if e.class == NewRelic::CommandExecutableNotFoundError
+          "'hostname' executable not found"
+        else
+          "'hostname' command failed - #{e.message}"
+        end
+        NewRelic::Agent.logger.debug message
+        get
       end
 
       def self.heroku_dyno_name_prefix(dyno_name)

lib/new_relic/agent/samplers/memory_sampler.rb

@@ -3,6 +3,7 @@
 # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
 
 require 'new_relic/agent/sampler'
+require 'new_relic/helper'
 
 module NewRelic
   module Agent
@@ -46,7 +47,11 @@ def self.supported_on_this_platform?
 
         def self.platform
           if RUBY_PLATFORM =~ /java/
-            %x(uname -s).downcase
+            begin
+              NewRelic::Helper.run_command('uname -s').downcase
+            rescue NewRelic::CommandRunFailedError, NewRelic::CommandExecutableNotFoundError
+              'unknown'
+            end
           else
             RUBY_PLATFORM.downcase
           end

lib/new_relic/helper.rb

@@ -3,8 +3,12 @@
 # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
 
 require 'new_relic/language_support'
+require 'open3'
 
 module NewRelic
+  class CommandExecutableNotFoundError < StandardError; end
+  class CommandRunFailedError < StandardError; end
+
   # A singleton for shared generic helper methods
   module Helper
     extend self
@@ -40,5 +44,24 @@ def instance_methods_include?(klass, method_name)
     def time_to_millis(time)
       (time.to_f * 1000).round
     end
+
+    def run_command(command)
+      executable = command.split(' ').first
+      raise NewRelic::CommandExecutableNotFoundError.new(executable) unless executable_in_path?(executable)
+
+      output, status = Open3.capture2e(command)
+      raise NewRelic::CommandRunFailedError.new(output) unless status.success?
+
+      output.chomp
+    end
+
+    def executable_in_path?(executable)
+      return false unless ENV['PATH']
+
+      !ENV['PATH'].split(File::PATH_SEPARATOR).detect do |bin_path|
+        executable_path = File.join(bin_path, executable)
+        File.exist?(executable_path) && File.file?(executable_path) && File.executable?(executable_path)
+      end.nil?
+    end
   end
 end

newrelic_rpm.gemspec

@@ -52,6 +52,7 @@ https://github.com/newrelic/newrelic-ruby-agent/
   s.add_development_dependency 'rb-inotify', '0.9.10' # locked to support < Ruby 2.3 (and listen 3.0.8)
   s.add_development_dependency 'listen', '3.0.8' # locked to support < Ruby 2.3
   s.add_development_dependency 'minitest', '4.7.5'
+  s.add_development_dependency 'minitest-stub-const', '0.6'
   s.add_development_dependency 'mocha', '~> 1.9.0'
   s.add_development_dependency 'yard'
   s.add_development_dependency 'pry-nav', '~> 0.3.0'

test/new_relic/agent/hostname_test.rb

@@ -17,11 +17,6 @@ def teardown
         NewRelic::Agent::Hostname.instance_variable_set(:@hostname, nil)
       end
 
-      def test_get_fqdn
-        fqdn = NewRelic::Agent::Hostname.get_fqdn.to_s
-        refute_equal '', fqdn
-      end
-
       def test_get_returns_socket_hostname
         assert_equal 'Rivendell', NewRelic::Agent::Hostname.get
       end
@@ -120,6 +115,48 @@ def with_dyno_name(dyno_name, config_options)
       ensure
         ENV.delete('DYNO')
       end
+
+      # begin fqdn tests
+
+      # allow the real fqdn determination code to fire and make sure it works
+      def test_get_fqdn_no_stubs
+        fqdn = NewRelic::Agent::Hostname.get_fqdn
+        refute_equal '', fqdn
+      end
+
+      # 'hostname -f' succeeds
+      def test_get_fqdn_hostname_f
+        stubbed = 'Rivendell.Eriador.MiddleEarth'
+        NewRelic::Helper.stubs('run_command').with('hostname -f').returns(stubbed)
+        fqdn = NewRelic::Agent::Hostname.get_fqdn
+        assert_equal stubbed, fqdn
+      end
+
+      # 'hostname -f' fails, 'hostname' succeeds
+      def test_get_fqdn_hostname
+        stubbed = 'Lauterbrunnen.Switzerland.Earth'
+        NewRelic::Helper.stubs('run_command').with('hostname -f').raises(NewRelic::CommandRunFailedError)
+        NewRelic::Helper.stubs('run_command').with('hostname').returns(stubbed)
+        fqdn = NewRelic::Agent::Hostname.get_fqdn
+        assert_equal stubbed, fqdn
+      end
+
+      # 'hostname -f' and 'hostname' both fail
+      def test_get_fqdn_hostname_fails
+        NewRelic::Helper.stubs('run_command').with('hostname -f').raises(NewRelic::CommandRunFailedError)
+        NewRelic::Helper.stubs('run_command').with('hostname').raises(NewRelic::CommandRunFailedError)
+        fqdn = NewRelic::Agent::Hostname.get_fqdn
+        assert_equal 'Rivendell', fqdn # stubbed in 'setup' above
+      end
+
+      # the 'hostname' executable doesn't even exist
+      def test_get_fqdn_hostname_nonexistent
+        NewRelic::Helper.stubs('run_command').with('hostname -f').raises(NewRelic::CommandExecutableNotFoundError)
+        fqdn = NewRelic::Agent::Hostname.get_fqdn
+        assert_equal 'Rivendell', fqdn # stubbed in 'setup' above
+      end
+
+      # end fqdn tests
     end
   end
 end

test/new_relic/agent/samplers/memory_sampler_test.rb

@@ -55,6 +55,34 @@ def test_memory__is_supported
     assert !NewRelic::Agent::Samplers::MemorySampler.supported_on_this_platform? || defined? JRuby
   end
 
+  # leverage the 'uname' binary when running on JRuby
+  def test_platform_uses_uname_for_jruby
+    stubbed = 'MCP'
+    NewRelic::Helper.stubs('run_command').with('uname -s').returns(stubbed)
+    NewRelic::Agent::Samplers::MemorySampler.stub_const(:RUBY_PLATFORM, 'java') do
+      platform = NewRelic::Agent::Samplers::MemorySampler.platform
+      assert_equal platform, stubbed.downcase
+    end
+  end
+
+  # if using 'uname' fails, use 'unknown' for the platform
+  def test_platform_uses_unknown_if_uname_fails
+    NewRelic::Helper.stubs('run_command').with('uname -s').raises(NewRelic::CommandRunFailedError)
+    NewRelic::Agent::Samplers::MemorySampler.stub_const(:RUBY_PLATFORM, 'java') do
+      platform = NewRelic::Agent::Samplers::MemorySampler.platform
+      assert_equal platform, 'unknown'
+    end
+  end
+
+  # use RUBY_PLATFORM for the platform for CRuby
+  def test_platform_uses_ruby_platform
+    stubbed = 'ENCOM OS-12'
+    NewRelic::Agent::Samplers::MemorySampler.stub_const(:RUBY_PLATFORM, stubbed) do
+      platform = NewRelic::Agent::Samplers::MemorySampler.platform
+      assert_equal platform, stubbed.downcase
+    end
+  end
+
   def stub_sampler_get_memory
     if defined? JRuby
       NewRelic::Agent::Samplers::MemorySampler::JavaHeapSampler.any_instance.stubs(:get_memory).returns 333

test/new_relic/helper_test.rb

@@ -0,0 +1,66 @@
+# encoding: utf-8
+# This file is distributed under New Relic's license terms.
+# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
+
+require File.expand_path(File.join(File.dirname(__FILE__), '..', 'test_helper'))
+
+# tests NewRelic::Helper
+class HelperTest < Minitest::Test
+  #
+  # executable_in_path?
+  #
+  def test_executable_is_in_path
+    executable = 'seagulls'
+    fake_dir = '/usr/local/existent'
+    ENV.stubs(:[]).with('PATH').returns(fake_dir)
+
+    executable_path = File.join(fake_dir, executable)
+    File.stubs(:exist?).with(executable_path).returns(true)
+    File.stubs(:file?).with(executable_path).returns(true)
+    File.stubs(:executable?).with(executable_path).returns(true)
+    exists = NewRelic::Helper.executable_in_path?(executable)
+    assert_truthy exists
+  end
+
+  def test_executable_is_not_in_path
+    executable = 'seagulls'
+    fake_dir = '/dev/null/nonexistent'
+    ENV.stubs(:[]).with('PATH').returns(fake_dir)
+    executable_path = File.join(fake_dir, executable)
+    File.stubs(:exist?).with(executable_path).returns(false)
+    exists = NewRelic::Helper.executable_in_path?(executable)
+    assert_false exists
+  end
+
+  def test_path_does_not_exist
+    ENV.stubs(:[]).with('PATH').returns(nil)
+    exists = NewRelic::Helper.executable_in_path?('Whisper of the Heart')
+    assert_false exists
+  end
+
+  #
+  # run_command
+  #
+  def test_run_command_when_executable_does_not_exist
+    NewRelic::Helper.stubs('executable_in_path?').returns(false)
+    assert_raises(NewRelic::CommandExecutableNotFoundError) do
+      NewRelic::Helper.run_command('mksh -v')
+    end
+  end
+
+  def test_run_commmand_happy
+    stubbed = 'Jinba ittai'
+    NewRelic::Helper.stubs('executable_in_path?').returns(true)
+    Open3.stubs('capture2e').returns([stubbed, OpenStruct.new(success?: true)])
+    result = NewRelic::Helper.run_command('figlet Zoom Zoom')
+    assert_equal result, stubbed
+  end
+
+  def test_run_command_sad
+    NewRelic::Helper.stubs('executable_in_path?').returns(true)
+    Open3.stubs('capture2e').returns([nil, OpenStruct.new(success?: false)])
+    assert_raises(NewRelic::CommandRunFailedError) do
+      NewRelic::Helper.run_command('find / -name tetris')
+    end
+  end
+end

test/test_helper.rb

@@ -15,6 +15,7 @@ module NewRelic; TEST = true; end unless defined? NewRelic::TEST
 require 'rake'
 
 require 'minitest/autorun'
+require 'minitest/stub_const'
 require 'mocha/setup'
 
 require 'hometown'