Merge branch 'jj-update-gitlab-experiment' into 'master'

Removes the cache and adds it to the gem (v0.4.12) [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!53672

Gemfile

@@ -479,7 +479,7 @@ gem 'flipper', '~> 0.17.1'
 gem 'flipper-active_record', '~> 0.17.1'
 gem 'flipper-active_support_cache_store', '~> 0.17.1'
 gem 'unleash', '~> 0.1.5'
-gem 'gitlab-experiment', '~> 0.4.9'
+gem 'gitlab-experiment', '~> 0.4.12'
 
 # Structured logging
 gem 'lograge', '~> 0.5'

Gemfile.lock

@@ -425,7 +425,7 @@ GEM
     github-markup (1.7.0)
     gitlab-chronic (0.10.5)
       numerizer (~> 0.2)
-    gitlab-experiment (0.4.9)
+    gitlab-experiment (0.4.12)
       activesupport (>= 3.0)
       scientist (~> 1.5, >= 1.5.0)
     gitlab-fog-azure-rm (1.0.0)
@@ -1371,7 +1371,7 @@ DEPENDENCIES
   gitaly (~> 13.9.0.pre.rc1)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
-  gitlab-experiment (~> 0.4.9)
+  gitlab-experiment (~> 0.4.12)
   gitlab-fog-azure-rm (~> 1.0)
   gitlab-labkit (= 0.14.0)
   gitlab-license (~> 1.3)

app/experiments/application_experiment.rb

@@ -3,19 +3,21 @@
 class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/NamespacedClass
   def enabled?
     return false if Feature::Definition.get(feature_flag_name).nil? # there has to be a feature flag yaml file
-    return false unless Gitlab.dev_env_or_com? # we're in an environment that allows experiments
+    return false unless Gitlab.dev_env_or_com? # we have to be in an environment that allows experiments
 
+    # the feature flag has to be rolled out
     Feature.get(feature_flag_name).state != :off # rubocop:disable Gitlab/AvoidFeatureGet
   end
 
   def publish(_result)
     track(:assignment) # track that we've assigned a variant for this context
-    Gon.global.push({ experiment: { name => signature } }, true) # push to client
+    Gon.global.push({ experiment: { name => signature } }, true) # push the experiment data to the client
   end
 
   def track(action, **event_args)
-    return unless should_track? # no events for opted out actors or excluded subjects
+    return unless should_track? # don't track events for excluded contexts
 
+    # track the event, and mix in the experiment signature data
     Gitlab::Tracking.event(name, action.to_s, **event_args.merge(
       context: (event_args[:context] || []) << SnowplowTracker::SelfDescribingJson.new(
         'iglu:com.gitlab/gitlab_experiment/jsonschema/0-3-0', signature
@@ -59,62 +61,4 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
 
     nil # Returning nil vs. :control is important for not caching and rollouts.
   end
-
-  # Cache is an implementation on top of Gitlab::Redis::SharedState that also
-  # adheres to the ActiveSupport::Cache::Store interface and uses the redis
-  # hash data type.
-  #
-  # Since Gitlab::Experiment can use any type of caching layer, utilizing the
-  # long lived shared state interface here gives us an efficient way to store
-  # context keys and the variant they've been assigned -- while also giving us
-  # a simple way to clean up an experiments data upon resolution.
-  #
-  # The data structure:
-  #   key: experiment.name
-  #   fields: context key => variant name
-  #
-  # The keys are expected to be `experiment_name:context_key`, which is the
-  # default cache key strategy. So running `cache.fetch("foo:bar", "value")`
-  # would create/update a hash with the key of "foo", with a field named
-  # "bar" that has "value" assigned to it.
-  class Cache < ActiveSupport::Cache::Store # rubocop:disable Gitlab/NamespacedClass
-    # Clears the entire cache for a given experiment. Be careful with this
-    # since it would reset all resolved variants for the entire experiment.
-    def clear(key:)
-      key = hkey(key)[0] # extract only the first part of the key
-      pool do |redis|
-        case redis.type(key)
-        when 'hash', 'none' then redis.del(key)
-        else raise ArgumentError, 'invalid call to clear a non-hash cache key'
-        end
-      end
-    end
-
-    private
-
-    def pool
-      raise ArgumentError, 'missing block' unless block_given?
-
-      Gitlab::Redis::SharedState.with { |redis| yield redis }
-    end
-
-    def hkey(key)
-      key.to_s.split(':') # this assumes the default strategy in gitlab-experiment
-    end
-
-    def read_entry(key, **options)
-      value = pool { |redis| redis.hget(*hkey(key)) }
-      value.nil? ? nil : ActiveSupport::Cache::Entry.new(value)
-    end
-
-    def write_entry(key, entry, **options)
-      return false if entry.value.blank? # don't cache any empty values
-
-      pool { |redis| redis.hset(*hkey(key), entry.value) }
-    end
-
-    def delete_entry(key, **options)
-      pool { |redis| redis.hdel(*hkey(key)) }
-    end
-  end
 end

config/initializers/gitlab_experiment.rb

@@ -2,5 +2,7 @@
 
 Gitlab::Experiment.configure do |config|
   config.base_class = 'ApplicationExperiment'
-  config.cache = ApplicationExperiment::Cache.new
+  config.cache = Gitlab::Experiment::Cache::RedisHashStore.new(
+    pool: ->(&block) { Gitlab::Redis::SharedState.with { |redis| block.call(redis) } }
+  )
 end

spec/experiments/application_experiment/cache_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe ApplicationExperiment::Cache do
-  let(:key_name) { 'experiment_name' }
-  let(:field_name) { 'abc123' }
-  let(:key_field) { [key_name, field_name].join(':') }
-  let(:shared_state) { Gitlab::Redis::SharedState }
-
-  around do |example|
-    shared_state.with { |r| r.del(key_name) }
-    example.run
-    shared_state.with { |r| r.del(key_name) }
-  end
-
-  it "allows reading, writing and deleting", :aggregate_failures do
-    # we test them all together because they are largely interdependent
-
-    expect(subject.read(key_field)).to be_nil
-    expect(shared_state.with { |r| r.hget(key_name, field_name) }).to be_nil
-
-    subject.write(key_field, 'value')
-
-    expect(subject.read(key_field)).to eq('value')
-    expect(shared_state.with { |r| r.hget(key_name, field_name) }).to eq('value')
-
-    subject.delete(key_field)
-
-    expect(subject.read(key_field)).to be_nil
-    expect(shared_state.with { |r| r.hget(key_name, field_name) }).to be_nil
-  end
-
-  it "handles the fetch with a block behavior (which is read/write)" do
-    expect(subject.fetch(key_field) { 'value1' }).to eq('value1') # rubocop:disable Style/RedundantFetchBlock
-    expect(subject.fetch(key_field) { 'value2' }).to eq('value1') # rubocop:disable Style/RedundantFetchBlock
-  end
-
-  it "can clear a whole experiment cache key" do
-    subject.write(key_field, 'value')
-    subject.clear(key: key_field)
-
-    expect(shared_state.with { |r| r.get(key_name) }).to be_nil
-  end
-
-  it "doesn't allow clearing a key from the cache that's not a hash (definitely not an experiment)" do
-    shared_state.with { |r| r.set(key_name, 'value') }
-
-    expect { subject.clear(key: key_name) }.to raise_error(
-      ArgumentError,
-      'invalid call to clear a non-hash cache key'
-    )
-  end
-end

spec/experiments/application_experiment_spec.rb

@@ -191,15 +191,15 @@ RSpec.describe ApplicationExperiment, :experiment do
   end
 
   context "when caching" do
-    let(:cache) { ApplicationExperiment::Cache.new }
+    let(:cache) { Gitlab::Experiment::Configuration.cache }
 
     before do
+      allow(Gitlab::Experiment::Configuration).to receive(:cache).and_call_original
+
       cache.clear(key: subject.name)
 
       subject.use { } # setup the control
       subject.try { } # setup the candidate
-
-      allow(Gitlab::Experiment::Configuration).to receive(:cache).and_return(cache)
     end
 
     it "caches the variant determined by the variant resolver" do
@@ -207,7 +207,7 @@ RSpec.describe ApplicationExperiment, :experiment do
 
       subject.run
 
-      expect(cache.read(subject.cache_key)).to eq('candidate')
+      expect(subject.cache.read).to eq('candidate')
     end
 
     it "doesn't cache a variant if we don't explicitly provide one" do
@@ -222,7 +222,7 @@ RSpec.describe ApplicationExperiment, :experiment do
 
       subject.run
 
-      expect(cache.read(subject.cache_key)).to be_nil
+      expect(subject.cache.read).to be_nil
     end
 
     it "caches a control variant if we assign it specifically" do
@@ -232,7 +232,26 @@ RSpec.describe ApplicationExperiment, :experiment do
       # write code that would specify a different variant.
       subject.run(:control)
 
-      expect(cache.read(subject.cache_key)).to eq('control')
+      expect(subject.cache.read).to eq('control')
+    end
+
+    context "arbitrary attributes" do
+      before do
+        subject.cache.store.clear(key: subject.name + '_attrs')
+      end
+
+      it "sets and gets attributes about an experiment" do
+        subject.cache.attr_set(:foo, :bar)
+
+        expect(subject.cache.attr_get(:foo)).to eq('bar')
+      end
+
+      it "increments a value for an experiment" do
+        expect(subject.cache.attr_get(:foo)).to be_nil
+
+        expect(subject.cache.attr_inc(:foo)).to eq(1)
+        expect(subject.cache.attr_inc(:foo)).to eq(2)
+      end
     end
   end
 end

spec/support/gitlab_experiment.rb

@@ -12,5 +12,9 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
   end
 end
 
-# Disable all caching for experiments in tests.
-Gitlab::Experiment::Configuration.cache = nil
+RSpec.configure do |config|
+  # Disable all caching for experiments in tests.
+  config.before do
+    allow(Gitlab::Experiment::Configuration).to receive(:cache).and_return(nil)
+  end
+end