Skip to content

[FSSDK-10265] fix: UPS Lookup & Save during batched Decide #374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
963fc07
chore: updated during Rider upgrade
mikechu-optimizely Oct 4, 2024
d0f59b6
feat: call UPS once during batch decideForKeys or decideAll
mikechu-optimizely Oct 4, 2024
14dface
revert: SaveVariation to include userProfile
mikechu-optimizely Oct 4, 2024
66708d8
doc: fix param
mikechu-optimizely Oct 4, 2024
aa0d018
Merge branch 'master' into mike/FSSDK-10265-ups-during-batch-decide
mikechu-optimizely Oct 4, 2024
dd072b9
test: fix hacked tests
mikechu-optimizely Oct 4, 2024
f9f3f3f
revert: auto-formatting
mikechu-optimizely Oct 4, 2024
b43e10f
refactor: SaveToUserProfileService for simplicity
mikechu-optimizely Oct 4, 2024
0dd1b74
revert: formatting
mikechu-optimizely Oct 4, 2024
2b17438
bug: only SaveToUserProfileService if batch is finishing
mikechu-optimizely Oct 7, 2024
c9047a0
test: WIP add coverage for single `Lookup` & `Save` via USP
mikechu-optimizely Oct 7, 2024
dfe1367
refactor: less movement of code as the ref implementation
mikechu-optimizely Oct 8, 2024
57c1ccd
Revert "refactor: less movement of code as the ref implementation"
mikechu-optimizely Oct 8, 2024
9ca271c
test: finish coverage
mikechu-optimizely Oct 8, 2024
c97f36e
Update OptimizelySDK.Tests/OptimizelyUserContextTest.cs
mikechu-optimizely Oct 14, 2024
5b7e5ae
fix: reset _userProfile after batch
mikechu-optimizely Oct 16, 2024
c6889e8
test: add content verification of UPS Save()
mikechu-optimizely Oct 16, 2024
2f46b18
Merge remote-tracking branch 'origin/mike/FSSDK-10265-ups-during-batc…
mikechu-optimizely Oct 16, 2024
54a2dc2
ci: stop CI run if Draft PR = save GH runner costs
mikechu-optimizely Oct 17, 2024
9f2f471
feat: WIP better way to handle loading & saving for UPS
mikechu-optimizely Oct 17, 2024
73658f6
ci: better error echoed
mikechu-optimizely Oct 17, 2024
80e73d4
test: add coverage
mikechu-optimizely Oct 17, 2024
236728b
revert: doc differences
mikechu-optimizely Oct 17, 2024
d6258f1
fix: WIP tests & code under tests
mikechu-optimizely Oct 17, 2024
9c6e9bb
fix: for GetVariation & Activate never use cached user profile
mikechu-optimizely Oct 18, 2024
49f9584
test: corrections
mikechu-optimizely Oct 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions OptimizelySDK.Tests/OptimizelyTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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."),
Expand Down
5 changes: 5 additions & 0 deletions OptimizelySDK.sln.DotSettings
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,15 @@
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/PredefinedNamingRules/=Interfaces/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="I" Suffix="" Style="AaBb" /&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/PredefinedNamingRules/=LocalConstants/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="" Suffix="" Style="AA_BB" /&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/PredefinedNamingRules/=PrivateStaticReadonly/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=15b5b1f1_002D457c_002D4ca6_002Db278_002D5615aedc07d3/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Static" AccessRightKinds="Private" Description="Static readonly fields (private)"&gt;&lt;ElementKinds&gt;&lt;Kind Name="READONLY_FIELD" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /&gt;&lt;/Policy&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=8b8504e3_002Df0be_002D4c14_002D9103_002Dc732f2bddc15/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Any" AccessRightKinds="Any" Description="Enum members"&gt;&lt;ElementKinds&gt;&lt;Kind Name="ENUM_MEMBER" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="" Suffix="" Style="AaBb"&gt;&lt;ExtraRule Prefix="" Suffix="" Style="AaBb" /&gt;&lt;/Policy&gt;&lt;/Policy&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=a4f433b8_002Dabcd_002D4e55_002Da08f_002D82e78cef0f0c/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Any" AccessRightKinds="Any" Description="Local constants"&gt;&lt;ElementKinds&gt;&lt;Kind Name="LOCAL_CONSTANT" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="" Suffix="" Style="AA_BB" /&gt;&lt;/Policy&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=a7a3339e_002D4e89_002D4319_002D9735_002Da9dc4cb74cc7/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Any" AccessRightKinds="Any" Description="Interfaces"&gt;&lt;ElementKinds&gt;&lt;Kind Name="INTERFACE" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="I" Suffix="" Style="AaBb" /&gt;&lt;/Policy&gt;</s:String>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpKeepExistingMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpPlaceEmbeddedOnSameLineMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpUseContinuousIndentInsideBracesMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EMigrateBlankLinesAroundFieldToBlankLinesAroundProperty/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EPredefinedNamingRulesToUserRulesUpgrade/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Bucketer/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=ODP_0027s/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Optly/@EntryIndexedValue">True</s:Boolean>
Expand Down
151 changes: 101 additions & 50 deletions OptimizelySDK/Bucketing/DecisionService.cs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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();
}
}
}

/// <summary>
/// Associative array of user IDs to an associative array
Expand All @@ -60,10 +76,10 @@ public class DecisionService
/// <summary>
/// Initialize a decision service for the Optimizely client.
/// </summary>
/// <param name = "bucketer" > Base bucketer to allocate new users to an experiment.</param>
/// <param name = "errorHandler" > The error handler of the Optimizely client.</param>
/// <param name = "userProfileService" ></ param >
/// < param name= "logger" > UserProfileService implementation for storing user info.</param>
/// <param name="bucketer"> Base bucketer to allocate new users to an experiment.</param>
/// <param name="errorHandler"> The error handler of the Optimizely client.</param>
/// <param name="userProfileService">The injected implementation providing control over the bucketing.</param >
/// < param name="logger"> UserProfileService implementation for storing user info.</param>
public DecisionService(Bucketer bucketer, IErrorHandler errorHandler,
UserProfileService userProfileService, ILogger logger
)
Expand All @@ -85,7 +101,7 @@ public DecisionService(Bucketer bucketer, IErrorHandler errorHandler,
/// Get a Variation of an Experiment for a user to be allocated into.
/// </summary>
/// <param name = "experiment" > The Experiment the user will be bucketed into.</param>
/// <param name = "user" > Optimizely user context.
/// <param name = "user" > Optimizely user context.</param>
/// <param name = "config" > Project config.</param>
/// <returns>The Variation the user is allocated into.</returns>
public virtual Result<Variation> GetVariation(Experiment experiment,
Expand All @@ -99,10 +115,10 @@ ProjectConfig config
/// <summary>
/// Get a Variation of an Experiment for a user to be allocated into.
/// </summary>
/// <param name = "experiment" > The Experiment the user will be bucketed into.</param>
/// <param name = "user" > optimizely user context.
/// <param name = "config" > Project Config.</param>
/// <param name = "options" >An array of decision options.</param>
/// <param name="experiment">The Experiment the user will be bucketed into.</param>
/// <param name="user">optimizely user context.</param>
/// <param name="config">Project Config.</param>
/// <param name="options">An array of decision options.</param>
/// <returns>The Variation the user is allocated into.</returns>
public virtual Result<Variation> GetVariation(Experiment experiment,
OptimizelyUserContext user,
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -197,7 +220,7 @@ OptimizelyDecideOption[] options
{
if (UserProfileService != null && !ignoreUPS)
{
var bucketerUserProfile = userProfile ??
var bucketerUserProfile = _userProfile ??
new UserProfile(userId,
new Dictionary<string, Decision>());
SaveVariation(experiment, decisionVariationResult.ResultObject,
Expand Down Expand Up @@ -454,9 +477,9 @@ ProjectConfig config
/// <summary>
/// Save a { @link Variation } of an { @link Experiment } for a user in the {@link UserProfileService}.
/// </summary>
/// <param name = "experiment" > The experiment the user was buck</param>
/// <param name = "variation" > The Variation to save.</param>
/// <param name = "userProfile" > instance of the user information.</param>
/// <param name="experiment">The experiment the user was buck</param>
/// <param name="variation">The Variation to save.</param>
/// <param name="userProfile">Instance of the user information.</param>
public void SaveVariation(Experiment experiment, Variation variation,
UserProfile userProfile
)
Expand All @@ -467,29 +490,57 @@ UserProfile userProfile
return;
}

if (_userProfile == null)
{
_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
{
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 hasExperimentDetails = experiment != null && variation != null &&
!string.IsNullOrEmpty(_userProfile.UserId);
try
{
UserProfileService.Save(userProfile.ToMap());
if (_userProfile == null)
{
return;
}

UserProfileService.Save(_userProfile.ToMap());

Logger.Log(LogLevel.INFO,
$"Saved variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{userProfile.UserId}\".");
hasExperimentDetails ?
$"Saved variation \"{variation.Id}\" of experiment \"{experiment.Id}\" for user \"{_userProfile.UserId}\"." :
"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}\".");
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));
}
Expand Down
5 changes: 4 additions & 1 deletion OptimizelySDK/Optimizely.cs
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -141,7 +141,7 @@
try
{
#if USE_ODP
InitializeComponents(eventDispatcher, logger, errorHandler, userProfileService,

Check warning on line 144 in OptimizelySDK/Optimizely.cs

View workflow job for this annotation

GitHub Actions / Build Standard 2.0

'Optimizely.InitializeComponents(IEventDispatcher, ILogger, IErrorHandler, UserProfileService, NotificationCenter, EventProcessor, OptimizelyDecideOption[], IOdpManager)' is obsolete

Check warning on line 144 in OptimizelySDK/Optimizely.cs

View workflow job for this annotation

GitHub Actions / Build Standard 2.0

'Optimizely.InitializeComponents(IEventDispatcher, ILogger, IErrorHandler, UserProfileService, NotificationCenter, EventProcessor, OptimizelyDecideOption[], IOdpManager)' is obsolete
null, eventProcessor, defaultDecideOptions, odpManager);
#else
InitializeComponents(eventDispatcher, logger, errorHandler, userProfileService,
Expand Down Expand Up @@ -209,7 +209,7 @@
ProjectConfigManager = configManager;

#if USE_ODP
InitializeComponents(eventDispatcher, logger, errorHandler, userProfileService,

Check warning on line 212 in OptimizelySDK/Optimizely.cs

View workflow job for this annotation

GitHub Actions / Build Standard 2.0

'Optimizely.InitializeComponents(IEventDispatcher, ILogger, IErrorHandler, UserProfileService, NotificationCenter, EventProcessor, OptimizelyDecideOption[], IOdpManager)' is obsolete

Check warning on line 212 in OptimizelySDK/Optimizely.cs

View workflow job for this annotation

GitHub Actions / Build Standard 2.0

'Optimizely.InitializeComponents(IEventDispatcher, ILogger, IErrorHandler, UserProfileService, NotificationCenter, EventProcessor, OptimizelyDecideOption[], IOdpManager)' is obsolete
notificationCenter, eventProcessor, defaultDecideOptions, odpManager);

var projectConfig = ProjectConfigManager.CachedProjectConfig;
Expand Down Expand Up @@ -1056,6 +1056,7 @@

var allOptions = GetAllOptions(options);

DecisionService.DecisionBatchInProgress = true;
foreach (var key in keys)
{
var decision = Decide(user, key, options);
Expand All @@ -1066,6 +1067,8 @@
}
}

DecisionService.DecisionBatchInProgress = false;

return decisionDictionary;
}

Expand Down
Loading