fix(logs): Added and refactored audience and feature variable evaluation logs #229

mnoman09 · 2020-07-07T12:43:33Z

Summary

Added and updated missing audience evaluation logs.

Test plan

added unit tests must pass and all FSC scenarios should pass

msohailhussain · 2020-07-07T19:52:40Z

OptimizelySDK.Tests/UtilsTests/ExperimentUtilsTest.cs

@@ -126,7 +126,7 @@ public void TestIsUserInExperimentReturnsTrueIfAllAudiencesInANDConditionPass()
                { "favorite_ice_cream", "walls" }
            };

-            Assert.True(ExperimentUtils.IsUserInExperiment(Config, experiment, userAttributes, Logger));
+            Assert.True(ExperimentUtils.IsUserInExperiment(Config, experiment, userAttributes, "experiment", experiment.Key, Logger));


IsUserInExperiment Please rename it.

msohailhussain · 2020-07-07T19:54:31Z

OptimizelySDK/Bucketing/DecisionService.cs

@@ -120,7 +120,7 @@ public virtual Variation GetVariation(Experiment experiment, string userId, Proj
                }
            }

-            if (ExperimentUtils.IsUserInExperiment(config, experiment, filteredAttributes, Logger))
+            if (ExperimentUtils.IsUserInExperiment(config, experiment, filteredAttributes, "experiment", experiment.Key, Logger))


Please use const experiment, don't pass string as it is.

msohailhussain

mostly looks good. please rename method name and add AUDIENCE_FOR_EXPERIMENT and AUDIENCE_FOR_ROLLOUT as const.

msohailhussain · 2020-07-08T19:47:43Z

OptimizelySDK/Optimizely.cs

@@ -489,9 +489,9 @@ public virtual bool IsFeatureEnabled(string featureKey, string userId, UserAttri
            }

            if (featureEnabled == true)
-                Logger.Log(LogLevel.INFO, $@"Feature flag ""{featureKey}"" is enabled for user ""{userId}"".");
+                Logger.Log(LogLevel.INFO, $@"Feature ""{featureKey}"" is enabled for user ""{userId}"".");


Following logs doesn't seem correct to me.

msohailhussain · 2020-07-08T20:04:16Z

OptimizelySDK/Optimizely.cs

                    }
                    else
                    {
-                        Logger.Log(LogLevel.INFO, $@"Feature ""{featureKey}"" is not enabled for user {userId}. Returning default value for variable ""{variableKey}"".");
+                        Logger.Log(LogLevel.INFO, $@"Feature ""{featureKey}"" is not enabled for user {userId}. Returning the default variable value ""{variableValue}"".");


'Feature "%s" is not enabled for user "%s". ' 'Returning the default variable value "%s".' % (feature_key, user_id, variable_value) )```

msohailhussain · 2020-07-08T20:05:54Z

OptimizelySDK/Optimizely.cs

@@ -569,11 +569,11 @@ public virtual bool IsFeatureEnabled(string featureKey, string userId, UserAttri
                    if (variation.FeatureEnabled == true)
                    {
                        variableValue = featureVariableUsageInstance.Value;
-                        Logger.Log(LogLevel.INFO, $@"Returning variable value ""{variableValue}"" for variation ""{variation.Key}"" of feature flag ""{featureKey}"".");
+                        Logger.Log(LogLevel.INFO, $@"Feature ""{featureKey}"" is enabled for user ""{userId}"".");


'Got variable value "%s" for variable "%s" of feature flag "%s".' % (variable_value, variable_key, feature_key) )```

aliabbasrizvi

Small changes needed. Looks good mostly.

aliabbasrizvi · 2020-07-21T01:04:55Z

OptimizelySDK.Tests/UtilsTests/ValidatorTest.cs

@@ -81,15 +81,15 @@ public void TestValidateJsonSchemaNoJsonContent()
        [Test]
        public void TestIsUserInExperimentNoAudienceUsedInExperiment()


The name of the method has changed to: DoesUserMeetAudienceConditions. Please update test names accordingly.

aliabbasrizvi · 2020-07-21T01:06:05Z

OptimizelySDK/Bucketing/DecisionService.cs

+        public const string AUDIENCE_FOR_EXPERIMENT = "experiment";
+        public const string AUDIENCE_FOR_RULE = "rule";


These variable names and the docstring above it do not make sense.

aliabbasrizvi · 2020-07-21T01:07:42Z

OptimizelySDK/Utils/ExperimentUtils.cs

+        public static bool DoesUserMeetAudienceConditions(ProjectConfig config,
+            Experiment experiment,


Parameter descriptions above need to be updated.

aliabbasrizvi · 2020-07-21T01:08:25Z

OptimizelySDK/Utils/ExperimentUtils.cs

+        public static bool DoesUserMeetAudienceConditions(ProjectConfig config,
+            Experiment experiment,
+            UserAttributes userAttributes,
+            string audienceFor,


audienceFor? Why not something like loggingKeyType?

aliabbasrizvi

Looks good. Small change needed in docstring.

aliabbasrizvi · 2020-07-24T17:26:36Z

OptimizelySDK/Utils/ExperimentUtils.cs

@@ -42,11 +42,13 @@ public static bool IsExperimentActive(Experiment experiment, ILogger logger)
        /// <param name="config">ProjectConfig Configuration for the project</param>
        /// <param name="experiment">Experiment Entity representing the experiment</param>
        /// <param name="userAttributes">Attributes of the user. Defaults to empty attributes array if not provided</param>
+        /// <param name="loggingKeyType">It can be either experiment or rule.</param>
+        /// <param name="loggingKey">In case of loggingKeyType is experiment it will be experiment key or else it will be rule number.</param>


nit. In case loggingKeyType is experiment

msohailhussain

lgtm

mnoman09 added 2 commits July 6, 2020 19:29

Added audience audit logging

c51d2a7

updated logs

9156371

optimizely-codereview-bot assigned mnoman09 Jul 7, 2020

mnoman09 requested a review from msohailhussain July 7, 2020 12:44

Merge branch 'master' into mnoman/audienceAuditLogging

a667632

msohailhussain reviewed Jul 7, 2020

View reviewed changes

msohailhussain suggested changes Jul 7, 2020

View reviewed changes

mnoman09 and others added 2 commits July 8, 2020 17:57

Merge branch 'master' into mnoman/audienceAuditLogging

f56aa9a

changed isUserInexperiment name to DoesUserMeetConditions

fb509ed

msohailhussain unassigned mnoman09 Jul 8, 2020

optimizely-codereview-bot requested a review from msohailhussain July 8, 2020 16:48

msohailhussain marked this pull request as ready for review July 8, 2020 16:48

msohailhussain requested a review from a team as a code owner July 8, 2020 16:48

mnoman09 added 2 commits July 8, 2020 23:06

logs of feature flag is changed

da60b9d

commented a line which is failing

b543785

mnoman09 changed the title ~~refact(audience-logs): Added and refactored audience evaluation logs~~ refact(audience-logs): Added and refactored audience and feature variable evaluation logs Jul 8, 2020

log fixed

4ced6a5

msohailhussain changed the title ~~refact(audience-logs): Added and refactored audience and feature variable evaluation logs~~ fix(logs): Added and refactored audience and feature variable evaluation logs Jul 8, 2020

msohailhussain reviewed Jul 8, 2020

View reviewed changes

mnoman09 added 2 commits July 9, 2020 01:29

logs fixed

b746b35

Added isFeatureEnabled log in getAllFeatureVaribles

92b8f1b

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs 🤖 16 hrs and removed 🤖 4 hrs 🤖 8 hrs labels Jul 8, 2020

optimizely-codereview-bot added 🤖 20 hrs and removed 🤖 12 hrs 🤖 16 hrs labels Jul 10, 2020

Merge branch 'master' into mnoman/audienceAuditLogging

e3ea730

optimizely-codereview-bot added 🤖 24 hrs and removed 🤖 20 hrs labels Jul 13, 2020

Merge branch 'master' into mnoman/audienceAuditLogging

fc0d697

aliabbasrizvi requested a review from msohailhussain July 21, 2020 00:50

aliabbasrizvi suggested changes Jul 21, 2020

View reviewed changes

optimizely-codereview-bot removed the 🤖 24 hrs label Jul 21, 2020

optimizely-codereview-bot assigned mnoman09 Jul 21, 2020

resolved comments

84c1e8a

mnoman09 requested a review from aliabbasrizvi July 22, 2020 08:53

mnoman09 removed their assignment Jul 22, 2020

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs 🤖 16 hrs and removed 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs labels Jul 22, 2020

Merge branch 'master' into mnoman/audienceAuditLogging

6205399

aliabbasrizvi approved these changes Jul 24, 2020

View reviewed changes

nit fix

6fa794f

msohailhussain approved these changes Jul 27, 2020

View reviewed changes

mikeproeng37 merged commit 11e7df7 into master Jul 27, 2020

mikeproeng37 deleted the mnoman/audienceAuditLogging branch July 27, 2020 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(logs): Added and refactored audience and feature variable evaluation logs #229

fix(logs): Added and refactored audience and feature variable evaluation logs #229

mnoman09 commented Jul 7, 2020 •

edited

Loading

msohailhussain Jul 7, 2020

msohailhussain Jul 7, 2020

msohailhussain Jul 7, 2020

msohailhussain left a comment

msohailhussain Jul 8, 2020

msohailhussain Jul 8, 2020

msohailhussain Jul 8, 2020 •

edited

Loading

aliabbasrizvi left a comment

aliabbasrizvi Jul 21, 2020

aliabbasrizvi Jul 21, 2020

aliabbasrizvi Jul 21, 2020

aliabbasrizvi Jul 21, 2020

aliabbasrizvi left a comment

aliabbasrizvi Jul 24, 2020

msohailhussain left a comment

		@@ -81,15 +81,15 @@ public void TestValidateJsonSchemaNoJsonContent()
		[Test]
		public void TestIsUserInExperimentNoAudienceUsedInExperiment()

		public const string AUDIENCE_FOR_EXPERIMENT = "experiment";
		public const string AUDIENCE_FOR_RULE = "rule";

		public static bool DoesUserMeetAudienceConditions(ProjectConfig config,
		Experiment experiment,

fix(logs): Added and refactored audience and feature variable evaluation logs #229

fix(logs): Added and refactored audience and feature variable evaluation logs #229

Conversation

mnoman09 commented Jul 7, 2020 • edited Loading

Summary

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

aliabbasrizvi left a 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

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain left a comment

Choose a reason for hiding this comment

mnoman09 commented Jul 7, 2020 •

edited

Loading

msohailhussain Jul 8, 2020 •

edited

Loading