[Cookies] Expire the AB test cookie in a year (#8275)

zhhyu · web-flow · commit a79b0a5d8307 · 2020-10-22T18:48:42.000-07:00
* Expire the cookie in a year

* Upgrade AB test cookie to V3

* Fix build

* Combine V2 and V3
diff --git a/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs b/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs
@@ -26,7 +26,7 @@ public void TrackABTestEnrollmentInitialized(int schemaVersion, int previewSearc
             throw new NotImplementedException();
         }
 
-        public void TrackABTestEnrollmentUpgraded(int schemaVersion, int previewSearchBucket, int packageDepentsBucket)
+        public void TrackABTestEnrollmentUpgraded(int oldSchemaVersion, int newSchemaVersion, int previewSearchBucket, int packageDepentsBucket)
         {
             throw new NotImplementedException();
         }
diff --git a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs
@@ -378,11 +378,13 @@ void TrackABTestEnrollmentInitialized(
         /// <summary>
         /// Track when an A/B test enrollment is upgraded
         /// </summary>
-        /// <param name="schemaVersion">The schema version.</param>
+        /// <param name="oldSchemaVersion">The old schema version.</param>
+        /// <param name="newSchemaVersion">The new schema version.</param>
         /// <param name="previewSearchBucket">The bucket for the preview search test.</param>
         /// <param name="packageDependentBucket">The bucket for the package dependents test</param>
         void TrackABTestEnrollmentUpgraded(
-            int schemaVersion,
+            int oldSchemaVersion,
+            int newSchemaVersion,
             int previewSearchBucket,
             int packageDependentBucket);
 
diff --git a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs
@@ -1055,12 +1055,14 @@ public void TrackABTestEnrollmentInitialized(
         }
 
         public void TrackABTestEnrollmentUpgraded(
+            int oldSchemaVersion,
             int newSchemaVersion,
             int previewSearchBucket,
             int packageDependentBucket)
         {
             TrackMetric(Events.ABTestEnrollmentUpgraded, 1, properties =>
             {
+                properties.Add(OldSchemaVersion, oldSchemaVersion.ToString());
                 properties.Add(NewSchemaVersion, newSchemaVersion.ToString());
                 properties.Add(PreviewSearchBucket, previewSearchBucket.ToString());
                 properties.Add(PackageDependentBucket, packageDependentBucket.ToString());
diff --git a/src/NuGetGallery/Infrastructure/ABTestEnrollmentFactory.cs b/src/NuGetGallery/Infrastructure/ABTestEnrollmentFactory.cs
@@ -17,6 +17,7 @@ public class ABTestEnrollmentFactory : IABTestEnrollmentFactory
     {
         private const int SchemaVersion1 = 1; // PreviewSearch: {"v":1,"ps":100}
         private const int SchemaVersion2 = 2; // PreviewSearch + PackageDependent: {"v":2,"ps":100,"pd":100}
+        private const int SchemaVersion3 = 3; // PreviewSearch + PackageDependent: {"v":2,"ps":100,"pd":100}, and expired in a year.
 
         // Note that a new schema version could theoretically reuse any currently unused cookie properties. However
         // this does have questionable statistical correctness due treatment assignment of one A/B test being reused
@@ -36,7 +37,7 @@ public ABTestEnrollment Initialize()
         {
             var enrollment = new ABTestEnrollment(
                 ABTestEnrollmentState.FirstHit,
-                SchemaVersion2,
+                SchemaVersion3,
                 previewSearchBucket: GetRandomWholePercentage(),
                 packageDependentBucket: GetRandomWholePercentage());
 
@@ -66,19 +67,19 @@ private static int GetRandomWholePercentage()
 
         public string Serialize(ABTestEnrollment enrollment)
         {
-            if (enrollment.SchemaVersion != SchemaVersion2)
+            if (enrollment.SchemaVersion != SchemaVersion3)
             {
                 throw new NotImplementedException($"Serializing schema version {enrollment.SchemaVersion} is not implemented.");
             }
 
-            var deserialized2 = new StateVersion2
+            var deserialized3 = new StateVersion2OrVersion3
             {
-                SchemaVersion = SchemaVersion2,
+                SchemaVersion = SchemaVersion3,
                 PreviewSearchBucket = enrollment.PreviewSearchBucket,
                 PackageDependentBucket = enrollment.PackageDependentBucket,
             };
 
-            return JsonConvert.SerializeObject(deserialized2);
+            return JsonConvert.SerializeObject(deserialized3);
         }
 
         public bool TryDeserialize(string serialized, out ABTestEnrollment enrollment)
@@ -89,7 +90,8 @@ public bool TryDeserialize(string serialized, out ABTestEnrollment enrollment)
                 return false;
             }
 
-            return TryDeserializeStateVer2(serialized, out enrollment)
+            return TryDeserializeStateVer3(serialized, out enrollment)
+                || TryDeserializeStateVer2(serialized, out enrollment)
                 || TryDeserializeStateVer1(serialized, out enrollment);
         }
 
@@ -108,14 +110,15 @@ private bool TryDeserializeStateVer1(string serialized, out ABTestEnrollment enr
 
                 enrollment = new ABTestEnrollment(
                     ABTestEnrollmentState.Upgraded,
-                    SchemaVersion2,
+                    SchemaVersion3,
                     v1.PreviewSearchBucket,
                     packageDependentBucket: GetRandomWholePercentage());
 
                 _telemetryService.TrackABTestEnrollmentUpgraded(
-                enrollment.SchemaVersion,
-                enrollment.PreviewSearchBucket,
-                enrollment.PackageDependentBucket);
+                    SchemaVersion1,
+                    enrollment.SchemaVersion,
+                    enrollment.PreviewSearchBucket,
+                    enrollment.PackageDependentBucket);
 
                 return true;
             }
@@ -130,7 +133,7 @@ private bool TryDeserializeStateVer2(string serialized, out ABTestEnrollment enr
             enrollment = null;
             try
             {
-                var v2 = JsonConvert.DeserializeObject<StateVersion2>(serialized);
+                var v2 = JsonConvert.DeserializeObject<StateVersion2OrVersion3>(serialized);
                 if (v2 == null
                     || v2.SchemaVersion != SchemaVersion2
                     || IsNotPercentage(v2.PreviewSearchBucket)
@@ -140,11 +143,45 @@ private bool TryDeserializeStateVer2(string serialized, out ABTestEnrollment enr
                 }
 
                 enrollment = new ABTestEnrollment(
-                    ABTestEnrollmentState.Active,
-                    v2.SchemaVersion,
+                    ABTestEnrollmentState.Upgraded,
+                    SchemaVersion3,
                     v2.PreviewSearchBucket,
                     v2.PackageDependentBucket);
 
+                _telemetryService.TrackABTestEnrollmentUpgraded(
+                    SchemaVersion2,
+                    enrollment.SchemaVersion,
+                    enrollment.PreviewSearchBucket,
+                    enrollment.PackageDependentBucket);
+
+                return true;
+            }
+            catch (JsonException)
+            {
+                return false;
+            }
+        }
+
+        private bool TryDeserializeStateVer3(string serialized, out ABTestEnrollment enrollment)
+        {
+            enrollment = null;
+            try
+            {
+                var v3 = JsonConvert.DeserializeObject<StateVersion2OrVersion3>(serialized);
+                if (v3 == null
+                    || v3.SchemaVersion != SchemaVersion3
+                    || IsNotPercentage(v3.PreviewSearchBucket)
+                    || IsNotPercentage(v3.PackageDependentBucket))
+                {
+                    return false;
+                }
+
+                enrollment = new ABTestEnrollment(
+                    ABTestEnrollmentState.Active,
+                    v3.SchemaVersion,
+                    v3.PreviewSearchBucket,
+                    v3.PackageDependentBucket);
+
                 return true;
             }
             catch (JsonException)
@@ -167,7 +204,7 @@ private class StateVersion1
             public int PreviewSearchBucket { get; set; }
         }
 
-        private class StateVersion2
+        private class StateVersion2OrVersion3
         {
             [JsonProperty("v", Required = Required.Always)]
             public int SchemaVersion { get; set; }
diff --git a/src/NuGetGallery/Infrastructure/CookieBasedABTestService.cs b/src/NuGetGallery/Infrastructure/CookieBasedABTestService.cs
@@ -20,14 +20,16 @@ public class CookieBasedABTestService : IABTestService
         private readonly ITelemetryService _telemetryService;
         private readonly ILogger<CookieBasedABTestService> _logger;
         private readonly Lazy<ABTestEnrollment> _lazyEnrollment;
+        private readonly IDateTimeProvider _dateTimeProvider;
 
         public CookieBasedABTestService(
             HttpContextBase httpContext,
             IFeatureFlagService featureFlagService,
             IABTestEnrollmentFactory enrollmentFactory,
             IContentObjectService contentObjectService,
             ITelemetryService telemetryService,
-            ILogger<CookieBasedABTestService> logger)
+            ILogger<CookieBasedABTestService> logger,
+            IDateTimeProvider dateTimeProvider)
         {
             _httpContext = httpContext ?? throw new ArgumentNullException(nameof(httpContext));
             _featureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService));
@@ -36,6 +38,7 @@ public CookieBasedABTestService(
             _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
             _logger = logger ?? throw new ArgumentNullException(nameof(logger));
             _lazyEnrollment = new Lazy<ABTestEnrollment>(DetermineEnrollment);
+            _dateTimeProvider = dateTimeProvider ?? throw new ArgumentNullException(nameof(dateTimeProvider));
         }
 
         public bool IsPreviewSearchEnabled(User user)
@@ -74,7 +77,7 @@ private ABTestEnrollment DetermineEnrollment()
                     HttpOnly = true,
                     Secure = true,
                     Value = _enrollmentFactory.Serialize(enrollment),
-                    Expires = DateTime.MaxValue,
+                    Expires = _dateTimeProvider.UtcNow.AddYears(1),
                 };
                 _httpContext.Response.Cookies.Add(responseCookie);
             }
diff --git a/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs b/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs
@@ -23,7 +23,7 @@ public void TrackABTestEnrollmentInitialized(int schemaVersion, int previewSearc
             throw new NotImplementedException();
         }
 
-        public void TrackABTestEnrollmentUpgraded(int schemaVersion, int previewSearchBucket, int packageDepentsBucket)
+        public void TrackABTestEnrollmentUpgraded(int oldSchemaVersion, int newSchemaVersion, int previewSearchBucket, int packageDepentsBucket)
         {
             throw new NotImplementedException();
         }
diff --git a/tests/NuGetGallery.Facts/Infrastructure/ABTestEnrollmentFactoryFacts.cs b/tests/NuGetGallery.Facts/Infrastructure/ABTestEnrollmentFactoryFacts.cs
@@ -1,6 +1,7 @@
 ﻿// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using Microsoft.Extensions.Logging;
 using Moq;
 using Xunit;
@@ -18,40 +19,45 @@ public void CreatesANewEnrollmentInstance()
 
                 Assert.NotNull(enrollment);
                 Assert.Equal(ABTestEnrollmentState.FirstHit, enrollment.State);
-                Assert.Equal(2, enrollment.SchemaVersion);
+                Assert.Equal(3, enrollment.SchemaVersion);
                 Assert.InRange(enrollment.PreviewSearchBucket, 1, 100);
                 Assert.InRange(enrollment.PackageDependentBucket, 1, 100);
             }
         }
 
         public class Serialize : Facts
         {
-            [Fact]
-            public void ProducesExpectedString()
+            [Theory]
+            [InlineData(1)]
+            [InlineData(2)]
+            [InlineData(4)]
+            public void SerializeWithNotImplementedSchemaVersion_ThrowNotImplementedException(int schemaVersion)
             {
                 var enrollment = new ABTestEnrollment(
-                    ABTestEnrollmentState.Active,
-                    schemaVersion: 2,
-                    previewSearchBucket: 42,
-                    packageDependentBucket: 82);
+                    state: It.IsAny<ABTestEnrollmentState>(),
+                    schemaVersion: schemaVersion,
+                    previewSearchBucket: It.IsAny<int>(),
+                    packageDependentBucket: It.IsAny<int>());
 
-                var serialized = Target.Serialize(enrollment);
-
-                Assert.Equal(@"{""v"":2,""ps"":42,""pd"":82}", serialized);
+                var exception = Assert.Throws<NotImplementedException>(() => Target.Serialize(enrollment));
+                Assert.Equal($"Serializing schema version {schemaVersion} is not implemented.", exception.Message);
             }
 
-            [Fact]
-            public void ProducesExpectedStringVersionTwo()
+            [Theory]
+            [InlineData(3, ABTestEnrollmentState.Active)]
+            [InlineData(3, ABTestEnrollmentState.FirstHit)]
+            [InlineData(3, ABTestEnrollmentState.Upgraded)]
+            public void ProducesExpectedString(int schemaVersion, ABTestEnrollmentState state)
             {
                 var enrollment = new ABTestEnrollment(
-                    ABTestEnrollmentState.Active,
-                    schemaVersion: 2,
+                    state: state,
+                    schemaVersion: schemaVersion,
                     previewSearchBucket: 42,
-                    packageDependentBucket: 74);
+                    packageDependentBucket: 82);
 
                 var serialized = Target.Serialize(enrollment);
 
-                Assert.Equal(@"{""v"":2,""ps"":42,""pd"":74}", serialized);
+                Assert.Equal(@"{""v"":3,""ps"":42,""pd"":82}", serialized);
             }
         }
 
@@ -66,22 +72,33 @@ public class Deserialize : Facts
             [InlineData("[]")]
             [InlineData("null")]
             [InlineData(@"{""ps"":42}")]
-            [InlineData(@"{""v"":2,""ps"":42}")]
+            [InlineData(@"{""pd"":42}")]
+            [InlineData(@"{""ps"":20,""pd"":82}")]
             [InlineData(@"{""v"":1}")]
             [InlineData(@"{""v"":1,""ps"":-1}")]
             [InlineData(@"{""v"":1,""ps"":0}")]
             [InlineData(@"{""v"":1,""ps"":101}")]
-            [InlineData(@"{""v"":2,""ps"":10,""pd"":101}")]
             [InlineData(@"{""v"":2}")]
-            [InlineData(@"{""pd"":42}")]
+            [InlineData(@"{""v"":2,""ps"":42}")]
+            [InlineData(@"{""v"":2,""pd"":42}")]
+            [InlineData(@"{""v"":2,""ps"":10,""pd"":101}")]
             [InlineData(@"{""v"":2,""ps"":1,""pd"":-1}")]
             [InlineData(@"{""v"":2,""ps"":10,""pd"":0}")]
-            [InlineData(@"{""v"":3,""ps"":42,""pd"":53}")]
             [InlineData(@"{""v"":2,""ps"":-1,""pd"":24}")]
             [InlineData(@"{""v"":2,""ps"":0,""pd"":35}")]
             [InlineData(@"{""v"":2,""ps"":101,""pd"":56}")]
             [InlineData(@"{""v"":2,""ps"":200,""pd"":82}")]
-            [InlineData(@"{""ps"":20,""pd"":82}")]
+            [InlineData(@"{""v"":3}")]
+            [InlineData(@"{""v"":3,""ps"":42}")]
+            [InlineData(@"{""v"":3,""pd"":42}")]
+            [InlineData(@"{""v"":3,""ps"":10,""pd"":101}")]
+            [InlineData(@"{""v"":3,""ps"":1,""pd"":-1}")]
+            [InlineData(@"{""v"":3,""ps"":10,""pd"":0}")]
+            [InlineData(@"{""v"":3,""ps"":-1,""pd"":24}")]
+            [InlineData(@"{""v"":3,""ps"":0,""pd"":35}")]
+            [InlineData(@"{""v"":3,""ps"":101,""pd"":56}")]
+            [InlineData(@"{""v"":3,""ps"":200,""pd"":82}")]
+            [InlineData(@"{""v"":4,""ps"":42,""pd"":53}")]
             public void RejectsInvalid(string input)
             {
                 var success = Target.TryDeserialize(input, out var enrollment);
@@ -95,14 +112,16 @@ public void RejectsInvalid(string input)
             [InlineData(@"{""v"":1,""ps"":42,""zzz"":false}", 42)]
             [InlineData(@"{""v"":1,""ps"":1}", 1)]
             [InlineData(@"{""v"":1,""ps"":100}", 100)]
-            public void UpgradesValidVersionV1(string input, int previewSearchBucket)
+            public void UpgradesValidVersion(string input, int previewSearchBucket)
             {
+                TelemetryService.Setup(t => t.TrackABTestEnrollmentUpgraded(1, 3, It.IsAny<int>(), It.IsAny<int>()));
                 var success = Target.TryDeserialize(input, out var enrollment);
 
+                TelemetryService.Verify(t => t.TrackABTestEnrollmentUpgraded(1, 3, It.IsAny<int>(), It.IsAny<int>()), Times.Once);
                 Assert.True(success, "The derialization should have succeeded.");
                 Assert.NotNull(enrollment);
                 Assert.Equal(ABTestEnrollmentState.Upgraded, enrollment.State);
-                Assert.Equal(2, enrollment.SchemaVersion);
+                Assert.Equal(3, enrollment.SchemaVersion);
                 Assert.Equal(previewSearchBucket, enrollment.PreviewSearchBucket);
                 Assert.InRange(enrollment.PackageDependentBucket, 1, 100);
             }
@@ -116,13 +135,35 @@ public void UpgradesValidVersionV1(string input, int previewSearchBucket)
             [InlineData(@"{""v"":2,""ps"":52,""pd"":1}", 52, 1)]
             [InlineData(@"{""v"":2,""ps"":68,""pd"":100}", 68, 100)]
             public void ParsesValidVersion2(string input, int previewSearchBucket, int packageDependentBucket)
+            {
+                TelemetryService.Setup(t => t.TrackABTestEnrollmentUpgraded(2, 3, It.IsAny<int>(), It.IsAny<int>()));
+                var success = Target.TryDeserialize(input, out var enrollment);
+
+                TelemetryService.Verify(t => t.TrackABTestEnrollmentUpgraded(2, 3, It.IsAny<int>(), It.IsAny<int>()), Times.Once);
+                Assert.True(success, "The derialization should have succeeded.");
+                Assert.NotNull(enrollment);
+                Assert.Equal(ABTestEnrollmentState.Upgraded, enrollment.State);
+                Assert.Equal(3, enrollment.SchemaVersion);
+                Assert.Equal(previewSearchBucket, enrollment.PreviewSearchBucket);
+                Assert.Equal(packageDependentBucket, enrollment.PackageDependentBucket);
+            }
+
+            [Theory]
+            [InlineData(@"{""v"":3,""ps"":42,""pd"":57}", 42, 57)]
+            [InlineData(@"{""v"":3,""ps"":1,""pd"":1}", 1, 1)]
+            [InlineData(@"{""v"":3,""ps"":100,""pd"":100}", 100, 100)]
+            [InlineData(@"{""v"":3,""ps"":1,""pd"":32}", 1, 32)]
+            [InlineData(@"{""v"":3,""ps"":100,""pd"":57}", 100, 57)]
+            [InlineData(@"{""v"":3,""ps"":52,""pd"":1}", 52, 1)]
+            [InlineData(@"{""v"":3,""ps"":68,""pd"":100}", 68, 100)]
+            public void ParsesValidVersion3(string input, int previewSearchBucket, int packageDependentBucket)
             {
                 var success = Target.TryDeserialize(input, out var enrollment);
 
                 Assert.True(success, "The derialization should have succeeded.");
                 Assert.NotNull(enrollment);
                 Assert.Equal(ABTestEnrollmentState.Active, enrollment.State);
-                Assert.Equal(2, enrollment.SchemaVersion);
+                Assert.Equal(3, enrollment.SchemaVersion);
                 Assert.Equal(previewSearchBucket, enrollment.PreviewSearchBucket);
                 Assert.Equal(packageDependentBucket, enrollment.PackageDependentBucket);
             }
diff --git a/tests/NuGetGallery.Facts/Infrastructure/CookieBasedABTestServiceFacts.cs b/tests/NuGetGallery.Facts/Infrastructure/CookieBasedABTestServiceFacts.cs
diff --git a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ public void TrackABTestEnrollmentInitialized(int schemaVersion, int previewSearc`
`26`	`26`	`throw new NotImplementedException();`
`27`	`27`	`}`
`28`	`28`
`29`		`- public void TrackABTestEnrollmentUpgraded(int schemaVersion, int previewSearchBucket, int packageDepentsBucket)`
	`29`	`+ public void TrackABTestEnrollmentUpgraded(int oldSchemaVersion, int newSchemaVersion, int previewSearchBucket, int packageDepentsBucket)`
`30`	`30`	`{`
`31`	`31`	`throw new NotImplementedException();`
`32`	`32`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1055,12 +1055,14 @@ public void TrackABTestEnrollmentInitialized(`
`1055`	`1055`	`}`
`1056`	`1056`
`1057`	`1057`	`public void TrackABTestEnrollmentUpgraded(`
	`1058`	`+ int oldSchemaVersion,`
`1058`	`1059`	`int newSchemaVersion,`
`1059`	`1060`	`int previewSearchBucket,`
`1060`	`1061`	`int packageDependentBucket)`
`1061`	`1062`	`{`
`1062`	`1063`	`TrackMetric(Events.ABTestEnrollmentUpgraded, 1, properties =>`
`1063`	`1064`	`{`
	`1065`	`+ properties.Add(OldSchemaVersion, oldSchemaVersion.ToString());`
`1064`	`1066`	`properties.Add(NewSchemaVersion, newSchemaVersion.ToString());`
`1065`	`1067`	`properties.Add(PreviewSearchBucket, previewSearchBucket.ToString());`
`1066`	`1068`	`properties.Add(PackageDependentBucket, packageDependentBucket.ToString());`
Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,7 @@ public void TrackABTestEnrollmentInitialized(int schemaVersion, int previewSearc`
`23`	`23`	`throw new NotImplementedException();`
`24`	`24`	`}`
`25`	`25`
`26`		`- public void TrackABTestEnrollmentUpgraded(int schemaVersion, int previewSearchBucket, int packageDepentsBucket)`
	`26`	`+ public void TrackABTestEnrollmentUpgraded(int oldSchemaVersion, int newSchemaVersion, int previewSearchBucket, int packageDepentsBucket)`
`27`	`27`	`{`
`28`	`28`	`throw new NotImplementedException();`
`29`	`29`	`}`