update: bugbash fixes. (#377)

1. cacheSize, cacheTtl, and customCache are exposed to public
2. logs and reasons added in DefaultCmabService
3. Default_max_retry set to 1

Author: FarhanAnjum-opti

lib/optimizely.rb

@@ -50,10 +50,6 @@ module Optimizely
   class Project
     include Optimizely::Decide
 
-    # CMAB Constants
-    DEFAULT_CMAB_CACHE_TIMEOUT = (30 * 60 * 1000)
-    DEFAULT_CMAB_CACHE_SIZE = 1000
-
     attr_reader :notification_center
     # @api no-doc
     attr_reader :config_manager, :decision_service, :error_handler, :event_dispatcher,
@@ -90,7 +86,8 @@ def initialize(
       event_processor: nil,
       default_decide_options: [],
       event_processor_options: {},
-      settings: nil
+      settings: nil,
+      cmab_service: nil
     )
       @logger = logger || NoOpLogger.new
       @error_handler = error_handler || NoOpErrorHandler.new
@@ -137,18 +134,22 @@ def initialize(
 
       setup_odp!(@config_manager.sdk_key)
 
-      # Initialize CMAB components
-      @cmab_client = DefaultCmabClient.new(
-        nil,
-        CmabRetryConfig.new,
-        @logger
-      )
-      @cmab_cache = LRUCache.new(DEFAULT_CMAB_CACHE_SIZE, DEFAULT_CMAB_CACHE_TIMEOUT)
-      @cmab_service = DefaultCmabService.new(
-        @cmab_cache,
-        @cmab_client,
-        @logger
-      )
+      # Initialize CMAB components if cmab service is nil
+      if cmab_service.nil?
+        @cmab_client = DefaultCmabClient.new(
+          nil,
+          CmabRetryConfig.new,
+          @logger
+        )
+        @cmab_cache = LRUCache.new(Optimizely::DefaultCmabCacheOptions::DEFAULT_CMAB_CACHE_SIZE, Optimizely::DefaultCmabCacheOptions::DEFAULT_CMAB_CACHE_TIMEOUT)
+        @cmab_service = DefaultCmabService.new(
+          @cmab_cache,
+          @cmab_client,
+          @logger
+        )
+      else
+        @cmab_service = cmab_service
+      end
 
       @decision_service = DecisionService.new(@logger, @cmab_service, @user_profile_service)
 

lib/optimizely/cmab/cmab_client.rb

@@ -20,7 +20,7 @@
 
 module Optimizely
   # Default constants for CMAB requests
-  DEFAULT_MAX_RETRIES = 3
+  DEFAULT_MAX_RETRIES = 1
   DEFAULT_INITIAL_BACKOFF = 0.1 # in seconds (100 ms)
   DEFAULT_MAX_BACKOFF = 10 # in seconds
   DEFAULT_BACKOFF_MULTIPLIER = 2.0

lib/optimizely/cmab/cmab_service.rb

@@ -17,6 +17,7 @@
 #
 require 'optimizely/odp/lru_cache'
 require 'optimizely/decide/optimizely_decide_option'
+require 'optimizely/logger'
 require 'digest'
 require 'json'
 require 'securerandom'
@@ -26,6 +27,12 @@ module Optimizely
   CmabDecision = Struct.new(:variation_id, :cmab_uuid, keyword_init: true)
   CmabCacheValue = Struct.new(:attributes_hash, :variation_id, :cmab_uuid, keyword_init: true)
 
+  class DefaultCmabCacheOptions
+    # CMAB Constants
+    DEFAULT_CMAB_CACHE_TIMEOUT = (30 * 60) # in seconds
+    DEFAULT_CMAB_CACHE_SIZE = 1000
+  end
+
   # Default CMAB service implementation
   class DefaultCmabService
     # Initializes a new instance of the CmabService.
@@ -42,7 +49,7 @@ class DefaultCmabService
     def initialize(cmab_cache, cmab_client, logger = nil)
       @cmab_cache = cmab_cache
       @cmab_client = cmab_client
-      @logger = logger
+      @logger = logger || NoOpLogger.new
       @locks = Array.new(NUM_LOCK_STRIPES) { Mutex.new }
     end
 
@@ -81,30 +88,65 @@ def get_decision_impl(project_config, user_context, rule_id, options)
       # @return [CmabDecision] The decision object containing variation_id and cmab_uuid.
 
       filtered_attributes = filter_attributes(project_config, user_context, rule_id)
+      reasons = []
+
+      if options&.include?(Decide::OptimizelyDecideOption::IGNORE_CMAB_CACHE)
+        reason = "Ignoring CMAB cache for user '#{user_context.user_id}' and rule '#{rule_id}'"
+        @logger.log(Logger::DEBUG, reason)
+        reasons << reason
+        cmab_decision = fetch_decision(rule_id, user_context.user_id, filtered_attributes)
+        return [cmab_decision, reasons]
+      end
 
-      return fetch_decision(rule_id, user_context.user_id, filtered_attributes) if options&.include?(Decide::OptimizelyDecideOption::IGNORE_CMAB_CACHE)
-
-      @cmab_cache.reset if options&.include?(Decide::OptimizelyDecideOption::RESET_CMAB_CACHE)
+      if options&.include?(Decide::OptimizelyDecideOption::RESET_CMAB_CACHE)
+        reason = "Resetting CMAB cache for user '#{user_context.user_id}' and rule '#{rule_id}'"
+        @logger.log(Logger::DEBUG, reason)
+        reasons << reason
+        @cmab_cache.reset
+      end
 
       cache_key = get_cache_key(user_context.user_id, rule_id)
 
-      @cmab_cache.remove(cache_key) if options&.include?(Decide::OptimizelyDecideOption::INVALIDATE_USER_CMAB_CACHE)
+      if options&.include?(Decide::OptimizelyDecideOption::INVALIDATE_USER_CMAB_CACHE)
+        reason = "Invalidating CMAB cache for user '#{user_context.user_id}' and rule '#{rule_id}'"
+        @logger.log(Logger::DEBUG, reason)
+        reasons << reason
+        @cmab_cache.remove(cache_key)
+      end
+
       cached_value = @cmab_cache.lookup(cache_key)
       attributes_hash = hash_attributes(filtered_attributes)
 
       if cached_value
-        return CmabDecision.new(variation_id: cached_value.variation_id, cmab_uuid: cached_value.cmab_uuid) if cached_value.attributes_hash == attributes_hash
-
-        @cmab_cache.remove(cache_key)
+        if cached_value.attributes_hash == attributes_hash
+          reason = "CMAB cache hit for user '#{user_context.user_id}' and rule '#{rule_id}'"
+          @logger.log(Logger::DEBUG, reason)
+          reasons << reason
+          return [CmabDecision.new(variation_id: cached_value.variation_id, cmab_uuid: cached_value.cmab_uuid), reasons]
+        else
+          reason = "CMAB cache attributes mismatch for user '#{user_context.user_id}' and rule '#{rule_id}', fetching new decision."
+          @logger.log(Logger::DEBUG, reason)
+          reasons << reason
+          @cmab_cache.remove(cache_key)
+        end
+      else
+        reason = "CMAB cache miss for user '#{user_context.user_id}' and rule '#{rule_id}'"
+        @logger.log(Logger::DEBUG, reason)
+        reasons << reason
       end
+
       cmab_decision = fetch_decision(rule_id, user_context.user_id, filtered_attributes)
+      reason = "CMAB decision is #{cmab_decision.to_h}"
+      @logger.log(Logger::DEBUG, reason)
+      reasons << reason
+
       @cmab_cache.save(cache_key,
                        CmabCacheValue.new(
                          attributes_hash: attributes_hash,
                          variation_id: cmab_decision.variation_id,
                          cmab_uuid: cmab_decision.cmab_uuid
                        ))
-      cmab_decision
+      [cmab_decision, reasons]
     end
 
     def fetch_decision(rule_id, user_id, attributes)

lib/optimizely/decision_service.rb

@@ -649,9 +649,10 @@ def get_decision_for_cmab_experiment(project_config, experiment, user_context, b
 
       # User is in CMAB allocation, proceed to CMAB decision
       begin
-        cmab_decision = @cmab_service.get_decision(
+        cmab_decision, reasons = @cmab_service.get_decision(
           project_config, user_context, experiment['id'], decide_options
         )
+        decide_reasons.push(*reasons)
         CmabDecisionResult.new(false, cmab_decision, decide_reasons)
       rescue StandardError => e
         error_message = "Failed to fetch CMAB data for experiment #{experiment['key']}."

lib/optimizely/optimizely_factory.rb

@@ -22,6 +22,8 @@
 require 'optimizely/event/batch_event_processor'
 require 'optimizely/logger'
 require 'optimizely/notification_center'
+require 'optimizely/cmab/cmab_client'
+require 'optimizely/cmab/cmab_service'
 
 module Optimizely
   class OptimizelyFactory
@@ -83,6 +85,40 @@ def self.blocking_timeout(blocking_timeout)
       @blocking_timeout = blocking_timeout
     end
 
+    # Convenience method for setting CMAB cache size.
+    # @param cache_size Integer - Maximum number of items in CMAB cache.
+    # @param logger - Optional LoggerInterface Provides a log method to log messages.
+    def self.cmab_cache_size(cache_size, logger = NoOpLogger.new)
+      unless cache_size.is_a?(Integer) && cache_size.positive?
+        logger.log(
+          Logger::ERROR,
+          "CMAB cache size is invalid, setting to default size #{Optimizely::DefaultCmabCacheOptions::DEFAULT_CMAB_CACHE_SIZE}."
+        )
+        return
+      end
+      @cmab_cache_size = cache_size
+    end
+
+    # Convenience method for setting CMAB cache TTL.
+    # @param cache_ttl Numeric - Time in seconds for cache entries to live.
+    # @param logger - Optional LoggerInterface Provides a log method to log messages.
+    def self.cmab_cache_ttl(cache_ttl, logger = NoOpLogger.new)
+      unless cache_ttl.is_a?(Numeric) && cache_ttl.positive?
+        logger.log(
+          Logger::ERROR,
+          "CMAB cache TTL is invalid, setting to default TTL #{Optimizely::DefaultCmabCacheOptions::DEFAULT_CMAB_CACHE_TIMEOUT}."
+        )
+        return
+      end
+      @cmab_cache_ttl = cache_ttl
+    end
+
+    # Convenience method for setting custom CMAB cache.
+    # @param custom_cache - Cache implementation responding to lookup, save, remove, and reset methods.
+    def self.cmab_custom_cache(custom_cache)
+      @cmab_custom_cache = custom_cache
+    end
+
     # Returns a new optimizely instance.
     #
     # @params sdk_key - Required String uniquely identifying the fallback datafile corresponding to project.
@@ -165,6 +201,14 @@ def self.custom_instance( # rubocop:disable Metrics/ParameterLists
         notification_center: notification_center
       )
 
+      # Initialize CMAB components
+      cmab_client = DefaultCmabClient.new(logger: logger)
+      cmab_cache = @cmab_custom_cache || LRUCache.new(
+        @cmab_cache_size || Optimizely::DefaultCmabCacheOptions::DEFAULT_CMAB_CACHE_SIZE,
+        @cmab_cache_ttl || Optimizely::DefaultCmabCacheOptions::DEFAULT_CMAB_CACHE_TIMEOUT
+      )
+      cmab_service = DefaultCmabService.new(cmab_cache, cmab_client, logger)
+
       Optimizely::Project.new(
         datafile: datafile,
         event_dispatcher: event_dispatcher,
@@ -176,7 +220,8 @@ def self.custom_instance( # rubocop:disable Metrics/ParameterLists
         config_manager: config_manager,
         notification_center: notification_center,
         event_processor: event_processor,
-        settings: settings
+        settings: settings,
+        cmab_service: cmab_service
       )
     end
   end

spec/cmab/cmab_service_spec.rb

@@ -5,11 +5,12 @@
 require 'optimizely/odp/lru_cache'
 require 'optimizely/cmab/cmab_client'
 require 'optimizely/decide/optimizely_decide_option'
+require 'optimizely/logger'
 
 describe Optimizely::DefaultCmabService do
   let(:mock_cmab_cache) { instance_double(Optimizely::LRUCache) }
   let(:mock_cmab_client) { instance_double(Optimizely::DefaultCmabClient) }
-  let(:mock_logger) { double('logger') }
+  let(:mock_logger) { Optimizely::NoOpLogger.new }
   let(:cmab_service) { described_class.new(mock_cmab_cache, mock_cmab_client, mock_logger) }
 
   let(:mock_project_config) { double('project_config') }
@@ -47,18 +48,19 @@
 
       allow(mock_cmab_cache).to receive(:lookup).with(expected_key).and_return(cached_value)
 
-      decision = cmab_service.get_decision(mock_project_config, mock_user_context, rule_id, [])
+      decision, reasons = cmab_service.get_decision(mock_project_config, mock_user_context, rule_id, [])
 
       expect(mock_cmab_cache).to have_received(:lookup).with(expected_key)
       expect(decision.variation_id).to eq('varA')
       expect(decision.cmab_uuid).to eq('uuid-123')
+      expect(reasons).to include(match(/CMAB cache hit for user '#{user_id}' and rule '#{rule_id}'/))
     end
 
     it 'ignores cache when option given' do
       allow(mock_cmab_client).to receive(:fetch_decision).and_return('varB')
       expected_attributes = {'age' => 25, 'location' => 'USA'}
 
-      decision = cmab_service.get_decision(
+      decision, reasons = cmab_service.get_decision(
         mock_project_config,
         mock_user_context,
         rule_id,
@@ -73,6 +75,7 @@
         expected_attributes,
         decision.cmab_uuid
       )
+      expect(reasons).to include(match(/Ignoring CMAB cache for user '#{user_id}' and rule '#{rule_id}'/))
     end
 
     it 'invalidates user cache when option given' do
@@ -98,7 +101,7 @@
       allow(mock_cmab_cache).to receive(:lookup).and_return(nil)
       allow(mock_cmab_cache).to receive(:save)
 
-      decision = cmab_service.get_decision(
+      decision, reasons = cmab_service.get_decision(
         mock_project_config,
         mock_user_context,
         rule_id,
@@ -108,6 +111,7 @@
       expect(mock_cmab_cache).to have_received(:reset)
       expect(decision.variation_id).to eq('varD')
       expect(decision.cmab_uuid).to be_a(String)
+      expect(reasons).to include(match(/Resetting CMAB cache for user '#{user_id}' and rule '#{rule_id}'/))
     end
 
     it 'fetches new decision when hash changes' do
@@ -126,7 +130,7 @@
       cmab_service.send(:hash_attributes, expected_attributes)
       expected_key = cmab_service.send(:get_cache_key, user_id, rule_id)
 
-      decision = cmab_service.get_decision(mock_project_config, mock_user_context, rule_id, [])
+      decision, reasons = cmab_service.get_decision(mock_project_config, mock_user_context, rule_id, [])
 
       expect(mock_cmab_cache).to have_received(:remove).with(expected_key)
       expect(mock_cmab_cache).to have_received(:save).with(
@@ -140,6 +144,7 @@
         expected_attributes,
         decision.cmab_uuid
       )
+      expect(reasons).to include(match(/CMAB cache attributes mismatch for user '#{user_id}' and rule '#{rule_id}', fetching new decision./))
     end
 
     it 'only passes cmab attributes to client' do
@@ -151,7 +156,7 @@
                                                                        })
       allow(mock_cmab_client).to receive(:fetch_decision).and_return('varF')
 
-      decision = cmab_service.get_decision(
+      decision, reasons = cmab_service.get_decision(
         mock_project_config,
         mock_user_context,
         rule_id,
@@ -165,6 +170,7 @@
         {'age' => 25, 'location' => 'USA'},
         decision.cmab_uuid
       )
+      expect(reasons).to include(match(/Ignoring CMAB cache for user '#{user_id}' and rule '#{rule_id}'/))
     end
   end