Merge pull request #2175 from newrelic/status_code_without_middleware…

…_instrumentation

Record status codes when middleware instrumentation is disabled

CHANGELOG.md

@@ -1,5 +1,18 @@
 # New Relic Ruby Agent Release Notes
 
+
+## dev
+
+Version <dev> allows the agent to record additional response information on a transaction when middleware instrumentation is disabled and fixes a bug in `NewRelic::Rack::AgentHooks.needed?`.
+
+
+- **Feature: Report transaction HTTP status codes when middleware instrumentation is disabled**
+  Previously, when `disable_middleware_instrumentation` was set to `true`, the agent would not record the value of the response code or content type on the transaction. This was due to the possibility that a middleware could alter the response, which would not be captured by the agent when the middleware instrumentation was disabled. However, based on customer feedback, the agent will now report the HTTP status code and content type on a transaction when middleware instrumentation is disabled. [PR#2175](https://github.com/newrelic/newrelic-ruby-agent/pull/2175)
+
+- **Bugfix: Resolve inverted logic of NewRelic::Rack::AgentHooks.needed?**
+  Previously, `NewRelic::Rack::AgentHooks.needed?` incorrectly used inverted logic. This has now been resolved, allowing AgentHooks to be installed when `disable_middleware_instrumentation` is set to true. [PR#2175](https://github.com/newrelic/newrelic-ruby-agent/pull/2175)
+
+
 ## v9.4.2
 
 Version 9.4.2 of the agent re-addresses the 9.4.0 issue of `NoMethodError` seen when using the `uppy-s3_multipart` gem.

lib/new_relic/agent/configuration/default_source.rb

@@ -1212,7 +1212,13 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil)
           :public => true,
           :type => Boolean,
           :allowed_from_server => false,
-          :description => 'If `true`, the agent won\'t wrap third-party middlewares in instrumentation (regardless of whether they are installed via `Rack::Builder` or Rails).'
+          :description => <<~DESCRIPTION
+            If `true`, the agent won't wrap third-party middlewares in instrumentation (regardless of whether they are installed via `Rack::Builder` or Rails).
+
+              <Callout variant="important">
+                When middleware instrumentation is disabled, if an application is using middleware that could alter the response code, the HTTP status code reported on the transaction may not reflect the altered value.
+              </Callout>
+          DESCRIPTION
         },
         :disable_samplers => {
           :default => false,

lib/new_relic/rack/agent_hooks.rb

@@ -23,7 +23,7 @@ module NewRelic::Rack
   #
   class AgentHooks < AgentMiddleware
     def self.needed?
-      !NewRelic::Agent.config[:disable_middleware_instrumentation]
+      NewRelic::Agent.config[:disable_middleware_instrumentation]
     end
 
     def traced_call(env)

lib/new_relic/rack/agent_middleware.rb

@@ -26,22 +26,6 @@ def build_transaction_name
         prefix = ::NewRelic::Agent::Instrumentation::ControllerInstrumentation::TransactionNamer.prefix_for_category(nil, @category)
         "#{prefix}#{self.class.name}/call"
       end
-
-      # If middleware tracing is disabled, we'll still inject our agent-specific
-      # middlewares, and still trace those, but we don't want to capture HTTP
-      # response codes, since middleware that's outside of ours might change the
-      # response code before it goes back to the client.
-      def capture_http_response_code(state, result)
-        return if NewRelic::Agent.config[:disable_middleware_instrumentation]
-
-        super
-      end
-
-      def capture_response_content_type(state, result)
-        return if NewRelic::Agent.config[:disable_middleware_instrumentation]
-
-        super
-      end
     end
   end
 end

test/multiverse/suites/rack/http_response_code_test.rb

@@ -35,17 +35,17 @@ def test_records_http_response_code_on_analytics_events
       assert_equal(302, get_last_analytics_event[2][:'http.statusCode'])
     end
 
-    def test_skips_http_response_code_if_middleware_tracing_disabled
+    def test_records_http_response_code_if_middleware_tracing_disabled
       with_config(:disable_middleware_instrumentation => true) do
         rsp = get('/', {'override-response-code' => 404})
 
         assert_equal(404, rsp.status)
-        refute get_last_analytics_event[2][:'http.statusCode']
+        assert get_last_analytics_event[2][:'http.statusCode']
 
         rsp = get('/', {'override-response-code' => 302})
 
         assert_equal(302, rsp.status)
-        refute get_last_analytics_event[2][:'http.statusCode']
+        assert get_last_analytics_event[2][:'http.statusCode']
       end
     end
   end

test/multiverse/suites/rack/response_content_type_test.rb

@@ -35,17 +35,17 @@ def test_records_response_content_type_on_analytics_events
       assert_equal('application/xml', get_last_analytics_event[2][:'response.headers.contentType'])
     end
 
-    def test_skips_response_content_type_if_middleware_tracing_disabled
+    def test_records_response_content_type_if_middleware_tracing_disabled
       with_config(:disable_middleware_instrumentation => true) do
         rsp = get('/', {'override-content-type' => 'application/json'})
 
         assert_equal('application/json', rsp.headers['Content-Type'])
-        refute get_last_analytics_event[2][:'response.headers.contentType']
+        assert get_last_analytics_event[2][:'response.headers.contentType']
 
         rsp = get('/', {'override-content-type' => 'application/xml'})
 
         assert_equal('application/xml', rsp.headers['Content-Type'])
-        refute get_last_analytics_event[2][:'response.headers.contentType']
+        assert get_last_analytics_event[2][:'response.headers.contentType']
       end
     end
   end