diff --git a/Gemfile b/Gemfile
index 886434a89faf446e23983068d0bf33697ef40f50..c2c176e12b2f27ad39ebd661f552efae9b11e75c 100644
--- a/Gemfile
+++ b/Gemfile
@@ -496,7 +496,7 @@ gem 'flipper', '~> 0.21.0'
gem 'flipper-active_record', '~> 0.21.0'
gem 'flipper-active_support_cache_store', '~> 0.21.0'
gem 'unleash', '~> 0.1.5'
-gem 'gitlab-experiment', '~> 0.5.4'
+gem 'gitlab-experiment', '~> 0.6.1'
# Structured logging
gem 'lograge', '~> 0.5'
diff --git a/Gemfile.lock b/Gemfile.lock
index ac113252f85b41404657ec3ce3384695b4a4c50b..cb9e2342ad388fce438bb31d045bf973115a4b1f 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -464,7 +464,7 @@ GEM
numerizer (~> 0.2)
gitlab-dangerfiles (2.1.2)
danger-gitlab
- gitlab-experiment (0.5.4)
+ gitlab-experiment (0.6.1)
activesupport (>= 3.0)
request_store (>= 1.0)
scientist (~> 1.6, >= 1.6.0)
@@ -1487,7 +1487,7 @@ DEPENDENCIES
github-markup (~> 1.7.0)
gitlab-chronic (~> 0.10.5)
gitlab-dangerfiles (~> 2.1.2)
- gitlab-experiment (~> 0.5.4)
+ gitlab-experiment (~> 0.6.1)
gitlab-fog-azure-rm (~> 1.1.1)
gitlab-fog-google (~> 1.13)
gitlab-labkit (~> 0.18.0)
diff --git a/app/experiments/application_experiment.rb b/app/experiments/application_experiment.rb
index 53ea8ea2d3ab78998bdad1c976d2b70d06c58214..c777fdc63a1bc2be6a95dcb52b8a844765f495ae 100644
--- a/app/experiments/application_experiment.rb
+++ b/app/experiments/application_experiment.rb
@@ -10,22 +10,28 @@ def enabled?
end
def publish(_result = nil)
- return unless should_track? # don't track events for excluded contexts
-
- record_experiment if @record # record the subject in the database if the context contains a namespace, group, project, actor or user
-
- track(:assignment) # track that we've assigned a variant for this context
+ super
- push_to_client
+ publish_to_client if should_track? # publish the experiment data to the client
+ publish_to_database if @record # publish the experiment context to the database
end
- # push the experiment data to the client
- def push_to_client
+ def publish_to_client
Gon.push({ experiment: { name => signature } }, true)
rescue NoMethodError
# means we're not in the request cycle, and can't add to Gon. Log a warning maybe?
end
+ def publish_to_database
+ # if the context contains a namespace, group, project, user, or actor
+ value = context.value
+ subject = value[:namespace] || value[:group] || value[:project] || value[:user] || value[:actor]
+ return unless ExperimentSubject.valid_subject?(subject)
+
+ variant = :experimental if @variant_name != :control
+ Experiment.add_subject(name, variant: variant || :control, subject: subject)
+ end
+
def track(action, **event_args)
return unless should_track? # don't track events for excluded contexts
@@ -41,14 +47,23 @@ def record!
@record = true
end
- def exclude!
- @excluded = true
- end
-
def control_behavior
# define a default nil control behavior so we can omit it when not needed
end
+ # TODO: remove
+ # This is deprecated logic as of v0.6.0 and should eventually be removed, but
+ # needs to stay intact for actively running experiments. The new strategy
+ # utilizes Digest::SHA2, a secret seed, and generates a 64-byte string.
+ def key_for(source, seed = name)
+ source = source.keys + source.values if source.is_a?(Hash)
+
+ ingredients = Array(source).map { |v| identify(v) }
+ ingredients.unshift(seed)
+
+ Digest::MD5.hexdigest(ingredients.join('|'))
+ end
+
private
def feature_flag_name
@@ -58,13 +73,4 @@ def feature_flag_name
def experiment_group?
Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
end
-
- def record_experiment
- subject = context.value[:namespace] || context.value[:group] || context.value[:project] || context.value[:user] || context.value[:actor]
- return unless ExperimentSubject.valid_subject?(subject)
-
- variant = :experimental if @variant_name != :control
-
- Experiment.add_subject(name, variant: variant || :control, subject: subject)
- end
end
diff --git a/config/feature_flags/development/gitlab_experiment_middleware.yml b/config/feature_flags/development/gitlab_experiment_middleware.yml
new file mode 100644
index 0000000000000000000000000000000000000000..76f8a36812d64694a910eaf844a188d2f4a2f30a
--- /dev/null
+++ b/config/feature_flags/development/gitlab_experiment_middleware.yml
@@ -0,0 +1,8 @@
+---
+name: gitlab_experiment_middleware
+introduced_by_url:
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/323643
+milestone: '14.1'
+type: development
+group: group::adoption
+default_enabled: false
diff --git a/config/initializers/gitlab_experiment.rb b/config/initializers/gitlab_experiment.rb
index 055979bb66bdeb761ee202d68dd3d9cd9574fbc7..39131e1126086972b184a97b12c9daab7c263b78 100644
--- a/config/initializers/gitlab_experiment.rb
+++ b/config/initializers/gitlab_experiment.rb
@@ -2,16 +2,22 @@
Gitlab::Experiment.configure do |config|
config.base_class = 'ApplicationExperiment'
+ config.mount_at = '/-/experiment'
config.cache = Gitlab::Experiment::Cache::RedisHashStore.new(
pool: ->(&block) { Gitlab::Redis::SharedState.with { |redis| block.call(redis) } }
)
+end
+
+# TODO: This shim should be removed after the feature flag is rolled out, as
+# it only exists to facilitate the feature flag control of the behavior.
+module Gitlab::Experiment::MiddlewareWithFeatureFlags
+ attr_reader :app
- # TODO: This will be deprecated as of v0.6.0, but needs to stay intact for
- # actively running experiments until a versioning concept is put in place to
- # enable migrating into the new SHA2 strategy.
- config.context_hash_strategy = lambda do |source, seed|
- source = source.keys + source.values if source.is_a?(Hash)
- data = Array(source).map { |v| (v.respond_to?(:to_global_id) ? v.to_global_id : v).to_s }
- Digest::MD5.hexdigest(data.unshift(seed).join('|'))
+ def call(env)
+ return app.call(env) unless Feature.enabled?(:gitlab_experiment_middleware)
+
+ super
end
end
+
+Gitlab::Experiment::Middleware.prepend(Gitlab::Experiment::MiddlewareWithFeatureFlags)
diff --git a/spec/experiments/application_experiment_spec.rb b/spec/experiments/application_experiment_spec.rb
index 2d2b911749b73316d7224cb9665109a59ff12a28..25e91dfdedd625cab907ec21bcbb6ebd8e7d579c 100644
--- a/spec/experiments/application_experiment_spec.rb
+++ b/spec/experiments/application_experiment_spec.rb
@@ -3,11 +3,10 @@
require 'spec_helper'
RSpec.describe ApplicationExperiment, :experiment do
- subject { described_class.new('namespaced/stub') }
+ subject { described_class.new('namespaced/stub', **context) }
- let(:feature_definition) do
- { name: 'namespaced_stub', type: 'experiment', group: 'group::adoption', default_enabled: false }
- end
+ let(:context) { {} }
+ let(:feature_definition) { { name: 'namespaced_stub', type: 'experiment', default_enabled: false } }
around do |example|
Feature::Definition.definitions[:namespaced_stub] = Feature::Definition.new('namespaced_stub.yml', feature_definition)
@@ -25,7 +24,7 @@
expect { experiment('namespaced/stub') { } }.not_to raise_error
end
- describe "enabled" do
+ describe "#enabled?" do
before do
allow(subject).to receive(:enabled?).and_call_original
@@ -57,103 +56,100 @@
end
end
- describe "publishing results" do
- it "doesn't record, track or push data to the client if we shouldn't track", :snowplow do
+ describe "#publish" do
+ it "doesn't track or publish to the client or database if we can't track", :snowplow do
allow(subject).to receive(:should_track?).and_return(false)
- subject.record!
- expect(subject).not_to receive(:record_experiment)
- expect(subject).not_to receive(:track)
- expect(Gon).not_to receive(:push)
+ expect(subject).not_to receive(:publish_to_client)
+ expect(subject).not_to receive(:publish_to_database)
- subject.publish(:action)
+ subject.publish
expect_no_snowplow_event
end
- describe 'recording the experiment' do
- it 'does not record the experiment if we do not tell it to' do
- expect(subject).not_to receive(:record_experiment)
-
- subject.publish
- end
-
- it 'records the experiment if we tell it to' do
- subject.record!
-
- expect(subject).to receive(:record_experiment)
-
- subject.publish
- end
- end
-
it "tracks the assignment" do
expect(subject).to receive(:track).with(:assignment)
subject.publish
end
- it "pushes the experiment knowledge into the client using Gon" do
- expect(Gon).to receive(:push).with({ experiment: { 'namespaced/stub' => subject.signature } }, true)
+ it "publishes the to the client" do
+ expect(subject).to receive(:publish_to_client)
subject.publish
end
- it "handles when Gon raises exceptions (like when it can't be pushed into)" do
- expect(Gon).to receive(:push).and_raise(NoMethodError)
+ it "publishes to the database if we've opted for that" do
+ subject.record!
+
+ expect(subject).to receive(:publish_to_database)
- expect { subject.publish }.not_to raise_error
+ subject.publish
end
- end
- it "can exclude from within the block" do
- expect(described_class.new('namespaced/stub') { |e| e.exclude! }).to be_excluded
- end
+ describe "#publish_to_client" do
+ it "adds the data into Gon" do
+ signature = { key: '86208ac54ca798e11f127e8b23ec396a', variant: 'control' }
+ expect(Gon).to receive(:push).with({ experiment: { 'namespaced/stub' => hash_including(signature) } }, true)
- describe 'recording the experiment subject' do
- using RSpec::Parameterized::TableSyntax
+ subject.publish_to_client
+ end
- subject { described_class.new('namespaced/stub', nil, **context) }
+ it "handles when Gon raises exceptions (like when it can't be pushed into)" do
+ expect(Gon).to receive(:push).and_raise(NoMethodError)
- before do
- subject.record!
+ expect { subject.publish_to_client }.not_to raise_error
+ end
end
- context 'when providing a compatible context' do
- where(:context_key, :object_type) do
- :namespace | :namespace
- :group | :namespace
- :project | :project
- :user | :user
- :actor | :user
+ describe "#publish_to_database" do
+ using RSpec::Parameterized::TableSyntax
+ let(:context) { { context_key => context_value }}
+
+ before do
+ subject.record!
end
- with_them do
- let(:context) { { context_key => build(object_type) }}
+ context "when there's a usable subject" do
+ where(:context_key, :context_value, :object_type) do
+ :namespace | build(:namespace) | :namespace
+ :group | build(:namespace) | :namespace
+ :project | build(:project) | :project
+ :user | build(:user) | :user
+ :actor | build(:user) | :user
+ end
- it 'records the experiment and the experiment subject from the context' do
- expect { subject.publish }.to change(Experiment, :count).by(1)
+ with_them do
+ it "creates an experiment and experiment subject record" do
+ expect { subject.publish_to_database }.to change(Experiment, :count).by(1)
- expect(Experiment.last.name).to eq('namespaced/stub')
- expect(ExperimentSubject.last.send(object_type)).to eq(context[context_key])
+ expect(Experiment.last.name).to eq('namespaced/stub')
+ expect(ExperimentSubject.last.send(object_type)).to eq(context[context_key])
+ end
end
end
- end
- context 'when providing an incompatible or no context' do
- where(context_hash: [{ foo: :bar }, {}])
+ context "when there's not a usable subject" do
+ where(:context_key, :context_value) do
+ :namespace | nil
+ :foo | :bar
+ end
- with_them do
- let(:context) { context_hash }
+ with_them do
+ it "doesn't create an experiment record" do
+ expect { subject.publish_to_database }.not_to change(Experiment, :count)
+ end
- it 'does not record the experiment' do
- expect { subject.publish }.not_to change(Experiment, :count)
+ it "doesn't create an experiment subject record" do
+ expect { subject.publish_to_database }.not_to change(ExperimentSubject, :count)
+ end
end
end
end
end
- describe "tracking events", :snowplow do
+ describe "#track", :snowplow do
it "doesn't track if we shouldn't track" do
allow(subject).to receive(:should_track?).and_return(false)
@@ -185,7 +181,13 @@
end
end
- describe "variant resolution" do
+ describe "#key_for" do
+ it "generates MD5 hashes" do
+ expect(subject.key_for(foo: :bar)).to eq('6f9ac12afdb9b58c2f19a136d09f9153')
+ end
+ end
+
+ context "when resolving variants" do
it "uses the default value as specified in the yaml" do
expect(Feature).to receive(:enabled?).with('namespaced_stub', subject, type: :experiment, default_enabled: :yaml)