FFM-12192 Only create global target once / Add guard for nil target, and add test for invalid metric / Fix tests

Author: erdirowlands

lib/ff/ruby/server/sdk/api/inner_client_flag_evaluate_callback.rb

@@ -23,7 +23,7 @@ def initialize(metrics_processor, logger = nil)
 
   def process_evaluation(feature_config, target, variation)
 
-    @logger.debug "Processing evaluation: " + feature_config.feature.to_s + ", " + target.identifier.to_s
+    @logger.debug "Processing evaluation: #{feature_config&.feature || 'nil feature'}, #{target&.identifier || 'nil target'}"
 
     @metrics_processor.register_evaluation(target, feature_config, variation)
   end

lib/ff/ruby/server/sdk/api/metrics_event.rb

@@ -16,6 +16,10 @@ def ==(other)
   end
 
   def eql?(other)
+    # Guard clause other is the same type.
+    # While it should be, this adds protection for an edge case we are seeing with very large
+    # project sizes. Issue being tracked in FFM-12192, and once resolved, can feasibly remove
+    # these checks in a future release.
     unless other.is_a?(MetricsEvent)
       @logger.warn("Warning: Attempted to compare MetricsEvent with #{other.class.name}" )
       return false

lib/ff/ruby/server/sdk/api/metrics_processor.rb

@@ -9,6 +9,7 @@
 require_relative "../api/summary_metrics"
 
 class MetricsProcessor < Closeable
+  GLOBAL_TARGET = Target.new(identifier: "__global__cf_target", name: "Global Target").freeze
 
   class FrequencyMap < Concurrent::Map
     def initialize(options = nil, &block)
@@ -66,7 +67,6 @@ def init(connector, config, callback)
     @target_attribute = "target"
     @global_target_identifier = "__global__cf_target" # <--- This target identifier is used to aggregate and send data for all
     #                                             targets as a summary
-    @global_target = Target.new("RubySDK1", identifier = @global_target_identifier, name = @global_target_name)
     @ready = false
     @jar_version = Ff::Ruby::Server::Sdk::VERSION
     @server = "server"
@@ -112,7 +112,9 @@ def close
 
   def register_evaluation(target, feature_config, variation)
     register_evaluation_metric(feature_config, variation)
-    register_target_metric(target)
+    if target
+      register_target_metric(target)
+    end
   end
 
   private
@@ -123,17 +125,17 @@ def register_evaluation_metric(feature_config, variation)
     # project sizes.  Issue being tracked in FFM-12192, and once resolved, can feasibly remove
     # these checks in a future release.
     if feature_config.nil? || !feature_config.respond_to?(:feature) || feature_config.feature.nil?
-      @config.logger.warn("Invalid MetricsEvent: feature_config is missing or incomplete. feature_config=#{feature_config.inspect}")
+      @config.logger.warn("Skipping invalid MetricsEvent: feature_config is missing or incomplete. feature_config=#{feature_config.inspect}")
       return
     end
 
-    if @global_target.nil? || !@global_target.respond_to?(:identifier) || @global_target.identifier.nil?
-      @config.logger.warn("Invalid MetricsEvent: global_target is missing or incomplete. global_target=#{@global_target.inspect}")
+    if GLOBAL_TARGET.nil? || !GLOBAL_TARGET.respond_to?(:identifier) || GLOBAL_TARGET.identifier.nil?
+      @config.logger.warn("Skipping invalid MetricsEvent: global_target is missing or incomplete. global_target=#{GLOBAL_TARGET.inspect}")
       return
     end
 
     if variation.nil? || !variation.respond_to?(:identifier) || variation.identifier.nil?
-      @config.logger.warn("Invalid MetricsEvent: variation is missing or incomplete. variation=#{variation.inspect}")
+      @config.logger.warn("Skipping iInvalid MetricsEvent: variation is missing or incomplete. variation=#{variation.inspect}")
       return
     end
 
@@ -146,7 +148,7 @@ def register_evaluation_metric(feature_config, variation)
       return
     end
 
-    event = MetricsEvent.new(feature_config, @global_target, variation, @config.logger)
+    event = MetricsEvent.new(feature_config, GLOBAL_TARGET, variation, @config.logger)
     @evaluation_metrics.increment event
   end
 

lib/ff/ruby/server/sdk/version.rb

@@ -5,7 +5,7 @@ module Ruby
     module Server
       module Sdk
 
-        VERSION = "1.4.0"
+        VERSION = "1.4.1"
       end
     end
   end

scripts/sdk_specs.sh

@@ -1,4 +1,4 @@
 #!/bin/bash
 
 export ff_ruby_sdk="ff-ruby-server-sdk"
-export ff_ruby_sdk_version="1.4.0"
+export ff_ruby_sdk_version="1.4.1"

test/ff/ruby/server/sdk/cf_client_test.rb

@@ -37,7 +37,7 @@ class CfClientTest < Minitest::Test
                                             .event_url("http://localhost:#{port}/api/1.0")
                                             .config_url("http://localhost:#{port}/api/1.0").build)
 
-      client.wait_for_initialization
+      client.wait_for_initialization(timeout_ms: 5000)
 
       target = Target.new("RubyTestSDK", identifier="rubytestsdk", attributes={"location": "emea"})
 

test/ff/ruby/server/sdk/metrics_processor_test.rb

@@ -132,7 +132,7 @@ def test_metrics
 
     assert_equal 0, freq_map.size, "not all pending metrics were flushed"
     assert_equal 1, connector.get_captured_metrics.size
-    assert_equal 2, connector.get_captured_metrics[0].target_data.size, "there should only be 2 targets"
+    assert_equal 1, connector.get_captured_metrics[0].target_data.size, "there should only be 1 targets"
 
     connector.get_captured_metrics.each do |metric|
       metric.metrics_data.each do |metric_data|
@@ -156,16 +156,15 @@ def assert_target_data(target_data)
       end
     end
 
-    assert targets.key?("__global__cf_target")
     assert targets.key?("target-id")
   end
 
   def test_frequency_map_increment
 
     map = MetricsProcessor::FrequencyMap.new
 
-    event1 = MetricsEvent.new(@feature1, @target, @variation1)
-    event2 = MetricsEvent.new(@feature2, @target, @variation2)
+    event1 = MetricsEvent.new(@feature1, @target, @variation1, Logger.new(STDOUT))
+    event2 = MetricsEvent.new(@feature2, @target, @variation2, Logger.new(STDOUT))
 
     map.increment event1
     map.increment event2
@@ -180,8 +179,8 @@ def test_frequency_map_increment
   def test_frequency_map_drain_to_map
     map = MetricsProcessor::FrequencyMap.new
 
-    event1 = MetricsEvent.new(@feature1, @target, @variation1)
-    event2 = MetricsEvent.new(@feature2, @target, @variation2)
+    event1 = MetricsEvent.new(@feature1, @target, @variation1, Logger.new(STDOUT))
+    event2 = MetricsEvent.new(@feature2, @target, @variation2, Logger.new(STDOUT))
 
     map.increment event1
     map.increment event2
@@ -209,39 +208,70 @@ def test_comparable_metrics_event_equals_and_hash
     assert(event1 != event2)
   end
 
-  def test_metrics_event_eql_with_invalid_object
-    event = MetricsEvent.new(@feature1, @target, @variation1,Logger.new(STDOUT))
-    non_event = "Not a MetricsEvent"
-
-    refute_equal(event, non_event, "MetricsEvent should not be equal to a non-MetricsEvent object")
-  end
-
-  def test_flush_map_when_buffer_fills
-
+  def test_metrics_processor_prevents_invalid_metrics_event
     logger = Logger.new(STDOUT)
     callback = TestCallback.new
     connector = TestConnector.new
     config = Minitest::Mock.new
     config.expect :kind_of?, true, [Config.class]
     config.expect :metrics_service_acceptable_duration, 10000
 
-    (1..30).each { |_| config.expect :buffer_size, 2 }
-    (1..30).each { |_| config.expect :logger, logger }
+    (1..20).each { |_| config.expect :buffer_size, 10 }
+    (1..20).each { |_| config.expect :logger, logger }
 
     metrics_processor = MetricsProcessor.new
-    metrics_processor.init connector, config, callback
+    metrics_processor.init(connector, config, callback)
 
     callback.wait_until_ready
 
-    # several evaluations with a buffer size of 2
-    metrics_processor.register_evaluation @target, @feature1, @variation1
-    metrics_processor.register_evaluation @target, @feature1, @variation2
-    metrics_processor.register_evaluation @target, @feature2, @variation1
-    metrics_processor.register_evaluation @target, @feature2, @variation2
+    # Attempt to register invalid evaluations
+    metrics_processor.register_evaluation(@target,nil, @variation1)
+    metrics_processor.register_evaluation(@target,nil, nil)
+    metrics_processor.register_evaluation(nil, @feature1, nil)
+
 
-    assert connector.wait_for_metrics, "no metrics were posted"
+    # Register some valid evaluations
+    metrics_processor.register_evaluation(@target, @feature1, @variation1)
+    metrics_processor.register_evaluation(@target, @feature2, @variation2)
+    # Nil target, which is a valid input to variation methods
+    metrics_processor.register_evaluation(nil, @feature2, @variation2)
 
+    # Run iteration
+    metrics_processor.send(:run_one_iteration)
+
+    # Wait for metrics to be posted
+    connector.wait_for_metrics
+
+    # Check that only valid metrics are sent
+    captured_metrics = connector.get_captured_metrics
+    assert_equal 1, captured_metrics.size, "Only one metrics batch should be sent"
+
+    metrics_data = captured_metrics[0].metrics_data
+
+    # Since we have two valid events, two metrics_data should be present
+    assert_equal 2, metrics_data.size, "Invalid metrics should be ignored"
+
+    # Verify that only valid features are present in sent metrics
+    sent_features = metrics_data.map { |md| md.attributes.find { |kv| kv.key == "featureName" }.value }
+    assert_includes(sent_features, "feature-name")
+    assert_includes(sent_features, "feature-name2") # Assuming @feature2 has "feature-name2"
+
+    # Invalid event is not among the sent features
+    refute_includes(sent_features, nil, "Invalid MetricsEvent should not be included in metrics_data")
+
+    # Valid events were processed correctly
+    assert_equal 2, metrics_data.size, "There should be two metrics_data entries for valid events"
+
+    # Target data is still correctly sent
+    assert_equal 1, captured_metrics[0].target_data.size, "There should only be a single target"
   end
 
 
+  def test_metrics_event_eql_with_invalid_object
+    event = MetricsEvent.new(@feature1, @target, @variation1,Logger.new(STDOUT))
+    non_event = "Not a MetricsEvent"
+
+    refute_equal(event, non_event, "MetricsEvent should not be equal to a non-MetricsEvent object")
+  end
+
 end