Merge branch 'jj-add-feature-cache-interface' into 'master'

Add ApplicationExperiment and caching interface for experiments

See merge request gitlab-org/gitlab!49669

Gemfile

@@ -478,7 +478,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.2'
+gem 'gitlab-experiment', '~> 0.4.4'
 
 # 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.2)
+    gitlab-experiment (0.4.4)
       activesupport (>= 3.0)
       scientist (~> 1.5, >= 1.5.0)
     gitlab-fog-azure-rm (1.0.0)
@@ -1354,7 +1354,7 @@ DEPENDENCIES
   gitaly (~> 13.7.0.pre.rc1)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
-  gitlab-experiment (~> 0.4.2)
+  gitlab-experiment (~> 0.4.4)
   gitlab-fog-azure-rm (~> 1.0)
   gitlab-labkit (= 0.13.3)
   gitlab-license (~> 1.0)

app/experiments/application_experiment.rb

+# frozen_string_literal: true
+
+class ApplicationExperiment < Gitlab::Experiment
+  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
+  end
+
+  def track(action, **event_args)
+    return if excluded? # no events for opted out actors or excluded subjects
+
+    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
+      )
+    ))
+  end
+
+  private
+
+  def resolve_variant_name
+    variant_names.first if Feature.enabled?(name, self, type: :experiment)
+  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
+    # 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.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 unless Feature.enabled?(:caching_experiments)
+      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/feature_flags/development/caching_experiments.yml

+---
+name: caching_experiments
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49669
+rollout_issue_url: 
+milestone: '13.7'
+type: development
+group: group::adoption
+default_enabled: false

config/initializers/gitlab_experiment.rb

 # frozen_string_literal: true
 
 Gitlab::Experiment.configure do |config|
-  # Logic this project uses to resolve a variant for a given experiment.
-  #
-  # This can return an instance of any object that responds to `name`, or can
-  # return a variant name as a symbol or string.
-  #
-  # This block will be executed within the scope of the experiment instance,
-  # so can easily access experiment methods, like getting the name or context.
-  config.variant_resolver = lambda do |requested_variant|
-    # Return the variant if one was requested in code:
-    break requested_variant if requested_variant.present?
-
-    # Use Feature interface to determine the variant by passing the experiment,
-    # which responds to `flipper_id` and `session_id` to accommodate adapters.
-    variant_names.first if Feature.enabled?(name, self, type: :experiment)
-  end
-
-  # Tracking behavior can be implemented to link an event to an experiment.
-  #
-  # Similar to the variant_resolver, this is called within the scope of the
-  # experiment instance and so can access any methods on the experiment,
-  # such as name and signature.
-  config.tracking_behavior = lambda do |event, args|
-    Gitlab::Tracking.event(name, event.to_s, **args.merge(
-      context: (args[:context] || []) << SnowplowTracker::SelfDescribingJson.new(
-        'iglu:com.gitlab/gitlab_experiment/jsonschema/0-3-0', signature
-      )
-    ))
-  end
-
-  # Called at the end of every experiment run, with the result.
-  #
-  # You may want to track that you've assigned a variant to a given context,
-  # or push the experiment into the client or publish results elsewhere, like
-  # into redis or postgres. Also called within the scope of the experiment
-  # instance.
-  config.publishing_behavior = lambda do |result|
-    # Track the event using our own configured tracking logic.
-    track(:assignment)
-
-    # Push the experiment knowledge into the front end. The signature contains
-    # the context key, and the variant that has been determined.
-    Gon.push({ experiment: { name => signature } }, true)
-  end
+  config.base_class = 'ApplicationExperiment'
+  config.cache = ApplicationExperiment::Cache.new
 end

lib/quality/test_level.rb

@@ -23,6 +23,7 @@ module Quality
         dependencies
         elastic
         elastic_integration
+        experiments
         factories
         finders
         frontend

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
+
+  context "when the :caching_experiments feature is disabled" do
+    before do
+      stub_feature_flags(caching_experiments: false)
+    end
+
+    it "doesn't write to the cache" do
+      subject.write(key_field, 'value')
+
+      expect(subject.read(key_field)).to be_nil
+    end
+  end
+end

spec/experiments/application_experiment_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ApplicationExperiment do
+  subject { described_class.new(:stub) }
+
+  describe "publishing results" do
+    it "tracks the assignment" do
+      expect(subject).to receive(:track).with(:assignment)
+
+      subject.publish(nil)
+    end
+
+    it "pushes the experiment knowledge into the client using Gon.global" do
+      expect(Gon.global).to receive(:push).with(
+        {
+          experiment: {
+            'stub' => { # string key because it can be namespaced
+              experiment: 'stub',
+              key: 'e8f65fd8d973f9985dc7ea3cf1614ae1',
+              variant: 'control'
+            }
+          }
+        },
+        true
+      )
+
+      subject.publish(nil)
+    end
+  end
+
+  describe "tracking events", :snowplow do
+    it "doesn't track if excluded" do
+      subject.exclude { true }
+
+      subject.track(:action)
+
+      expect_no_snowplow_event
+    end
+
+    it "tracks the event with the expected arguments and merged contexts" do
+      subject.track(:action, property: '_property_', context: [
+        SnowplowTracker::SelfDescribingJson.new('iglu:com.gitlab/fake/jsonschema/0-0-0', { data: '_data_' })
+      ])
+
+      expect_snowplow_event(
+        category: 'stub',
+        action: 'action',
+        property: '_property_',
+        context: [
+          {
+            schema: 'iglu:com.gitlab/fake/jsonschema/0-0-0',
+            data: { data: '_data_' }
+          },
+          {
+            schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/0-3-0',
+            data: { experiment: 'stub', key: 'e8f65fd8d973f9985dc7ea3cf1614ae1', variant: 'control' }
+          }
+        ]
+      )
+    end
+  end
+
+  describe "variant resolution" do
+    it "returns nil when not rolled out" do
+      stub_feature_flags(stub: false)
+
+      expect(subject.variant.name).to eq('control')
+    end
+
+    context "when rolled out to 100%" do
+      it "returns the first variant name" do
+        subject.try(:variant1) {}
+        subject.try(:variant2) {}
+
+        expect(subject.variant.name).to eq('variant1')
+      end
+    end
+  end
+
+  context "when caching" do
+    let(:cache) { ApplicationExperiment::Cache.new }
+
+    before do
+      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
+      expect(subject.variant.name).to eq('candidate') # we should be in the experiment
+
+      subject.run
+
+      expect(cache.read(subject.cache_key)).to eq('candidate')
+    end
+
+    it "doesn't cache a variant if we don't explicitly provide one" do
+      # by not caching "empty" variants, we effectively create a mostly
+      # optimal combination of caching and rollout flexibility. If we cached
+      # every control variant assigned, we'd inflate the cache size and
+      # wouldn't be able to roll out to subjects that we'd already assigned to
+      # the control.
+      stub_feature_flags(stub: false) # simulate being not rolled out
+
+      expect(subject.variant.name).to eq('control') # if we ask, it should be control
+
+      subject.run
+
+      expect(cache.read(subject.cache_key)).to be_nil
+    end
+
+    it "caches a control variant if we assign it specifically" do
+      # by specifically assigning the control variant here, we're guaranteeing
+      # that this context will always get the control variant unless we delete
+      # the field from the cache (or clear the entire experiment cache) -- or
+      # write code that would specify a different variant.
+      subject.run(:control)
+
+      expect(cache.read(subject.cache_key)).to eq('control')
+    end
+  end
+end

spec/lib/quality/test_level_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe Quality::TestLevel do
     context 'when level is unit' do
       it 'returns a pattern' do
         expect(subject.pattern(:unit))
-          .to eq("spec/{bin,channels,config,db,dependencies,elastic,elastic_integration,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,models,policies,presenters,rack_servers,replicators,routing,rubocop,serializers,services,sidekiq,support_specs,tasks,uploaders,validators,views,workers,tooling}{,/**/}*_spec.rb")
+          .to eq("spec/{bin,channels,config,db,dependencies,elastic,elastic_integration,experiments,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,models,policies,presenters,rack_servers,replicators,routing,rubocop,serializers,services,sidekiq,support_specs,tasks,uploaders,validators,views,workers,tooling}{,/**/}*_spec.rb")
       end
     end
 
@@ -103,7 +103,7 @@ RSpec.describe Quality::TestLevel do
     context 'when level is unit' do
       it 'returns a regexp' do
         expect(subject.regexp(:unit))
-          .to eq(%r{spec/(bin|channels|config|db|dependencies|elastic|elastic_integration|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|models|policies|presenters|rack_servers|replicators|routing|rubocop|serializers|services|sidekiq|support_specs|tasks|uploaders|validators|views|workers|tooling)})
+          .to eq(%r{spec/(bin|channels|config|db|dependencies|elastic|elastic_integration|experiments|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|models|policies|presenters|rack_servers|replicators|routing|rubocop|serializers|services|sidekiq|support_specs|tasks|uploaders|validators|views|workers|tooling)})
       end
     end
 

spec/support/gitlab_experiment.rb

+# frozen_string_literal: true
+
+# Disable all caching for experiments in tests.
+Gitlab::Experiment::Configuration.cache = nil