From 281dc5269ba01206af5cc9afb7be0dfc90a62d1a Mon Sep 17 00:00:00 2001 From: fallwith Date: Wed, 18 Oct 2023 13:06:21 -0700 Subject: [PATCH] Rails fixes These Rails fixes were driven by extensive testing of the newly released Rails version 7.1, but some may be relevant to older Rails versions as well. *lib/* - When adding new segment attributes, anticipate and ignore numeric parameter keys - For `Transaction#finish`, don't attempt to invoke methods on a non-existent initial segment *test/* - Inline controller method rendering with a proc was a thing in early Rails versions that was no longer supported in Rails 3. We were defining our own `proc_render` controller method to simulate the old style behavior and it finally caused issues for us in Rails 7. Given that we don't even support Rails versions older than 4 now, let's just stop testing the Rails 2 and below proc functionality. - `ActiveJobTest`: a single test needed an additional `expect` - Remove the temporary `before_suite.rb` based hacks for the Rails multiverse suite. - Modify the `ActionController::Live` RUM test to set a `last-modified` header on the streaming response. This prevents Rack v3 from attempting to call `to_ary` on the streaming response object returned. In the future we could author a better streaming test that properly chunks data and possibly even uses a layout. But for now, this header fix gives us Rack 3 compatibility with the old test. resolves #2254 --- lib/new_relic/agent.rb | 4 +++- lib/new_relic/agent/transaction.rb | 2 +- .../rails/action_controller_live_rum_test.rb | 1 + .../multiverse/suites/rails/activejob_test.rb | 2 +- test/multiverse/suites/rails/before_suite.rb | 23 ------------------- .../suites/rails/view_instrumentation_test.rb | 17 ++------------ 6 files changed, 8 insertions(+), 41 deletions(-) delete mode 100644 test/multiverse/suites/rails/before_suite.rb diff --git a/lib/new_relic/agent.rb b/lib/new_relic/agent.rb index dcf33aed8d..2edaacf191 100644 --- a/lib/new_relic/agent.rb +++ b/lib/new_relic/agent.rb @@ -633,7 +633,9 @@ def add_custom_attributes(params) # THREAD_LOCAL_ACCESS def add_new_segment_attributes(params, segment) # Make sure not to override existing segment-level custom attributes segment_custom_keys = segment.attributes.custom_attributes.keys.map(&:to_sym) - segment.add_custom_attributes(params.reject { |k, _v| segment_custom_keys.include?(k.to_sym) }) + segment.add_custom_attributes(params.reject do |k, _v| + segment_custom_keys.include?(k.to_sym) if k.respond_to?(:to_sym) # param keys can be integers + end) end # Add custom attributes to the span event for the current span. Attributes will be visible on spans in the diff --git a/lib/new_relic/agent/transaction.rb b/lib/new_relic/agent/transaction.rb index cb3af595c4..7ca7ec8152 100644 --- a/lib/new_relic/agent/transaction.rb +++ b/lib/new_relic/agent/transaction.rb @@ -520,7 +520,7 @@ def needs_middleware_summary_metrics?(name) end def finish - return unless state.is_execution_traced? + return unless state.is_execution_traced? && initial_segment @end_time = Process.clock_gettime(Process::CLOCK_REALTIME) @duration = @end_time - @start_time diff --git a/test/multiverse/suites/rails/action_controller_live_rum_test.rb b/test/multiverse/suites/rails/action_controller_live_rum_test.rb index 32b30ec04a..ed21ace9d8 100644 --- a/test/multiverse/suites/rails/action_controller_live_rum_test.rb +++ b/test/multiverse/suites/rails/action_controller_live_rum_test.rb @@ -13,6 +13,7 @@ def brains end def brain_stream + headers['last-modified'] = Time.now # tell Rack not to call #to_ary on the response object render(:inline => RESPONSE_BODY, :stream => true) end end diff --git a/test/multiverse/suites/rails/activejob_test.rb b/test/multiverse/suites/rails/activejob_test.rb index 37d0e72e4d..5ecc6a769d 100644 --- a/test/multiverse/suites/rails/activejob_test.rb +++ b/test/multiverse/suites/rails/activejob_test.rb @@ -104,7 +104,7 @@ def test_code_information_recorded_with_new_transaction (NewRelic::Agent::Instrumentation::ActiveJobSubscriber::PAYLOAD_KEYS.size + 1).times do segment.expect(:params, {}, []) end - 3.times do + 4.times do segment.expect(:finish, []) end segment.expect(:record_scoped_metric=, nil, [false]) diff --git a/test/multiverse/suites/rails/before_suite.rb b/test/multiverse/suites/rails/before_suite.rb deleted file mode 100644 index 6ba45b158a..0000000000 --- a/test/multiverse/suites/rails/before_suite.rb +++ /dev/null @@ -1,23 +0,0 @@ -# This file is distributed under New Relic's license terms. -# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. -# frozen_string_literal: true - -# These are hacks to make the 'rails' multiverse test suite compatible with -# Rails v7.1 released on 2023-10-05. -# -# TODO: refactor these out with non-hack replacements as time permits - -if Gem::Version.new(Rails.version) >= Gem::Version.new('7.1.0') - # NoMethodError (undefined method `to_ary' for an instance of ActionController::Streaming::Body): - # actionpack (7.1.0) lib/action_dispatch/http/response.rb:107:in `to_ary' - # actionpack (7.1.0) lib/action_dispatch/http/response.rb:509:in `to_ary' - # rack (3.0.8) lib/rack/body_proxy.rb:41:in `method_missing' - # rack (3.0.8) lib/rack/etag.rb:32:in `call' - # newrelic-ruby-agent/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call' - require 'action_controller/railtie' - class ActionController::Streaming::Body - def to_ary - self - end - end -end diff --git a/test/multiverse/suites/rails/view_instrumentation_test.rb b/test/multiverse/suites/rails/view_instrumentation_test.rb index 9a2f2c3f97..00ae52d14d 100644 --- a/test/multiverse/suites/rails/view_instrumentation_test.rb +++ b/test/multiverse/suites/rails/view_instrumentation_test.rb @@ -65,19 +65,6 @@ def collection_render render((1..3).map { |x| Foo.new }) end - # proc rendering isn't available in rails 3 but you can do nonsense like this - # and assign an enumerable object to the response body. - def proc_render - streamer = Class.new do - def each - 10_000.times do |i| - yield("This is line #{i}\n") - end - end - end - self.response_body = streamer.new - end - def raise_render raise 'this is an uncaught RuntimeError' end @@ -85,7 +72,7 @@ def raise_render class ViewInstrumentationTest < ActionDispatch::IntegrationTest include MultiverseHelpers - RENDERING_OPTIONS = [:js_render, :xml_render, :proc_render, :json_render] + RENDERING_OPTIONS = [:js_render, :xml_render, :json_render] setup_and_teardown_agent do # ActiveSupport testing keeps blowing away my subscribers on @@ -98,7 +85,7 @@ class ViewInstrumentationTest < ActionDispatch::IntegrationTest end end - (ViewsController.action_methods - %w[raise_render collection_render haml_render proc_render]).each do |method| + (ViewsController.action_methods - %w[raise_render collection_render haml_render]).each do |method| define_method("test_sanity_#{method}") do get "/views/#{method}"