Merge pull request #2147 from newrelic/it-s_too_bad_she_won-t_live_-_…

…but_then_again_-_who_does

CHANGELOG.md

@@ -2,12 +2,15 @@
 
 ## dev
 
-Version <dev> of the agent introduces improved error tracking functionality by associating a transaction id with each error.
+Version <dev> of the agent introduces improved error tracking functionality by associating a transaction id with each error, and uses more reliable network timeout logic.
 
 - **Feature: Improved error tracking transaction linking**
 
   Errors tracked and sent to the New Relic errors inbox will now be associated with a transaction id to enable improved UI/UX associations between transactions and errors. [PR#2035](https://github.com/newrelic/newrelic-ruby-agent/pull/2035) 
 
+- **Feature: Use Net::HTTP native timeout logic**
+
+  In line with current Ruby best practices, make use of Net::HTTP's own timeout logic and avoid the use of `Timeout.timeout()` when possible. The agent's data transmissions and cloud provider detection routines have been updated accordingly. [PR#2147](https://github.com/newrelic/newrelic-ruby-agent/pull/2147)
 
 ## v9.3.1
 
@@ -23,7 +26,6 @@ Version 9.3.1 of the agent fixes `NewRelic::Agent.require_test_helper`.
 
   Community member [@olleolleolle](https://github.com/olleolleolle) noticed that our source code was referencing a now defunct URL for the Rack specification and submitted [PR#2121](https://github.com/newrelic/newrelic-ruby-agent/pull/2121) to update it. He also provided a terrific recommendation that we automate the checking of links to proactively catch defunct ones in future. Thanks, @olleolleolle!
 
-
 ## v9.3.0
 
 Version 9.3.0 of the agent adds log-level filtering, adds custom attributes for log events, and updates instrumentation for Action Cable. It also provides fixes for how `Fiber` args are treated, Code-Level Metrics, unnecessary files being included in the gem, and `NewRelic::Agent::Logging::DecoratingFormatter#clear_tags!` being incorrectly private.

lib/new_relic/agent/new_relic_service.rb

@@ -3,7 +3,6 @@
 # frozen_string_literal: true
 
 require 'zlib'
-require 'timeout'
 require 'new_relic/agent/audit_logger'
 require 'new_relic/agent/new_relic_service/encoders'
 require 'new_relic/agent/new_relic_service/marshaller'
@@ -19,7 +18,13 @@ class NewRelicService
 
       # These include Errno connection errors, and all indicate that the
       # underlying TCP connection may be in a bad state.
-      CONNECTION_ERRORS = [Timeout::Error, EOFError, SystemCallError, SocketError].freeze
+      CONNECTION_ERRORS = [Net::OpenTimeout, Net::ReadTimeout, EOFError, SystemCallError, SocketError]
+      # TODO: MAJOR VERSION - Net::WriteTimeout wasn't defined until Ruby 2.6.
+      #       Once support for Ruby 2.5 is dropped, we should simply include
+      #       Net::WriteTimeout in the connection errors array directly instead
+      #       of with a conditional
+      CONNECTION_ERRORS << Net::WriteTimeout if defined?(Net::WriteTimeout)
+      CONNECTION_ERRORS.freeze
 
       # The maximum number of times to attempt an HTTP request
       MAX_ATTEMPTS = 2
@@ -319,13 +324,15 @@ def set_cert_store(conn)
 
       def start_connection(conn)
         NewRelic::Agent.logger.debug("Opening TCP connection to #{conn.address}:#{conn.port}")
-        Timeout.timeout(@request_timeout) { conn.start }
-        conn
+        conn.start
       end
 
       def setup_connection_timeouts(conn)
-        # We use Timeout explicitly instead of this
-        conn.read_timeout = nil
+        conn.open_timeout = @request_timeout
+        conn.read_timeout = @request_timeout
+        # TODO: MAJOR VERSION - #write_timeout= requires Ruby 2.6+, so remove
+        #       the conditional check once support for Ruby 2.5 is dropped
+        conn.write_timeout = @request_timeout if conn.respond_to?(:write_timeout=)
 
         if conn.respond_to?(:keep_alive_timeout) && NewRelic::Agent.config[:aggressive_keepalive]
           conn.keep_alive_timeout = NewRelic::Agent.config[:keep_alive_timeout]
@@ -362,8 +369,8 @@ def create_and_start_http_connection
         conn = create_http_connection
         start_connection(conn)
         conn
-      rescue Timeout::Error
-        ::NewRelic::Agent.logger.info('Timeout while attempting to connect. You may need to install system-level CA Certificates, as the ruby agent no longer includes these.')
+      rescue Net::OpenTimeout
+        ::NewRelic::Agent.logger.info('Timed out while attempting to connect. For SSL issues, you may need to install system-level CA Certificates to be used by Net::HTTP.')
         raise
       end
 
@@ -436,21 +443,19 @@ def relay_request(request, opts)
       end
 
       def attempt_request(request, opts)
-        response = nil
         conn = http_connection
         ::NewRelic::Agent.logger.debug("Sending request to #{opts[:collector]}#{opts[:uri]} with #{request.method}")
-        Timeout.timeout(@request_timeout) do
-          response = conn.request(request)
-        end
-        response
+        conn.request(request)
       end
 
       def handle_error_response(response, endpoint)
         case response
         when Net::HTTPRequestTimeOut,
              Net::HTTPTooManyRequests,
              Net::HTTPInternalServerError,
-             Net::HTTPServiceUnavailable
+             Net::HTTPServiceUnavailable,
+             Net::OpenTimeout,
+             Net::ReadTimeout
           handle_server_connection_exception(response, endpoint)
         when Net::HTTPBadRequest,
              Net::HTTPForbidden,
@@ -471,9 +476,20 @@ def handle_error_response(response, endpoint)
         when Net::HTTPGone
           handle_gone_response(response, endpoint)
         else
-          record_endpoint_attempts_supportability_metrics(endpoint)
-          record_error_response_supportability_metrics(response.code)
-          raise UnrecoverableServerException, "#{response.code}: #{response.message}"
+          # TODO: MAJOR VERSION - Net::WriteTimeout wasn't defined until
+          #       Ruby 2.6, so it can't be included in the case statement
+          #       as a constant and instead needs to be found here. Once
+          #       support for Ruby 2.5 is dropped, we should have
+          #       Net::WriteTimeout sit in the 'when' clause above alongside
+          #       Net::OpenTimeout and Net::ReadTimeout and this entire if/else
+          #       conditional can be removed.
+          if response.respond_to?(:name) && response.name == 'Net::WriteTimeout'
+            handle_server_connection_exception(response, endpoint)
+          else
+            record_endpoint_attempts_supportability_metrics(endpoint)
+            record_error_response_supportability_metrics(response.code)
+            raise UnrecoverableServerException, "#{response.code}: #{response.message}"
+          end
         end
         response
       end

lib/new_relic/agent/utilization/vendor.rb

@@ -75,14 +75,12 @@ def request_metadata
           processed_headers = headers
           raise if processed_headers.value?(:error)
 
-          Timeout.timeout(1) do
-            response = nil
-            Net::HTTP.start(endpoint.host, endpoint.port) do |http|
-              req = Net::HTTP::Get.new(endpoint, processed_headers)
-              response = http.request(req)
-            end
-            response
+          response = nil
+          Net::HTTP.start(endpoint.host, endpoint.port, open_timeout: 1, read_timeout: 1) do |http|
+            req = Net::HTTP::Get.new(endpoint, processed_headers)
+            response = http.request(req)
           end
+          response
         rescue
           NewRelic::Agent.logger.debug("#{vendor_name} environment not detected")
         end

test/multiverse/suites/agent_only/utilization_data_collection_test.rb

@@ -96,12 +96,12 @@ def redirect_link_local_address(port)
       @dummy_port = p
 
       class << self
-        def start_with_patch(address, port = nil, p_addr = nil, p_port = nil, p_user = nil, p_pass = nil, &block)
+        def start_with_patch(address, port, *_args, &block)
           if address == '169.254.169.254'
             address = 'localhost'
             port = @dummy_port
           end
-          start_without_patch(address, port, p_addr, p_port, p_user, p_pass, &block)
+          start_without_patch(address, port, &block)
         end
 
         alias_method :start_without_patch, :start

test/new_relic/agent/new_relic_service_test.rb

@@ -31,7 +31,7 @@ def create_http_handle(name = 'connection')
   end
 
   def test_session_handles_timeouts_opening_connection_gracefully
-    @http_handle.stubs(:start).raises(Timeout::Error)
+    @http_handle.stubs(:start).raises(Net::OpenTimeout)
 
     block_ran = false