Properly deprecate instrumenter option

Author: bkeepers

lib/flipper.rb

@@ -68,7 +68,15 @@ def instance=(flipper)
                  :memoize=, :memoizing?,
                  :sync, :sync_secret # For Flipper::Cloud. Will error for OSS Flipper.
 
+  # Public: Get or set the configured instrumenter.
   def_delegators :configuration, :instrumenter, :instrumenter=
+
+  # Public: Instrument a call using the configured instrumenter.
+  #
+  #   Flipper.instrument(name, payload) do |payload|
+  #     payload[:result] = something
+  #   end
+  #
   def_delegators :instrumenter, :instrument
 
   # Public: Use this to register a group by name.
@@ -143,6 +151,7 @@ def groups_registry=(registry)
   end
 end
 
+require 'flipper/deprecated_instrumenter'
 require 'flipper/actor'
 require 'flipper/adapter'
 require 'flipper/adapters/memoizable'

lib/flipper/adapters/instrumented.rb

@@ -6,6 +6,7 @@ module Adapters
     # operations.
     class Instrumented < SimpleDelegator
       include ::Flipper::Adapter
+      include DeprecatedInstrumenter
 
       # Private: The name of instrumentation events.
       InstrumentationName = "adapter_operation.#{InstrumentationNamespace}".freeze
@@ -17,8 +18,9 @@ class Instrumented < SimpleDelegator
       #
       # adapter - Vanilla adapter instance to wrap.
       #
-      def initialize(adapter)
+      def initialize(adapter, options = {})
         super(adapter)
+        deprecated_instrumenter_option options
         @adapter = adapter
         @name = :instrumented
       end

lib/flipper/adapters/sync.rb

@@ -7,6 +7,7 @@ module Adapters
     # rather than in the main thread only when reads happen.
     class Sync
       include ::Flipper::Adapter
+      include DeprecatedInstrumenter
 
       # Public: The name of the adapter.
       attr_reader :name
@@ -22,6 +23,7 @@ class Sync
       # interval - The Float or Integer number of seconds between syncs from
       # remote to local. Default value is set in IntervalSynchronizer.
       def initialize(local, remote, options = {})
+        deprecated_instrumenter_option options
         @name = :sync
         @local = local
         @remote = remote

lib/flipper/adapters/sync/synchronizer.rb

@@ -8,6 +8,8 @@ class Sync
       # Public: Given a local and remote adapter, it can update the local to
       # match the remote doing only the necessary enable/disable operations.
       class Synchronizer
+        include DeprecatedInstrumenter
+
         # Public: Initializes a new synchronizer.
         #
         # local - The Flipper adapter to get in sync with the remote.
@@ -16,6 +18,7 @@ class Synchronizer
         # options - The Hash of options.
         #           :raise - Should errors be raised (default: true).
         def initialize(local, remote, options = {})
+          deprecated_instrumenter_option options
           @local = local
           @remote = remote
           @raise = options.fetch(:raise, true)

lib/flipper/cloud/configuration.rb

@@ -9,6 +9,8 @@
 module Flipper
   module Cloud
     class Configuration
+      include DeprecatedInstrumenter
+
       # The set of valid ways that syncing can happpen.
       VALID_SYNC_METHODS = Set[
         :poll,
@@ -58,6 +60,7 @@ class Configuration
       attr_accessor :sync_secret
 
       def initialize(options = {})
+        deprecated_instrumenter_option options
         @token = options.fetch(:token) { ENV["FLIPPER_CLOUD_TOKEN"] }
 
         if @token.nil?

lib/flipper/configuration.rb

@@ -1,7 +1,5 @@
 module Flipper
   class Configuration
-    attr_accessor :instrumenter
-
     def initialize(options = {})
       adapter { Flipper::Adapters::Memory.new }
       default { Flipper.new(adapter) }

lib/flipper/deprecated_instrumenter.rb

@@ -0,0 +1,18 @@
+module Flipper
+  # Private: Deprecation warnings for instrumenter option
+  module DeprecatedInstrumenter
+    def deprecated_instrumenter_option options
+      if options.has_key?(:instrumenter)
+        warn "The `:instrumenter` option is deprecated and has no effect. Set `Flipper.instrumenter` globally."
+        warn caller[1]
+      end
+    end
+
+    def instrumenter
+      warn "`#instrumenter` is deprecated. Use `Flipper.instrument` or `Flipper.instrumenter` instead."
+      warn caller[0]
+
+      Flipper.instrumenter
+    end
+  end
+end

lib/flipper/dsl.rb

@@ -3,6 +3,7 @@
 module Flipper
   class DSL
     extend Forwardable
+    include DeprecatedInstrumenter
 
     # Private
     attr_reader :adapter
@@ -15,6 +16,7 @@ class DSL
     # options - The Hash of options.
     #           :memoize - Should adapter be wrapped by memoize adapter or not.
     def initialize(adapter, options = {})
+      deprecated_instrumenter_option options
       memoize = options.fetch(:memoize, true)
       adapter = Adapters::Memoizable.new(adapter) if memoize
       @adapter = adapter

lib/flipper/feature.rb

@@ -6,6 +6,8 @@
 
 module Flipper
   class Feature
+    include DeprecatedInstrumenter
+
     # Private: The name of feature instrumentation events.
     InstrumentationName = "feature_operation.#{InstrumentationNamespace}".freeze
 
@@ -23,7 +25,8 @@ class Feature
     # name - The Symbol or String name of the feature.
     # adapter - The adapter that will be used to store details about this feature.
     #
-    def initialize(name, adapter)
+    def initialize(name, adapter, options = {})
+      deprecated_instrumenter_option options
       @name = name
       @key = name.to_s
       @adapter = adapter

lib/flipper/instrumenters/memory.rb

@@ -26,11 +26,10 @@ def instrument(name, payload = {})
         @events << Event.new(name, payload, result)
       end
 
-      def self.subscribe(_name, _subscriber)
+      def self.subscribe(_name, _callback = nil, &_block)
         # noop
       end
 
-
       def events_by_name(name)
         @events.select { |event| event.name == name }
       end

lib/flipper/instrumenters/noop.rb

@@ -5,7 +5,7 @@ def self.instrument(_name, payload = {})
         yield payload if block_given?
       end
 
-      def self.subscribe(_name, _subscriber)
+      def self.subscribe(_name, _callback = nil, &_block)
         # noop
       end
     end

spec/flipper/adapters/sync/synchronizer_spec.rb

@@ -32,10 +32,7 @@
 
   context "when raise disabled" do
     subject do
-      options = {
-        instrumenter: instrumenter,
-        raise: false,
-      }
+      options = { raise: false }
       described_class.new(local, remote, options)
     end
 

spec/spec_helper.rb

@@ -27,6 +27,14 @@
     Flipper.configuration = nil
   end
 
+  config.after(:each) do
+    # Remove all subscribers once AS::Notifications is loaded
+    if defined?(ActiveSupport::Notifications)
+      ActiveSupport::Notifications.unsubscribe /\.flipper$/
+    end
+  end
+
+
   config.disable_monkey_patching!
 
   config.filter_run focus: true