From 8e0376373e3e914f10df190953b5fc107426a6b9 Mon Sep 17 00:00:00 2001 From: Bilal Al-Shahwany Date: Wed, 2 Jul 2025 09:49:38 -0700 Subject: [PATCH 1/2] Updated engine and sync classes --- .../cache/senders/impressions_formatter.rb | 6 ++- .../engine/common/impressions_manager.rb | 13 +++++-- .../impressions_repository_spec.rb | 38 ++++++++++--------- .../senders/impressions_formatter_spec.rb | 17 ++++++--- spec/cache/senders/impressions_sender_spec.rb | 8 ++-- spec/engine/common/impression_manager_spec.rb | 14 ++++--- 6 files changed, 58 insertions(+), 38 deletions(-) diff --git a/lib/splitclient-rb/cache/senders/impressions_formatter.rb b/lib/splitclient-rb/cache/senders/impressions_formatter.rb index b2f1de95..49e15f70 100644 --- a/lib/splitclient-rb/cache/senders/impressions_formatter.rb +++ b/lib/splitclient-rb/cache/senders/impressions_formatter.rb @@ -44,7 +44,8 @@ def current_impressions(feature_impressions) b: impression[:i][:b], r: impression[:i][:r], c: impression[:i][:c], - pt: impression[:i][:pt] + pt: impression[:i][:pt], + properties: impression[:i][:properties].to_json.to_s } end end @@ -73,7 +74,8 @@ def impression_hash(impression) "#{impression[:i][:b]}:" \ "#{impression[:i][:c]}:" \ "#{impression[:i][:t]}:" \ - "#{impression[:i][:pt]}" + "#{impression[:i][:pt]}" \ + "#{impression[:i][:properties]}" \ end end end diff --git a/lib/splitclient-rb/engine/common/impressions_manager.rb b/lib/splitclient-rb/engine/common/impressions_manager.rb index adb71c6f..cdc7990e 100644 --- a/lib/splitclient-rb/engine/common/impressions_manager.rb +++ b/lib/splitclient-rb/engine/common/impressions_manager.rb @@ -18,15 +18,19 @@ def initialize(config, @unique_keys_tracker = unique_keys_tracker end - def build_impression(matching_key, bucketing_key, split_name, treatment_data, impressions_disabled, params = {}) - impression_data = impression_data(matching_key, bucketing_key, split_name, treatment_data, params[:time]) + def build_impression(matching_key, bucketing_key, split_name, treatment_data, impressions_disabled, params = {}, properties = nil) + impression_data = impression_data(matching_key, bucketing_key, split_name, treatment_data, params[:time], properties) begin if @config.impressions_mode == :none || impressions_disabled @impression_counter.inc(split_name, impression_data[:m]) @unique_keys_tracker.track(split_name, matching_key) elsif @config.impressions_mode == :debug # In DEBUG mode we should calculate the pt only. + return impression(impression_data, params[:attributes]) unless properties.nil? + impression_data[:pt] = @impression_observer.test_and_set(impression_data) else # In OPTIMIZED mode we should track the total amount of evaluations and deduplicate the impressions. + return impression(impression_data, params[:attributes]) unless properties.nil? + impression_data[:pt] = @impression_observer.test_and_set(impression_data) @impression_counter.inc(split_name, impression_data[:m]) unless impression_data[:pt].nil? end @@ -79,7 +83,7 @@ def record_stats(stats) end # added param time for test - def impression_data(matching_key, bucketing_key, split_name, treatment, time = nil) + def impression_data(matching_key, bucketing_key, split_name, treatment, time = nil, properties = nil) { k: matching_key, b: bucketing_key, @@ -88,7 +92,8 @@ def impression_data(matching_key, bucketing_key, split_name, treatment, time = n r: applied_rule(treatment[:label]), c: treatment[:change_number], m: time || (Time.now.to_f * 1000.0).to_i, - pt: nil + pt: nil, + properties: properties } end diff --git a/spec/cache/repositories/impressions_repository_spec.rb b/spec/cache/repositories/impressions_repository_spec.rb index c422d34c..4b815e34 100644 --- a/spec/cache/repositories/impressions_repository_spec.rb +++ b/spec/cache/repositories/impressions_repository_spec.rb @@ -25,7 +25,8 @@ r: 'sample_rule', c: 1_533_177_602_748, m: 1_478_113_516_002, - pt: nil + pt: nil, + properties: nil }, attributes: {} }, @@ -39,7 +40,8 @@ r: 'sample_rule', c: 1_533_177_602_749, m: 1_478_113_516_002, - pt: nil + pt: nil, + properties: nil }, attributes: {} } @@ -55,8 +57,8 @@ it 'adds impressions' do params = { attributes: {}, time: 1_478_113_516_002 } impressions = [] - impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo, treatment1, false, params), :disabled => false } - impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :bar, treatment2, false, params), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo, treatment1, false, params, nil), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :bar, treatment2, false, params, nil), :disabled => false } impressions_manager.track(impressions) expect(repository.batch).to match_array(result) @@ -67,8 +69,8 @@ it 'adds impressions in bulk' do params = { attributes: {}, time: 1_478_113_516_002 } impressions = [] - impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo, treatment1, false, params), :disabled => false } - impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :bar, treatment2, false, params), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo, treatment1, false, params, nil), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :bar, treatment2, false, params, nil), :disabled => false } impressions_manager.track(impressions) expect(repository.batch).to match_array(result) @@ -80,7 +82,7 @@ config.labels_enabled = false params = { attributes: {}, time: 1_478_113_516_002 } impressions = [] - impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo, treatment1, false, params), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo, treatment1, false, params, nil), :disabled => false } impressions_manager.track(impressions) expect(repository.batch.first[:i][:r]).to be_nil @@ -89,8 +91,8 @@ it 'bulk size less than the actual queue' do params = { attributes: {}, time: 1_478_113_516_002 } impressions = [] - impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo, treatment1, false, params), :disabled => false } - impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo, treatment2, false, params), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo, treatment1, false, params, nil), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo, treatment2, false, params, nil), :disabled => false } impressions_manager.track(impressions) config.impressions_bulk_size = 1 @@ -142,8 +144,8 @@ treatment = { treatment: 'on', label: 'sample_rule', change_number: 1_533_177_602_748 } params = { attributes: {}, time: 1_478_113_516_002 } impressions = [] - impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo1, treatment, false, params), :disabled => false } - impressions << { :impression => impressions_manager.build_impression('matching_key2', nil, :foo1, treatment, false, params), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key1', nil, :foo1, treatment, false, params, nil), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key2', nil, :foo1, treatment, false, params, nil), :disabled => false } impressions_manager.track(impressions) expect(repository.batch.size).to eq(1) @@ -200,8 +202,8 @@ expect(config.impressions_adapter).to receive(:expire).once.with(anything, 3600) params = { attributes: {}, time: 1_478_113_516_002 } impressions = [] - impressions << { :impression => impressions_manager.build_impression('matching_key', nil, :foo1, treatment, false, params), :disabled => false } - impressions << { :impression => impressions_manager.build_impression('matching_key', nil, :foo1, treatment, false, params), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key', nil, :foo1, treatment, false, params, nil), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key', nil, :foo1, treatment, false, params, nil), :disabled => false } impressions_manager.track(impressions) end @@ -211,7 +213,7 @@ params = { attributes: {}, time: 1_478_113_516_002 } impressions = [] - impressions << { :impression => impressions_manager.build_impression('matching_key', nil, :foo1, treatment, false, params), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key', nil, :foo1, treatment, false, params, nil), :disabled => false } impressions_manager.track(impressions) expect(repository.batch).to eq([]) @@ -221,8 +223,8 @@ other_treatment = { treatment: 'on', label: 'sample_rule_2', change_number: 1_533_177_602_748 } params = { attributes: {}, time: 1_478_113_516_002 } impressions = [] - impressions << { :impression => impressions_manager.build_impression('matching_key', nil, :foo1, treatment, false, params), :disabled => false } - impressions << { :impression => impressions_manager.build_impression('matching_key', nil, :foo2, other_treatment, false, params), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key', nil, :foo1, treatment, false, params, nil), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key', nil, :foo2, other_treatment, false, params, nil), :disabled => false } impressions_manager.track(impressions) adapter.get_from_queue('SPLITIO.impressions', 0).map do |e| @@ -252,8 +254,8 @@ params = { attributes: {}, time: 1_478_113_516_002 } impressions = [] - impressions << { :impression => custom_impressions_manager.build_impression('matching_key', nil, :foo1, treatment, false, params), :disabled => false } - impressions << { :impression => custom_impressions_manager.build_impression('matching_key', nil, :foo2, other_treatment, false, params), :disabled => false } + impressions << { :impression => custom_impressions_manager.build_impression('matching_key', nil, :foo1, treatment, false, params, nil), :disabled => false } + impressions << { :impression => custom_impressions_manager.build_impression('matching_key', nil, :foo2, other_treatment, false, params, nil), :disabled => false } custom_impressions_manager.track(impressions) custom_adapter.get_from_queue('SPLITIO.impressions', 0).map do |e| diff --git a/spec/cache/senders/impressions_formatter_spec.rb b/spec/cache/senders/impressions_formatter_spec.rb index e55228ff..272c0455 100644 --- a/spec/cache/senders/impressions_formatter_spec.rb +++ b/spec/cache/senders/impressions_formatter_spec.rb @@ -58,7 +58,8 @@ b: 'foo1', r: 'custom_label1', c: 123_456, - pt: nil }] + pt: nil, + properties: "null" }] }, { f: :foo2, @@ -68,14 +69,15 @@ b: 'foo2', r: 'custom_label2', c: 123_499, - pt: nil }] + pt: nil, + properties: "null" }] }]) end it 'formats multiple impressions for one key' do params = { attributes: {}, time: 1_478_113_518_900 } impressions = [] - impressions << { :impression => impressions_manager.build_impression('matching_key3', nil, 'foo2', treatment3, false, params), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key3', nil, 'foo2', treatment3, false, params, {"prop": "val"}), :disabled => false } impressions_manager.track(impressions) expect(formatted_impressions.find { |i| i[:f] == :foo1 }[:i]).to match_array( @@ -87,7 +89,8 @@ b: 'foo1', r: 'custom_label1', c: 123_456, - pt: nil + pt: nil, + properties: 'null' } ] ) @@ -101,7 +104,8 @@ b: 'foo2', r: 'custom_label2', c: 123_499, - pt: nil + pt: nil, + properties: "null" }, { k: 'matching_key3', @@ -110,7 +114,8 @@ b: nil, r: nil, c: nil, - pt: nil + pt: nil, + properties: '{"prop":"val"}' } ] ) diff --git a/spec/cache/senders/impressions_sender_spec.rb b/spec/cache/senders/impressions_sender_spec.rb index 96da8aa9..39011264 100644 --- a/spec/cache/senders/impressions_sender_spec.rb +++ b/spec/cache/senders/impressions_sender_spec.rb @@ -46,7 +46,7 @@ params2 = { attributes: {}, time: 1_478_113_518_285 } impressions = [] impressions << { :impression => impressions_manager.build_impression('matching_key', 'foo1', 'foo1', treatment1, false, params), :disabled => false } - impressions << { :impression => impressions_manager.build_impression('matching_key2', 'foo2', 'foo2', treatment2, false, params2), :disabled => false } + impressions << { :impression => impressions_manager.build_impression('matching_key2', 'foo2', 'foo2', treatment2, false, params2, {:prop=>"val"}), :disabled => false } impressions_manager.track(impressions) end @@ -75,7 +75,8 @@ b: 'foo1', r: 'custom_label1', c: 123_456, - pt: nil + pt: nil, + properties: "null" } ] }, @@ -89,7 +90,8 @@ b: 'foo2', r: 'custom_label2', c: 123_499, - pt: nil + pt: nil, + properties: '{"prop":"val"}' } ] } diff --git a/spec/engine/common/impression_manager_spec.rb b/spec/engine/common/impression_manager_spec.rb index 21875236..ea93d3ac 100644 --- a/spec/engine/common/impression_manager_spec.rb +++ b/spec/engine/common/impression_manager_spec.rb @@ -59,7 +59,8 @@ r: 'default label', c: 1_478_113_516_002, m: 1_478_113_516_222, - pt: nil + pt: nil, + properties: {"prop":"val"} }, attributes: {} } @@ -71,7 +72,7 @@ 'split_name_test', treatment, false, - params) + params, {"prop":"val"}) expect(impression).to match(expected) result_count = impression_counter.pop_all @@ -97,7 +98,8 @@ r: 'default label', c: 1_478_113_516_002, m: 1_478_113_516_222, - pt: nil + pt: nil, + properties: nil }, attributes: {} } @@ -155,7 +157,8 @@ r: 'default label', c: 1_478_113_516_002, m: 1_478_113_516_222, - pt: nil + pt: nil, + properties: nil }, attributes: {} } @@ -297,7 +300,8 @@ r: 'default label', c: 1_478_113_516_002, m: 1_478_113_516_222, - pt: nil + pt: nil, + properties: nil }, attributes: {} } From f8a9d9357af82a6ade2f96b4b8bcf999a6c3f151 Mon Sep 17 00:00:00 2001 From: Bilal Al-Shahwany Date: Wed, 2 Jul 2025 11:56:59 -0700 Subject: [PATCH 2/2] Removed properties if nil in sender --- .../cache/senders/impressions_formatter.rb | 33 +++++++++++++------ .../engine/common/impressions_manager.rb | 6 ++-- .../senders/impressions_formatter_spec.rb | 12 +++---- spec/cache/senders/impressions_sender_spec.rb | 3 +- 4 files changed, 32 insertions(+), 22 deletions(-) diff --git a/lib/splitclient-rb/cache/senders/impressions_formatter.rb b/lib/splitclient-rb/cache/senders/impressions_formatter.rb index 49e15f70..eed8570e 100644 --- a/lib/splitclient-rb/cache/senders/impressions_formatter.rb +++ b/lib/splitclient-rb/cache/senders/impressions_formatter.rb @@ -36,17 +36,30 @@ def feature_impressions(filtered_impressions, feature) end def current_impressions(feature_impressions) + formatted = [] feature_impressions.map do |impression| - { - k: impression[:i][:k], - t: impression[:i][:t], - m: impression[:i][:m], - b: impression[:i][:b], - r: impression[:i][:r], - c: impression[:i][:c], - pt: impression[:i][:pt], - properties: impression[:i][:properties].to_json.to_s - } + if impression[:i][:properties].nil? + impression = { + k: impression[:i][:k], + t: impression[:i][:t], + m: impression[:i][:m], + b: impression[:i][:b], + r: impression[:i][:r], + c: impression[:i][:c], + pt: impression[:i][:pt] + } + else + impression = { + k: impression[:i][:k], + t: impression[:i][:t], + m: impression[:i][:m], + b: impression[:i][:b], + r: impression[:i][:r], + c: impression[:i][:c], + pt: impression[:i][:pt], + properties: impression[:i][:properties].to_json.to_s + } + end end end diff --git a/lib/splitclient-rb/engine/common/impressions_manager.rb b/lib/splitclient-rb/engine/common/impressions_manager.rb index cdc7990e..46efc3b2 100644 --- a/lib/splitclient-rb/engine/common/impressions_manager.rb +++ b/lib/splitclient-rb/engine/common/impressions_manager.rb @@ -18,7 +18,8 @@ def initialize(config, @unique_keys_tracker = unique_keys_tracker end - def build_impression(matching_key, bucketing_key, split_name, treatment_data, impressions_disabled, params = {}, properties = nil) + def build_impression(matching_key, bucketing_key, split_name, treatment_data, impressions_disabled, params = {}, + properties = nil) impression_data = impression_data(matching_key, bucketing_key, split_name, treatment_data, params[:time], properties) begin if @config.impressions_mode == :none || impressions_disabled @@ -83,7 +84,8 @@ def record_stats(stats) end # added param time for test - def impression_data(matching_key, bucketing_key, split_name, treatment, time = nil, properties = nil) + def impression_data(matching_key, bucketing_key, split_name, treatment, time = nil, + properties = nil) { k: matching_key, b: bucketing_key, diff --git a/spec/cache/senders/impressions_formatter_spec.rb b/spec/cache/senders/impressions_formatter_spec.rb index 272c0455..965665be 100644 --- a/spec/cache/senders/impressions_formatter_spec.rb +++ b/spec/cache/senders/impressions_formatter_spec.rb @@ -58,8 +58,7 @@ b: 'foo1', r: 'custom_label1', c: 123_456, - pt: nil, - properties: "null" }] + pt: nil}] }, { f: :foo2, @@ -69,8 +68,7 @@ b: 'foo2', r: 'custom_label2', c: 123_499, - pt: nil, - properties: "null" }] + pt: nil}] }]) end @@ -89,8 +87,7 @@ b: 'foo1', r: 'custom_label1', c: 123_456, - pt: nil, - properties: 'null' + pt: nil } ] ) @@ -104,8 +101,7 @@ b: 'foo2', r: 'custom_label2', c: 123_499, - pt: nil, - properties: "null" + pt: nil }, { k: 'matching_key3', diff --git a/spec/cache/senders/impressions_sender_spec.rb b/spec/cache/senders/impressions_sender_spec.rb index 39011264..219b789f 100644 --- a/spec/cache/senders/impressions_sender_spec.rb +++ b/spec/cache/senders/impressions_sender_spec.rb @@ -75,8 +75,7 @@ b: 'foo1', r: 'custom_label1', c: 123_456, - pt: nil, - properties: "null" + pt: nil } ] },