Deprecate Contrib::Configuration::Settings#tracer= (#1079)

ddtrace.gemspec

@@ -46,6 +46,8 @@ Gem::Specification.new do |spec|
   spec.add_development_dependency 'rspec', '~> 3.0'
   spec.add_development_dependency 'rspec-collection_matchers', '~> 1.1'
   spec.add_development_dependency 'minitest', '= 5.10.1'
+  spec.add_development_dependency 'minitest-around', '0.5.0'
+  spec.add_development_dependency 'minitest-stub_any_instance', '1.0.2'
   spec.add_development_dependency 'appraisal', '~> 2.2'
   spec.add_development_dependency 'yard', '~> 0.9'
   spec.add_development_dependency 'webmock', '~> 2.0'

lib/ddtrace/contrib/active_support/cache/redis.rb

@@ -14,7 +14,7 @@ module Patcher
             # We need to do a per-method monkey patching as some of them might
             # be redefined, and some of them not. The latest version of redis-activesupport
             # redefines write but leaves untouched read and delete:
-            # https://github.com/redis-store/redis-activesupport/blob/master/lib/active_support/cache/redis_store.rb
+            # https://github.com/redis-store/redis-activesupport/blob/v4.1.5/lib/active_support/cache/redis_store.rb
             #
             # For Rails >= 5.2 w/o redis-activesupport...
             # ActiveSupport includes a Redis cache store internally, and does not require these overrides.

lib/ddtrace/contrib/active_support/notifications/event.rb

@@ -18,6 +18,8 @@ def self.included(base)
           # Redefines some class behaviors for a Subscriber to make
           # it a bit simpler for an Event.
           module ClassMethods
+            DEFAULT_TRACER = -> { Datadog.tracer }
+
             def subscribe!
               super
             end
@@ -52,7 +54,7 @@ def span_options
             end
 
             def tracer
-              Datadog.tracer
+              DEFAULT_TRACER
             end
           end
         end

lib/ddtrace/contrib/concurrent_ruby/context_composite_executor_service.rb

@@ -16,11 +16,17 @@ def initialize(composited_executor)
 
         # post method runs the task within composited executor - in a different thread
         def post(*args, &task)
-          context = datadog_configuration.tracer.provider.context
+          parent_context = datadog_configuration.tracer.provider.context
 
           @composited_executor.post(*args) do
-            datadog_configuration.tracer.provider.context = context
-            yield
+            begin
+              original_context = datadog_configuration.tracer.provider.context
+              datadog_configuration.tracer.provider.context = parent_context
+              yield
+            ensure
+              # Restore context in case the current thread gets reused
+              datadog_configuration.tracer.provider.context = original_context
+            end
           end
         end
 

lib/ddtrace/contrib/configuration/settings.rb

@@ -12,6 +12,9 @@ class Settings
         option :service_name
         option :tracer do |o|
           o.delegate_to { Datadog.tracer }
+          o.on_set do |_value|
+            log_deprecation_warning(:tracer)
+          end
         end
 
         def configure(options = {})
@@ -29,6 +32,21 @@ def [](name)
         def []=(name, value)
           respond_to?("#{name}=") ? send("#{name}=", value) : set_option(name, value)
         end
+
+        DEPRECATION_WARNING = %(
+          Explicitly providing a tracer instance is DEPRECATED.
+          It's recommended to not provide an explicit tracer instance
+          and let Datadog::Contrib::Configuration::Settings resolve
+          the correct tracer internally.
+          ).freeze
+
+        include Datadog::Patcher # DEV includes #do_once here. We should move that logic to a generic component.
+
+        def log_deprecation_warning(method_name)
+          do_once(method_name) do
+            Datadog.logger.warn("#{method_name}:#{DEPRECATION_WARNING}:#{caller.join("\n")}")
+          end
+        end
       end
     end
   end

lib/ddtrace/contrib/graphql/patcher.rb

@@ -22,15 +22,16 @@ def patch
         end
 
         def patch_schema!(schema)
-          tracer = get_option(:tracer)
           service_name = get_option(:service_name)
           analytics_enabled = Contrib::Analytics.enabled?(get_option(:analytics_enabled))
           analytics_sample_rate = get_option(:analytics_sample_rate)
 
           if schema.respond_to?(:use)
             schema.use(
               ::GraphQL::Tracing::DataDogTracing,
-              tracer: tracer,
+              # By default, Tracing::DataDogTracing indirectly delegates the tracer instance
+              # to +Datadog.tracer+. If we provide a tracer argument here it will be eagerly cached,
+              # and Tracing::DataDogTracing will send traces to a stale tracer instance.
               service: service_name,
               analytics_enabled: analytics_enabled,
               analytics_sample_rate: analytics_sample_rate
@@ -39,7 +40,9 @@ def patch_schema!(schema)
             schema.define do
               use(
                 ::GraphQL::Tracing::DataDogTracing,
-                tracer: tracer,
+                # By default, Tracing::DataDogTracing indirectly delegates the tracer instance
+                # to +Datadog.tracer+. If we provide a tracer argument here it will be eagerly cached,
+                # and Tracing::DataDogTracing will send traces to a stale tracer instance.
                 service: service_name,
                 analytics_enabled: analytics_enabled,
                 analytics_sample_rate: analytics_sample_rate

lib/ddtrace/contrib/rails/configuration/settings.rb

@@ -76,18 +76,6 @@ def initialize(options = {})
               Datadog.configuration[:action_view][:template_base_path] = value
             end
           end
-
-          option :tracer do |o|
-            o.delegate_to { Datadog.tracer }
-            o.on_set do |value|
-              Datadog.configuration[:action_cable][:tracer] = value
-              Datadog.configuration[:active_record][:tracer] = value
-              Datadog.configuration[:active_support][:tracer] = value
-              Datadog.configuration[:action_pack][:tracer] = value
-              Datadog.configuration[:action_view][:tracer] = value
-              Datadog.configuration[:rack][:tracer] = value
-            end
-          end
         end
       end
     end

lib/ddtrace/contrib/rails/framework.rb

@@ -19,10 +19,12 @@ module Rails
       # - handle configuration entries which are specific to Datadog tracing
       # - instrument parts of the framework when needed
       module Framework
-        # configure Datadog settings
+        # After the Rails application finishes initializing, we configure the Rails
+        # integration and all its sub-components with the application information
+        # available.
+        # We do this after the initialization because not all the information we
+        # require is available before then.
         def self.setup
-          rails_config = config_with_defaults
-
           # NOTE: #configure has the side effect of rebuilding trace components.
           #       During a typical Rails application lifecycle, we will see trace
           #       components initialized twice because of this. This is necessary
@@ -31,6 +33,8 @@ def self.setup
           #       used to reconfigure tracer components with Rails-sourced defaults.
           #       This is a trade-off we take to get nice defaults.
           Datadog.configure do |datadog_config|
+            rails_config = config_with_defaults(datadog_config)
+
             # By default, default service would be guessed from the script
             # being executed, but here we know better, get it from Rails config.
             # Don't set this if service has been explicitly provided by the user.
@@ -43,17 +47,12 @@ def self.setup
             activate_action_view!(datadog_config, rails_config)
             activate_active_record!(datadog_config, rails_config)
           end
-
-          # Update the tracer if its not the default tracer.
-          if rails_config[:tracer] != Datadog.configuration.tracer
-            rails_config[:tracer].default_service = rails_config[:service_name]
-          end
         end
 
-        def self.config_with_defaults
+        def self.config_with_defaults(datadog_config)
           # We set defaults here instead of in the patcher because we need to wait
           # for the Rails application to be fully initialized.
-          Datadog.configuration[:rails].tap do |config|
+          datadog_config[:rails].tap do |config|
             config[:service_name] ||= (Datadog.configuration.service || Utils.app_name)
             config[:database_service] ||= "#{config[:service_name]}-#{Contrib::ActiveRecord::Utils.adapter_name}"
             config[:controller_service] ||= config[:service_name]
@@ -64,7 +63,6 @@ def self.config_with_defaults
         def self.activate_rack!(datadog_config, rails_config)
           datadog_config.use(
             :rack,
-            tracer: rails_config[:tracer],
             application: ::Rails.application,
             service_name: rails_config[:service_name],
             middleware_names: rails_config[:middleware_names],
@@ -77,8 +75,7 @@ def self.activate_active_support!(datadog_config, rails_config)
 
           datadog_config.use(
             :active_support,
-            cache_service: rails_config[:cache_service],
-            tracer: rails_config[:tracer]
+            cache_service: rails_config[:cache_service]
           )
         end
 
@@ -87,8 +84,7 @@ def self.activate_action_cable!(datadog_config, rails_config)
 
           datadog_config.use(
             :action_cable,
-            service_name: "#{rails_config[:service_name]}-#{Contrib::ActionCable::Ext::SERVICE_NAME}",
-            tracer: rails_config[:tracer]
+            service_name: "#{rails_config[:service_name]}-#{Contrib::ActionCable::Ext::SERVICE_NAME}"
           )
         end
 
@@ -101,8 +97,7 @@ def self.activate_action_pack!(datadog_config, rails_config)
 
           datadog_config.use(
             :action_pack,
-            service_name: rails_config[:service_name],
-            tracer: rails_config[:tracer]
+            service_name: rails_config[:service_name]
           )
         end
 
@@ -111,8 +106,7 @@ def self.activate_action_view!(datadog_config, rails_config)
 
           datadog_config.use(
             :action_view,
-            service_name: rails_config[:service_name],
-            tracer: rails_config[:tracer]
+            service_name: rails_config[:service_name]
           )
         end
 
@@ -121,8 +115,7 @@ def self.activate_active_record!(datadog_config, rails_config)
 
           datadog_config.use(
             :active_record,
-            service_name: rails_config[:database_service],
-            tracer: rails_config[:tracer]
+            service_name: rails_config[:database_service]
           )
         end
       end

lib/ddtrace/tracer.rb

@@ -314,7 +314,7 @@ def record(context)
     def record_context(context)
       trace = @context_flush.consume!(context)
 
-      write(trace) if trace && !trace.empty?
+      write(trace) if @enabled && trace && !trace.empty?
     end
 
     # Return the current active span or +nil+.
@@ -335,7 +335,7 @@ def active_correlation
     # Send the trace to the writer to enqueue the spans list in the agent
     # sending queue.
     def write(trace)
-      return if @writer.nil? || !@enabled
+      return if @writer.nil?
 
       if Datadog.configuration.diagnostics.debug
         Datadog.logger.debug("Writing #{trace.length} spans (enabled: #{@enabled})")

spec/ddtrace/contrib/action_cable/instrumentation_spec.rb

@@ -16,10 +16,7 @@
   include Rack::Test::Methods
   include_context 'Rails test application'
 
-  let(:tracer) { get_test_tracer }
-  let(:spans) { tracer.writer.spans(:keep) }
-
-  let(:options) { { tracer: tracer } }
+  let(:options) { {} }
 
   before do
     Datadog.configure do |c|
@@ -46,8 +43,13 @@
     subject! { get '/cable' }
 
     it 'overrides parent Rack resource' do
-      expect(span.name).to eq('rack.request')
-      expect(span.resource).to eq('ActionCable::Connection::Base#on_open')
+      action_cable, rack = spans
+
+      expect(action_cable.name).to eq('action_cable.on_open')
+      expect(action_cable.resource).to eq('ActionCable::Connection::Base#on_open')
+
+      expect(rack.name).to eq('rack.request')
+      expect(rack.resource).to eq('ActionCable::Connection::Base#on_open')
     end
   end
 end

spec/ddtrace/contrib/action_cable/patcher_spec.rb

@@ -15,8 +15,7 @@
 RSpec.describe 'ActionCable patcher' do
   before { skip('ActionCable not supported') unless Datadog::Contrib::ActionCable::Integration.compatible? }
 
-  let(:tracer) { get_test_tracer }
-  let(:configuration_options) { { tracer: tracer } }
+  let(:configuration_options) { {} }
 
   before do
     Datadog.configure do |c|
@@ -31,11 +30,9 @@
     Datadog.registry[:action_cable].reset_configuration!
   end
 
-  let(:all_spans) { tracer.writer.spans(:keep) }
-
   let(:span) do
-    expect(all_spans).to have(1).item
-    all_spans.find { |s| s.service == 'action_cable' }
+    expect(spans).to have(1).item
+    spans.find { |s| s.service == 'action_cable' }
   end
 
   context 'with server' do
@@ -135,12 +132,12 @@ def foo(data)
         end)
       end
 
-      let(:span) { all_spans.last } # Skip 'perform_action' span
+      let(:span) { spans.last } # Skip 'perform_action' span
 
       it 'traces transmit event' do
         perform
 
-        expect(all_spans).to have(2).items
+        expect(spans).to have(2).items
         expect(span.service).to eq('action_cable')
         expect(span.name).to eq('action_cable.transmit')
         expect(span.span_type).to eq('web')

spec/ddtrace/contrib/active_model_serializers/patcher_spec.rb

@@ -11,12 +11,7 @@
 RSpec.describe 'ActiveModelSerializers patcher' do
   include_context 'AMS serializer'
 
-  let(:tracer) { get_test_tracer }
-  let(:configuration_options) { { tracer: tracer } }
-
-  def all_spans
-    tracer.writer.spans(:keep)
-  end
+  let(:configuration_options) { {} }
 
   before(:each) do
     # Supress active_model_serializers log output in the test run
@@ -56,7 +51,7 @@ def all_spans
     end
 
     let(:active_model_serializers_span) do
-      all_spans.select { |s| s.name == name }.first
+      spans.select { |s| s.name == name }.first
     end
 
     if ActiveModelSerializersHelpers.ams_0_10_or_newer?