Merge pull request #1791 from newrelic/bugfix/rails-init-env-toggle

Enable defer_rails_initialization config by environment variable only

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 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.
+
+  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).

lib/new_relic/agent/configuration/default_source.rb

@@ -1614,12 +1614,17 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil)
         # Rails
         :'defer_rails_initialization' => {
           :default => false,
-          :public => false,
+          :public => true,
           :type => Boolean,
+          :external => true, # this config is used directly from the ENV variables
           :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.
+
+            <Callout variant="caution">
+              This option may only be set by environment variable.
+            </Callout>
+          DESCRIPTION
         },
         # Rake
         :'rake.tasks' => {

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

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

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)

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.
-  # defer_rails_initialization: false
-
   # If true, disables Action Cable instrumentation.
   # disable_action_cable_instrumentation: false
 

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

test/new_relic/newrelic_rpm_test.rb

@@ -5,81 +5,21 @@
 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
-  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)
-    # 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
-    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
 end