Merge pull request #693 from DataDog/refactor/dalli_replace_pin_with_…

…config

Deprecate Dalli pin in favor of configuration API

lib/ddtrace/contrib/dalli/instrumentation.rb

@@ -1,3 +1,4 @@
+require 'ddtrace/ext/app_types'
 require 'ddtrace/ext/net'
 require 'ddtrace/contrib/dalli/quantize'
 
@@ -6,27 +7,51 @@ module Contrib
     module Dalli
       # Instruments every interaction with the memcached server
       module Instrumentation
-        module_function
-
-        def patch!
-          ::Dalli::Server.class_eval do
-            alias_method :__request, :request
-
-            def request(op, *args)
-              pin = Datadog::Pin.get_from(::Dalli)
-
-              pin.tracer.trace(Datadog::Contrib::Dalli::Ext::SPAN_COMMAND) do |span|
-                span.resource = op.to_s.upcase
-                span.service = pin.service
-                span.span_type = pin.app_type
-                span.set_tag(Datadog::Ext::NET::TARGET_HOST, hostname)
-                span.set_tag(Datadog::Ext::NET::TARGET_PORT, port)
-                cmd = Datadog::Contrib::Dalli::Quantize.format_command(op, args)
-                span.set_tag(Datadog::Contrib::Dalli::Ext::TAG_COMMAND, cmd)
-
-                __request(op, *args)
-              end
+        def self.included(base)
+          if Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.0.0')
+            base.class_eval do
+              alias_method :request_without_datadog, :request
+              remove_method :request
+              include InstanceMethods
             end
+          else
+            base.send(:prepend, InstanceMethods)
+          end
+        end
+
+        # Compatibility shim for Rubies not supporting `.prepend`
+        module InstanceMethodsCompatibility
+          def request(*args, &block)
+            request_without_datadog(*args, &block)
+          end
+        end
+
+        # InstanceMethods - implementing instrumentation
+        module InstanceMethods
+          include InstanceMethodsCompatibility unless Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.0.0')
+
+          def request(op, *args)
+            tracer.trace(Datadog::Contrib::Dalli::Ext::SPAN_COMMAND) do |span|
+              span.resource = op.to_s.upcase
+              span.service = datadog_configuration[:service_name]
+              span.span_type = Datadog::Ext::AppTypes::CACHE
+              span.set_tag(Datadog::Ext::NET::TARGET_HOST, hostname)
+              span.set_tag(Datadog::Ext::NET::TARGET_PORT, port)
+              cmd = Datadog::Contrib::Dalli::Quantize.format_command(op, args)
+              span.set_tag(Datadog::Contrib::Dalli::Ext::TAG_COMMAND, cmd)
+
+              super
+            end
+          end
+
+          private
+
+          def tracer
+            datadog_configuration[:tracer]
+          end
+
+          def datadog_configuration
+            Datadog.configuration[:dalli]
           end
         end
       end

lib/ddtrace/contrib/dalli/patcher.rb

@@ -20,15 +20,24 @@ def patch
           do_once(:dalli) do
             begin
               add_pin!
-              Instrumentation.patch!
+              ::Dalli::Server.send(:include, Instrumentation)
+
+              # TODO: When Dalli pin is removed, set service info.
+              # get_option(:tracer).set_service_info(
+              #   get_option(:service_name),
+              #   Ext::APP,
+              #   Datadog::Ext::AppTypes::CACHE
+              # )
             rescue StandardError => e
               Datadog::Tracer.log.error("Unable to apply Dalli integration: #{e}")
             end
           end
         end
 
+        # DEPRECATED: Only kept for users still using `Dalli.datadog_pin` to configure.
+        #             Replaced by configuration API, i.e. `c.use :dalli`.
         def add_pin!
-          Pin
+          DeprecatedPin
             .new(
               get_option(:service_name),
               app: Ext::APP,
@@ -40,6 +49,31 @@ def add_pin!
         def get_option(option)
           Datadog.configuration[:dalli].get_option(option)
         end
+
+        # Implementation of deprecated Pin, which raises warnings when accessed.
+        # To be removed when support for Datadog::Pin with Dalli is removed.
+        class DeprecatedPin < Datadog::Pin
+          include Datadog::DeprecatedPin
+
+          DEPRECATION_WARNING = %(
+            Use of Datadog::Pin with Dalli is DEPRECATED.
+            Upgrade to the configuration API using the migration guide here:
+            https://github.com/DataDog/dd-trace-rb/releases/tag/v0.11.0).freeze
+
+          def tracer=(tracer)
+            Datadog.configuration[:dalli][:tracer] = tracer
+          end
+
+          def service_name=(service_name)
+            Datadog.configuration[:dalli][:service_name] = service_name
+          end
+
+          def log_deprecation_warning(method_name)
+            do_once(method_name) do
+              Datadog::Tracer.log.warn("#{method_name}:#{DEPRECATION_WARNING}")
+            end
+          end
+        end
       end
     end
   end

spec/ddtrace/contrib/dalli/instrumentation_spec.rb

@@ -10,18 +10,21 @@
 
   let(:client) { ::Dalli::Client.new("#{test_host}:#{test_port}") }
   let(:tracer) { get_test_tracer }
-  let(:pin) { ::Dalli.datadog_pin }
+  let(:configuration_options) { { tracer: tracer } }
 
   def all_spans
     tracer.writer.spans(:keep)
   end
 
   # Enable the test tracer
   before(:each) do
-    Datadog.configure { |c| c.use :dalli }
-    pin.tracer = tracer
+    Datadog.configure do |c|
+      c.use :dalli, configuration_options
+    end
   end
 
+  after(:each) { Datadog.registry[:dalli].reset_configuration! }
+
   it 'calls instrumentation' do
     client.set('abc', 123)
     try_wait_until { all_spans.any? }

spec/ddtrace/contrib/dalli/patcher_spec.rb

@@ -0,0 +1,113 @@
+require 'spec_helper'
+
+require 'dalli'
+require 'ddtrace'
+require 'ddtrace/contrib/dalli/patcher'
+
+RSpec.describe 'Dalli instrumentation' do
+  include_context 'tracer logging'
+
+  let(:tracer) { get_test_tracer }
+  let(:configuration_options) { { tracer: tracer } }
+
+  # Enable the test tracer
+  before(:each) do
+    Datadog.configure do |c|
+      c.use :dalli, configuration_options
+    end
+  end
+
+  def reset_deprecation_warnings!(pin)
+    if pin.instance_variable_defined?(:@done_once)
+      pin.instance_variable_get(:@done_once).delete('#datadog_pin')
+      pin.instance_variable_get(:@done_once).delete('#datadog_pin=')
+    end
+  end
+
+  let(:deprecation_warnings) do
+    [
+      /.*#datadog_pin.*/,
+      /.*Use of Datadog::Pin with Dalli is DEPRECATED.*/
+    ]
+  end
+
+  it 'does not generate deprecation warnings' do
+    expect(log_buffer.length).to eq(0)
+    expect(log_buffer).to_not contain_line_with(*deprecation_warnings)
+    expect(log_buffer).to_not contain_line_with(*deprecation_warnings)
+  end
+
+  context 'when pin is referenced by' do
+    describe 'Datadog::Pin.get_from' do
+      subject(:pin) { Datadog::Pin.get_from(Dalli) }
+      before(:each) { pin }
+      after(:each) { reset_deprecation_warnings!(pin) }
+
+      it { expect(log_buffer).to contain_line_with(*deprecation_warnings).once }
+
+      context 'twice' do
+        before(:each) { Datadog::Pin.get_from(Dalli) }
+        it { expect(log_buffer).to contain_line_with(*deprecation_warnings).once }
+      end
+
+      context 'and then calls' do
+        # Make sure 'service_name' passes through to underlying configuration
+        describe '#service_name=' do
+          before(:each) do
+            original_service_name
+            pin.service_name = service_name
+          end
+          let(:original_service_name) { Datadog.configuration[:dalli][:service_name] }
+          let(:service_name) { 'new_service' }
+          after(:each) { pin.service_name = original_service_name }
+          it { expect(Datadog.configuration[:dalli][:service_name]).to eq(service_name) }
+        end
+
+        # Make sure 'tracer' passes through to underlying configuration
+        describe 'tracer=' do
+          before(:each) { pin.tracer = new_tracer }
+          let(:new_tracer) { double('tracer') }
+          it { expect(Datadog.configuration[:dalli][:tracer]).to eq(new_tracer) }
+        end
+      end
+    end
+
+    describe '#datadog_pin' do
+      subject(:pin) { Dalli.datadog_pin }
+      before(:each) { pin }
+      after(:each) { reset_deprecation_warnings!(pin) }
+      it { expect(log_buffer).to contain_line_with(*deprecation_warnings).once }
+
+      context 'twice' do
+        it { expect(log_buffer).to contain_line_with(*deprecation_warnings).once }
+      end
+    end
+
+    describe '#datadog_pin=' do
+      before(:each) do
+        # Store original pin first
+        original_pin
+
+        # Set new pin
+        Dalli.datadog_pin = new_pin
+      end
+      let(:original_pin) do
+        # We know this to create deprecation warnings...
+        # Retrieve the pin and reset the buffer
+        Dalli.datadog_pin.tap do
+          log_buffer.truncate(0)
+          log_buffer.rewind
+        end
+      end
+      let(:new_pin) { Datadog::Pin.new('new_service') }
+
+      after(:each) do
+        # Restore original pin
+        Dalli.datadog_pin = original_pin
+        reset_deprecation_warnings!(original_pin)
+      end
+
+      it { expect(log_buffer).to contain_line_with(*deprecation_warnings).once }
+    end
+  end
+end