From 318683c876cce1b13db5590b87aa30fe16d30b90 Mon Sep 17 00:00:00 2001 From: Hannah Ramadan Date: Thu, 14 Dec 2023 10:17:21 -0800 Subject: [PATCH 1/9] ViewComponent base instrumentation --- .rubocop_todo.yml | 5 +++ .../agent/configuration/default_source.rb | 7 ++++ .../agent/instrumentation/view_component.rb | 30 ++++++++++++++ .../instrumentation/view_component/chain.rb | 21 ++++++++++ .../view_component/instrumentation.rb | 40 +++++++++++++++++++ .../instrumentation/view_component/prepend.rb | 13 ++++++ .../suites/view_component/config/newrelic.yml | 19 +++++++++ .../view_component_instrumentation_test.rb | 15 +++++++ 8 files changed, 150 insertions(+) create mode 100644 lib/new_relic/agent/instrumentation/view_component.rb create mode 100644 lib/new_relic/agent/instrumentation/view_component/chain.rb create mode 100644 lib/new_relic/agent/instrumentation/view_component/instrumentation.rb create mode 100644 lib/new_relic/agent/instrumentation/view_component/prepend.rb create mode 100644 test/multiverse/suites/view_component/config/newrelic.yml create mode 100644 test/multiverse/suites/view_component/view_component_instrumentation_test.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index b1f2816be7..b918e909c8 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -24,6 +24,11 @@ Metrics/AbcSize: - lib/new_relic/cli/commands/deployments.rb - test/**/* +# Offense count: 1 +Metrics/CollectionLiteralLength: + Exclude: + - 'lib/new_relic/agent/configuration/default_source.rb' + # Offense count: 7 Minitest/AssertRaisesCompoundBody: Exclude: diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 7eed49ece0..9dc59e1dd1 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1657,6 +1657,13 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :allowed_from_server => false, :description => 'Controls auto-instrumentation of Stripe at startup. May be one of: `enabled`, `disabled`.' }, + :'instrumentation.view_component' => { + :default => 'enabled', + :public => true, + :type => String, + :allowed_from_server => false, + :description => 'Controls auto-instrumentation of ViewComponent at startup. May be one of: `enabled`, `disabled`.' + }, :'stripe.user_data.include' => { default: NewRelic::EMPTY_ARRAY, public: true, diff --git a/lib/new_relic/agent/instrumentation/view_component.rb b/lib/new_relic/agent/instrumentation/view_component.rb new file mode 100644 index 0000000000..7ce2173592 --- /dev/null +++ b/lib/new_relic/agent/instrumentation/view_component.rb @@ -0,0 +1,30 @@ +# 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 + +require_relative 'view_component/instrumentation' +require_relative 'view_component/chain' +require_relative 'view_component/prepend' + +DependencyDetection.defer do + named :'view_component' + + depends_on do + NewRelic::Agent.config[:'instrumentation.view_component'] == 'enabled' + end + + depends_on do + defined?(ViewComponent) && + ViewComponent::Base.method_defined?(:render_in) + end + + executes do + NewRelic::Agent.logger.info('Installing view_component instrumentation') + + if use_prepend? + prepend_instrument ViewComponent::Base, NewRelic::Agent::Instrumentation::ViewComponent::Prepend + else + chain_instrument NewRelic::Agent::Instrumentation::ViewComponent::Chain + end + end +end diff --git a/lib/new_relic/agent/instrumentation/view_component/chain.rb b/lib/new_relic/agent/instrumentation/view_component/chain.rb new file mode 100644 index 0000000000..41a2f21f03 --- /dev/null +++ b/lib/new_relic/agent/instrumentation/view_component/chain.rb @@ -0,0 +1,21 @@ +# 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 + +module NewRelic::Agent::Instrumentation + module ViewComponent::Chain + def self.instrument! + ::ViewComponent.class_eval do + include NewRelic::Agent::Instrumentation::ViewComponent + + alias_method(:render_in_without_tracing, :render_in) + + def render_in(*args) + render_in_with_tracing(*args) do + render_in_without_tracing(*args) + end + end + end + end + end +end diff --git a/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb b/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb new file mode 100644 index 0000000000..28739e4b5c --- /dev/null +++ b/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb @@ -0,0 +1,40 @@ +# 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 + +module NewRelic::Agent::Instrumentation + module ViewComponent + INSTRUMENTATION_NAME = 'view_component' + + def render_in_with_tracing(*args) + NewRelic::Agent.record_instrumentation_invocation(INSTRUMENTATION_NAME) + + component = self.class.name + identifier = self.class.identifier + + segment = NewRelic::Agent::Tracer.start_segment( + name: metric_name(identifier, component) + ) + + begin + NewRelic::Agent::Tracer.capture_segment_error(segment) { yield } + ensure + NewRelic::Agent::Transaction::Segment.finish(segment) + end + end + + def metric_name(identifier, component) + "View/#{metric_path(identifier)}/#{component}" + end + + def metric_path(identifier) + if identifier.nil? + 'component' + elsif identifier && (parts = identifier.split('/')).size > 1 + parts[-2..-1].join('/') # Get filepath by assuming the Rails' structure: app/components/home/example_component.rb + else + NewRelic::Agent::UNKNOWN_METRIC + end + end + end +end diff --git a/lib/new_relic/agent/instrumentation/view_component/prepend.rb b/lib/new_relic/agent/instrumentation/view_component/prepend.rb new file mode 100644 index 0000000000..190c6b9344 --- /dev/null +++ b/lib/new_relic/agent/instrumentation/view_component/prepend.rb @@ -0,0 +1,13 @@ +# 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 + +module NewRelic::Agent::Instrumentation + module ViewComponent::Prepend + include NewRelic::Agent::Instrumentation::ViewComponent + + def render_in(*args) + render_in_with_tracing(*args) { super } + end + end +end diff --git a/test/multiverse/suites/view_component/config/newrelic.yml b/test/multiverse/suites/view_component/config/newrelic.yml new file mode 100644 index 0000000000..2eae61572b --- /dev/null +++ b/test/multiverse/suites/view_component/config/newrelic.yml @@ -0,0 +1,19 @@ +--- +development: + error_collector: + enabled: true + apdex_t: 0.5 + monitor_mode: true + license_key: bootstrap_newrelic_admin_license_key_000 + instrumentation: + view_component: <%= $render_in %> + app_name: test + log_level: debug + host: 127.0.0.1 + api_host: 127.0.0.1 + transaction_trace: + record_sql: obfuscated + enabled: true + stack_trace_threshold: 0.5 + transaction_threshold: 1.0 + capture_params: false diff --git a/test/multiverse/suites/view_component/view_component_instrumentation_test.rb b/test/multiverse/suites/view_component/view_component_instrumentation_test.rb new file mode 100644 index 0000000000..e2e3609a5f --- /dev/null +++ b/test/multiverse/suites/view_component/view_component_instrumentation_test.rb @@ -0,0 +1,15 @@ +# 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 + +class ViewComponentInstrumentationTest < Minitest::Test + def setup + @stats_engine = NewRelic::Agent.instance.stats_engine + end + + def teardown + NewRelic::Agent.instance.stats_engine.clear_stats + end + + # Add tests here +end From b101b88d8bc8746df806701503d9aa3c05f83d85 Mon Sep 17 00:00:00 2001 From: Hannah Ramadan Date: Mon, 18 Dec 2023 11:30:42 -0700 Subject: [PATCH 2/9] Code review edits --- lib/new_relic/agent/configuration/default_source.rb | 4 ++-- lib/new_relic/agent/instrumentation/view_component.rb | 8 ++------ .../instrumentation/view_component/instrumentation.rb | 2 +- test/multiverse/suites/view_component/config/newrelic.yml | 2 +- 4 files changed, 6 insertions(+), 10 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 9dc59e1dd1..29607b29a2 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1658,11 +1658,11 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :description => 'Controls auto-instrumentation of Stripe at startup. May be one of: `enabled`, `disabled`.' }, :'instrumentation.view_component' => { - :default => 'enabled', + :default => 'auto', :public => true, :type => String, :allowed_from_server => false, - :description => 'Controls auto-instrumentation of ViewComponent at startup. May be one of: `enabled`, `disabled`.' + :description => 'Controls auto-instrumentation of ViewComponent at startup. May be one of: `auto`, `prepend`, `chain`, `disabled`.' }, :'stripe.user_data.include' => { default: NewRelic::EMPTY_ARRAY, diff --git a/lib/new_relic/agent/instrumentation/view_component.rb b/lib/new_relic/agent/instrumentation/view_component.rb index 7ce2173592..900336c67a 100644 --- a/lib/new_relic/agent/instrumentation/view_component.rb +++ b/lib/new_relic/agent/instrumentation/view_component.rb @@ -7,11 +7,7 @@ require_relative 'view_component/prepend' DependencyDetection.defer do - named :'view_component' - - depends_on do - NewRelic::Agent.config[:'instrumentation.view_component'] == 'enabled' - end + named :view_component depends_on do defined?(ViewComponent) && @@ -19,7 +15,7 @@ end executes do - NewRelic::Agent.logger.info('Installing view_component instrumentation') + NewRelic::Agent.logger.info('Installing ViewComponent instrumentation') if use_prepend? prepend_instrument ViewComponent::Base, NewRelic::Agent::Instrumentation::ViewComponent::Prepend diff --git a/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb b/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb index 28739e4b5c..c45a5c8cc4 100644 --- a/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb @@ -4,7 +4,7 @@ module NewRelic::Agent::Instrumentation module ViewComponent - INSTRUMENTATION_NAME = 'view_component' + INSTRUMENTATION_NAME = NewRelic::Agent.base_name(name) def render_in_with_tracing(*args) NewRelic::Agent.record_instrumentation_invocation(INSTRUMENTATION_NAME) diff --git a/test/multiverse/suites/view_component/config/newrelic.yml b/test/multiverse/suites/view_component/config/newrelic.yml index 2eae61572b..2c2a295aec 100644 --- a/test/multiverse/suites/view_component/config/newrelic.yml +++ b/test/multiverse/suites/view_component/config/newrelic.yml @@ -6,7 +6,7 @@ development: monitor_mode: true license_key: bootstrap_newrelic_admin_license_key_000 instrumentation: - view_component: <%= $render_in %> + view_component: <%= $instrumentation_method %> app_name: test log_level: debug host: 127.0.0.1 From 427ad20f8b3e0b14386fa9d05a748aba021b3b2f Mon Sep 17 00:00:00 2001 From: Hannah Ramadan Date: Mon, 18 Dec 2023 12:37:50 -0700 Subject: [PATCH 3/9] Config update --- lib/new_relic/agent/configuration/default_source.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 29607b29a2..f6f88adb75 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1661,6 +1661,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :default => 'auto', :public => true, :type => String, + :dynamic_name => true, :allowed_from_server => false, :description => 'Controls auto-instrumentation of ViewComponent at startup. May be one of: `auto`, `prepend`, `chain`, `disabled`.' }, From d57e3e95804785cde4467aff7df499dda0620c99 Mon Sep 17 00:00:00 2001 From: Hannah Ramadan Date: Fri, 22 Dec 2023 09:54:59 -0700 Subject: [PATCH 4/9] ViewComponent tests Inline disable rubocop Cleanup tests Appease rubocop --- .rubocop_todo.yml | 5 - .../agent/configuration/default_source.rb | 2 + .../instrumentation/view_component/chain.rb | 2 +- .../view_component/instrumentation.rb | 12 +- .../suites/rails/rails3_app/my_app.rb | 2 + .../suites/rails/view_instrumentation_test.rb | 227 ------------------ test/multiverse/suites/view_component/Envfile | 20 ++ .../view_component/instrumentation_test.rb | 40 +++ .../view_component_instrumentation_test.rb | 15 -- 9 files changed, 71 insertions(+), 254 deletions(-) delete mode 100644 test/multiverse/suites/rails/view_instrumentation_test.rb create mode 100644 test/multiverse/suites/view_component/Envfile create mode 100644 test/multiverse/suites/view_component/instrumentation_test.rb delete mode 100644 test/multiverse/suites/view_component/view_component_instrumentation_test.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index b918e909c8..b1f2816be7 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -24,11 +24,6 @@ Metrics/AbcSize: - lib/new_relic/cli/commands/deployments.rb - test/**/* -# Offense count: 1 -Metrics/CollectionLiteralLength: - Exclude: - - 'lib/new_relic/agent/configuration/default_source.rb' - # Offense count: 7 Minitest/AssertRaisesCompoundBody: Exclude: diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index f6f88adb75..807d2774f3 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -313,6 +313,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) 'webpacker:compile' ].join(',').freeze + # rubocop:disable Metrics/CollectionLiteralLength DEFAULTS = { # Critical :agent_enabled => { @@ -2413,6 +2414,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :description => 'This value represents the total amount of memory available to the host (not the process), in mebibytes (1024 squared or 1,048,576 bytes).' } }.freeze + # rubocop:enable Metrics/CollectionLiteralLength end end end diff --git a/lib/new_relic/agent/instrumentation/view_component/chain.rb b/lib/new_relic/agent/instrumentation/view_component/chain.rb index 41a2f21f03..88885bbcde 100644 --- a/lib/new_relic/agent/instrumentation/view_component/chain.rb +++ b/lib/new_relic/agent/instrumentation/view_component/chain.rb @@ -5,7 +5,7 @@ module NewRelic::Agent::Instrumentation module ViewComponent::Chain def self.instrument! - ::ViewComponent.class_eval do + ::ViewComponent::Base.class_eval do include NewRelic::Agent::Instrumentation::ViewComponent alias_method(:render_in_without_tracing, :render_in) diff --git a/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb b/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb index c45a5c8cc4..eb49979f2f 100644 --- a/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb @@ -12,14 +12,14 @@ def render_in_with_tracing(*args) component = self.class.name identifier = self.class.identifier - segment = NewRelic::Agent::Tracer.start_segment( - name: metric_name(identifier, component) - ) - begin - NewRelic::Agent::Tracer.capture_segment_error(segment) { yield } + segment = NewRelic::Agent::Tracer.start_segment( + name: metric_name(identifier, component) + ) + rescue => e + ::NewRelic::Agent.logger.debug('Failed starting ViewComponent segment', e) ensure - NewRelic::Agent::Transaction::Segment.finish(segment) + segment&.finish end end diff --git a/test/multiverse/suites/rails/rails3_app/my_app.rb b/test/multiverse/suites/rails/rails3_app/my_app.rb index 561c6c88b0..96b0ad582f 100644 --- a/test/multiverse/suites/rails/rails3_app/my_app.rb +++ b/test/multiverse/suites/rails/rails3_app/my_app.rb @@ -94,6 +94,8 @@ class MyApp < Rails::Application post '/parameter_capture', :to => 'parameter_capture#create' + get '/view_components', :to => 'view_component#index' + get '/:controller(/:action(/:id))' end diff --git a/test/multiverse/suites/rails/view_instrumentation_test.rb b/test/multiverse/suites/rails/view_instrumentation_test.rb deleted file mode 100644 index 00ae52d14d..0000000000 --- a/test/multiverse/suites/rails/view_instrumentation_test.rb +++ /dev/null @@ -1,227 +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 - -require './app' -require 'haml' - -ActionController::Base.view_paths = ['app/views'] - -class ViewsController < ApplicationController - def template_render_with_3_partial_renders - render((+'index')) - end - - def render_with_delays - nr_freeze_process_time - @delay = 1 - render((+'index')) - end - - def deep_partial_render - render((+'deep_partial')) - end - - def text_render - render(body: 'Yay') - end - - def json_render - render(:json => {'a' => 'b'}) - end - - def xml_render - render(:xml => {'a' => 'b'}) - end - - def js_render - render(:js => 'alert("this is js");') - end - - def file_render - # The choice of filename is significant here: we want a dot in the filename - # in order to expose an issue on Rails 2. - file = File.expand_path(File.join(File.dirname(__FILE__), 'dummy.txt')) - render(:file => file, :content_type => 'text/plain', :layout => false) - end - - def nothing_render - render(body: nil) - end - - def inline_render - render(:inline => '<% Time.now %>

<%= Time.now %>

') - end - - def haml_render - render((+'haml_view')) - end - - def no_template - render([]) - end - - def collection_render - render((1..3).map { |x| Foo.new }) - end - - def raise_render - raise 'this is an uncaught RuntimeError' - end -end - -class ViewInstrumentationTest < ActionDispatch::IntegrationTest - include MultiverseHelpers - RENDERING_OPTIONS = [:js_render, :xml_render, :json_render] - - setup_and_teardown_agent do - # ActiveSupport testing keeps blowing away my subscribers on - # teardown for some reason. Have to keep putting it back. - if Rails::VERSION::MAJOR.to_i == 4 - NewRelic::Agent::Instrumentation::ActionViewSubscriber \ - .subscribe(/render_.+\.action_view$/) - NewRelic::Agent::Instrumentation::ActionControllerSubscriber \ - .subscribe(/^process_action.action_controller$/) - end - end - - (ViewsController.action_methods - %w[raise_render collection_render haml_render]).each do |method| - define_method("test_sanity_#{method}") do - get "/views/#{method}" - - assert_equal 200, status, "Expected 200, got #{status} for /views/#{method}" - end - - def test_should_allow_uncaught_exception_to_propagate - get('/views/raise_render') - - assert_equal 500, status - end - - def test_should_count_all_the_template_and_partial_nodes - get('/views/template_render_with_3_partial_renders') - sample = last_transaction_trace - nodes = find_all_nodes_with_name_matching(sample, ['^Nested/Controller/views', '^View']) - nodes_list = "Found these nodes:\n #{nodes.map(&:metric_name).join("\n ")}" - - assert_equal 5, nodes.length, "Should be a node for the controller action, the template, and 3 partials (5). #{nodes_list}" - end - - def test_should_have_3_nodes_with_the_correct_metric_name - get('/views/template_render_with_3_partial_renders') - - sample = last_transaction_trace - partial_nodes = find_all_nodes_with_name_matching(sample, 'View/views/_a_partial.html.erb/Partial') - - assert_equal 3, partial_nodes.size, 'sanity check' - assert_equal ['View/views/_a_partial.html.erb/Partial'], partial_nodes.map(&:metric_name).uniq - end - - def test_should_create_a_metric_for_the_rendered_inline_template - get('/views/inline_render') - - sample = last_transaction_trace - text_node = find_node_with_name(sample, 'View/inline template/Rendering') - - assert text_node, 'Failed to find a node named View/inline template/Rendering' - assert_metrics_recorded('View/inline template/Rendering') - end - - # It doesn't seem worth it to get consistent behavior here. - if Rails::VERSION::MAJOR.to_i == 3 && Rails::VERSION::MINOR.to_i == 0 - def test_should_not_instrument_rendering_of_text - get('/views/text_render') - sample = last_transaction_trace - - refute find_node_with_name(sample, 'View/text template/Rendering') - end - else - def test_should_create_a_metric_for_the_rendered_text - get('/views/text_render') - - sample = last_transaction_trace - text_node = find_node_with_name(sample, 'View/text template/Rendering') - - assert text_node, 'Failed to find a node named View/text template/Rendering' - assert_metrics_recorded('View/text template/Rendering') - end - end - - def test_should_create_a_metric_for_the_rendered_haml_template - get('/views/haml_render') - - sample = last_transaction_trace - text_node = find_node_with_name(sample, 'View/views/haml_view.html.haml/Rendering') - - assert text_node, 'Failed to find a node named View/views/haml_view.html.haml/Rendering' - assert_metrics_recorded('View/views/haml_view.html.haml/Rendering') - end - - def test_should_create_a_proper_metric_when_the_template_is_unknown - get('/views/no_template') - sample = last_transaction_trace - - # Different versions have significant difference in handling, but we're - # happy enough with what each of them does in the unknown case - if Rails::VERSION::MAJOR.to_i == 3 && Rails::VERSION::MINOR.to_i == 0 - refute find_node_with_name_matching(sample, /^View/) - elsif Rails::VERSION::MAJOR.to_i == 3 - assert find_node_with_name(sample, 'View/collection/Partial') - else - assert find_node_with_name(sample, 'View/(unknown)/Partial') - end - end - - def test_should_create_a_proper_metric_when_we_render_a_collection - get('/views/collection_render') - sample = last_transaction_trace - - assert find_node_with_name(sample, 'View/foos/_foo.html.haml/Partial') - end - - RENDERING_OPTIONS.each do |action| - define_method("test_should_not_instrument_rendering_of_#{action}") do - get "/views/#{action}" - sample = last_transaction_trace - view_node = find_node_with_name_matching(sample, /^View\//) - - refute view_node, "Should not instrument rendering of #{action}, found #{view_node}." - end - end - - def test_should_create_a_metric_for_rendered_file_that_does_not_include_the_filename_so_it_doesnt_metric_explode - get('/views/file_render') - sample = last_transaction_trace - - assert find_node_with_name(sample, 'Nested/Controller/views/file_render') - refute find_node_with_name_matching(sample, 'dummy') - end - - def test_exclusive_time_for_template_render_metrics_should_not_include_partial_rendering_time - get('/views/render_with_delays') - - expected_stats_partial = { - :call_count => 3, - :total_call_time => 3.0, - :total_exclusive_time => 3.0 - } - - expected_stats_template = { - :call_count => 1, - :total_call_time => 4.0, - :total_exclusive_time => 1.0 # top-level template takes 1s itself - } - - scope = 'Controller/views/render_with_delays' - template_metric = 'View/views/index.html.erb/Rendering' - partial_metric = 'View/views/_a_partial.html.erb/Partial' - - assert_metrics_recorded( - partial_metric => expected_stats_partial, - template_metric => expected_stats_template, - [partial_metric, scope] => expected_stats_partial, - [template_metric, scope] => expected_stats_template - ) - end - end -end diff --git a/test/multiverse/suites/view_component/Envfile b/test/multiverse/suites/view_component/Envfile new file mode 100644 index 0000000000..a6f01f97e2 --- /dev/null +++ b/test/multiverse/suites/view_component/Envfile @@ -0,0 +1,20 @@ +# 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 + +instrumentation_methods :chain, :prepend + +VIEW_COMPONENT_VERSIONS = [ + [nil, 2.7], + ['2.0.0', 2.4] +] + +def gem_list(view_component_version = nil) + <<-RB + gem 'rails' + gem 'view_component'#{view_component_version} + gem 'rack-test' + RB +end + +create_gemfiles(VIEW_COMPONENT_VERSIONS) diff --git a/test/multiverse/suites/view_component/instrumentation_test.rb b/test/multiverse/suites/view_component/instrumentation_test.rb new file mode 100644 index 0000000000..01ebcbb07b --- /dev/null +++ b/test/multiverse/suites/view_component/instrumentation_test.rb @@ -0,0 +1,40 @@ +# 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 + +require_relative '../rails/app' + +class ExampleComponent < ViewComponent::Base + <<-ERB + <%= @title %> + ERB + + def initialize(title:) + @title = title + end +end + +class ViewComponentController < ActionController::Base + def index + render(ExampleComponent.new(title: 'Hello World')) + end +end + +class ViewComponentInstrumentationTest < ActionDispatch::IntegrationTest + include MultiverseHelpers + setup_and_teardown_agent + + def test_metric_recorded + get('/view_components') + + assert_metrics_recorded('View/view_component/instrumentation_test.rb/ExampleComponent') + end + + def test_records_nothing_if_tracing_disabled + NewRelic::Agent.disable_all_tracing do + get('/view_components') + end + + assert_metrics_not_recorded('View/view_component/instrumentation_test.rb/ExampleComponent') + end +end diff --git a/test/multiverse/suites/view_component/view_component_instrumentation_test.rb b/test/multiverse/suites/view_component/view_component_instrumentation_test.rb deleted file mode 100644 index e2e3609a5f..0000000000 --- a/test/multiverse/suites/view_component/view_component_instrumentation_test.rb +++ /dev/null @@ -1,15 +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 - -class ViewComponentInstrumentationTest < Minitest::Test - def setup - @stats_engine = NewRelic::Agent.instance.stats_engine - end - - def teardown - NewRelic::Agent.instance.stats_engine.clear_stats - end - - # Add tests here -end From d746f68aa2b1756ddf65fd48fd5b8b72ad87090c Mon Sep 17 00:00:00 2001 From: Hannah Ramadan <76922290+hannahramadan@users.noreply.github.com> Date: Wed, 3 Jan 2024 13:44:25 -0800 Subject: [PATCH 5/9] Apply suggestions from code review Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> Code feedback Add mistakenly deleted file Move file Test update Test updates Use newer vc version --- .../view_component/instrumentation.rb | 11 +- .../suites/rails/rails3_app/my_app.rb | 2 +- .../suites/rails/view_instrumentation_test.rb | 227 ++++++++++++++++++ test/multiverse/suites/view_component/Envfile | 5 +- ...=> view_component_instrumentation_test.rb} | 6 +- 5 files changed, 238 insertions(+), 13 deletions(-) create mode 100644 test/multiverse/suites/rails/view_instrumentation_test.rb rename test/multiverse/suites/view_component/{instrumentation_test.rb => view_component_instrumentation_test.rb} (78%) diff --git a/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb b/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb index eb49979f2f..c232336702 100644 --- a/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb @@ -9,12 +9,9 @@ module ViewComponent def render_in_with_tracing(*args) NewRelic::Agent.record_instrumentation_invocation(INSTRUMENTATION_NAME) - component = self.class.name - identifier = self.class.identifier - begin segment = NewRelic::Agent::Tracer.start_segment( - name: metric_name(identifier, component) + name: metric_name(self.class.identifier, self.class.name) ) rescue => e ::NewRelic::Agent.logger.debug('Failed starting ViewComponent segment', e) @@ -28,9 +25,9 @@ def metric_name(identifier, component) end def metric_path(identifier) - if identifier.nil? - 'component' - elsif identifier && (parts = identifier.split('/')).size > 1 + return 'component' unless identifier + + if (parts = identifier.split('/')).size > 1 parts[-2..-1].join('/') # Get filepath by assuming the Rails' structure: app/components/home/example_component.rb else NewRelic::Agent::UNKNOWN_METRIC diff --git a/test/multiverse/suites/rails/rails3_app/my_app.rb b/test/multiverse/suites/rails/rails3_app/my_app.rb index 96b0ad582f..c6bf971dfb 100644 --- a/test/multiverse/suites/rails/rails3_app/my_app.rb +++ b/test/multiverse/suites/rails/rails3_app/my_app.rb @@ -94,7 +94,7 @@ class MyApp < Rails::Application post '/parameter_capture', :to => 'parameter_capture#create' - get '/view_components', :to => 'view_component#index' + get '/view_components', :to => 'view_component#index' # This app and route is used in ViewComponent tests get '/:controller(/:action(/:id))' end diff --git a/test/multiverse/suites/rails/view_instrumentation_test.rb b/test/multiverse/suites/rails/view_instrumentation_test.rb new file mode 100644 index 0000000000..00ae52d14d --- /dev/null +++ b/test/multiverse/suites/rails/view_instrumentation_test.rb @@ -0,0 +1,227 @@ +# 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 + +require './app' +require 'haml' + +ActionController::Base.view_paths = ['app/views'] + +class ViewsController < ApplicationController + def template_render_with_3_partial_renders + render((+'index')) + end + + def render_with_delays + nr_freeze_process_time + @delay = 1 + render((+'index')) + end + + def deep_partial_render + render((+'deep_partial')) + end + + def text_render + render(body: 'Yay') + end + + def json_render + render(:json => {'a' => 'b'}) + end + + def xml_render + render(:xml => {'a' => 'b'}) + end + + def js_render + render(:js => 'alert("this is js");') + end + + def file_render + # The choice of filename is significant here: we want a dot in the filename + # in order to expose an issue on Rails 2. + file = File.expand_path(File.join(File.dirname(__FILE__), 'dummy.txt')) + render(:file => file, :content_type => 'text/plain', :layout => false) + end + + def nothing_render + render(body: nil) + end + + def inline_render + render(:inline => '<% Time.now %>

<%= Time.now %>

') + end + + def haml_render + render((+'haml_view')) + end + + def no_template + render([]) + end + + def collection_render + render((1..3).map { |x| Foo.new }) + end + + def raise_render + raise 'this is an uncaught RuntimeError' + end +end + +class ViewInstrumentationTest < ActionDispatch::IntegrationTest + include MultiverseHelpers + RENDERING_OPTIONS = [:js_render, :xml_render, :json_render] + + setup_and_teardown_agent do + # ActiveSupport testing keeps blowing away my subscribers on + # teardown for some reason. Have to keep putting it back. + if Rails::VERSION::MAJOR.to_i == 4 + NewRelic::Agent::Instrumentation::ActionViewSubscriber \ + .subscribe(/render_.+\.action_view$/) + NewRelic::Agent::Instrumentation::ActionControllerSubscriber \ + .subscribe(/^process_action.action_controller$/) + end + end + + (ViewsController.action_methods - %w[raise_render collection_render haml_render]).each do |method| + define_method("test_sanity_#{method}") do + get "/views/#{method}" + + assert_equal 200, status, "Expected 200, got #{status} for /views/#{method}" + end + + def test_should_allow_uncaught_exception_to_propagate + get('/views/raise_render') + + assert_equal 500, status + end + + def test_should_count_all_the_template_and_partial_nodes + get('/views/template_render_with_3_partial_renders') + sample = last_transaction_trace + nodes = find_all_nodes_with_name_matching(sample, ['^Nested/Controller/views', '^View']) + nodes_list = "Found these nodes:\n #{nodes.map(&:metric_name).join("\n ")}" + + assert_equal 5, nodes.length, "Should be a node for the controller action, the template, and 3 partials (5). #{nodes_list}" + end + + def test_should_have_3_nodes_with_the_correct_metric_name + get('/views/template_render_with_3_partial_renders') + + sample = last_transaction_trace + partial_nodes = find_all_nodes_with_name_matching(sample, 'View/views/_a_partial.html.erb/Partial') + + assert_equal 3, partial_nodes.size, 'sanity check' + assert_equal ['View/views/_a_partial.html.erb/Partial'], partial_nodes.map(&:metric_name).uniq + end + + def test_should_create_a_metric_for_the_rendered_inline_template + get('/views/inline_render') + + sample = last_transaction_trace + text_node = find_node_with_name(sample, 'View/inline template/Rendering') + + assert text_node, 'Failed to find a node named View/inline template/Rendering' + assert_metrics_recorded('View/inline template/Rendering') + end + + # It doesn't seem worth it to get consistent behavior here. + if Rails::VERSION::MAJOR.to_i == 3 && Rails::VERSION::MINOR.to_i == 0 + def test_should_not_instrument_rendering_of_text + get('/views/text_render') + sample = last_transaction_trace + + refute find_node_with_name(sample, 'View/text template/Rendering') + end + else + def test_should_create_a_metric_for_the_rendered_text + get('/views/text_render') + + sample = last_transaction_trace + text_node = find_node_with_name(sample, 'View/text template/Rendering') + + assert text_node, 'Failed to find a node named View/text template/Rendering' + assert_metrics_recorded('View/text template/Rendering') + end + end + + def test_should_create_a_metric_for_the_rendered_haml_template + get('/views/haml_render') + + sample = last_transaction_trace + text_node = find_node_with_name(sample, 'View/views/haml_view.html.haml/Rendering') + + assert text_node, 'Failed to find a node named View/views/haml_view.html.haml/Rendering' + assert_metrics_recorded('View/views/haml_view.html.haml/Rendering') + end + + def test_should_create_a_proper_metric_when_the_template_is_unknown + get('/views/no_template') + sample = last_transaction_trace + + # Different versions have significant difference in handling, but we're + # happy enough with what each of them does in the unknown case + if Rails::VERSION::MAJOR.to_i == 3 && Rails::VERSION::MINOR.to_i == 0 + refute find_node_with_name_matching(sample, /^View/) + elsif Rails::VERSION::MAJOR.to_i == 3 + assert find_node_with_name(sample, 'View/collection/Partial') + else + assert find_node_with_name(sample, 'View/(unknown)/Partial') + end + end + + def test_should_create_a_proper_metric_when_we_render_a_collection + get('/views/collection_render') + sample = last_transaction_trace + + assert find_node_with_name(sample, 'View/foos/_foo.html.haml/Partial') + end + + RENDERING_OPTIONS.each do |action| + define_method("test_should_not_instrument_rendering_of_#{action}") do + get "/views/#{action}" + sample = last_transaction_trace + view_node = find_node_with_name_matching(sample, /^View\//) + + refute view_node, "Should not instrument rendering of #{action}, found #{view_node}." + end + end + + def test_should_create_a_metric_for_rendered_file_that_does_not_include_the_filename_so_it_doesnt_metric_explode + get('/views/file_render') + sample = last_transaction_trace + + assert find_node_with_name(sample, 'Nested/Controller/views/file_render') + refute find_node_with_name_matching(sample, 'dummy') + end + + def test_exclusive_time_for_template_render_metrics_should_not_include_partial_rendering_time + get('/views/render_with_delays') + + expected_stats_partial = { + :call_count => 3, + :total_call_time => 3.0, + :total_exclusive_time => 3.0 + } + + expected_stats_template = { + :call_count => 1, + :total_call_time => 4.0, + :total_exclusive_time => 1.0 # top-level template takes 1s itself + } + + scope = 'Controller/views/render_with_delays' + template_metric = 'View/views/index.html.erb/Rendering' + partial_metric = 'View/views/_a_partial.html.erb/Partial' + + assert_metrics_recorded( + partial_metric => expected_stats_partial, + template_metric => expected_stats_template, + [partial_metric, scope] => expected_stats_partial, + [template_metric, scope] => expected_stats_template + ) + end + end +end diff --git a/test/multiverse/suites/view_component/Envfile b/test/multiverse/suites/view_component/Envfile index a6f01f97e2..468501e2c2 100644 --- a/test/multiverse/suites/view_component/Envfile +++ b/test/multiverse/suites/view_component/Envfile @@ -6,14 +6,15 @@ instrumentation_methods :chain, :prepend VIEW_COMPONENT_VERSIONS = [ [nil, 2.7], - ['2.0.0', 2.4] + ['2.25.0', 2.4] ] def gem_list(view_component_version = nil) - <<-RB + <<~RB gem 'rails' gem 'view_component'#{view_component_version} gem 'rack-test' + gem 'loofah', '~> 2.20.0' if RUBY_VERSION >= '2.4.0' && RUBY_VERSION < '2.5.0' RB end diff --git a/test/multiverse/suites/view_component/instrumentation_test.rb b/test/multiverse/suites/view_component/view_component_instrumentation_test.rb similarity index 78% rename from test/multiverse/suites/view_component/instrumentation_test.rb rename to test/multiverse/suites/view_component/view_component_instrumentation_test.rb index 01ebcbb07b..c35ce988ea 100644 --- a/test/multiverse/suites/view_component/instrumentation_test.rb +++ b/test/multiverse/suites/view_component/view_component_instrumentation_test.rb @@ -5,7 +5,7 @@ require_relative '../rails/app' class ExampleComponent < ViewComponent::Base - <<-ERB + <<~ERB <%= @title %> ERB @@ -27,7 +27,7 @@ class ViewComponentInstrumentationTest < ActionDispatch::IntegrationTest def test_metric_recorded get('/view_components') - assert_metrics_recorded('View/view_component/instrumentation_test.rb/ExampleComponent') + assert_metrics_recorded('View/view_component/view_component_instrumentation_test.rb/ExampleComponent') end def test_records_nothing_if_tracing_disabled @@ -35,6 +35,6 @@ def test_records_nothing_if_tracing_disabled get('/view_components') end - assert_metrics_not_recorded('View/view_component/instrumentation_test.rb/ExampleComponent') + assert_metrics_not_recorded('View/view_component/view_component_instrumentation_test.rb/ExampleComponent') end end From 732d23f55af849fd018c5af39602cf7b53c884f0 Mon Sep 17 00:00:00 2001 From: Hannah Ramadan Date: Thu, 4 Jan 2024 14:29:41 -0800 Subject: [PATCH 6/9] Add CHANGELOG --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9decccd4c..46571d9f83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,11 @@ ## v9.7.0 -Version 9.7.0 changes the endpoint used to access the cluster name for Elasticsearch instrumentation and adds support for Falcon. +Version 9.7.0 introduces ViewComponent instrumentation, changes the endpoint used to access the cluster name for Elasticsearch instrumentation, and adds support for Falcon. + +- **Feature: ViewComponent instrumentation** + + [ViewComponent](https://viewcomponent.org/) is a now an instrumented framework. The agent currently supports Roda versions 2.0.0+. [PR#2367](https://github.com/newrelic/newrelic-ruby-agent/pull/2367) - **Feature: Use root path to access Elasticsearch cluster name** From d115441752e0577c783d491c6a05e9ec4a7fd72e Mon Sep 17 00:00:00 2001 From: Hannah Ramadan Date: Fri, 5 Jan 2024 09:32:32 -0800 Subject: [PATCH 7/9] test version change version change Only test latest version --- test/multiverse/suites/view_component/Envfile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/multiverse/suites/view_component/Envfile b/test/multiverse/suites/view_component/Envfile index 468501e2c2..a528030153 100644 --- a/test/multiverse/suites/view_component/Envfile +++ b/test/multiverse/suites/view_component/Envfile @@ -5,8 +5,7 @@ instrumentation_methods :chain, :prepend VIEW_COMPONENT_VERSIONS = [ - [nil, 2.7], - ['2.25.0', 2.4] + [nil, 2.7] ] def gem_list(view_component_version = nil) From 70c3c7732d157e07c5ef5b086cb28689151c3db1 Mon Sep 17 00:00:00 2001 From: Hannah Ramadan Date: Fri, 5 Jan 2024 14:46:39 -0800 Subject: [PATCH 8/9] Add yield --- .../agent/instrumentation/view_component/instrumentation.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb b/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb index c232336702..a95f31dee7 100644 --- a/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/view_component/instrumentation.rb @@ -13,8 +13,9 @@ def render_in_with_tracing(*args) segment = NewRelic::Agent::Tracer.start_segment( name: metric_name(self.class.identifier, self.class.name) ) + yield rescue => e - ::NewRelic::Agent.logger.debug('Failed starting ViewComponent segment', e) + ::NewRelic::Agent.logger.debug('Error capturing ViewComponent segment', e) ensure segment&.finish end From a0e188c92ed0b428c3afd6dd2fa5c00e616bb2bd Mon Sep 17 00:00:00 2001 From: Hannah Ramadan Date: Mon, 8 Jan 2024 06:15:34 -1000 Subject: [PATCH 9/9] Update tests and version test Version fix --- test/multiverse/suites/view_component/Envfile | 3 ++- .../view_component_instrumentation_test.rb | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/test/multiverse/suites/view_component/Envfile b/test/multiverse/suites/view_component/Envfile index a528030153..10100d9ee3 100644 --- a/test/multiverse/suites/view_component/Envfile +++ b/test/multiverse/suites/view_component/Envfile @@ -5,7 +5,8 @@ instrumentation_methods :chain, :prepend VIEW_COMPONENT_VERSIONS = [ - [nil, 2.7] + [nil, 2.7], + ['2.53.0', 2.4] ] def gem_list(view_component_version = nil) diff --git a/test/multiverse/suites/view_component/view_component_instrumentation_test.rb b/test/multiverse/suites/view_component/view_component_instrumentation_test.rb index c35ce988ea..9805313f03 100644 --- a/test/multiverse/suites/view_component/view_component_instrumentation_test.rb +++ b/test/multiverse/suites/view_component/view_component_instrumentation_test.rb @@ -20,10 +20,16 @@ def index end end +class DummyViewComponentInstrumentationClass + include NewRelic::Agent::Instrumentation::ViewComponent +end + class ViewComponentInstrumentationTest < ActionDispatch::IntegrationTest include MultiverseHelpers setup_and_teardown_agent + FAKE_CLASS = DummyViewComponentInstrumentationClass.new + def test_metric_recorded get('/view_components') @@ -37,4 +43,12 @@ def test_records_nothing_if_tracing_disabled assert_metrics_not_recorded('View/view_component/view_component_instrumentation_test.rb/ExampleComponent') end + + def test_metric_path_falsey + assert(FAKE_CLASS.metric_path(nil), 'component') + end + + def test_metric_path_unknown_file_pattern + assert(FAKE_CLASS.metric_path('nothing_to_see_here'), 'unknown') + end end