feat(dfm): Integrate DFM with optimizely #161

msohailhussain · 2019-06-04T22:35:05Z

Summary

Added ProjectConfigManager in Optimizely class
Added OptimizelyFactory
Inside every API, replaced config with ProjectConfig Manager
Removed ProjectConfig from constructor
DecisionService using ProjectConfig with every method.
Removed ForcedVariation from ProjectConfig & DatafileProjectConfig
Moved ForcedVariation to DecisionService
Added GetVariation private method to use ProjectConfig given by Activate.
Updated existing unit tests

TODO:

Resolve TODOs (Mostly documentation & unit tests to be added)

Test plan

Issues

"THING-1234" or "Fixes refac(ci): improve build time #123"

…tConfigManager.

…rojectConfig as its implementation. Created StaticProjectConfigManager and HttpProjectConfigManager for handling datafile retrievel. Created PollingProjectConfigManager for retrieving ProjectConfig on the specified interval. Added OnReady future in PollingProjectConfigManager class. Added unit tests.

…esolves.

# Conflicts: # OptimizelySDK/DatafileManagement/DatafileProjectConfig.cs

…n designing DFM.

# Conflicts: # OptimizelySDK/DatafileManagement/ProjectConfigManager.cs # OptimizelySDK/Optimizely.cs

# Conflicts: # OptimizelySDK.Net40/OptimizelySDK.Net40.csproj # OptimizelySDK.NetStandard16/OptimizelySDK.NetStandard16.csproj # OptimizelySDK.Tests/DecisionServiceTest.cs # OptimizelySDK.Tests/OptimizelySDK.Tests.csproj # OptimizelySDK.Tests/OptimizelyTest.cs # OptimizelySDK/Config/DatafileProjectConfig.cs # OptimizelySDK/Config/FallbackProjectConfigManager.cs # OptimizelySDK/Optimizely.cs # OptimizelySDK/OptimizelySDK.csproj # OptimizelySDK/ProjectConfig.cs

mjc1283

Is it possible to move forced variations to decision service in a separate PR? This one is large.

msohailhussain · 2019-06-12T20:54:28Z

OK, I will separate out ForcedVariation, infact @mfahadahmed will do it.

#162)" (#164) This reverts commit 6441b00.

mikeproeng37 · 2019-06-14T19:36:08Z

OptimizelySDK/Config/FallbackProjectConfigManager.cs

+    /// <summary>
+    /// Implementation of ProjectConfigManager interface that simply
+    /// returns the stored ProjectConfig instance.
+    /// </summary>


nit: Also mention that it is static and the config it holds cannot change

mikeproeng37 · 2019-06-14T19:39:57Z

OptimizelySDK/Optimizely.cs

+            Initialize(eventDispatcher, logger, errorHandler, userProfileService);
+        }
+
+        private void Initialize(IEventDispatcher eventDispatcher = null,


nit: InitializeComponents?

mikeproeng37 · 2019-06-14T19:42:23Z

OptimizelySDK/Optimizely.cs

+            var config = ProjectConfigManager?.GetConfig();
+            if (!IsValid && config == null) {
+
+                Logger.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'GetVariation'.");


... Failing 'IsFeatureEnabled'

mikeproeng37 · 2019-06-14T19:44:10Z

OptimizelySDK/Optimizely.cs

+                return null;
+            }
+
+            return GetFeatureVariableValueForType<bool?>(featureKey, variableKey, userId, userAttributes, FeatureVariable.VariableType.BOOLEAN, config);


Why do we need to pass the config to it? Why not have the GetFeatureVariableValueForType method use the ProjectConfigManager?

mikeproeng37 · 2019-06-14T19:44:26Z

OptimizelySDK/Optimizely.cs

+            var config = ProjectConfigManager?.GetConfig();
+
+            if (!IsValid && config == null) {
+                Logger.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'activate'.");


Not activate but GetFeatureVariableBoolean

mikeproeng37 · 2019-06-14T19:45:09Z

OptimizelySDK/Optimizely.cs

-        private void SendImpressionEvent(Experiment experiment, Variation variation, string userId, 
-        UserAttributes userAttributes)
+        private void SendImpressionEvent(Experiment experiment, Variation variation, string userId,
+                                         UserAttributes userAttributes, ProjectConfig config)


We don't need to take in the ProjectConfig as a param. We can use the ProjectConfigManager in here

This method is used by Activate, if we use ProjectConfigManager inside SendImpressionEvent, there are chances ProjectConfig used to get variation from DecisionService may not be same when get from ProjectConfigManagr inside this method.

Hmm, ok that could actually happen.

mikeproeng37 · 2019-06-14T19:45:26Z

OptimizelySDK/OptimizelyFactory.cs

+namespace OptimizelySDK
+{
+
+    /// TODO: Need to check with Mike, no need of this class 


This is not the case anymore right?

Yes, will add unit test and more convenience methods in separate PR. Removing this comment.

mikeproeng37 · 2019-06-14T19:46:05Z

OptimizelySDK/OptimizelyFactory.cs

+            var errorHandler = new DefaultErrorHandler();
+            var eventDispatcher = new DefaultEventDispatcher(logger);
+            var builder = new HttpProjectConfigManager.Builder();
+            // don't block


What is this doc comment for?

mjc1283 · 2019-06-14T20:03:31Z

OptimizelySDK/Optimizely.cs

@@ -168,7 +190,8 @@ private bool ValidatePreconditions(Experiment experiment, string userId, UserAtt
        /// <returns>null|Variation Representing variation</returns>
        public Variation Activate(string experimentKey, string userId, UserAttributes userAttributes = null)
        {
-            if (!IsValid)
+            var config = ProjectConfigManager?.GetConfig();
+            if (!IsValid && config == null)


Should this (and the same check elsewhere) be using an or operator instead of and operator? I.e., !IsValid || config == null

It should be AND, if isValid is false and also Config is null, then it's invalid.

ProjectConfigManager.GetConfig can return null. When it does, we must return null early, otherwise attempting to access config.GetExperimentFromKey could crash. If I'm misunderstanding, let me know.

So IsValid will always be false if we are using ProjectConfigManager to initialize Optimizely instance.
Please let me know we can chat for more clarity.

Since we are opening up the ProjectConfigManager to be overriden, we should guard against the custom implementation not returning null or not setting isValid to false. Therefore I think the right apporach is to be more careful and use !IsValid || config == null like @mjc1283 suggested.

mikeproeng37 · 2019-06-14T23:53:15Z

OptimizelySDK/Optimizely.cs

        {

+            var config = ProjectConfigManager?.GetConfig();
+            if (!IsValid && config == null) {
+                Logger.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'activate'.");


This still refers to activate. Please make sure it references the right API name.

mikeproeng37 · 2019-06-14T23:53:44Z

OptimizelySDK/Config/FallbackProjectConfigManager.cs

@@ -18,7 +18,7 @@ namespace OptimizelySDK.Config
 {
    /// <summary>
    /// Implementation of ProjectConfigManager interface that simply
-    /// returns the stored ProjectConfig instance.
+    /// returns the stored ProjectConfig instance which is mutable and cannot be changed.


Hmm this is confusing. The ProjectConfig in this manager should not be mutable.

msohailhussain · 2019-06-15T03:45:58Z

@mikeng13 Please check my last commit, feedback addressed.

mjc1283 · 2019-06-15T19:58:07Z

OptimizelySDK/Optimizely.cs

@@ -168,7 +190,8 @@ private bool ValidatePreconditions(Experiment experiment, string userId, UserAtt
        /// <returns>null|Variation Representing variation</returns>
        public Variation Activate(string experimentKey, string userId, UserAttributes userAttributes = null)
        {
-            if (!IsValid)
+            var config = ProjectConfigManager?.GetConfig();
+            if (!IsValid && config == null)


ProjectConfigManager.GetConfig can return null. When it does, we must return null early, otherwise attempting to access config.GetExperimentFromKey could crash. If I'm misunderstanding, let me know.

mikeproeng37

lgtm

mjc1283

LGTM, isValid changes look good.

msohailhussain and others added 30 commits April 9, 2019 11:47

Initial structure

5cbd740

cleanup

f6487ae

added builder class

e385a95

Extracted ProjectConfig into its own interface and updated HttpProjec…

4eb4104

…tConfigManager.

Added polling unit tests.

33cc9fa

Working on Config manager.

16729e9

Removed old ProjectConfig references.

904072e

Merge remote-tracking branch 'origin/master' into sohail/dfm

9586f2c

Fixed missing file references.

cadf4ee

Update ConfigManager classes to block execution until ProjectConfig r…

f49e833

…esolves.

Added config manager unit tests and fix failing tests.

6189f26

Updated fm

5f9a371

Added unit tests and AtomicProjectConfigManager class.

e287ee7

Adding unit tests.

74a2833

unit test added and 4.0 compat.

1d6b3d6

Resolved compatible issues.

44d94c4

Merge branch 'master' into sohail/dfm

2b220b0

# Conflicts: # OptimizelySDK/DatafileManagement/DatafileProjectConfig.cs

Removed DFM implementation in Optimizely from this PR. Mainly focus o…

dc20812

…n designing DFM.

optimizely changes.

b41e26b

Resolved datafile management namespace issue.

a285f1e

Datafilemore cleanup

72869c7

Merge branch 'sohail/dfm' into sohail/optimizelydfm

45cb133

# Conflicts: # OptimizelySDK/DatafileManagement/ProjectConfigManager.cs # OptimizelySDK/Optimizely.cs

integrated projectconfigmanagez

783c479

Moved ProjectConfig file

7f209a2

renamed to fallback project config manager

99e7a11

Remover parse and added logger and error handler

0c51253

Merge branch 'sohail/dfm' into sohail/optimizelydfm

8fdda5b

Changed namespace & removed SetConfig from interface.

249b201

Merge branch 'sohail/dfm' into sohail/optimizelydfm

6b69f7c

msohailhussain changed the title ~~feat(dfm): Integrate DFM with optimizely - WIP~~ feat(dfm): Integrate DFM with optimizely Jun 12, 2019

Added missing documentation.

2521a31

msohailhussain requested review from mikeproeng37, mjc1283 and a team June 12, 2019 19:40

mjc1283 suggested changes Jun 12, 2019

View reviewed changes

Move forced variation back to ProjectConfig.

8efa7a7

mfahadahmed requested a review from mjc1283 June 13, 2019 08:59

msohailhussain removed the chore label Jun 13, 2019

mfahadahmed and others added 2 commits June 14, 2019 10:49

refac(decisions): Move forced variations into DecisionService. (#162)

6441b00

Revert "refac(decisions): Move forced variations into DecisionService. (

221a81c

#162)" (#164) This reverts commit 6441b00.

mikeproeng37 suggested changes Jun 14, 2019

View reviewed changes

mjc1283 reviewed Jun 14, 2019

View reviewed changes

msohailhussain added 2 commits June 14, 2019 14:58

consolidated mike’s feedback

4ed3b89

Resolved one left comment.

ce6e962

msohailhussain requested review from mikeproeng37 and mjc1283 June 14, 2019 22:57

mikeproeng37 reviewed Jun 14, 2019

View reviewed changes

Logging issue fixed and fallback issue fixed.

9b12d38

mjc1283 suggested changes Jun 15, 2019

View reviewed changes

msohailhussain requested review from mikeproeng37 and mjc1283 June 17, 2019 17:49

Changed IsValid

d4a0476

mikeproeng37 approved these changes Jun 17, 2019

View reviewed changes

mjc1283 approved these changes Jun 17, 2019

View reviewed changes

msohailhussain merged commit 5ed605f into sohail/dfmcomplete Jun 17, 2019

msohailhussain deleted the sohail/optimizelydfm branch September 24, 2019 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dfm): Integrate DFM with optimizely #161

feat(dfm): Integrate DFM with optimizely #161

msohailhussain commented Jun 4, 2019

mjc1283 left a comment

msohailhussain commented Jun 12, 2019

mikeproeng37 Jun 14, 2019

mikeproeng37 Jun 14, 2019

mikeproeng37 Jun 14, 2019

mikeproeng37 Jun 14, 2019

mikeproeng37 Jun 14, 2019

mikeproeng37 Jun 14, 2019

msohailhussain Jun 14, 2019

mikeproeng37 Jun 14, 2019

mikeproeng37 Jun 14, 2019

msohailhussain Jun 14, 2019

mikeproeng37 Jun 14, 2019

mjc1283 Jun 14, 2019

msohailhussain Jun 14, 2019 •

edited

Loading

mjc1283 Jun 15, 2019

msohailhussain Jun 17, 2019

mikeproeng37 Jun 17, 2019

mikeproeng37 Jun 14, 2019

mikeproeng37 Jun 14, 2019

msohailhussain commented Jun 15, 2019

mjc1283 Jun 15, 2019

mikeproeng37 left a comment

mjc1283 left a comment

feat(dfm): Integrate DFM with optimizely #161

feat(dfm): Integrate DFM with optimizely #161

Conversation

msohailhussain commented Jun 4, 2019

Summary

Test plan

Issues

mjc1283 left a comment

Choose a reason for hiding this comment

msohailhussain commented Jun 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain commented Jun 15, 2019

Choose a reason for hiding this comment

mikeproeng37 left a comment

Choose a reason for hiding this comment

mjc1283 left a comment

Choose a reason for hiding this comment

msohailhussain Jun 14, 2019 •

edited

Loading