Skip to content

Call snowplow for gitlab-experiment specs [RUN ALL RSPEC]

Nicolas Dular requested to merge nicolasdular/call-snowplow-for-gitlab-experiment-tracking into master

What does this MR do?

Snowplow checks types at runtime and this could lead to failures when using the wrong type for an event tracking property. We already call the original method in specs when :snowplow tag is used, but haven't so far for the custom rspec matchers from gitlab-experiment.

With gitlab-experiment 0.5.x which includes https://gitlab.com/gitlab-org/gitlab-experiment/-/merge_requests/74 we no longer fully stub the call to track in the specs but call the original method as well. I've provided a diff to test it out locally how it should behave when gitlab-experiment 0.5.x is released. It has no impact on the current test suite.

Diff 
diff --git a/Gemfile b/Gemfile
index 3273ba6dfd6..3f274662841 100644
--- a/Gemfile
+++ b/Gemfile
@@ -485,7 +485,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.12'
+gem 'gitlab-experiment', git: 'https://gitlab.com/gitlab-org/gitlab-experiment.git', ref: 'release-0.5.0' # '~> 0.4.12'

 # Structured logging
 gem 'lograge', '~> 0.5'
diff --git a/Gemfile.lock b/Gemfile.lock
index f1d9d106838..1d1d5191636 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1,3 +1,12 @@
+GIT
+  remote: https://gitlab.com/gitlab-org/gitlab-experiment.git
+  revision: 9605b2f7f456fb0e385c86fa4e23838774892429
+  ref: release-0.5.0
+  specs:
+    gitlab-experiment (0.4.12)
+      activesupport (>= 3.0)
+      scientist (~> 1.5, >= 1.5.0)
+
 GEM
   remote: https://rubygems.org/
   specs:
@@ -436,9 +445,6 @@ GEM
     github-markup (1.7.0)
     gitlab-chronic (0.10.5)
       numerizer (~> 0.2)
-    gitlab-experiment (0.4.12)
-      activesupport (>= 3.0)
-      scientist (~> 1.5, >= 1.5.0)
     gitlab-fog-azure-rm (1.0.1)
       azure-storage-blob (~> 2.0)
       azure-storage-common (~> 2.0)
@@ -1139,7 +1145,7 @@ GEM
     sawyer (0.8.2)
       addressable (>= 2.3.5)
       faraday (> 0.8, < 2.0)
-    scientist (1.5.0)
+    scientist (1.6.0)
     securecompare (1.0.0)
     seed-fu (2.3.7)
       activerecord (>= 3.1)
@@ -1413,7 +1419,7 @@ DEPENDENCIES
   gitaly (~> 13.9.0.pre.rc1)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
-  gitlab-experiment (~> 0.4.12)
+  gitlab-experiment!
   gitlab-fog-azure-rm (~> 1.0.1)
   gitlab-labkit (~> 0.16.0)
   gitlab-license (~> 1.3)
diff --git a/ee/app/controllers/registrations/groups_controller.rb b/ee/app/controllers/registrations/groups_controller.rb
index b1b0a27c37e..e4c56d66e49 100644
--- a/ee/app/controllers/registrations/groups_controller.rb
+++ b/ee/app/controllers/registrations/groups_controller.rb
@@ -66,7 +66,7 @@ def apply_trial_for_trial_onboarding_flow
         experiment(:registrations_group_invite, actor: current_user) do |experiment_instance|
           experiment_instance.use { redirect_to new_users_sign_up_project_path(namespace_id: @group.id, trial: helpers.in_trial_during_signup_flow?, trial_onboarding_flow: true) } # control
           experiment_instance.try(:invite_page) { redirect_to new_users_sign_up_group_invite_path(group_id: @group.id, trial: helpers.in_trial_during_signup_flow?, trial_onboarding_flow: true) } # with separate page
-          experiment_instance.track(:created, property: @group.id.to_s)
+          experiment_instance.track(:created, property: @group.id)
         end
       else
         render action: :new
@@ -82,7 +82,7 @@ def registration_onboarding_flow
         experiment(:registrations_group_invite, actor: current_user) do |experiment_instance|
           experiment_instance.use { invite_on_create } # control
           experiment_instance.try(:invite_page) { redirect_to new_users_sign_up_group_invite_path(group_id: @group.id, trial: helpers.in_trial_during_signup_flow?) } # with separate page
-          experiment_instance.track(:created, property: @group.id.to_s)
+          experiment_instance.track(:created, property: @group.id)
         end
       end
     end
@@ -100,7 +100,7 @@ def trial_during_signup_flow
         experiment(:registrations_group_invite, actor: current_user) do |experiment_instance|
           experiment_instance.use { redirect_to new_users_sign_up_project_path(namespace_id: @group.id, trial: helpers.in_trial_during_signup_flow?) } # control
           experiment_instance.try(:invite_page) { redirect_to new_users_sign_up_group_invite_path(group_id: @group.id, trial: helpers.in_trial_during_signup_flow?) } # with separate page
-          experiment_instance.track(:created, property: @group.id.to_s)
+          experiment_instance.track(:created, property: @group.id)
         end
       end
     end

With this diff, we want to track an Integer instead of a String, which would be incorrect for snowplow tracking and we get the expected type-check error that we otherwise would not get:

   Failure/Error: tracker.track_struct_event(category, action, label, property, value, context, (Time.now.to_f * 1000).to_i)
     
     ContractError:
       Contract violation for argument 4 of 7:
               Expected: (String or nil),
               Actual: 2
               Value guarded in: SnowplowTracker::Tracker::track_struct_event
               With Contract: String, String, Maybe, Maybe, Maybe, Maybe, SnowplowTracker::Timestamp => SnowplowTracker::Tracker
               At: /home/nicolas/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/snowplow-tracker-0.6.1/lib/snowplow-tracker/tracker.rb:252

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Nicolas Dular

Merge request reports