diff --git a/lib/splitclient-rb.rb b/lib/splitclient-rb.rb index 16478f15..be1d24de 100644 --- a/lib/splitclient-rb.rb +++ b/lib/splitclient-rb.rb @@ -13,6 +13,7 @@ require 'splitclient-rb/cache/fetchers/segment_fetcher' require 'splitclient-rb/cache/fetchers/split_fetcher' require 'splitclient-rb/cache/hashers/impression_hasher' +require 'splitclient-rb/cache/observers/impression_observer' require 'splitclient-rb/cache/repositories/repository' require 'splitclient-rb/cache/repositories/segments_repository' require 'splitclient-rb/cache/repositories/splits_repository' diff --git a/lib/splitclient-rb/cache/observers/impression_observer.rb b/lib/splitclient-rb/cache/observers/impression_observer.rb new file mode 100644 index 00000000..663d54d0 --- /dev/null +++ b/lib/splitclient-rb/cache/observers/impression_observer.rb @@ -0,0 +1,22 @@ +module SplitIoClient + module Observers + class ImpressionObserver + LAST_SEEN_CACHE_SIZE = 500000 + + def initialize + @cache = LruRedux::TTL::ThreadSafeCache.new(LAST_SEEN_CACHE_SIZE) + @impression_hasher = Hashers::ImpressionHasher.new + end + + def test_and_set(impression) + return if impression.nil? + + hash = @impression_hasher.process(impression) + previous = @cache[hash] + @cache[hash] = impression[:m] + + previous.nil? ? nil : [previous, impression[:m]].min + end + end + end +end diff --git a/lib/splitclient-rb/cache/senders/impressions_formatter.rb b/lib/splitclient-rb/cache/senders/impressions_formatter.rb index b426836c..41871cab 100644 --- a/lib/splitclient-rb/cache/senders/impressions_formatter.rb +++ b/lib/splitclient-rb/cache/senders/impressions_formatter.rb @@ -45,7 +45,8 @@ def current_impressions(feature_impressions) time: impression[:i][:m], bucketingKey: impression[:i][:b], label: impression[:i][:r], - changeNumber: impression[:i][:c] + changeNumber: impression[:i][:c], + previousTime: impression[:i][:pt] } end end @@ -73,7 +74,8 @@ def impression_hash(impression) "#{impression[:i][:k]}:" \ "#{impression[:i][:b]}:" \ "#{impression[:i][:c]}:" \ - "#{impression[:i][:t]}" + "#{impression[:i][:t]}:" \ + "#{impression[:i][:pt]}" end end end diff --git a/lib/splitclient-rb/engine/common/impressions_manager.rb b/lib/splitclient-rb/engine/common/impressions_manager.rb index 482eb77f..4547ce6c 100644 --- a/lib/splitclient-rb/engine/common/impressions_manager.rb +++ b/lib/splitclient-rb/engine/common/impressions_manager.rb @@ -8,13 +8,14 @@ def initialize(config, impressions_repository) @config = config @impressions_repository = impressions_repository @impression_router = SplitIoClient::ImpressionRouter.new(@config) - # @impression_observer = impression_observer + @impression_observer = SplitIoClient::Observers::ImpressionObserver.new end # added param time for test def build_impression(matching_key, bucketing_key, split_name, treatment, params = {}) impression_data = impression_data(matching_key, bucketing_key, split_name, treatment, params[:time]) - # impression_data[:pt] = @impression_observer.test_and_set(impression) + + impression_data[:pt] = @impression_observer.test_and_set(impression_data) if should_add_pt { m: metadata, i: impression_data, attributes: params[:attributes] } rescue StandardError => error @@ -55,6 +56,10 @@ def metadata def applied_rule(label) @config.labels_enabled ? label : nil end + + def should_add_pt + @config.impressions_adapter.class.to_s != 'SplitIoClient::Cache::Adapters::RedisAdapter' + end end end end diff --git a/spec/cache/observers/impression_observer_spec.rb b/spec/cache/observers/impression_observer_spec.rb new file mode 100644 index 00000000..ca1e4205 --- /dev/null +++ b/spec/cache/observers/impression_observer_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SplitIoClient::Observers::ImpressionObserver do + subject { SplitIoClient::Observers::ImpressionObserver } + let(:log) { StringIO.new } + let(:config) { SplitIoClient::SplitConfig.new(logger: Logger.new(log)) } + let(:ip) { config.machine_ip } + let(:machine_name) { config.machine_name } + let(:version) { "#{config.language}-#{config.version}" } + let(:impression_data1) do + { + k: 'matching_key', + b: 'bucketing_key', + f: 'split_name', + t: 'treatment', + r: 'label', + c: 1_533_177_602_748, + m: 1_478_113_516_002, + pt: nil + } + end + + let(:impression_data2) do + { + k: 'matching_key_2', + b: 'bucketing_key', + f: 'split_name', + t: 'treatment', + r: 'label', + c: 1_533_177_602_748, + m: 1_478_113_516_022, + pt: nil + } + end + + context 'test_and_set' do + before do + @impression_observer = subject.new + end + + it 'first time should be nil and after that always return previous time' do + result1 = @impression_observer.test_and_set(impression_data1) + expect(result1).to be_nil + + # should return previous time + impression_data1[:m] = 1_478_113_516_500 + result1 = @impression_observer.test_and_set(impression_data1) + expect(result1).to eq(1_478_113_516_002) + + # should return new impression.time + result1 = @impression_observer.test_and_set(impression_data1) + expect(result1).to eq(1_478_113_516_500) + + # when impression.time < impression.pt should return the min. + impression_data1[:m] = 1_478_113_516_001 + result1 = @impression_observer.test_and_set(impression_data1) + expect(result1).to eq(1_478_113_516_001) + + # should return nil because is another impression + result2 = @impression_observer.test_and_set(impression_data2) + expect(result2).to be_nil + + # should return previous time + result2 = @impression_observer.test_and_set(impression_data2) + expect(result2).to eq(1_478_113_516_022) + end + + it 'return nil because impression is nil' do + result = @impression_observer.test_and_set(nil) + expect(result).to be_nil + end + end +end diff --git a/spec/cache/senders/impressions_formatter_spec.rb b/spec/cache/senders/impressions_formatter_spec.rb index 50f6ad8f..329d4c0c 100644 --- a/spec/cache/senders/impressions_formatter_spec.rb +++ b/spec/cache/senders/impressions_formatter_spec.rb @@ -18,10 +18,11 @@ Redis.new.flushall params = { attributes: {}, time: 1_478_113_516_002 } params2 = { attributes: {}, time: 1_478_113_518_285 } - impressions = [] - impressions << impressions_manager.build_impression('matching_key', 'foo1', 'foo1', treatment1, params) - impressions << impressions_manager.build_impression('matching_key2', 'foo2', 'foo2', treatment2, params2) - impressions_manager.track(impressions) + + @impressions = [] + @impressions << impressions_manager.build_impression('matching_key', 'foo1', 'foo1', treatment1, params) + @impressions << impressions_manager.build_impression('matching_key2', 'foo2', 'foo2', treatment2, params2) + impressions_manager.track(@impressions) end it 'formats impressions to be sent' do @@ -32,7 +33,8 @@ treatment: 'on', time: 1_478_113_516_002, bucketingKey: 'foo1', label: 'custom_label1', - changeNumber: 123_456 }], + changeNumber: 123_456, + previousTime: nil }], ip: ip }, { @@ -42,7 +44,8 @@ time: 1_478_113_518_285, bucketingKey: 'foo2', label: 'custom_label2', - changeNumber: 123_499 }], + changeNumber: 123_499, + previousTime: nil }], ip: ip }]) end @@ -61,7 +64,8 @@ time: 1_478_113_516_002, bucketingKey: 'foo1', label: 'custom_label1', - changeNumber: 123_456 + changeNumber: 123_456, + previousTime: nil } ] ) @@ -74,7 +78,8 @@ time: 1_478_113_518_285, bucketingKey: 'foo2', label: 'custom_label2', - changeNumber: 123_499 + changeNumber: 123_499, + previousTime: nil }, { keyName: 'matching_key3', @@ -82,31 +87,22 @@ time: 1_478_113_518_900, bucketingKey: nil, label: nil, - changeNumber: nil + changeNumber: nil, + previousTime: nil } ] ) end it 'filters out impressions with the same key/treatment' do - params = { attributes: {}, time: 1_478_113_516_902 } - params2 = { attributes: {}, time: 1_478_113_518_285 } - impressions = [] - impressions << impressions_manager.build_impression('matching_key', 'foo1', 'foo1', treatment1, params) - impressions << impressions_manager.build_impression('matching_key2', 'foo2', 'foo2', treatment2, params2) - impressions_manager.track(impressions) + impressions_manager.track(@impressions) expect(formatted_impressions.find { |i| i[:testName] == :foo1 }[:keyImpressions].size).to eq(1) expect(formatted_impressions.find { |i| i[:testName] == :foo2 }[:keyImpressions].size).to eq(1) end it 'filters out impressions with the same key/treatment legacy' do - params = { attributes: {}, time: 1_478_113_516_902 } - params2 = { attributes: {}, time: 1_478_113_518_285 } - impressions = [] - impressions << impressions_manager.build_impression('matching_key', 'foo1', 'foo1', treatment1, params) - impressions << impressions_manager.build_impression('matching_key2', 'foo2', 'foo2', treatment2, params2) - impressions_manager.track(impressions) + impressions_manager.track(@impressions) expect(formatted_impressions.find { |i| i[:testName] == :foo1 }[:keyImpressions].size).to eq(1) expect(formatted_impressions.find { |i| i[:testName] == :foo2 }[:keyImpressions].size).to eq(1) diff --git a/spec/cache/senders/impressions_sender_spec.rb b/spec/cache/senders/impressions_sender_spec.rb index f85dacf7..dbb8dbcf 100644 --- a/spec/cache/senders/impressions_sender_spec.rb +++ b/spec/cache/senders/impressions_sender_spec.rb @@ -50,7 +50,8 @@ time: 1_478_113_516_002, bucketingKey: 'foo1', label: 'custom_label1', - changeNumber: 123_456 + changeNumber: 123_456, + previousTime: nil } ], ip: config.machine_ip @@ -64,7 +65,8 @@ time: 1_478_113_518_285, bucketingKey: 'foo2', label: 'custom_label2', - changeNumber: 123_499 + changeNumber: 123_499, + previousTime: nil } ], ip: config.machine_ip