From 8b75702f94f8d6eb29ff6ff9285541cccbc8599e Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 15 Dec 2022 12:49:32 -0500 Subject: [PATCH 1/8] Enable defer_rails_initialization config by environment variable only Configs are loaded during initialization. This fix was placed behind a feature flag referencing the config value, but that value will always be falsey at initialization, so the bugfix was unreachable. Environment variables are accessible before initialization. The environment variable NEW_RELIC_DEFER_RAILS_INITIALIZATION may be set to true to enable this option. --- .../agent/configuration/default_source.rb | 12 ++- lib/new_relic/control/instance_methods.rb | 2 +- lib/newrelic_rpm.rb | 2 +- newrelic.yml | 2 +- .../rails60/config/initializers/test.rb | 3 +- test/new_relic/newrelic_rpm_test.rb | 82 +++++-------------- 6 files changed, 35 insertions(+), 68 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 4ec6901550..c2167e466f 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1907,12 +1907,16 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) # Rails :'defer_rails_initialization' => { :default => false, - :public => false, + :public => true, :type => Boolean, :allowed_from_server => false, - :description => 'This configuration option is currently unreachable. Please do not use. ' \ - 'If `true`, when the agent is in an application using Ruby on Rails, it will start after ' \ - 'config/initializers have run.' + :description => <<-DESCRIPTION + If `true`, when the agent is in an application using Ruby on Rails, it will start after `config/initializers` run. + + + This option may only be set by environment variable. + + DESCRIPTION }, # Rake :'rake.tasks' => { diff --git a/lib/new_relic/control/instance_methods.rb b/lib/new_relic/control/instance_methods.rb index bab3ef1032..1fe5738984 100644 --- a/lib/new_relic/control/instance_methods.rb +++ b/lib/new_relic/control/instance_methods.rb @@ -65,7 +65,7 @@ def init_plugin(options = {}) # An artifact of earlier implementation, we put both #add_method_tracer and #trace_execution # methods in the module methods. # Rails applications load the next two lines before any other initializers are run - unless defined?(Rails::VERSION) && NewRelic::Agent.config[:defer_rails_initialization] + unless defined?(Rails::VERSION) && ENV['NEW_RELIC_DEFER_RAILS_INITIALIZATION'] Module.send(:include, NewRelic::Agent::MethodTracer::ClassMethods) Module.send(:include, NewRelic::Agent::MethodTracer) end diff --git a/lib/newrelic_rpm.rb b/lib/newrelic_rpm.rb index c6917c2e93..67c472cd9a 100644 --- a/lib/newrelic_rpm.rb +++ b/lib/newrelic_rpm.rb @@ -20,7 +20,7 @@ if defined?(Rails::VERSION) module NewRelic class Railtie < Rails::Railtie - if NewRelic::Agent.config[:defer_rails_initialization] + if ENV['NEW_RELIC_DEFER_RAILS_INITIALIZATION'] initializer "newrelic_rpm.include_method_tracers", before: :load_config_initializers do |app| Module.send(:include, NewRelic::Agent::MethodTracer::ClassMethods) Module.send(:include, NewRelic::Agent::MethodTracer) diff --git a/newrelic.yml b/newrelic.yml index 4cc90ef448..465eef8278 100644 --- a/newrelic.yml +++ b/newrelic.yml @@ -163,7 +163,7 @@ common: &default_settings # datastore_tracer.instance_reporting.enabled: true # If true, when the agent is in an application using Ruby on Rails, it will start after - # config/initializers have run. + # config/initializers have run. This option may only be set by environment variable. # defer_rails_initialization: false # If true, disables Action Cable instrumentation. diff --git a/test/environments/rails60/config/initializers/test.rb b/test/environments/rails60/config/initializers/test.rb index 1d88e77b0a..d5d1a23d95 100644 --- a/test/environments/rails60/config/initializers/test.rb +++ b/test/environments/rails60/config/initializers/test.rb @@ -16,5 +16,6 @@ def sniff add_method_tracer :sniff end - +puts Rails.application.config.active_record.timestamped_migrations Rails.application.config.active_record.timestamped_migrations = false +puts Rails.application.config.active_record.timestamped_migrations diff --git a/test/new_relic/newrelic_rpm_test.rb b/test/new_relic/newrelic_rpm_test.rb index 4ea73ee1b7..874ae75db9 100644 --- a/test/new_relic/newrelic_rpm_test.rb +++ b/test/new_relic/newrelic_rpm_test.rb @@ -5,37 +5,22 @@ require_relative '../test_helper' class NewRelicRpmTest < Minitest::Test + RAILS_32_SKIP_MESSAGE = 'MySQL error for Rails 3.2 when we add an initializer to config/initializers' # This test examines the behavior of an initializer when the agent is started in a Rails context # The initializer used for these tests is defined in test/environments/*/config/inititalizers/test.rb # We have documentation recommending customers call add_method_tracer # in their initializers, let's make sure this works - def test_add_method_tracer_in_initializer_gets_traced_when_agent_initialized_before_config_initializers - skip unless defined?(Rails::VERSION) - skip 'MySQL error for Rails 3.2' if Rails::VERSION::MAJOR == 3 - - with_config(defer_rails_initialization: false) do - assert Bloodhound.newrelic_method_exists?('sniff'), - 'Bloodhound#sniff not found by' \ - 'NewRelic::Agent::MethodTracer::ClassMethods::AddMethodTracer.newrelic_method_exists?' - assert Bloodhound.method_traced?('sniff'), - 'Bloodhound#sniff not found by' \ - 'NewRelic::Agent::MethodTracer::ClassMethods::AddMethodTracer.method_traced?' - end - end - def test_add_method_tracer_in_initializer_gets_traced_when_agent_initialized_after_config_initializers skip unless defined?(Rails::VERSION) - skip 'MySQL error for Rails 3.2 if we define an initializer in config/initializers' if Rails::VERSION::MAJOR == 3 - - with_config(defer_rails_initialization: true) do - assert Bloodhound.newrelic_method_exists?('sniff'), - 'Bloodhound#sniff not found by' \ - 'NewRelic::Agent::MethodTracer::ClassMethods::AddMethodTracer.newrelic_method_exists?' - assert Bloodhound.method_traced?('sniff'), - 'Bloodhound#sniff not found by' \ - 'NewRelic::Agent::MethodTracer::ClassMethods::AddMethodTracer.method_traced?' - end + skip RAILS_32_SKIP_MESSAGE if Rails::VERSION::MAJOR == 3 + + assert Bloodhound.newrelic_method_exists?('sniff'), + 'Bloodhound#sniff not found by' \ + 'NewRelic::Agent::MethodTracer::ClassMethods::AddMethodTracer.newrelic_method_exists?' + assert Bloodhound.method_traced?('sniff'), + 'Bloodhound#sniff not found by' \ + 'NewRelic::Agent::MethodTracer::ClassMethods::AddMethodTracer.method_traced?' end # All supported Rails versions have the default value for @@ -44,42 +29,19 @@ def test_add_method_tracer_in_initializer_gets_traced_when_agent_initialized_aft def test_active_record_initializer_config_change_saved_when_agent_initialized_after_config_initializers skip unless defined?(Rails::VERSION) - # TODO: This test passes in a Rails console in a playground app and in the customer's environment - # but fails in this unit test context - skip if Gem::Version.new(NewRelic::VERSION::STRING) >= Gem::Version.new('8.13.0') - skip 'MySQL error for Rails 3.2' if Rails::VERSION::MAJOR == 3 - - with_config(defer_rails_initialization: true) do - # Verify the configuration value was set to the initializer value - refute Rails.application.config.active_record.timestamped_migrations, - "Rails.application.config.active_record.timestamped_migrations equals true, expected false" - - # Verify the configuration value was applied to the ActiveRecord class variable - if Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('7.1') - refute ActiveRecord.timestamped_migrations, "ActiveRecord.timestamped_migrations equals true, expected false" - else - refute ActiveRecord::Base.timestamped_migrations, - "ActiveRecord::Base.timestamped_migrations equals true, expected false" - end - end - end - - def test_active_record_initializer_config_change_not_saved_when_agent_initialized_before_config_initializers - skip unless defined?(Rails::VERSION) - skip 'MySQL error for Rails 3.2' if Rails::VERSION::MAJOR == 3 - - with_config(defer_rails_initialization: false) do - # Verify the configuration value was set to the initializer value - refute Rails.application.config.active_record.timestamped_migrations, - "Rails.application.config.active_record.timestamped_migrations equals true, expected false" - - # Rails.application.config value should not be applied (this is the state of the original bug) - if Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('7.1') - assert ActiveRecord.timestamped_migrations, "ActiveRecord.timestamped_migrations equals false, expected true" - else - assert ActiveRecord::Base.timestamped_migrations, - "ActiveRecord::Base.timestamped_migrations equals false, expected true" - end + skip RAILS_32_SKIP_MESSAGE if Rails::VERSION::MAJOR == 3 + # skip "Test passes in a Rails console on a playground app and in the customer's environment, but fails here" + + # Verify the configuration value was set to the initializer value + refute Rails.application.config.active_record.timestamped_migrations, + "Rails.application.config.active_record.timestamped_migrations equals true, expected false" + + # Verify the configuration value was applied to the ActiveRecord class variable + if Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('7.1') + refute ActiveRecord.timestamped_migrations, "ActiveRecord.timestamped_migrations equals true, expected false" + else + refute ActiveRecord::Base.timestamped_migrations, + "ActiveRecord::Base.timestamped_migrations equals true, expected false" end end end From 002afc034b438ec88ff6a878c4fe848338f507ea Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Thu, 9 Feb 2023 10:12:48 -0800 Subject: [PATCH 2/8] mark defer_rails_initialization as external --- 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 c2167e466f..14d2e3b8a4 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1909,6 +1909,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :default => false, :public => true, :type => Boolean, + :external => true, # this config is used directly from the ENV variables :allowed_from_server => false, :description => <<-DESCRIPTION If `true`, when the agent is in an application using Ruby on Rails, it will start after `config/initializers` run. From afe03d7248b18ad17557558d8957513297921a75 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Thu, 9 Feb 2023 10:19:32 -0800 Subject: [PATCH 3/8] check if rack mini profiler is using prepend --- lib/new_relic/agent/instrumentation/net_http.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/new_relic/agent/instrumentation/net_http.rb b/lib/new_relic/agent/instrumentation/net_http.rb index 104bb72b5a..53e2f83fa8 100644 --- a/lib/new_relic/agent/instrumentation/net_http.rb +++ b/lib/new_relic/agent/instrumentation/net_http.rb @@ -27,7 +27,7 @@ end conflicts_with_prepend do - defined?(Rack::MiniProfiler) + defined?(Rack::MiniProfiler) && !defined?(Rack::MINI_PROFILER_PREPEND_NET_HTTP_PATCH) end conflicts_with_prepend do From 7881f3540091a8d30721c4967a78763b9d57bed7 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Mon, 13 Feb 2023 09:08:10 -0800 Subject: [PATCH 4/8] remove env only config from newrelic.yml example --- newrelic.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/newrelic.yml b/newrelic.yml index 465eef8278..61bf89d577 100644 --- a/newrelic.yml +++ b/newrelic.yml @@ -162,10 +162,6 @@ common: &default_settings # or port_path_or_id parameters to transaction or slow SQL traces. # datastore_tracer.instance_reporting.enabled: true - # If true, when the agent is in an application using Ruby on Rails, it will start after - # config/initializers have run. This option may only be set by environment variable. - # defer_rails_initialization: false - # If true, disables Action Cable instrumentation. # disable_action_cable_instrumentation: false From 9188085a922621c0c9004110fcdf887d481f6199 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Mon, 13 Feb 2023 09:51:45 -0800 Subject: [PATCH 5/8] removed test that has never worked and is not possible to work in env tests --- test/new_relic/newrelic_rpm_test.rb | 42 ++++++++++++++--------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/test/new_relic/newrelic_rpm_test.rb b/test/new_relic/newrelic_rpm_test.rb index 874ae75db9..9ca377fbe3 100644 --- a/test/new_relic/newrelic_rpm_test.rb +++ b/test/new_relic/newrelic_rpm_test.rb @@ -23,25 +23,25 @@ def test_add_method_tracer_in_initializer_gets_traced_when_agent_initialized_aft 'NewRelic::Agent::MethodTracer::ClassMethods::AddMethodTracer.method_traced?' end - # All supported Rails versions have the default value for - # timestamped_migrations as true. Our initializer sets it to false. - # Test to resolve: https://github.com/newrelic/newrelic-ruby-agent/issues/662 - - def test_active_record_initializer_config_change_saved_when_agent_initialized_after_config_initializers - skip unless defined?(Rails::VERSION) - skip RAILS_32_SKIP_MESSAGE if Rails::VERSION::MAJOR == 3 - # skip "Test passes in a Rails console on a playground app and in the customer's environment, but fails here" - - # Verify the configuration value was set to the initializer value - refute Rails.application.config.active_record.timestamped_migrations, - "Rails.application.config.active_record.timestamped_migrations equals true, expected false" - - # Verify the configuration value was applied to the ActiveRecord class variable - if Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('7.1') - refute ActiveRecord.timestamped_migrations, "ActiveRecord.timestamped_migrations equals true, expected false" - else - refute ActiveRecord::Base.timestamped_migrations, - "ActiveRecord::Base.timestamped_migrations equals true, expected false" - end - end + # # All supported Rails versions have the default value for + # # timestamped_migrations as true. Our initializer sets it to false. + # # Test to resolve: https://github.com/newrelic/newrelic-ruby-agent/issues/662 + + # def test_active_record_initializer_config_change_saved_when_agent_initialized_after_config_initializers + # ENV['NEW_RELIC_DEFER_RAILS_INITIALIZATION'] = 'true' + # skip unless defined?(Rails::VERSION) + # skip RAILS_32_SKIP_MESSAGE if Rails::VERSION::MAJOR == 3 + # # skip "Test passes in a Rails console on a playground app and in the customer's environment, but fails here" + # # Verify the configuration value was set to the initializer value + # refute Rails.application.config.active_record.timestamped_migrations, + # "Rails.application.config.active_record.timestamped_migrations equals true, expected false" + + # # Verify the configuration value was applied to the ActiveRecord class variable + # if Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('7.1') + # refute ActiveRecord.timestamped_migrations, "ActiveRecord.timestamped_migrations equals true, expected false" + # else + # refute ActiveRecord::Base.timestamped_migrations, + # "ActiveRecord::Base.timestamped_migrations equals true, expected false" + # end + # end end From 3ae8614e80aa0eb39f09e90a0f9e316a14305ee7 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Mon, 13 Feb 2023 09:51:59 -0800 Subject: [PATCH 6/8] removed commented test --- test/new_relic/newrelic_rpm_test.rb | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/test/new_relic/newrelic_rpm_test.rb b/test/new_relic/newrelic_rpm_test.rb index 9ca377fbe3..abfac7b43f 100644 --- a/test/new_relic/newrelic_rpm_test.rb +++ b/test/new_relic/newrelic_rpm_test.rb @@ -22,26 +22,4 @@ def test_add_method_tracer_in_initializer_gets_traced_when_agent_initialized_aft 'Bloodhound#sniff not found by' \ 'NewRelic::Agent::MethodTracer::ClassMethods::AddMethodTracer.method_traced?' end - - # # All supported Rails versions have the default value for - # # timestamped_migrations as true. Our initializer sets it to false. - # # Test to resolve: https://github.com/newrelic/newrelic-ruby-agent/issues/662 - - # def test_active_record_initializer_config_change_saved_when_agent_initialized_after_config_initializers - # ENV['NEW_RELIC_DEFER_RAILS_INITIALIZATION'] = 'true' - # skip unless defined?(Rails::VERSION) - # skip RAILS_32_SKIP_MESSAGE if Rails::VERSION::MAJOR == 3 - # # skip "Test passes in a Rails console on a playground app and in the customer's environment, but fails here" - # # Verify the configuration value was set to the initializer value - # refute Rails.application.config.active_record.timestamped_migrations, - # "Rails.application.config.active_record.timestamped_migrations equals true, expected false" - - # # Verify the configuration value was applied to the ActiveRecord class variable - # if Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('7.1') - # refute ActiveRecord.timestamped_migrations, "ActiveRecord.timestamped_migrations equals true, expected false" - # else - # refute ActiveRecord::Base.timestamped_migrations, - # "ActiveRecord::Base.timestamped_migrations equals true, expected false" - # end - # end end From 96677d6db0d602eeb2a90ebc37ae763b01b58607 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Mon, 13 Feb 2023 09:54:40 -0800 Subject: [PATCH 7/8] add changelog entry --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd49d50659..2334e20b41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,15 @@ Community member [@fchatterji](https://github.com/fchatterji) helped standardize how we reference `NewRelic` throughout our codebase. Thanks fchatterji! [PR#1795](https://github.com/newrelic/newrelic-ruby-agent/pull/1795) +- **Bugfix: Allow rails initialization to be deferred by environment variable** + + The Ruby agent may force some Rails frameworks to load on agent initialization, preventing some settings defined in `config/initializers` from being applied. Changing the initialization process to run after `config/initializers`, however, may break the configuration for other gems (ex. Roadie Rails). + + For those having troubles with agent initialization and Rails initializers, you can now pass the environment variable `NEW_RELIC_DEFER_RAILS_INITIALIZATION=true` to make the agent initialize after `config/initializers` are run. This config option can only be set using an environment variable and can't be set using YAML. + + Thanks to [@jdelStrother](https://github.com/jdelStrother) for bringing this issue to our attention and testing our fixes along the way. [Issue#662](https://github.com/newrelic/newrelic-ruby-agent/issues/662) [PR#1791](https://github.com/newrelic/newrelic-ruby-agent/pull/1791) + + ## 8.16.0 Version 8.16.0 introduces more Ruby on Rails instrumentation (especially for Rails 6 and 7) for various Action\*/Active\* libraries whose actions produce [Active Support notifications events](https://guides.rubyonrails.org/active_support_instrumentation.html). From 319c9099e244997296781a53b7c3d5d8b87470a3 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Mon, 13 Feb 2023 10:10:36 -0800 Subject: [PATCH 8/8] Update CHANGELOG.md Co-authored-by: James Bunch --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2334e20b41..523e323a00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,7 +98,7 @@ - **Bugfix: Allow rails initialization to be deferred by environment variable** - The Ruby agent may force some Rails frameworks to load on agent initialization, preventing some settings defined in `config/initializers` from being applied. Changing the initialization process to run after `config/initializers`, however, may break the configuration for other gems (ex. Roadie Rails). + The Ruby agent may force some Rails libraries to load on agent initialization, preventing some settings defined in `config/initializers` from being applied. Changing the initialization process to run after `config/initializers`, however, may break the configuration for other gems (ex. Roadie Rails). For those having troubles with agent initialization and Rails initializers, you can now pass the environment variable `NEW_RELIC_DEFER_RAILS_INITIALIZATION=true` to make the agent initialize after `config/initializers` are run. This config option can only be set using an environment variable and can't be set using YAML.