From 3ad13f64a1d009fd4f5dd1f57f3ed78b8a4af62e Mon Sep 17 00:00:00 2001 From: hramadan Date: Thu, 28 Sep 2023 15:04:10 -0700 Subject: [PATCH 1/8] initial commit --- .../agent/instrumentation/roda/ignorer.rb | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 lib/new_relic/agent/instrumentation/roda/ignorer.rb diff --git a/lib/new_relic/agent/instrumentation/roda/ignorer.rb b/lib/new_relic/agent/instrumentation/roda/ignorer.rb new file mode 100644 index 0000000000..b00fcfb5aa --- /dev/null +++ b/lib/new_relic/agent/instrumentation/roda/ignorer.rb @@ -0,0 +1,46 @@ +# 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 Roda + module Ignorer + def self.should_ignore?(app, type) + return false if !app.settings.respond_to?(:newrelic_ignores) + + app.settings.newrelic_ignores[type].any? do |pattern| + pattern.match(app.request.path_info) + end + end + + def newrelic_ignore(*routes) + set_newrelic_ignore(:routes, *routes) + end + + def newrelic_ignore_apdex(*routes) + set_newrelic_ignore(:apdex, *routes) + end + + def newrelic_ignore_enduser(*routes) + set_newrelic_ignore(:enduser, *routes) + end + + private + + def set_newrelic_ignore(type, *routes) + # Important to default this in the context of the actual app + # If it's done at register time, ignores end up shared between apps. + set(:newrelic_ignores, Hash.new([])) if !respond_to?(:newrelic_ignores) + + # If we call an ignore without a route, it applies to the whole app + routes = ['*'] if routes.empty? + + settings.newrelic_ignores[type] += routes.map do |r| + # Ugly sending to private Base#compile, but we want to mimic + # exactly Sinatra's mapping of route text to regex + Array(send(:compile, r)).first + end + end + end + end +end From 115b088fad33aa152731d923fadc45faa3c90ddf Mon Sep 17 00:00:00 2001 From: hramadan Date: Tue, 17 Oct 2023 13:06:48 -0700 Subject: [PATCH 2/8] Add support for newrelic_ignore* --- lib/new_relic/agent/instrumentation/roda.rb | 2 ++ .../agent/instrumentation/roda/ignorer.rb | 28 +++++++++---------- .../instrumentation/roda/instrumentation.rb | 12 ++++++++ 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/roda.rb b/lib/new_relic/agent/instrumentation/roda.rb index 2606c602d3..c9f85b263f 100644 --- a/lib/new_relic/agent/instrumentation/roda.rb +++ b/lib/new_relic/agent/instrumentation/roda.rb @@ -4,6 +4,7 @@ require_relative 'roda/instrumentation' require_relative 'roda/roda_transaction_namer' +require_relative 'roda/ignorer' DependencyDetection.defer do named :roda @@ -30,5 +31,6 @@ chain_instrument NewRelic::Agent::Instrumentation::Roda::Build::Chain chain_instrument NewRelic::Agent::Instrumentation::Roda::Chain end + Roda.class_eval { extend NewRelic::Agent::Instrumentation::Roda::Ignorer } end end diff --git a/lib/new_relic/agent/instrumentation/roda/ignorer.rb b/lib/new_relic/agent/instrumentation/roda/ignorer.rb index b00fcfb5aa..acaa3210c9 100644 --- a/lib/new_relic/agent/instrumentation/roda/ignorer.rb +++ b/lib/new_relic/agent/instrumentation/roda/ignorer.rb @@ -6,10 +6,10 @@ module NewRelic::Agent::Instrumentation module Roda module Ignorer def self.should_ignore?(app, type) - return false if !app.settings.respond_to?(:newrelic_ignores) + return false if !app.opts.include?(:newrelic_ignores) - app.settings.newrelic_ignores[type].any? do |pattern| - pattern.match(app.request.path_info) + app.opts[:newrelic_ignores][type].any? do |pattern| + pattern === app.request.path_info end end @@ -28,17 +28,17 @@ def newrelic_ignore_enduser(*routes) private def set_newrelic_ignore(type, *routes) - # Important to default this in the context of the actual app - # If it's done at register time, ignores end up shared between apps. - set(:newrelic_ignores, Hash.new([])) if !respond_to?(:newrelic_ignores) - - # If we call an ignore without a route, it applies to the whole app - routes = ['*'] if routes.empty? - - settings.newrelic_ignores[type] += routes.map do |r| - # Ugly sending to private Base#compile, but we want to mimic - # exactly Sinatra's mapping of route text to regex - Array(send(:compile, r)).first + # Create a newrelic_ignores hash if one doesn't exist + opts[:newrelic_ignores] = Hash.new([]) if !opts.include?(:newrelic_ignores) + + if routes.empty? + opts[:newrelic_ignores][type] += [Regexp.new('.*')] + else + opts[:newrelic_ignores][type] += routes.map do |r| + # Roda adds leading slashes to routes, so we need to do the same + r = '/' + r unless r.start_with?('/') + r + end end end end diff --git a/lib/new_relic/agent/instrumentation/roda/instrumentation.rb b/lib/new_relic/agent/instrumentation/roda/instrumentation.rb index 28b0a9fc80..5862a59b67 100644 --- a/lib/new_relic/agent/instrumentation/roda/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/roda/instrumentation.rb @@ -51,6 +51,18 @@ def _roda_handle_main_route_with_tracing(*args) yield end end + + def do_not_trace? + NewRelic::Agent::Instrumentation::Roda::Ignorer.should_ignore?(self, :routes) + end + + def ignore_apdex? + NewRelic::Agent::Instrumentation::Roda::Ignorer.should_ignore?(self, :apdex) + end + + def ignore_enduser? + NewRelic::Agent::Instrumentation::Roda::Ignorer.should_ignore?(self, :enduser) + end end end end From c54cf72c025eea455678ec33a6d6ce1e8196a607 Mon Sep 17 00:00:00 2001 From: hramadan Date: Tue, 17 Oct 2023 13:08:28 -0700 Subject: [PATCH 3/8] newrelic_ignore* tests --- test/multiverse/suites/roda/ignorer_test.rb | 204 ++++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 test/multiverse/suites/roda/ignorer_test.rb diff --git a/test/multiverse/suites/roda/ignorer_test.rb b/test/multiverse/suites/roda/ignorer_test.rb new file mode 100644 index 0000000000..5b3986d6ae --- /dev/null +++ b/test/multiverse/suites/roda/ignorer_test.rb @@ -0,0 +1,204 @@ +# 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 '../../../../lib/new_relic/agent/instrumentation/roda/instrumentation' +require_relative '../../../../lib/new_relic/agent/instrumentation/roda/roda_transaction_namer' +require_relative '../../../../lib/new_relic/agent/instrumentation/roda/ignorer' + +JS_AGENT_LOADER = 'JS_AGENT_LOADER' + +def assert_enduser_ignored(response) + refute_match(/#{JS_AGENT_LOADER}/o, response.body) +end + +def refute_enduser_ignored(response) + assert_match(/#{JS_AGENT_LOADER}/o, response.body) +end + +def fake_html_for_browser_timing_header + '' +end + +class RodaIgnorerTestApp < Roda + newrelic_ignore('/ignore_me', '/ignore_me_too') + newrelic_ignore('no_leading_slash') + newrelic_ignore('/ignored_erroring') + + route do |r| + r.on('/home') { 'home' } + r.on('/ignore_me') { 'this page is ignored' } + r.on('/ignore_me_too') { 'this page is ignored too' } + r.on('/ignore_me_not') { 'our regex should not capture this' } + r.on('no_leading_slash') { 'user does not use leading slash for ignore' } + r.on('/no_apdex') { 'no apex should be recorded' } + r.on('/ignored_erroring') { raise 'boom' } + end +end + +class RodaIgnoreTest < Minitest::Test + include Rack::Test::Methods + include MultiverseHelpers + + setup_and_teardown_agent + + def app + RodaIgnorerTestApp + end + + def test_seen_route + get('/home') + + assert_metrics_recorded('Controller/Roda/RodaIgnorerTestApp/GET home') + end + + def test_ignore_route + get('/ignore_me') + + assert_metrics_not_recorded([ + 'Controller/Roda/RodaIgnorerTestApp/GET ignore_me', + 'Apdex/Roda/RodaIgnorerTestApp/GET ignore_me' + ]) + end + + def test_regex_ignores_intended_route + get('/ignore_me') + get('/ignore_me_not') + + assert_metrics_not_recorded([ + 'Controller/Roda/RodaIgnorerTestApp/GET ignore_me', + 'Apdex/Roda/RodaIgnorerTestApp/GET ignore_me' + ]) + + assert_metrics_recorded([ + 'Controller/Roda/RodaIgnorerTestApp/GET ignore_me_not', + 'Apdex/Roda/RodaIgnorerTestApp/GET ignore_me_not' + ]) + end + + def test_ignores_if_route_does_not_have_leading_slash + get('no_leading_slash') + + assert_metrics_not_recorded([ + 'Controller/Roda/RodaIgnorerTestApp/GET no_leading_slash', + 'Apdex/Roda/RodaIgnorerTestApp/GET no_leading_slash' + ]) + end + + def test_ignore_errors_in_ignored_transactions + get('/ignored_erroring') + + assert_metrics_not_recorded(['Errors/all']) + end +end + +class RodaIgnoreAllRoutesApp < Roda + # newrelic_ignore called without any arguments will ignore the entire app + newrelic_ignore + + route do |r| + r.on('home') { 'home' } + r.on('hello') { 'hello' } + end +end + +class RodaIgnoreAllRoutesAppTest < Minitest::Test + include Rack::Test::Methods + include MultiverseHelpers + + setup_and_teardown_agent + + def app + RodaIgnoreAllRoutesApp + end + + def test_ignores_by_splats + get('/hello') + get('/home') + + assert_metrics_not_recorded([ + 'Controller/Roda/RodaIgnoreAllTestApp/GET hello', + 'Apdex/Roda/RodaIgnoreAllTestApp/GET hello' + ]) + + assert_metrics_not_recorded([ + 'Controller/Roda/RodaIgnoreAllTestApp/GET home', + 'Apdex/Roda/RodaIgnoreAllTestApp/GET home' + ]) + end +end + +class RodaIgnoreApdexApp < Roda + # newrelic_ignore called without any arguments will ignore the entire app + newrelic_ignore_apdex('/no_apdex') + + route do |r| + r.on('home') { 'home' } + r.on('no_apdex') { 'do not record apdex' } + end +end + +class RodaIgnoreApdexAppTest < Minitest::Test + include Rack::Test::Methods + include MultiverseHelpers + + setup_and_teardown_agent + + def app + RodaIgnoreApdexApp + end + + def test_ignores_apdex_by_route + get('/no_apdex') + + assert_metrics_not_recorded('Apdex/Roda/RodaIgnoreApdexApp/GET no_apdex') + end + + def test_ignores_enduser_but_not_route + get('no_apdex') + + assert_metrics_recorded('Controller/Roda/RodaIgnoreApdexApp/GET no_apdex') + assert_metrics_not_recorded('Apdex/Roda/RodaIgnoreApdexApp/GET no_apdex') + end +end + +class RodaIgnoreEndUserApp < Roda + newrelic_ignore_enduser('ignore_enduser') + + route do |r| + r.on('home') { fake_html_for_browser_timing_header } + r.on('ignore_enduser') { fake_html_for_browser_timing_header } + end +end + +class RodaIgnoreEndUserAppTest < Minitest::Test + include Rack::Test::Methods + include MultiverseHelpers + + setup_and_teardown_agent(:application_id => 'appId', + :beacon => 'beacon', + :browser_key => 'browserKey', + :js_agent_loader => 'JS_AGENT_LOADER') + + def app + RodaIgnoreEndUserApp + end + + def test_ignore_enduser_should_only_apply_to_specified_route + with_config(:application_id => 'appId', + :beacon => 'beacon', + :browser_key => 'browserKey', + :js_agent_loader => 'JS_AGENT_LOADER') do + get('home') + + refute_enduser_ignored(last_response) + assert_metrics_recorded('Controller/Roda/RodaIgnoreEndUserApp/GET home') + end + end + + def test_ignores_enduser + get('ignore_enduser') + + assert_enduser_ignored(last_response) + end +end From 39138b518302111dd7d127a34879cbc5aac07020 Mon Sep 17 00:00:00 2001 From: hramadan Date: Tue, 17 Oct 2023 14:33:10 -0700 Subject: [PATCH 4/8] Add CHANGELOG --- CHANGELOG.md | 13 +++++++++++-- test/multiverse/suites/roda/ignorer_test.rb | 1 - 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46cdad1cb0..47b5cd1b3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,11 +20,20 @@ Version gleans Docker container IDs from cgroups v2-based containers, reco For compatibility with Ruby 3.4 and to silence compatibility warnings present in Ruby 3.3, declare a dependency on the `base64` gem. The New Relic Ruby agent uses the native Ruby `base64` gem for Base 64 encoding/decoding. The agent is joined by Ruby on Rails ([rails/rails@3e52adf](https://github.com/rails/rails/commit/3e52adf28e90af490f7e3bdc4bcc85618a4e0867)) and others in making this change in preparation for Ruby 3.3/3.4. [PR#2238](https://github.com/newrelic/newrelic-ruby-agent/pull/2238) -- **Fix: Stop sending duplicate log events for Rails 7.1 users** +-**Feature: Add Roda support for the newrelic_ignore\* family of methods** + + The agent can now selectively disable instrumentation for particular requests within Roda applications. Supported methods include: + - `newrelic_ignore`: ignore a given route. + - `newrelic_ignore_apdex`: exclude a given route from consideration in overall Apdex calculations. + - `newrelic_ignore_enduser`: prevent automatic injection of the page load timing JavaScript when a route is rendered. + + For more information, see [Roda Instrumentation](https://docs.newrelic.com/docs/apm/agents/ruby-agent/instrumented-gems/roda-instrumentation/). [PR#2267](https://github.com/newrelic/newrelic-ruby-agent/pull/2267) + +- **Bugfix: Stop sending duplicate log events for Rails 7.1 users** Rails 7.1 introduced the public API [`ActiveSupport::BroadcastLogger`](https://api.rubyonrails.org/classes/ActiveSupport/BroadcastLogger.html). This logger replaces a private API, `ActiveSupport::Logger.broadcast`. In Rails versions below 7.1, the agent uses the `broadcast` method to stop duplicate logs from being recoded by broadcasted loggers. Now, we've updated the code to provide a similar duplication fix for the `ActiveSupport::BroadcastLogger` class. [PR#2252](https://github.com/newrelic/newrelic-ruby-agent/pull/2252) -- **Fix: Resolve Sidekiq 8.0 error handler deprecation warning** +- **Bugfix: Resolve Sidekiq 8.0 error handler deprecation warning** Sidekiq 8.0 will require procs passed to the error handler to include three arguments: error, context, and config. Users running sidekiq/main would receive a deprecation warning with this change any time an error was raised within a job. Thank you, [@fukayatsu](https://github.com/fukayatsu) for your proactive fix! [PR#2261](https://github.com/newrelic/newrelic-ruby-agent/pull/2261) diff --git a/test/multiverse/suites/roda/ignorer_test.rb b/test/multiverse/suites/roda/ignorer_test.rb index 5b3986d6ae..a6b776623d 100644 --- a/test/multiverse/suites/roda/ignorer_test.rb +++ b/test/multiverse/suites/roda/ignorer_test.rb @@ -129,7 +129,6 @@ def test_ignores_by_splats end class RodaIgnoreApdexApp < Roda - # newrelic_ignore called without any arguments will ignore the entire app newrelic_ignore_apdex('/no_apdex') route do |r| From db3dc7c6dedcded4d254e188064b0a129ed0054a Mon Sep 17 00:00:00 2001 From: Hannah Ramadan <76922290+hannahramadan@users.noreply.github.com> Date: Tue, 17 Oct 2023 15:52:47 -0700 Subject: [PATCH 5/8] Update lib/new_relic/agent/instrumentation/roda/ignorer.rb Co-authored-by: James Bunch --- lib/new_relic/agent/instrumentation/roda/ignorer.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/roda/ignorer.rb b/lib/new_relic/agent/instrumentation/roda/ignorer.rb index acaa3210c9..d94c1ceaf6 100644 --- a/lib/new_relic/agent/instrumentation/roda/ignorer.rb +++ b/lib/new_relic/agent/instrumentation/roda/ignorer.rb @@ -36,8 +36,7 @@ def set_newrelic_ignore(type, *routes) else opts[:newrelic_ignores][type] += routes.map do |r| # Roda adds leading slashes to routes, so we need to do the same - r = '/' + r unless r.start_with?('/') - r + "#{'/' unless r.start_with?('/')}#{r}" end end end From 0a4c1404dbf04f2fa313cd07e3dfca27ef63142f Mon Sep 17 00:00:00 2001 From: hramadan Date: Tue, 17 Oct 2023 16:09:57 -0700 Subject: [PATCH 6/8] Make private test methods private --- test/multiverse/suites/roda/ignorer_test.rb | 30 +++++++++++---------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/test/multiverse/suites/roda/ignorer_test.rb b/test/multiverse/suites/roda/ignorer_test.rb index a6b776623d..610c554019 100644 --- a/test/multiverse/suites/roda/ignorer_test.rb +++ b/test/multiverse/suites/roda/ignorer_test.rb @@ -6,20 +6,6 @@ require_relative '../../../../lib/new_relic/agent/instrumentation/roda/roda_transaction_namer' require_relative '../../../../lib/new_relic/agent/instrumentation/roda/ignorer' -JS_AGENT_LOADER = 'JS_AGENT_LOADER' - -def assert_enduser_ignored(response) - refute_match(/#{JS_AGENT_LOADER}/o, response.body) -end - -def refute_enduser_ignored(response) - assert_match(/#{JS_AGENT_LOADER}/o, response.body) -end - -def fake_html_for_browser_timing_header - '' -end - class RodaIgnorerTestApp < Roda newrelic_ignore('/ignore_me', '/ignore_me_too') newrelic_ignore('no_leading_slash') @@ -201,3 +187,19 @@ def test_ignores_enduser assert_enduser_ignored(last_response) end end + +private + +JS_AGENT_LOADER = 'JS_AGENT_LOADER' + +def assert_enduser_ignored(response) + refute_match(/#{JS_AGENT_LOADER}/o, response.body) +end + +def refute_enduser_ignored(response) + assert_match(/#{JS_AGENT_LOADER}/o, response.body) +end + +def fake_html_for_browser_timing_header + '' +end From 580693255a9754f3295e383ab60f85fa3c3ecb33 Mon Sep 17 00:00:00 2001 From: hramadan Date: Tue, 17 Oct 2023 16:10:33 -0700 Subject: [PATCH 7/8] Ruby style cleanup --- lib/new_relic/agent/instrumentation/roda/ignorer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/new_relic/agent/instrumentation/roda/ignorer.rb b/lib/new_relic/agent/instrumentation/roda/ignorer.rb index d94c1ceaf6..f8bfcf8e74 100644 --- a/lib/new_relic/agent/instrumentation/roda/ignorer.rb +++ b/lib/new_relic/agent/instrumentation/roda/ignorer.rb @@ -6,7 +6,7 @@ module NewRelic::Agent::Instrumentation module Roda module Ignorer def self.should_ignore?(app, type) - return false if !app.opts.include?(:newrelic_ignores) + return false unless app.opts.include?(:newrelic_ignores) app.opts[:newrelic_ignores][type].any? do |pattern| pattern === app.request.path_info From 3b755c1cee3e25f8f02002427f0194d08d768606 Mon Sep 17 00:00:00 2001 From: hramadan Date: Tue, 17 Oct 2023 16:24:01 -0700 Subject: [PATCH 8/8] Move private methods to classes --- test/multiverse/suites/roda/ignorer_test.rb | 30 ++++++++++----------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/test/multiverse/suites/roda/ignorer_test.rb b/test/multiverse/suites/roda/ignorer_test.rb index 610c554019..95d5cb881c 100644 --- a/test/multiverse/suites/roda/ignorer_test.rb +++ b/test/multiverse/suites/roda/ignorer_test.rb @@ -6,6 +6,8 @@ require_relative '../../../../lib/new_relic/agent/instrumentation/roda/roda_transaction_namer' require_relative '../../../../lib/new_relic/agent/instrumentation/roda/ignorer' +JS_AGENT_LOADER = 'JS_AGENT_LOADER' + class RodaIgnorerTestApp < Roda newrelic_ignore('/ignore_me', '/ignore_me_too') newrelic_ignore('no_leading_slash') @@ -148,6 +150,10 @@ def test_ignores_enduser_but_not_route end class RodaIgnoreEndUserApp < Roda + def fake_html_for_browser_timing_header + '' + end + newrelic_ignore_enduser('ignore_enduser') route do |r| @@ -160,6 +166,14 @@ class RodaIgnoreEndUserAppTest < Minitest::Test include Rack::Test::Methods include MultiverseHelpers + def assert_enduser_ignored(response) + refute_match(/#{JS_AGENT_LOADER}/o, response.body) + end + + def refute_enduser_ignored(response) + assert_match(/#{JS_AGENT_LOADER}/o, response.body) + end + setup_and_teardown_agent(:application_id => 'appId', :beacon => 'beacon', :browser_key => 'browserKey', @@ -187,19 +201,3 @@ def test_ignores_enduser assert_enduser_ignored(last_response) end end - -private - -JS_AGENT_LOADER = 'JS_AGENT_LOADER' - -def assert_enduser_ignored(response) - refute_match(/#{JS_AGENT_LOADER}/o, response.body) -end - -def refute_enduser_ignored(response) - assert_match(/#{JS_AGENT_LOADER}/o, response.body) -end - -def fake_html_for_browser_timing_header - '' -end