From 963fc07fb0de5fa4720c358f009c1acc88eafb69 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 4 Oct 2024 12:15:28 -0400 Subject: [PATCH 01/24] chore: updated during Rider upgrade --- OptimizelySDK.sln.DotSettings | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/OptimizelySDK.sln.DotSettings b/OptimizelySDK.sln.DotSettings index 3ccf7ffc..8ee6e5a4 100644 --- a/OptimizelySDK.sln.DotSettings +++ b/OptimizelySDK.sln.DotSettings @@ -43,10 +43,15 @@ <Policy Inspect="True" Prefix="I" Suffix="" Style="AaBb" /> <Policy Inspect="True" Prefix="" Suffix="" Style="AA_BB" /> <Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /> + <Policy><Descriptor Staticness="Static" AccessRightKinds="Private" Description="Static readonly fields (private)"><ElementKinds><Kind Name="READONLY_FIELD" /></ElementKinds></Descriptor><Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Any" Description="Enum members"><ElementKinds><Kind Name="ENUM_MEMBER" /></ElementKinds></Descriptor><Policy Inspect="True" Prefix="" Suffix="" Style="AaBb"><ExtraRule Prefix="" Suffix="" Style="AaBb" /></Policy></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Any" Description="Local constants"><ElementKinds><Kind Name="LOCAL_CONSTANT" /></ElementKinds></Descriptor><Policy Inspect="True" Prefix="" Suffix="" Style="AA_BB" /></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Any" Description="Interfaces"><ElementKinds><Kind Name="INTERFACE" /></ElementKinds></Descriptor><Policy Inspect="True" Prefix="I" Suffix="" Style="AaBb" /></Policy> True True True True + True True True True From d0f59b6677d583c632a71937c92eb38197c13d3d Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 4 Oct 2024 12:18:12 -0400 Subject: [PATCH 02/24] feat: call UPS once during batch decideForKeys or decideAll --- OptimizelySDK/Bucketing/DecisionService.cs | 164 +++++++++++++-------- OptimizelySDK/Optimizely.cs | 40 ++--- 2 files changed, 127 insertions(+), 77 deletions(-) diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index e6088d7e..4d31f340 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -1,18 +1,18 @@ /* -* Copyright 2017-2022, Optimizely -* -* Licensed under the Apache License, Version 2.0 (the "License"); -* you may not use this file except in compliance with the License. -* You may obtain a copy of the License at -* -* https://www.apache.org/licenses/LICENSE-2.0 -* -* Unless required by applicable law or agreed to in writing, software -* distributed under the License is distributed on an "AS IS" BASIS, -* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -* See the License for the specific language governing permissions and -* limitations under the License. -*/ + * Copyright 2017-2022, 2024 Optimizely + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ using System; using System.Collections.Generic; @@ -43,6 +43,22 @@ public class DecisionService private IErrorHandler ErrorHandler; private UserProfileService UserProfileService; private ILogger Logger; + private UserProfile _userProfile; + + private bool _decisionBatchInProgress = false; + + public bool DecisionBatchInProgress + { + get => _decisionBatchInProgress; + set + { + _decisionBatchInProgress = value; + if (!_decisionBatchInProgress) + { + SaveToUserProfileService(); + } + } + } /// /// Associative array of user IDs to an associative array @@ -60,10 +76,10 @@ public class DecisionService /// /// Initialize a decision service for the Optimizely client. /// - /// Base bucketer to allocate new users to an experiment. - /// The error handler of the Optimizely client. - /// - /// < param name= "logger" > UserProfileService implementation for storing user info. + /// Base bucketer to allocate new users to an experiment. + /// The error handler of the Optimizely client. + /// The injected implementation providing control over the bucketing. + /// < param name="logger"> UserProfileService implementation for storing user info. public DecisionService(Bucketer bucketer, IErrorHandler errorHandler, UserProfileService userProfileService, ILogger logger ) @@ -85,7 +101,7 @@ public DecisionService(Bucketer bucketer, IErrorHandler errorHandler, /// Get a Variation of an Experiment for a user to be allocated into. /// /// The Experiment the user will be bucketed into. - /// Optimizely user context. + /// Optimizely user context. /// Project config. /// The Variation the user is allocated into. public virtual Result GetVariation(Experiment experiment, @@ -99,10 +115,10 @@ ProjectConfig config /// /// Get a Variation of an Experiment for a user to be allocated into. /// - /// The Experiment the user will be bucketed into. - /// optimizely user context. - /// Project Config. - /// An array of decision options. + /// The Experiment the user will be bucketed into. + /// optimizely user context. + /// Project Config. + /// An array of decision options. /// The Variation the user is allocated into. public virtual Result GetVariation(Experiment experiment, OptimizelyUserContext user, @@ -140,35 +156,42 @@ OptimizelyDecideOption[] options var ignoreUPS = Array.Exists(options, option => option == OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE); - UserProfile userProfile = null; + if (!ignoreUPS && UserProfileService != null) { try { - var userProfileMap = UserProfileService.Lookup(user.GetUserId()); - if (userProfileMap != null && - UserProfileUtil.IsValidUserProfileMap(userProfileMap)) + if (_userProfile == null) + { + var userProfileMap = UserProfileService.Lookup(user.GetUserId()); + if (userProfileMap != null && + UserProfileUtil.IsValidUserProfileMap(userProfileMap)) + { + _userProfile = UserProfileUtil.ConvertMapToUserProfile(userProfileMap); + } + else if (userProfileMap == null) + { + Logger.Log(LogLevel.INFO, + reasons.AddInfo( + "We were unable to get a user profile map from the UserProfileService.")); + } + else + { + Logger.Log(LogLevel.ERROR, + reasons.AddInfo("The UserProfileService returned an invalid map.")); + } + } + + if (_userProfile != null) { - userProfile = UserProfileUtil.ConvertMapToUserProfile(userProfileMap); decisionVariationResult = - GetStoredVariation(experiment, userProfile, config); + GetStoredVariation(experiment, _userProfile, config); reasons += decisionVariationResult.DecisionReasons; if (decisionVariationResult.ResultObject != null) { return decisionVariationResult.SetReasons(reasons); } } - else if (userProfileMap == null) - { - Logger.Log(LogLevel.INFO, - reasons.AddInfo( - "We were unable to get a user profile map from the UserProfileService.")); - } - else - { - Logger.Log(LogLevel.ERROR, - reasons.AddInfo("The UserProfileService returned an invalid map.")); - } } catch (Exception exception) { @@ -197,11 +220,7 @@ OptimizelyDecideOption[] options { if (UserProfileService != null && !ignoreUPS) { - var bucketerUserProfile = userProfile ?? - new UserProfile(userId, - new Dictionary()); - SaveVariation(experiment, decisionVariationResult.ResultObject, - bucketerUserProfile); + SaveVariation(experiment, decisionVariationResult.ResultObject); } else { @@ -454,12 +473,9 @@ ProjectConfig config /// /// Save a { @link Variation } of an { @link Experiment } for a user in the {@link UserProfileService}. /// - /// The experiment the user was buck - /// The Variation to save. - /// instance of the user information. - public void SaveVariation(Experiment experiment, Variation variation, - UserProfile userProfile - ) + /// The experiment the user was buck + /// The Variation to save. + public void SaveVariation(Experiment experiment, Variation variation) { //only save if the user has implemented a user profile service if (UserProfileService == null) @@ -468,9 +484,9 @@ UserProfile userProfile } Decision decision; - if (userProfile.ExperimentBucketMap.ContainsKey(experiment.Id)) + if (_userProfile.ExperimentBucketMap.ContainsKey(experiment.Id)) { - decision = userProfile.ExperimentBucketMap[experiment.Id]; + decision = _userProfile.ExperimentBucketMap[experiment.Id]; decision.VariationId = variation.Id; } else @@ -478,18 +494,48 @@ UserProfile userProfile decision = new Decision(variation.Id); } - userProfile.ExperimentBucketMap[experiment.Id] = decision; + _userProfile.ExperimentBucketMap[experiment.Id] = decision; + + if (!_decisionBatchInProgress) + { + SaveToUserProfileService(experiment, variation); + } + } + private void SaveToUserProfileService(Experiment experiment = null, + Variation variation = null + ) + { + var useSpecificLogEntry = experiment != null && variation != null && + !string.IsNullOrEmpty(_userProfile?.UserId); + try { - UserProfileService.Save(userProfile.ToMap()); - Logger.Log(LogLevel.INFO, - $"Saved variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{userProfile.UserId}\"."); + if (_userProfile != null) + { + UserProfileService.Save(_userProfile.ToMap()); + if (useSpecificLogEntry) + { + Logger.Log(LogLevel.INFO, + $"Saved variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{_userProfile.UserId}\"."); + } + else + { + Logger.Log(LogLevel.INFO, "Saved user profile after batch decision."); + } + } } catch (Exception exception) { - Logger.Log(LogLevel.ERROR, - $"Failed to save variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{userProfile.UserId}\"."); + if (useSpecificLogEntry) + { + Logger.Log(LogLevel.ERROR, + $"Failed to save variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{_userProfile.UserId}\"."); + } + else + { + Logger.Log(LogLevel.ERROR, "Failed to save user profile after batch decision."); + } ErrorHandler.HandleError( new Exceptions.OptimizelyRuntimeException(exception.Message)); } diff --git a/OptimizelySDK/Optimizely.cs b/OptimizelySDK/Optimizely.cs index 9da300f8..e455b206 100644 --- a/OptimizelySDK/Optimizely.cs +++ b/OptimizelySDK/Optimizely.cs @@ -1,5 +1,5 @@ /* - * Copyright 2017-2023, Optimizely + * Copyright 2017-2024, Optimizely * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use file except in compliance with the License. @@ -223,8 +223,8 @@ public Optimizely(ProjectConfigManager configManager, if (ProjectConfigManager.SdkKey != null) { - NotificationCenterRegistry.GetNotificationCenter(configManager.SdkKey, logger) - ?.AddNotification(NotificationCenter.NotificationType.OptimizelyConfigUpdate, + NotificationCenterRegistry.GetNotificationCenter(configManager.SdkKey, logger)?. + AddNotification(NotificationCenter.NotificationType.OptimizelyConfigUpdate, () => { projectConfig = ProjectConfigManager.CachedProjectConfig; @@ -268,10 +268,9 @@ private void InitializeComponents(IEventDispatcher eventDispatcher = null, Logger); DefaultDecideOptions = defaultDecideOptions ?? new OptimizelyDecideOption[] { }; #if USE_ODP - OdpManager = odpManager ?? new OdpManager.Builder() - .WithErrorHandler(errorHandler) - .WithLogger(logger) - .Build(); + OdpManager = odpManager ?? new OdpManager.Builder().WithErrorHandler(errorHandler). + WithLogger(logger). + Build(); #endif } @@ -442,8 +441,8 @@ private Variation GetVariation(string experimentKey, string userId, ProjectConfi userAttributes = userAttributes ?? new UserAttributes(); var userContext = CreateUserContextCopy(userId, userAttributes); - var variation = DecisionService.GetVariation(experiment, userContext, config) - ?.ResultObject; + var variation = DecisionService.GetVariation(experiment, userContext, config)?. + ResultObject; var decisionInfo = new Dictionary { { "experimentKey", experimentKey }, { "variationKey", variation?.Key }, @@ -553,8 +552,8 @@ public virtual bool IsFeatureEnabled(string featureKey, string userId, var sourceInfo = new Dictionary(); var decision = DecisionService.GetVariationForFeature(featureFlag, CreateUserContextCopy(userId, userAttributes), - config) - .ResultObject; + config). + ResultObject; var variation = decision?.Variation; var decisionSource = decision?.Source ?? FeatureDecision.DECISION_SOURCE_ROLLOUT; @@ -665,8 +664,8 @@ public virtual T GetFeatureVariableValueForType(string featureKey, string var var variableValue = featureVariable.DefaultValue; var decision = DecisionService.GetVariationForFeature(featureFlag, CreateUserContextCopy(userId, userAttributes), - config) - .ResultObject; + config). + ResultObject; if (decision?.Variation != null) { @@ -981,9 +980,9 @@ OptimizelyDecideOption[] options featureEnabled); } - var reasonsToReport = decisionReasons - .ToReport(allOptions.Contains(OptimizelyDecideOption.INCLUDE_REASONS)) - .ToArray(); + var reasonsToReport = decisionReasons. + ToReport(allOptions.Contains(OptimizelyDecideOption.INCLUDE_REASONS)). + ToArray(); var variationKey = decision?.Variation?.Key; // TODO: add ruleKey values when available later. use a copy of experimentKey until then. @@ -1056,6 +1055,7 @@ OptimizelyDecideOption[] options var allOptions = GetAllOptions(options); + DecisionService.DecisionBatchInProgress = true; foreach (var key in keys) { var decision = Decide(user, key, options); @@ -1066,6 +1066,8 @@ OptimizelyDecideOption[] options } } + DecisionService.DecisionBatchInProgress = false; + return decisionDictionary; } @@ -1347,7 +1349,8 @@ List segmentOptions if (config == null) { - Logger.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'FetchQualifiedSegments'."); + Logger.Log(LogLevel.ERROR, + "Datafile has invalid format. Failing 'FetchQualifiedSegments'."); return null; } @@ -1378,7 +1381,8 @@ internal void IdentifyUser(string userId) /// Dictionary for identifiers. The caller must provide at least one key-value pair. /// Type of event (defaults to `fullstack`) /// Optional event data in a key-value pair format - public void SendOdpEvent(string action, Dictionary identifiers, string type = Constants.ODP_EVENT_TYPE, + public void SendOdpEvent(string action, Dictionary identifiers, + string type = Constants.ODP_EVENT_TYPE, Dictionary data = null ) { From 14dfaced2309577a2e82984008cbece5f864a0c2 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 4 Oct 2024 12:29:31 -0400 Subject: [PATCH 03/24] revert: SaveVariation to include userProfile --- OptimizelySDK/Bucketing/DecisionService.cs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index 4d31f340..2c80d3bf 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -220,7 +220,11 @@ OptimizelyDecideOption[] options { if (UserProfileService != null && !ignoreUPS) { - SaveVariation(experiment, decisionVariationResult.ResultObject); + var bucketerUserProfile = _userProfile ?? + new UserProfile(userId, + new Dictionary()); + SaveVariation(experiment, decisionVariationResult.ResultObject, + bucketerUserProfile); } else { @@ -475,7 +479,10 @@ ProjectConfig config /// /// The experiment the user was buck /// The Variation to save. - public void SaveVariation(Experiment experiment, Variation variation) + /// instance of the user information. + public void SaveVariation(Experiment experiment, Variation variation, + UserProfile userProfile + ) { //only save if the user has implemented a user profile service if (UserProfileService == null) @@ -483,6 +490,11 @@ public void SaveVariation(Experiment experiment, Variation variation) return; } + if (_userProfile == null) + { + _userProfile = userProfile; + } + Decision decision; if (_userProfile.ExperimentBucketMap.ContainsKey(experiment.Id)) { @@ -507,8 +519,8 @@ private void SaveToUserProfileService(Experiment experiment = null, ) { var useSpecificLogEntry = experiment != null && variation != null && - !string.IsNullOrEmpty(_userProfile?.UserId); - + !string.IsNullOrEmpty(_userProfile?.UserId); + try { if (_userProfile != null) @@ -536,6 +548,7 @@ private void SaveToUserProfileService(Experiment experiment = null, { Logger.Log(LogLevel.ERROR, "Failed to save user profile after batch decision."); } + ErrorHandler.HandleError( new Exceptions.OptimizelyRuntimeException(exception.Message)); } From 66708d86217cf174e8f9570365872fa616898c63 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 4 Oct 2024 12:34:53 -0400 Subject: [PATCH 04/24] doc: fix param --- OptimizelySDK/Bucketing/DecisionService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index 2c80d3bf..1a374475 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -479,7 +479,7 @@ ProjectConfig config /// /// The experiment the user was buck /// The Variation to save. - /// instance of the user information. + /// Instance of the user information. public void SaveVariation(Experiment experiment, Variation variation, UserProfile userProfile ) From dd072b96247cf002d6489f988abd57b54396fb0b Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 4 Oct 2024 13:38:23 -0400 Subject: [PATCH 05/24] test: fix hacked tests --- OptimizelySDK.Tests/OptimizelyTest.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/OptimizelySDK.Tests/OptimizelyTest.cs b/OptimizelySDK.Tests/OptimizelyTest.cs index 5dab3aec..f83304bb 100644 --- a/OptimizelySDK.Tests/OptimizelyTest.cs +++ b/OptimizelySDK.Tests/OptimizelyTest.cs @@ -1394,19 +1394,19 @@ public void TestForcedVariationPreceedsUserProfile() LoggerMock.Verify( l => l.Log(LogLevel.INFO, "No previously activated variation of experiment \"etag1\" for user \"testUser3\" found in user profile."), - Times.Exactly(2)); + Times.Once); LoggerMock.Verify( l => l.Log(LogLevel.DEBUG, "Assigned bucket [4969] to user [testUser3] with bucketing ID [testUser3]."), - Times.Exactly(2)); + Times.Once); LoggerMock.Verify( l => l.Log(LogLevel.INFO, "User [testUser3] is in variation [vtag2] of experiment [etag1]."), - Times.Exactly(2)); + Times.Once); LoggerMock.Verify( l => l.Log(LogLevel.INFO, "Saved variation \"277\" of experiment \"223\" for user \"testUser3\"."), - Times.Exactly(2)); + Times.Once); LoggerMock.Verify( l => l.Log(LogLevel.DEBUG, "Set variation \"276\" for experiment \"223\" and user \"testUser3\" in the forced variation map."), From f9f3f3fe19c6826ecfdae3d2b00c31f90c990c8b Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 4 Oct 2024 13:43:39 -0400 Subject: [PATCH 06/24] revert: auto-formatting I want this reference PR to be clear about what needs changing. --- OptimizelySDK/Optimizely.cs | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/OptimizelySDK/Optimizely.cs b/OptimizelySDK/Optimizely.cs index e455b206..3a517174 100644 --- a/OptimizelySDK/Optimizely.cs +++ b/OptimizelySDK/Optimizely.cs @@ -223,8 +223,8 @@ public Optimizely(ProjectConfigManager configManager, if (ProjectConfigManager.SdkKey != null) { - NotificationCenterRegistry.GetNotificationCenter(configManager.SdkKey, logger)?. - AddNotification(NotificationCenter.NotificationType.OptimizelyConfigUpdate, + NotificationCenterRegistry.GetNotificationCenter(configManager.SdkKey, logger) + ?.AddNotification(NotificationCenter.NotificationType.OptimizelyConfigUpdate, () => { projectConfig = ProjectConfigManager.CachedProjectConfig; @@ -268,9 +268,9 @@ private void InitializeComponents(IEventDispatcher eventDispatcher = null, Logger); DefaultDecideOptions = defaultDecideOptions ?? new OptimizelyDecideOption[] { }; #if USE_ODP - OdpManager = odpManager ?? new OdpManager.Builder().WithErrorHandler(errorHandler). - WithLogger(logger). - Build(); + OdpManager = odpManager ?? new OdpManager.Builder().WithErrorHandler(errorHandler) + .WithLogger(logger) + .Build(); #endif } @@ -441,8 +441,8 @@ private Variation GetVariation(string experimentKey, string userId, ProjectConfi userAttributes = userAttributes ?? new UserAttributes(); var userContext = CreateUserContextCopy(userId, userAttributes); - var variation = DecisionService.GetVariation(experiment, userContext, config)?. - ResultObject; + var variation = DecisionService.GetVariation(experiment, userContext, config) + ?.ResultObject; var decisionInfo = new Dictionary { { "experimentKey", experimentKey }, { "variationKey", variation?.Key }, @@ -552,8 +552,8 @@ public virtual bool IsFeatureEnabled(string featureKey, string userId, var sourceInfo = new Dictionary(); var decision = DecisionService.GetVariationForFeature(featureFlag, CreateUserContextCopy(userId, userAttributes), - config). - ResultObject; + config) + .ResultObject; var variation = decision?.Variation; var decisionSource = decision?.Source ?? FeatureDecision.DECISION_SOURCE_ROLLOUT; @@ -664,8 +664,8 @@ public virtual T GetFeatureVariableValueForType(string featureKey, string var var variableValue = featureVariable.DefaultValue; var decision = DecisionService.GetVariationForFeature(featureFlag, CreateUserContextCopy(userId, userAttributes), - config). - ResultObject; + config) + .ResultObject; if (decision?.Variation != null) { @@ -980,9 +980,9 @@ OptimizelyDecideOption[] options featureEnabled); } - var reasonsToReport = decisionReasons. - ToReport(allOptions.Contains(OptimizelyDecideOption.INCLUDE_REASONS)). - ToArray(); + var reasonsToReport = decisionReasons + .ToReport(allOptions.Contains(OptimizelyDecideOption.INCLUDE_REASONS)) + .ToArray(); var variationKey = decision?.Variation?.Key; // TODO: add ruleKey values when available later. use a copy of experimentKey until then. @@ -1349,8 +1349,7 @@ List segmentOptions if (config == null) { - Logger.Log(LogLevel.ERROR, - "Datafile has invalid format. Failing 'FetchQualifiedSegments'."); + Logger.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'FetchQualifiedSegments'."); return null; } @@ -1381,8 +1380,7 @@ internal void IdentifyUser(string userId) /// Dictionary for identifiers. The caller must provide at least one key-value pair. /// Type of event (defaults to `fullstack`) /// Optional event data in a key-value pair format - public void SendOdpEvent(string action, Dictionary identifiers, - string type = Constants.ODP_EVENT_TYPE, + public void SendOdpEvent(string action, Dictionary identifiers, string type = Constants.ODP_EVENT_TYPE, Dictionary data = null ) { From b43e10f03c4619ec0237cdaf504670b5b8159720 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 4 Oct 2024 15:05:51 -0400 Subject: [PATCH 07/24] refactor: SaveToUserProfileService for simplicity --- OptimizelySDK/Bucketing/DecisionService.cs | 38 +++++++++------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index 1a374475..5391b54a 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -518,36 +518,28 @@ private void SaveToUserProfileService(Experiment experiment = null, Variation variation = null ) { - var useSpecificLogEntry = experiment != null && variation != null && - !string.IsNullOrEmpty(_userProfile?.UserId); - + var hasExperimentDetails = experiment != null && variation != null && + !string.IsNullOrEmpty(_userProfile.UserId); try { - if (_userProfile != null) + if (_userProfile == null) { - UserProfileService.Save(_userProfile.ToMap()); - if (useSpecificLogEntry) - { - Logger.Log(LogLevel.INFO, - $"Saved variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{_userProfile.UserId}\"."); - } - else - { - Logger.Log(LogLevel.INFO, "Saved user profile after batch decision."); - } + return; } + + UserProfileService.Save(_userProfile.ToMap()); + + Logger.Log(LogLevel.INFO, + hasExperimentDetails ? + $"Saved variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{_userProfile.UserId}\"." : + "Saved user profile after batch decision."); } catch (Exception exception) { - if (useSpecificLogEntry) - { - Logger.Log(LogLevel.ERROR, - $"Failed to save variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{_userProfile.UserId}\"."); - } - else - { - Logger.Log(LogLevel.ERROR, "Failed to save user profile after batch decision."); - } + Logger.Log(LogLevel.ERROR, + hasExperimentDetails ? + $"Failed to save variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{_userProfile.UserId}\"." : + "Failed to save user profile after batch decision."); ErrorHandler.HandleError( new Exceptions.OptimizelyRuntimeException(exception.Message)); From 0dd1b744df60f74f24558158eeabb515c13fadbb Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 4 Oct 2024 15:08:24 -0400 Subject: [PATCH 08/24] revert: formatting --- OptimizelySDK/Optimizely.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/OptimizelySDK/Optimizely.cs b/OptimizelySDK/Optimizely.cs index 3a517174..4f6d42d5 100644 --- a/OptimizelySDK/Optimizely.cs +++ b/OptimizelySDK/Optimizely.cs @@ -268,7 +268,8 @@ private void InitializeComponents(IEventDispatcher eventDispatcher = null, Logger); DefaultDecideOptions = defaultDecideOptions ?? new OptimizelyDecideOption[] { }; #if USE_ODP - OdpManager = odpManager ?? new OdpManager.Builder().WithErrorHandler(errorHandler) + OdpManager = odpManager ?? new OdpManager.Builder() + .WithErrorHandler(errorHandler) .WithLogger(logger) .Build(); #endif From 2b17438f535b9d97b7e4c14fab9845925af3f334 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Mon, 7 Oct 2024 09:53:29 -0400 Subject: [PATCH 09/24] bug: only SaveToUserProfileService if batch is finishing _decisionBatchInProgress from true to false --- OptimizelySDK/Bucketing/DecisionService.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index 5391b54a..a81b27ef 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -49,14 +49,14 @@ public class DecisionService public bool DecisionBatchInProgress { - get => _decisionBatchInProgress; set { - _decisionBatchInProgress = value; - if (!_decisionBatchInProgress) + // Only save if the value is changing from true to false + if (_decisionBatchInProgress && !value) { SaveToUserProfileService(); } + _decisionBatchInProgress = value; } } From c9047a0eae6e7b7609c818972262ecd7e20b8dc7 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Mon, 7 Oct 2024 17:51:59 -0400 Subject: [PATCH 10/24] test: WIP add coverage for single `Lookup` & `Save` via USP I'm hitting `Lookup` thrice for some reason in DecideAllWithUspShouldOnlyLookupSaveOnce --- .../OptimizelyUserContextTest.cs | 60 +++++++++++++++++-- 1 file changed, 55 insertions(+), 5 deletions(-) diff --git a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs index 49fd91a4..01842853 100644 --- a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs +++ b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs @@ -1,5 +1,5 @@ /* - * Copyright 2020-2021, 2022-2023 Optimizely and contributors + * Copyright 2020-2024 Optimizely and contributors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,7 +16,6 @@ using System; using System.Collections.Generic; -using System.Threading; using Castle.Core.Internal; using Moq; using NUnit.Framework; @@ -193,7 +192,7 @@ public void SetAttributeToOverrideAttribute() Assert.AreEqual(user.GetAttributes()["k1"], true); } - #region decide + #region Decide [Test] public void TestDecide() @@ -408,11 +407,43 @@ public void DecideWhenConfigIsNull() Assert.IsTrue(TestData.CompareObjects(decision, decisionExpected)); } + + [Test] + public void DecideWithUspShouldOnlyLookupSaveOnce() + { + var flagKeyFromTestDataJson = "double_single_variable_feature"; + var userProfileServiceMock = new Mock(); + var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object, + LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object); + var user = optimizely.CreateUserContext(UserID); + + _ = user.Decide(flagKeyFromTestDataJson); + + userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once); + userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), + Times.Once); + } - #endregion decide + #endregion Decide - #region decideAll + #region DecideForKeys + [Test] + public void DecideForKeysWithUspShouldOnlyLookupSaveOnceWithMultipleFlags() + { + var flagKeys = new [] { "double_single_variable_feature", "boolean_feature" }; + var userProfileServiceMock = new Mock(); + var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object, + LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object); + var userContext = optimizely.CreateUserContext(UserID); + + _ = userContext.DecideForKeys(flagKeys); + + userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once); + userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), + Times.Once); + } + [Test] public void DecideForKeysWithOneFlag() { @@ -442,6 +473,25 @@ public void DecideForKeysWithOneFlag() new string[0]); Assert.IsTrue(TestData.CompareObjects(decision, expDecision)); } + + #endregion DecideForKeys + + #region DecideAll + + [Test] + public void DecideAllWithUspShouldOnlyLookupSaveOnce() + { + var userProfileServiceMock = new Mock(); + var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object, + LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object); + var user = optimizely.CreateUserContext(UserID); + + _ = user.DecideAll(); + + userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once); + userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), + Times.Once); + } [Test] public void DecideAllTwoFlag() From dfe13678fdebd9d10cc64a516d9ec7504173ac92 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Tue, 8 Oct 2024 15:15:02 -0400 Subject: [PATCH 11/24] refactor: less movement of code as the ref implementation --- OptimizelySDK/Bucketing/DecisionService.cs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index a81b27ef..266b8f83 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -56,6 +56,7 @@ public bool DecisionBatchInProgress { SaveToUserProfileService(); } + _decisionBatchInProgress = value; } } @@ -168,6 +169,13 @@ OptimizelyDecideOption[] options UserProfileUtil.IsValidUserProfileMap(userProfileMap)) { _userProfile = UserProfileUtil.ConvertMapToUserProfile(userProfileMap); + decisionVariationResult = + GetStoredVariation(experiment, _userProfile, config); + reasons += decisionVariationResult.DecisionReasons; + if (decisionVariationResult.ResultObject != null) + { + return decisionVariationResult.SetReasons(reasons); + } } else if (userProfileMap == null) { @@ -181,17 +189,6 @@ OptimizelyDecideOption[] options reasons.AddInfo("The UserProfileService returned an invalid map.")); } } - - if (_userProfile != null) - { - decisionVariationResult = - GetStoredVariation(experiment, _userProfile, config); - reasons += decisionVariationResult.DecisionReasons; - if (decisionVariationResult.ResultObject != null) - { - return decisionVariationResult.SetReasons(reasons); - } - } } catch (Exception exception) { From 57c1ccdb4eecfedcb61b97c89efcc767d62920b2 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Tue, 8 Oct 2024 15:17:16 -0400 Subject: [PATCH 12/24] Revert "refactor: less movement of code as the ref implementation" This reverts commit dfe13678fdebd9d10cc64a516d9ec7504173ac92. --- OptimizelySDK/Bucketing/DecisionService.cs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index 266b8f83..a81b27ef 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -56,7 +56,6 @@ public bool DecisionBatchInProgress { SaveToUserProfileService(); } - _decisionBatchInProgress = value; } } @@ -169,13 +168,6 @@ OptimizelyDecideOption[] options UserProfileUtil.IsValidUserProfileMap(userProfileMap)) { _userProfile = UserProfileUtil.ConvertMapToUserProfile(userProfileMap); - decisionVariationResult = - GetStoredVariation(experiment, _userProfile, config); - reasons += decisionVariationResult.DecisionReasons; - if (decisionVariationResult.ResultObject != null) - { - return decisionVariationResult.SetReasons(reasons); - } } else if (userProfileMap == null) { @@ -189,6 +181,17 @@ OptimizelyDecideOption[] options reasons.AddInfo("The UserProfileService returned an invalid map.")); } } + + if (_userProfile != null) + { + decisionVariationResult = + GetStoredVariation(experiment, _userProfile, config); + reasons += decisionVariationResult.DecisionReasons; + if (decisionVariationResult.ResultObject != null) + { + return decisionVariationResult.SetReasons(reasons); + } + } } catch (Exception exception) { From 9ca271cf2a7fc8f1f3606da2f77108fe6385bbbb Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Tue, 8 Oct 2024 16:03:31 -0400 Subject: [PATCH 13/24] test: finish coverage --- .../OptimizelyUserContextTest.cs | 67 ++++++++++++++----- OptimizelySDK/Bucketing/DecisionService.cs | 2 +- 2 files changed, 53 insertions(+), 16 deletions(-) diff --git a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs index 01842853..e88e6dc3 100644 --- a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs +++ b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs @@ -61,6 +61,22 @@ public void SetUp() LoggerMock.Object, ErrorHandlerMock.Object); } + private Mock makeUserProfileServiceMock() + { + var projectConfig = DatafileProjectConfig.Create(TestData.Datafile, LoggerMock.Object, + ErrorHandlerMock.Object); + var experiment = projectConfig.Experiments[8]; + var variation = experiment.Variations[0]; + var decision = new Decision(variation.Id); + var userProfile = new UserProfile(UserID, new Dictionary + { + { experiment.Id, decision }, + }); + var userProfileServiceMock = new Mock(); + userProfileServiceMock.Setup(up => up.Lookup(UserID)).Returns(userProfile.ToMap()); + return userProfileServiceMock; + } + [Test] public void OptimizelyUserContextWithAttributes() { @@ -407,18 +423,25 @@ public void DecideWhenConfigIsNull() Assert.IsTrue(TestData.CompareObjects(decision, decisionExpected)); } - + [Test] public void DecideWithUspShouldOnlyLookupSaveOnce() { var flagKeyFromTestDataJson = "double_single_variable_feature"; - var userProfileServiceMock = new Mock(); + var userProfileServiceMock = makeUserProfileServiceMock(); var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object, LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object); var user = optimizely.CreateUserContext(UserID); - + _ = user.Decide(flagKeyFromTestDataJson); - + + LoggerMock.Verify( + l => l.Log(LogLevel.INFO, + "We were unable to get a user profile map from the UserProfileService."), + Times.Never); + LoggerMock.Verify( + l => l.Log(LogLevel.ERROR, "The UserProfileService returned an invalid map."), + Times.Never); userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once); userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), Times.Once); @@ -431,19 +454,26 @@ public void DecideWithUspShouldOnlyLookupSaveOnce() [Test] public void DecideForKeysWithUspShouldOnlyLookupSaveOnceWithMultipleFlags() { - var flagKeys = new [] { "double_single_variable_feature", "boolean_feature" }; - var userProfileServiceMock = new Mock(); + var flagKeys = new[] { "double_single_variable_feature", "boolean_feature" }; + var userProfileServiceMock = makeUserProfileServiceMock(); var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object, LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object); var userContext = optimizely.CreateUserContext(UserID); - + _ = userContext.DecideForKeys(flagKeys); - + + LoggerMock.Verify( + l => l.Log(LogLevel.INFO, + "We were unable to get a user profile map from the UserProfileService."), + Times.Never); + LoggerMock.Verify( + l => l.Log(LogLevel.ERROR, "The UserProfileService returned an invalid map."), + Times.Never); userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once); userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), Times.Once); } - + [Test] public void DecideForKeysWithOneFlag() { @@ -473,21 +503,28 @@ public void DecideForKeysWithOneFlag() new string[0]); Assert.IsTrue(TestData.CompareObjects(decision, expDecision)); } - + #endregion DecideForKeys - + #region DecideAll - + [Test] public void DecideAllWithUspShouldOnlyLookupSaveOnce() { - var userProfileServiceMock = new Mock(); + var userProfileServiceMock = makeUserProfileServiceMock(); var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object, LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object); var user = optimizely.CreateUserContext(UserID); - + _ = user.DecideAll(); - + + LoggerMock.Verify( + l => l.Log(LogLevel.INFO, + "We were unable to get a user profile map from the UserProfileService."), + Times.Never); + LoggerMock.Verify( + l => l.Log(LogLevel.ERROR, "The UserProfileService returned an invalid map."), + Times.Never); userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once); userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), Times.Once); diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index a81b27ef..c0d0d6c4 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -45,7 +45,7 @@ public class DecisionService private ILogger Logger; private UserProfile _userProfile; - private bool _decisionBatchInProgress = false; + private bool _decisionBatchInProgress; public bool DecisionBatchInProgress { From c97f36e7e5f81e9ec8b215e2e443dcc824a09347 Mon Sep 17 00:00:00 2001 From: Mike Chu <104384559+mikechu-optimizely@users.noreply.github.com> Date: Mon, 14 Oct 2024 17:40:28 -0400 Subject: [PATCH 14/24] Update OptimizelySDK.Tests/OptimizelyUserContextTest.cs Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com> --- OptimizelySDK.Tests/OptimizelyUserContextTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs index e88e6dc3..48412aee 100644 --- a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs +++ b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs @@ -425,7 +425,7 @@ public void DecideWhenConfigIsNull() } [Test] - public void DecideWithUspShouldOnlyLookupSaveOnce() + public void DecideWithUpsShouldOnlyLookupSaveOnce() { var flagKeyFromTestDataJson = "double_single_variable_feature"; var userProfileServiceMock = makeUserProfileServiceMock(); From 5b7e5aeceea330488ce32bf6a0764753a2c64364 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Wed, 16 Oct 2024 17:40:36 -0400 Subject: [PATCH 15/24] fix: reset _userProfile after batch --- OptimizelySDK/Bucketing/DecisionService.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index c0d0d6c4..9aa888cd 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -55,6 +55,7 @@ public bool DecisionBatchInProgress if (_decisionBatchInProgress && !value) { SaveToUserProfileService(); + _userProfile = null; } _decisionBatchInProgress = value; } From c6889e82854d9c7a1a8643a7235ddae262a73f7f Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Wed, 16 Oct 2024 17:41:27 -0400 Subject: [PATCH 16/24] test: add content verification of UPS Save() --- .../OptimizelyUserContextTest.cs | 46 +++++++++++++++---- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs index e88e6dc3..afea5b01 100644 --- a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs +++ b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs @@ -16,6 +16,7 @@ using System; using System.Collections.Generic; +using System.Linq; using Castle.Core.Internal; using Moq; using NUnit.Framework; @@ -61,7 +62,7 @@ public void SetUp() LoggerMock.Object, ErrorHandlerMock.Object); } - private Mock makeUserProfileServiceMock() + private Mock MakeUserProfileServiceMock() { var projectConfig = DatafileProjectConfig.Create(TestData.Datafile, LoggerMock.Object, ErrorHandlerMock.Object); @@ -425,13 +426,20 @@ public void DecideWhenConfigIsNull() } [Test] - public void DecideWithUspShouldOnlyLookupSaveOnce() + public void DecideWithUpsShouldOnlyLookupSaveOnce() { var flagKeyFromTestDataJson = "double_single_variable_feature"; - var userProfileServiceMock = makeUserProfileServiceMock(); + var userProfileServiceMock = MakeUserProfileServiceMock(); + var saveArgsCollector = new List>(); + userProfileServiceMock.Setup(up => up.Save(Capture.In(saveArgsCollector))); var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object, LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object); var user = optimizely.CreateUserContext(UserID); + var expectedUserProfile = new UserProfile(UserID, new Dictionary + { + { "224", new Decision("280") }, + { "122238", new Decision("122240") }, + }); _ = user.Decide(flagKeyFromTestDataJson); @@ -445,6 +453,7 @@ public void DecideWithUspShouldOnlyLookupSaveOnce() userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once); userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), Times.Once); + Assert.AreEqual(saveArgsCollector.First(), expectedUserProfile.ToMap()); } #endregion Decide @@ -452,13 +461,20 @@ public void DecideWithUspShouldOnlyLookupSaveOnce() #region DecideForKeys [Test] - public void DecideForKeysWithUspShouldOnlyLookupSaveOnceWithMultipleFlags() + public void DecideForKeysWithUpsShouldOnlyLookupSaveOnceWithMultipleFlags() { var flagKeys = new[] { "double_single_variable_feature", "boolean_feature" }; - var userProfileServiceMock = makeUserProfileServiceMock(); + var userProfileServiceMock = MakeUserProfileServiceMock(); + var saveArgsCollector = new List>(); + userProfileServiceMock.Setup(up => up.Save(Capture.In(saveArgsCollector))); var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object, LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object); var userContext = optimizely.CreateUserContext(UserID); + var expectedUserProfile = new UserProfile(UserID, new Dictionary + { + { "224", new Decision("280") }, + { "122238", new Decision("122240") }, + }); _ = userContext.DecideForKeys(flagKeys); @@ -472,6 +488,7 @@ public void DecideForKeysWithUspShouldOnlyLookupSaveOnceWithMultipleFlags() userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once); userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), Times.Once); + Assert.AreEqual(saveArgsCollector.First(), expectedUserProfile.ToMap()); } [Test] @@ -507,14 +524,23 @@ public void DecideForKeysWithOneFlag() #endregion DecideForKeys #region DecideAll - + [Test] - public void DecideAllWithUspShouldOnlyLookupSaveOnce() + public void DecideAllWithUpsShouldOnlyLookupSaveOnce() { - var userProfileServiceMock = makeUserProfileServiceMock(); + var userProfileServiceMock = MakeUserProfileServiceMock(); + var saveArgsCollector = new List>(); + userProfileServiceMock.Setup(up => up.Save(Capture.In(saveArgsCollector))); var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object, LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object); var user = optimizely.CreateUserContext(UserID); + var expectedUserProfile = new UserProfile(UserID, new Dictionary + { + { "224", new Decision("280") }, + { "122238", new Decision("122240") }, + { "122241", new Decision("122242") }, + { "122235", new Decision("122236") }, + }); _ = user.DecideAll(); @@ -526,8 +552,8 @@ public void DecideAllWithUspShouldOnlyLookupSaveOnce() l => l.Log(LogLevel.ERROR, "The UserProfileService returned an invalid map."), Times.Never); userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once); - userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), - Times.Once); + userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), Times.Once); + Assert.AreEqual(saveArgsCollector.First(), expectedUserProfile.ToMap()); } [Test] From 54a2dc2a7a9a7623b348ffd75cd2853a9371f684 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 17 Oct 2024 09:58:06 -0400 Subject: [PATCH 17/24] ci: stop CI run if Draft PR = save GH runner costs --- .github/workflows/csharp.yml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/csharp.yml b/.github/workflows/csharp.yml index 4303e0e2..86552461 100644 --- a/.github/workflows/csharp.yml +++ b/.github/workflows/csharp.yml @@ -7,9 +7,19 @@ on: branches: [ master ] jobs: + failOnDraftPullRequest: + if: github.event.pull_request.draft == true + runs-on: ubuntu-latest + steps: + - name: Fails Draft pull request. + run: | + echo "Draft pull requests are not allowed. Please mark the pull request as ready for review." + exit 1 + lintCodebase: + name: Lint Codebase + needs: [ failOnDraftPullRequest ] runs-on: ubuntu-latest - name: Lint Codebase steps: - name: Checkout code uses: actions/checkout@v3 From 9f2f471f2458cd2e1dc67c05fdd78ba917c6e99b Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 17 Oct 2024 14:05:08 -0400 Subject: [PATCH 18/24] feat: WIP better way to handle loading & saving for UPS I need more time to fix tests --- .../OptimizelyUserContextTest.cs | 1 + OptimizelySDK/Bucketing/DecisionService.cs | 140 ++++++------------ OptimizelySDK/Bucketing/DecisionUnitOfWork.cs | 48 ++++++ OptimizelySDK/Bucketing/UserProfileCache.cs | 64 ++++++++ OptimizelySDK/Optimizely.cs | 23 ++- OptimizelySDK/OptimizelySDK.csproj | 2 + 6 files changed, 181 insertions(+), 97 deletions(-) create mode 100644 OptimizelySDK/Bucketing/DecisionUnitOfWork.cs create mode 100644 OptimizelySDK/Bucketing/UserProfileCache.cs diff --git a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs index afea5b01..73551cdc 100644 --- a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs +++ b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs @@ -540,6 +540,7 @@ public void DecideAllWithUpsShouldOnlyLookupSaveOnce() { "122238", new Decision("122240") }, { "122241", new Decision("122242") }, { "122235", new Decision("122236") }, + { "188880", new Decision("188881") }, }); _ = user.DecideAll(); diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index 9aa888cd..c133ec20 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -43,23 +43,8 @@ public class DecisionService private IErrorHandler ErrorHandler; private UserProfileService UserProfileService; private ILogger Logger; - private UserProfile _userProfile; - - private bool _decisionBatchInProgress; - - public bool DecisionBatchInProgress - { - set - { - // Only save if the value is changing from true to false - if (_decisionBatchInProgress && !value) - { - SaveToUserProfileService(); - _userProfile = null; - } - _decisionBatchInProgress = value; - } - } + private readonly DecisionUnitOfWork _decisionUnitOfWork; + private readonly UserProfileCache _userProfileCache; /// /// Associative array of user IDs to an associative array @@ -89,6 +74,8 @@ public DecisionService(Bucketer bucketer, IErrorHandler errorHandler, ErrorHandler = errorHandler; UserProfileService = userProfileService; Logger = logger; + _decisionUnitOfWork = new DecisionUnitOfWork(); + _userProfileCache = new UserProfileCache(userProfileService, logger); #if NET35 ForcedVariationMap = new Dictionary>(); #else @@ -157,41 +144,17 @@ OptimizelyDecideOption[] options var ignoreUPS = Array.Exists(options, option => option == OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE); - + UserProfile userProfile = null; if (!ignoreUPS && UserProfileService != null) { try { - if (_userProfile == null) - { - var userProfileMap = UserProfileService.Lookup(user.GetUserId()); - if (userProfileMap != null && - UserProfileUtil.IsValidUserProfileMap(userProfileMap)) - { - _userProfile = UserProfileUtil.ConvertMapToUserProfile(userProfileMap); - } - else if (userProfileMap == null) - { - Logger.Log(LogLevel.INFO, - reasons.AddInfo( - "We were unable to get a user profile map from the UserProfileService.")); - } - else - { - Logger.Log(LogLevel.ERROR, - reasons.AddInfo("The UserProfileService returned an invalid map.")); - } - } - - if (_userProfile != null) + userProfile = _userProfileCache.GetUserProfile(userId); + decisionVariationResult = GetStoredVariation(experiment, userProfile, config); + reasons += decisionVariationResult.DecisionReasons; + if (decisionVariationResult.ResultObject != null) { - decisionVariationResult = - GetStoredVariation(experiment, _userProfile, config); - reasons += decisionVariationResult.DecisionReasons; - if (decisionVariationResult.ResultObject != null) - { - return decisionVariationResult.SetReasons(reasons); - } + return decisionVariationResult.SetReasons(reasons); } } catch (Exception exception) @@ -221,7 +184,7 @@ OptimizelyDecideOption[] options { if (UserProfileService != null && !ignoreUPS) { - var bucketerUserProfile = _userProfile ?? + var bucketerUserProfile = userProfile ?? new UserProfile(userId, new Dictionary()); SaveVariation(experiment, decisionVariationResult.ResultObject, @@ -491,60 +454,55 @@ UserProfile userProfile return; } - if (_userProfile == null) - { - _userProfile = userProfile; - } + var decision = new Decision(variation.Id); + _decisionUnitOfWork.AddDecision(userProfile.UserId, experiment.Id, decision); - Decision decision; - if (_userProfile.ExperimentBucketMap.ContainsKey(experiment.Id)) - { - decision = _userProfile.ExperimentBucketMap[experiment.Id]; - decision.VariationId = variation.Id; - } - else - { - decision = new Decision(variation.Id); - } - - _userProfile.ExperimentBucketMap[experiment.Id] = decision; + var cachedProfile = _userProfileCache.GetUserProfile(userProfile.UserId); + cachedProfile.ExperimentBucketMap[experiment.Id] = decision; + } + + public void AddDecisionToUnitOfWork(string userId, string experimentId, Decision decision) + { + _decisionUnitOfWork.AddDecision(userId, experimentId, decision); + } - if (!_decisionBatchInProgress) + /// + /// Commits the decisions to the user profile service. + /// + public void CommitDecisionsToUserProfileService() + { + if (UserProfileService == null || !_decisionUnitOfWork.HasDecisions) { - SaveToUserProfileService(experiment, variation); + return; } - } - private void SaveToUserProfileService(Experiment experiment = null, - Variation variation = null - ) - { - var hasExperimentDetails = experiment != null && variation != null && - !string.IsNullOrEmpty(_userProfile.UserId); - try + var decisions = _decisionUnitOfWork.GetDecisions(); + foreach (var userDecisions in decisions) { - if (_userProfile == null) + var userId = userDecisions.Key; + var experimentDecisions = userDecisions.Value; + + var userProfile = _userProfileCache.GetUserProfile(userId); + foreach (var experimentDecision in experimentDecisions) { - return; + userProfile.ExperimentBucketMap[experimentDecision.Key] = + experimentDecision.Value; } - UserProfileService.Save(_userProfile.ToMap()); - - Logger.Log(LogLevel.INFO, - hasExperimentDetails ? - $"Saved variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{_userProfile.UserId}\"." : - "Saved user profile after batch decision."); + try + { + UserProfileService.Save(userProfile.ToMap()); + Logger.Log(LogLevel.INFO, $"Saved decisions for user \"{userId}\"."); + } + catch (Exception exception) + { + Logger.Log(LogLevel.ERROR, $"Failed to save decisions for user \"{userId}\"."); + ErrorHandler.HandleError( + new Exceptions.OptimizelyRuntimeException(exception.Message)); + } } - catch (Exception exception) - { - Logger.Log(LogLevel.ERROR, - hasExperimentDetails ? - $"Failed to save variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{_userProfile.UserId}\"." : - "Failed to save user profile after batch decision."); - ErrorHandler.HandleError( - new Exceptions.OptimizelyRuntimeException(exception.Message)); - } + _decisionUnitOfWork.ClearDecisions(); } /// diff --git a/OptimizelySDK/Bucketing/DecisionUnitOfWork.cs b/OptimizelySDK/Bucketing/DecisionUnitOfWork.cs new file mode 100644 index 00000000..ecbf809d --- /dev/null +++ b/OptimizelySDK/Bucketing/DecisionUnitOfWork.cs @@ -0,0 +1,48 @@ +/* + * Copyright 2024 Optimizely + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +using System.Collections.Generic; + +namespace OptimizelySDK.Bucketing +{ + public class DecisionUnitOfWork + { + private readonly Dictionary> _decisions = + new Dictionary>(); + + public void AddDecision(string userId, string experimentId, Decision decision) + { + if (!_decisions.ContainsKey(userId)) + { + _decisions[userId] = new Dictionary(); + } + + _decisions[userId][experimentId] = decision; + } + + public Dictionary> GetDecisions() + { + return _decisions; + } + + public bool HasDecisions => _decisions.Count > 0; + + public void ClearDecisions() + { + _decisions.Clear(); + } + } +} diff --git a/OptimizelySDK/Bucketing/UserProfileCache.cs b/OptimizelySDK/Bucketing/UserProfileCache.cs new file mode 100644 index 00000000..7611e8da --- /dev/null +++ b/OptimizelySDK/Bucketing/UserProfileCache.cs @@ -0,0 +1,64 @@ +/* + * Copyright 2024 Optimizely + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +using System.Collections.Generic; +using OptimizelySDK.Logger; + +namespace OptimizelySDK.Bucketing +{ + public class UserProfileCache + { + private readonly Dictionary _cache = + new Dictionary(); + + private readonly UserProfileService _userProfileService; + private readonly ILogger _logger; + + public UserProfileCache(UserProfileService userProfileService, ILogger logger) + { + _userProfileService = userProfileService; + _logger = logger; + } + + public UserProfile GetUserProfile(string userId) + { + if (_cache.TryGetValue(userId, out var userProfile)) + { + return _cache[userId]; + } + + var userProfileMap = _userProfileService.Lookup(userId); + if (userProfileMap != null && UserProfileUtil.IsValidUserProfileMap(userProfileMap)) + { + userProfile = UserProfileUtil.ConvertMapToUserProfile(userProfileMap); + } + else if (userProfileMap == null) + { + _logger.Log(LogLevel.INFO, + "We were unable to get a user profile map from the UserProfileService."); + } + else + { + _logger.Log(LogLevel.ERROR, "The UserProfileService returned an invalid map."); + } + + _cache[userId] = userProfile ?? + new UserProfile(userId, new Dictionary()); + + return _cache[userId]; + } + } +} diff --git a/OptimizelySDK/Optimizely.cs b/OptimizelySDK/Optimizely.cs index 4f6d42d5..ddfc1f2e 100644 --- a/OptimizelySDK/Optimizely.cs +++ b/OptimizelySDK/Optimizely.cs @@ -861,12 +861,15 @@ private OptimizelyUserContext CreateUserContextCopy(string userId, ///
  • If the SDK finds an error, it’ll return a decision with null for variationKey. The decision will include an error message in reasons. /// ///
    • + /// User context. /// A flag key for which a decision will be made. /// A list of options for decision-making. + /// Whether to commit decisions to the user profile service (if in use) immediately. /// A decision result. internal OptimizelyDecision Decide(OptimizelyUserContext user, string key, - OptimizelyDecideOption[] options + OptimizelyDecideOption[] options, + bool commitImmediately = true ) { var config = ProjectConfigManager?.GetConfig(); @@ -929,6 +932,8 @@ OptimizelyDecideOption[] options if (decision?.Variation != null) { featureEnabled = decision.Variation.FeatureEnabled.GetValueOrDefault(); + DecisionService.AddDecisionToUnitOfWork(userId, decision.Experiment?.Id, + new Decision(decision.Variation.Id)); } if (featureEnabled) @@ -1004,7 +1009,7 @@ OptimizelyDecideOption[] options DecisionNotificationTypes.FLAG, userId, userAttributes ?? new UserAttributes(), decisionInfo); - return new OptimizelyDecision( + var result = new OptimizelyDecision( variationKey, featureEnabled, optimizelyJSON, @@ -1012,6 +1017,13 @@ OptimizelyDecideOption[] options key, user, reasonsToReport); + + if (commitImmediately) + { + DecisionService.CommitDecisionsToUserProfileService(); + } + + return result; } internal Dictionary DecideAll(OptimizelyUserContext user, @@ -1056,18 +1068,17 @@ OptimizelyDecideOption[] options var allOptions = GetAllOptions(options); - DecisionService.DecisionBatchInProgress = true; foreach (var key in keys) { - var decision = Decide(user, key, options); + var decision = Decide(user, key, options, false); if (!allOptions.Contains(OptimizelyDecideOption.ENABLED_FLAGS_ONLY) || decision.Enabled) { decisionDictionary.Add(key, decision); } } - - DecisionService.DecisionBatchInProgress = false; + + DecisionService.CommitDecisionsToUserProfileService(); return decisionDictionary; } diff --git a/OptimizelySDK/OptimizelySDK.csproj b/OptimizelySDK/OptimizelySDK.csproj index 1812a2ad..024eb3f7 100644 --- a/OptimizelySDK/OptimizelySDK.csproj +++ b/OptimizelySDK/OptimizelySDK.csproj @@ -75,6 +75,8 @@ + + From 73658f6018bbdd59eb2c2c2761e536fe22aeedc6 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 17 Oct 2024 14:10:42 -0400 Subject: [PATCH 19/24] ci: better error echoed --- .github/workflows/csharp.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/csharp.yml b/.github/workflows/csharp.yml index 86552461..4ad1218b 100644 --- a/.github/workflows/csharp.yml +++ b/.github/workflows/csharp.yml @@ -13,7 +13,7 @@ jobs: steps: - name: Fails Draft pull request. run: | - echo "Draft pull requests are not allowed. Please mark the pull request as ready for review." + echo "Draft pull requests should not trigger CI. Please mark the pull request as Ready for review to continue." exit 1 lintCodebase: From 80e73d4e4f7b5d4e7bb7018f8cf99777ea5bc021 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 17 Oct 2024 17:29:49 -0400 Subject: [PATCH 20/24] test: add coverage --- .../OptimizelyUserContextTest.cs | 52 +++++++++++++++++-- OptimizelySDK/Optimizely.cs | 8 +-- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs index 73551cdc..c4558839 100644 --- a/OptimizelySDK.Tests/OptimizelyUserContextTest.cs +++ b/OptimizelySDK.Tests/OptimizelyUserContextTest.cs @@ -425,6 +425,52 @@ public void DecideWhenConfigIsNull() Assert.IsTrue(TestData.CompareObjects(decision, decisionExpected)); } + [Test] + public void SeparateDecideShouldHaveSameNumberOfUpsSaveOnlyOneLookup() + { + var experimentFlagKeys = new[] { "double_single_variable_feature", "integer_single_variable_feature" }; + // var rolloutFlagKey = "boolean_single_variable_feature"; + var userProfileServiceMock = MakeUserProfileServiceMock(); + var saveArgsCollector = new List>(); + userProfileServiceMock.Setup(up => up.Save(Capture.In(saveArgsCollector))); + var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object, + LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object); + var user = optimizely.CreateUserContext(UserID); + var expectedUserProfileExperiment1 = new UserProfile(UserID, new Dictionary + { + { "224", new Decision("280") }, + { "122238", new Decision("122240") }, + }); + var expectedUserProfileExperiment2 = new UserProfile(UserID, new Dictionary + { + { "224", new Decision("280") }, + { "122238", new Decision("122240") }, + { "122241", new Decision("122242") }, + }); + // var expectedUserProfileRollout = new UserProfile(UserID, new Dictionary + // { + // // expected decisions for rollout + // }); + + user.Decide(experimentFlagKeys[0]); + user.Decide(experimentFlagKeys[1]); + // user.Decide(rolloutFlagKey); // TODO: Test UPS with rollout? + + LoggerMock.Verify( + l => l.Log(LogLevel.INFO, + "We were unable to get a user profile map from the UserProfileService."), + Times.Never); + LoggerMock.Verify( + l => l.Log(LogLevel.ERROR, "The UserProfileService returned an invalid map."), + Times.Never); + userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once); + userProfileServiceMock.Verify(l => l.Save(It.IsAny>()), + Times.Exactly(2)); + Assert.AreEqual(saveArgsCollector[0], expectedUserProfileExperiment1.ToMap()); + Assert.AreEqual(saveArgsCollector[1], expectedUserProfileExperiment2.ToMap()); + // Assert.AreEqual(saveArgsCollector[2], expectedUserProfileRollout.ToMap()); + } + [Test] public void DecideWithUpsShouldOnlyLookupSaveOnce() { @@ -441,7 +487,7 @@ public void DecideWithUpsShouldOnlyLookupSaveOnce() { "122238", new Decision("122240") }, }); - _ = user.Decide(flagKeyFromTestDataJson); + user.Decide(flagKeyFromTestDataJson); LoggerMock.Verify( l => l.Log(LogLevel.INFO, @@ -476,7 +522,7 @@ public void DecideForKeysWithUpsShouldOnlyLookupSaveOnceWithMultipleFlags() { "122238", new Decision("122240") }, }); - _ = userContext.DecideForKeys(flagKeys); + userContext.DecideForKeys(flagKeys); LoggerMock.Verify( l => l.Log(LogLevel.INFO, @@ -543,7 +589,7 @@ public void DecideAllWithUpsShouldOnlyLookupSaveOnce() { "188880", new Decision("188881") }, }); - _ = user.DecideAll(); + user.DecideAll(); LoggerMock.Verify( l => l.Log(LogLevel.INFO, diff --git a/OptimizelySDK/Optimizely.cs b/OptimizelySDK/Optimizely.cs index ddfc1f2e..1d5d314a 100644 --- a/OptimizelySDK/Optimizely.cs +++ b/OptimizelySDK/Optimizely.cs @@ -927,13 +927,15 @@ internal OptimizelyDecision Decide(OptimizelyUserContext user, decision = flagDecisionResult.ResultObject; } + // TODO: Fix when the flag is a rollout instead of an experiment + DecisionService.AddDecisionToUnitOfWork(userId, decision.Experiment?.Id, + new Decision(decision.Variation?.Id ?? "")); + var featureEnabled = false; if (decision?.Variation != null) { featureEnabled = decision.Variation.FeatureEnabled.GetValueOrDefault(); - DecisionService.AddDecisionToUnitOfWork(userId, decision.Experiment?.Id, - new Decision(decision.Variation.Id)); } if (featureEnabled) @@ -1077,7 +1079,7 @@ OptimizelyDecideOption[] options decisionDictionary.Add(key, decision); } } - + DecisionService.CommitDecisionsToUserProfileService(); return decisionDictionary; From 236728b046488905c885f388145cff1463ab0fd5 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 17 Oct 2024 17:33:17 -0400 Subject: [PATCH 21/24] revert: doc differences --- OptimizelySDK/Bucketing/DecisionService.cs | 28 +++++++++++----------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index c133ec20..2ceaecbe 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -1,18 +1,18 @@ /* - * Copyright 2017-2022, 2024 Optimizely - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ +* Copyright 2017-2022, 2024 Optimizely +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* https://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ using System; using System.Collections.Generic; From d6258f1c42b879255d7cd98fbf8c32d42d9701da Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Thu, 17 Oct 2024 18:02:05 -0400 Subject: [PATCH 22/24] fix: WIP tests & code under tests --- OptimizelySDK/Bucketing/DecisionService.cs | 10 +++++++++- OptimizelySDK/Bucketing/DecisionUnitOfWork.cs | 5 +++++ OptimizelySDK/Optimizely.cs | 5 ++--- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index 2ceaecbe..dc2a2361 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -463,7 +463,15 @@ UserProfile userProfile public void AddDecisionToUnitOfWork(string userId, string experimentId, Decision decision) { - _decisionUnitOfWork.AddDecision(userId, experimentId, decision); + if (UserProfileService == null) + { + return; + } + + if (!string.IsNullOrEmpty(experimentId)) + { + _decisionUnitOfWork.AddDecision(userId, experimentId, decision); + } } /// diff --git a/OptimizelySDK/Bucketing/DecisionUnitOfWork.cs b/OptimizelySDK/Bucketing/DecisionUnitOfWork.cs index ecbf809d..2c4aee7e 100644 --- a/OptimizelySDK/Bucketing/DecisionUnitOfWork.cs +++ b/OptimizelySDK/Bucketing/DecisionUnitOfWork.cs @@ -25,6 +25,11 @@ public class DecisionUnitOfWork public void AddDecision(string userId, string experimentId, Decision decision) { + if (string.IsNullOrEmpty(userId) || string.IsNullOrEmpty(experimentId)) + { + return; + } + if (!_decisions.ContainsKey(userId)) { _decisions[userId] = new Dictionary(); diff --git a/OptimizelySDK/Optimizely.cs b/OptimizelySDK/Optimizely.cs index 1d5d314a..8235882a 100644 --- a/OptimizelySDK/Optimizely.cs +++ b/OptimizelySDK/Optimizely.cs @@ -926,10 +926,9 @@ internal OptimizelyDecision Decide(OptimizelyUserContext user, decisionReasons += flagDecisionResult.DecisionReasons; decision = flagDecisionResult.ResultObject; } - - // TODO: Fix when the flag is a rollout instead of an experiment + DecisionService.AddDecisionToUnitOfWork(userId, decision.Experiment?.Id, - new Decision(decision.Variation?.Id ?? "")); + new Decision(decision.Variation?.Id)); var featureEnabled = false; From 9c6e9bbb2ee207a43bdbda1dd677f45998563874 Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 18 Oct 2024 10:31:14 -0400 Subject: [PATCH 23/24] fix: for GetVariation & Activate never use cached user profile --- .github/workflows/csharp.yml | 1 + OptimizelySDK/Bucketing/DecisionService.cs | 25 +++++++-------------- OptimizelySDK/Bucketing/UserProfileCache.cs | 4 ++-- OptimizelySDK/Optimizely.cs | 15 ++++++++----- 4 files changed, 21 insertions(+), 24 deletions(-) diff --git a/.github/workflows/csharp.yml b/.github/workflows/csharp.yml index 4ad1218b..25a3fb48 100644 --- a/.github/workflows/csharp.yml +++ b/.github/workflows/csharp.yml @@ -8,6 +8,7 @@ on: jobs: failOnDraftPullRequest: + name: Fail If Draft Pull Request if: github.event.pull_request.draft == true runs-on: ubuntu-latest steps: diff --git a/OptimizelySDK/Bucketing/DecisionService.cs b/OptimizelySDK/Bucketing/DecisionService.cs index dc2a2361..60830f3f 100644 --- a/OptimizelySDK/Bucketing/DecisionService.cs +++ b/OptimizelySDK/Bucketing/DecisionService.cs @@ -85,21 +85,6 @@ public DecisionService(Bucketer bucketer, IErrorHandler errorHandler, #endif } - /// - /// Get a Variation of an Experiment for a user to be allocated into. - /// - /// The Experiment the user will be bucketed into. - /// Optimizely user context. - /// Project config. - /// The Variation the user is allocated into. - public virtual Result GetVariation(Experiment experiment, - OptimizelyUserContext user, - ProjectConfig config - ) - { - return GetVariation(experiment, user, config, new OptimizelyDecideOption[] { }); - } - /// /// Get a Variation of an Experiment for a user to be allocated into. /// @@ -107,13 +92,19 @@ ProjectConfig config /// optimizely user context. /// Project Config. /// An array of decision options. + /// Whether to force a lookup of the user profile when UPS is enabled. /// The Variation the user is allocated into. public virtual Result GetVariation(Experiment experiment, OptimizelyUserContext user, ProjectConfig config, - OptimizelyDecideOption[] options + OptimizelyDecideOption[] options = null, + bool forceUserProfileLookup = false ) { + if (options == null) + { + options = new OptimizelyDecideOption[] { }; + } var reasons = new DecisionReasons(); var userId = user.GetUserId(); if (!ExperimentUtils.IsExperimentActive(experiment, Logger)) @@ -149,7 +140,7 @@ OptimizelyDecideOption[] options { try { - userProfile = _userProfileCache.GetUserProfile(userId); + userProfile = _userProfileCache.GetUserProfile(userId, forceUserProfileLookup); decisionVariationResult = GetStoredVariation(experiment, userProfile, config); reasons += decisionVariationResult.DecisionReasons; if (decisionVariationResult.ResultObject != null) diff --git a/OptimizelySDK/Bucketing/UserProfileCache.cs b/OptimizelySDK/Bucketing/UserProfileCache.cs index 7611e8da..d34833e2 100644 --- a/OptimizelySDK/Bucketing/UserProfileCache.cs +++ b/OptimizelySDK/Bucketing/UserProfileCache.cs @@ -33,9 +33,9 @@ public UserProfileCache(UserProfileService userProfileService, ILogger logger) _logger = logger; } - public UserProfile GetUserProfile(string userId) + public UserProfile GetUserProfile(string userId, bool forceLookup = false) { - if (_cache.TryGetValue(userId, out var userProfile)) + if (_cache.TryGetValue(userId, out var userProfile) && !forceLookup) { return _cache[userId]; } diff --git a/OptimizelySDK/Optimizely.cs b/OptimizelySDK/Optimizely.cs index 8235882a..3b5c37fa 100644 --- a/OptimizelySDK/Optimizely.cs +++ b/OptimizelySDK/Optimizely.cs @@ -310,7 +310,8 @@ public Variation Activate(string experimentKey, string userId, return null; } - var variation = GetVariation(experimentKey, userId, config, userAttributes); + var variation = GetVariation(experimentKey, userId, config, userAttributes, + forceUserProfileLookup: true); if (variation == null || variation.Key == null) { @@ -404,7 +405,8 @@ public Variation GetVariation(string experimentKey, string userId, ) { var config = ProjectConfigManager?.GetConfig(); - return GetVariation(experimentKey, userId, config, userAttributes); + return GetVariation(experimentKey, userId, config, userAttributes, + forceUserProfileLookup: true); } /// @@ -414,9 +416,10 @@ public Variation GetVariation(string experimentKey, string userId, /// ID for the user /// ProjectConfig to be used for variation /// Attributes for the users + /// Whether to force a lookup of the user profile when UPS is enabled. /// null|Variation Representing variation private Variation GetVariation(string experimentKey, string userId, ProjectConfig config, - UserAttributes userAttributes = null + UserAttributes userAttributes = null, bool forceUserProfileLookup = false ) { if (config == null) @@ -442,8 +445,10 @@ private Variation GetVariation(string experimentKey, string userId, ProjectConfi userAttributes = userAttributes ?? new UserAttributes(); var userContext = CreateUserContextCopy(userId, userAttributes); - var variation = DecisionService.GetVariation(experiment, userContext, config) - ?.ResultObject; + var variation = DecisionService. + GetVariation(experiment, userContext, config, + forceUserProfileLookup: forceUserProfileLookup)?. + ResultObject; var decisionInfo = new Dictionary { { "experimentKey", experimentKey }, { "variationKey", variation?.Key }, From 49f9584f80f803c23734eac2bd6403c61c5f93ac Mon Sep 17 00:00:00 2001 From: Mike Chu Date: Fri, 18 Oct 2024 11:23:20 -0400 Subject: [PATCH 24/24] test: corrections --- OptimizelySDK.Tests/DecisionServiceTest.cs | 10 ++-- OptimizelySDK.Tests/OptimizelyTest.cs | 65 +++++++++++++--------- OptimizelySDK/Optimizely.cs | 11 ++-- 3 files changed, 50 insertions(+), 36 deletions(-) diff --git a/OptimizelySDK.Tests/DecisionServiceTest.cs b/OptimizelySDK.Tests/DecisionServiceTest.cs index 633847ae..94c47a38 100644 --- a/OptimizelySDK.Tests/DecisionServiceTest.cs +++ b/OptimizelySDK.Tests/DecisionServiceTest.cs @@ -700,7 +700,7 @@ public void TestGetVariationForFeatureExperimentGivenNonMutexGroupAndUserNotBuck DecisionServiceMock. Setup(ds => ds.GetVariation(multiVariateExp, OptimizelyUserContextMock.Object, - ProjectConfig, null)). + ProjectConfig, It.IsAny(), It.IsAny())). Returns(null); var featureFlag = ProjectConfig.GetFeatureFlagFromKey("multi_variate_feature"); @@ -736,7 +736,7 @@ public void TestGetVariationForFeatureExperimentGivenNonMutexGroupAndUserIsBucke DecisionServiceMock.Setup(ds => ds.GetVariation( ProjectConfig.GetExperimentFromKey("test_experiment_multivariate"), OptimizelyUserContextMock.Object, ProjectConfig, - It.IsAny())). + It.IsAny(), It.IsAny())). Returns(variation); var featureFlag = ProjectConfig.GetFeatureFlagFromKey("multi_variate_feature"); @@ -771,7 +771,7 @@ public void TestGetVariationForFeatureExperimentGivenMutexGroupAndUserIsBucketed DecisionServiceMock. Setup(ds => ds.GetVariation(ProjectConfig.GetExperimentFromKey("group_experiment_1"), - OptimizelyUserContextMock.Object, ProjectConfig)). + OptimizelyUserContextMock.Object, ProjectConfig, It.IsAny(), It.IsAny())). Returns(variation); var featureFlag = ProjectConfig.GetFeatureFlagFromKey("boolean_feature"); @@ -795,7 +795,7 @@ public void TestGetVariationForFeatureExperimentGivenMutexGroupAndUserNotBuckete DecisionServiceMock. Setup(ds => ds.GetVariation(It.IsAny(), It.IsAny(), ProjectConfig, - It.IsAny())). + It.IsAny(), It.IsAny())). Returns(Result.NullResult(null)); var featureFlag = ProjectConfig.GetFeatureFlagFromKey("boolean_feature"); @@ -1311,7 +1311,7 @@ public void TestGetVariationForFeatureWhenTheUserIsBuckedtedInBothExperimentAndR DecisionServiceMock.Setup(ds => ds.GetVariation(experiment, OptimizelyUserContextMock.Object, ProjectConfig, - It.IsAny())). + It.IsAny(), It.IsAny())). Returns(variation); var actualDecision = DecisionServiceMock.Object.GetVariationForFeatureExperiment( featureFlag, OptimizelyUserContextMock.Object, userAttributes, ProjectConfig, diff --git a/OptimizelySDK.Tests/OptimizelyTest.cs b/OptimizelySDK.Tests/OptimizelyTest.cs index f83304bb..f2d85fe9 100644 --- a/OptimizelySDK.Tests/OptimizelyTest.cs +++ b/OptimizelySDK.Tests/OptimizelyTest.cs @@ -1,5 +1,5 @@ /* - * Copyright 2017-2023, Optimizely + * Copyright 2017-2024, Optimizely * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -3395,8 +3395,9 @@ public void TestActivateListener(UserAttributes userAttributes) mockUserContext.Setup(ouc => ouc.GetUserId()).Returns(TestUserId); DecisionServiceMock.Setup(ds => ds.GetVariation(experiment, - It.IsAny(), It.IsAny())) - .Returns(variation); + It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny())). + Returns(variation); DecisionServiceMock.Setup(ds => ds.GetVariationForFeature(featureFlag, It.IsAny(), It.IsAny())) .Returns(decision); @@ -3509,9 +3510,10 @@ public void TestTrackListener(UserAttributes userAttributes, EventTags eventTags ErrorHandlerMock.Object, LoggerMock.Object); mockUserContext.Setup(ouc => ouc.GetUserId()).Returns(TestUserId); - DecisionServiceMock - .Setup(ds => ds.GetVariation(experiment, It.IsAny(), Config)) - .Returns(variation); + DecisionServiceMock.Setup(ds => ds.GetVariation(experiment, + It.IsAny(), Config, It.IsAny(), + It.IsAny())). + Returns(variation); // Adding notification listeners. var notificationType = NotificationCenter.NotificationType.Track; @@ -3565,9 +3567,10 @@ public void TestActivateSendsDecisionNotificationWithActualVariationKey() It.IsAny(), It.IsAny(), It.IsAny>())); - DecisionServiceMock - .Setup(ds => ds.GetVariation(experiment, It.IsAny(), Config)) - .Returns(variation); + DecisionServiceMock.Setup(ds => ds.GetVariation(experiment, + It.IsAny(), Config, It.IsAny(), + It.IsAny())). + Returns(variation); var optly = Helper.CreatePrivateOptimizely(); optly.SetFieldOrProperty("ProjectConfigManager", ConfigManager); @@ -3622,9 +3625,10 @@ public void It.IsAny(), It.IsAny(), It.IsAny>())); - DecisionServiceMock - .Setup(ds => ds.GetVariation(experiment, It.IsAny(), Config)) - .Returns(variation); + DecisionServiceMock.Setup(ds => ds.GetVariation(experiment, + It.IsAny(), Config, It.IsAny(), + It.IsAny())). + Returns(variation); var optly = Helper.CreatePrivateOptimizely(); optly.SetFieldOrProperty("ProjectConfigManager", ConfigManager); @@ -3666,8 +3670,9 @@ public void TestActivateSendsDecisionNotificationWithNullVariationKey() DecisionServiceMock.Setup(ds => ds.GetVariation(experiment, It.IsAny(), - It.IsAny(), null)) - .Returns(Result.NullResult(null)); + It.IsAny(), It.IsAny(), + It.IsAny())). + Returns(Result.NullResult(null)); optStronglyTyped.NotificationCenter.AddNotification( NotificationCenter.NotificationType.Decision, @@ -3727,9 +3732,10 @@ public void TestGetVariationSendsDecisionNotificationWithActualVariationKey() ErrorHandlerMock.Object, LoggerMock.Object); mockUserContext.Setup(ouc => ouc.GetUserId()).Returns(TestUserId); - DecisionServiceMock - .Setup(ds => ds.GetVariation(experiment, It.IsAny(), Config)) - .Returns(variation); + DecisionServiceMock.Setup(ds => ds.GetVariation(experiment, + It.IsAny(), Config, It.IsAny(), + It.IsAny())). + Returns(variation); optStronglyTyped.NotificationCenter.AddNotification( NotificationCenter.NotificationType.Decision, @@ -3788,9 +3794,10 @@ public void ErrorHandlerMock.Object, LoggerMock.Object); mockUserContext.Setup(ouc => ouc.GetUserId()).Returns(TestUserId); - DecisionServiceMock - .Setup(ds => ds.GetVariation(experiment, It.IsAny(), Config)) - .Returns(variation); + DecisionServiceMock. + Setup(ds => ds.GetVariation(experiment, It.IsAny(), Config + , It.IsAny(), It.IsAny())). + Returns(variation); optStronglyTyped.NotificationCenter.AddNotification( NotificationCenter.NotificationType.Decision, @@ -3828,8 +3835,9 @@ public void TestGetVariationSendsDecisionNotificationWithNullVariationKey() It.IsAny(), It.IsAny>())); DecisionServiceMock.Setup(ds => ds.GetVariation(It.IsAny(), - It.IsAny(), It.IsAny())) - .Returns(Result.NullResult(null)); + It.IsAny(), It.IsAny() + , It.IsAny(), It.IsAny())). + Returns(Result.NullResult(null)); //DecisionServiceMock.Setup(ds => ds.GetVariation(experiment, TestUserId, Config, null)).Returns(Result.NullResult(null)); optStronglyTyped.NotificationCenter.AddNotification( @@ -3875,8 +3883,9 @@ public void It.IsAny(), It.IsAny>())); DecisionServiceMock.Setup(ds => ds.GetVariation(experiment, - It.IsAny(), ConfigManager.GetConfig(), null)) - .Returns(variation); + It.IsAny(), ConfigManager.GetConfig() + , It.IsAny(), It.IsAny())). + Returns(variation); var optly = Helper.CreatePrivateOptimizely(); var optStronglyTyped = optly.GetObject() as Optimizely; @@ -3936,8 +3945,9 @@ public void It.IsAny(), It.IsAny>())); DecisionServiceMock.Setup(ds => - ds.GetVariation(experiment, It.IsAny(), Config, null)) - .Returns(variation); + ds.GetVariation(experiment, It.IsAny(), Config + , It.IsAny(), It.IsAny())). + Returns(variation); var optly = Helper.CreatePrivateOptimizely(); var optStronglyTyped = optly.GetObject() as Optimizely; @@ -5322,7 +5332,8 @@ public void TestGetAllFeatureVariablesReturnsNullScenarios() LoggerMock.Verify( log => log.Log(LogLevel.ERROR, - "Optimizely instance is not valid, failing getAllFeatureVariableValues call. type"), + "Optimizely instance is not valid, failing getAllFeatureVariableValues call. type") + , Times.Once); } diff --git a/OptimizelySDK/Optimizely.cs b/OptimizelySDK/Optimizely.cs index 3b5c37fa..19e2a417 100644 --- a/OptimizelySDK/Optimizely.cs +++ b/OptimizelySDK/Optimizely.cs @@ -931,10 +931,13 @@ internal OptimizelyDecision Decide(OptimizelyUserContext user, decisionReasons += flagDecisionResult.DecisionReasons; decision = flagDecisionResult.ResultObject; } - - DecisionService.AddDecisionToUnitOfWork(userId, decision.Experiment?.Id, - new Decision(decision.Variation?.Id)); - + + if (!options.Contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) + { + DecisionService.AddDecisionToUnitOfWork(userId, decision.Experiment?.Id, + new Decision(decision.Variation?.Id)); + } + var featureEnabled = false; if (decision?.Variation != null)