refact(audience-logs): Added and refactored audience and feature variable evaluation logs #380

mnoman09 · 2020-07-01T16:06:31Z

Summary

Added and updated missing audience evaluation logs.

Test plan

added unit tests must pass and all FSC scenarios should pass

coveralls · 2020-07-06T07:49:52Z

Pull Request Test Coverage Report for Build 1467

23 of 25 (92.0%) changed or added relevant lines in 8 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.07%) to 89.419%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/internal/LoggingConstants.java	0	2	0.0%

Files with Coverage Reduction	New Missed Lines	%
core-api/src/main/java/com/optimizely/ab/config/audience/Audience.java	1	90.0%

Totals
Change from base Build 1459:	-0.07%
Covered Lines:	3955
Relevant Lines:	4423

💛 - Coveralls

…nditions

msohailhussain · 2020-07-08T23:46:06Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

@@ -221,25 +223,24 @@ FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag
        Variation variation;
        for (int i = 0; i < rolloutRulesLength - 1; i++) {
            Experiment rolloutRule = rollout.getExperiments().get(i);
-            Audience audience = projectConfig.getAudienceIdMapping().get(rolloutRule.getAudienceIds().get(0));


was it additional?

yes as we removed the audience name this became redundant.

core-api/src/main/java/com/optimizely/ab/Optimizely.java

msohailhussain · 2020-07-08T23:55:28Z

core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java

        if (experiment.getAudienceConditions() != null) {
-            Boolean resolveReturn = evaluateAudienceConditions(projectConfig, experiment, attributes);
+            Boolean resolveReturn = evaluateAudienceConditions(projectConfig, experiment, attributes, audienceFor, loggingKey);


evaluation log should be here. Not in the method.

msohailhussain · 2020-07-08T23:56:06Z

core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java

@@ -93,26 +99,28 @@ public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig,

        OrCondition implicitOr = new OrCondition(conditions);

-        logger.debug("Evaluating audiences for experiment \"{}\": \"{}\"", experiment.getKey(), conditions);
+        logger.debug("Evaluating audiences for {} \"{}\": {}.", audienceFor, loggingKey, conditions);


It should be in doesUser...

we should not move it to doesUserMeetCondition method because list of conditions object is created here. To move this log we would have to move the loop too.

msohailhussain · 2020-07-08T23:57:09Z

core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java


        Condition conditions = experiment.getAudienceConditions();
        if (conditions == null) return null;
-        logger.debug("Evaluating audiences for experiment \"{}\": \"{}\"", experiment.getKey(), conditions.toString());
+        logger.debug("Evaluating audiences for {} \"{}\": {}.", audienceFor, loggingKey, conditions);


Please move it.

aliabbasrizvi

Minor changes suggested. Mostly looks good.

aliabbasrizvi · 2020-07-21T18:50:03Z

core-api/src/main/java/com/optimizely/ab/config/audience/AudienceIdCondition.java

@@ -74,9 +74,9 @@ public Boolean evaluate(ProjectConfig config, Map<String, ?> attributes) {
            logger.error("Audience {} could not be found.", audienceId);
            return null;
        }
-        logger.debug("Starting to evaluate audience {} with conditions: \"{}\"", audience.getName(), audience.getConditions());
+        logger.debug("Starting to evaluate audience \"{}\" with conditions: {}.", audience.getId(), audience.getConditions());


Let's get rid of this log message. I don't see this in other places.

Actually this log is getting logged in all other sdks, including python (which we followed).

aliabbasrizvi · 2020-07-21T18:54:41Z

core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java

+
+        logbackVerifier.expectMessage(
+            Level.INFO,
+            "Variable \"integer_variable\" is not used in variation \"589640735\", returning default value \"7\"."


This log message denotes that the code needs to be updated. Please see recommendations to what this message should be. I think it is Variable <variable name> 's value is not defined. Returning default value <value>

aliabbasrizvi · 2020-07-21T18:56:38Z

core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java

@@ -59,24 +59,31 @@ public static boolean isExperimentActive(@Nonnull Experiment experiment) {
     * @param projectConfig the current projectConfig
     * @param experiment    the experiment we are evaluating audiences for
     * @param attributes    the attributes of the user
+     * @param audienceFor   It can be either experiment or rule.


May be call this loggingEntityType or entityType?

aliabbasrizvi · 2020-07-21T18:57:41Z

core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java

@@ -664,6 +672,9 @@ public void getVariationForFeatureInRolloutReturnsVariationWhenUserFailsTrafficI
        assertEquals(expectedVariation, featureDecision.variation);
        assertEquals(FeatureDecision.DecisionSource.ROLLOUT, featureDecision.decisionSource);

+        logbackVerifier.expectMessage(Level.DEBUG, "There is no Audience associated with experiment 828245624");


We should get rid of this log message in code I feel. Where is this exactly?

aliabbasrizvi · 2020-07-24T17:23:01Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

                } else {
                    variableValue = variable.getDefaultValue();
+                    logger.info("Variable \"{}\" value is not defined. Returning default value \"{}\".", variableKey, variableValue);


Slight correction needed in the first sentence. It will be awkward to put apostrophe. May be say Value is not defined for variable "{}"

aliabbasrizvi

Just 1 small change needed before merging.

mnoman09 added 2 commits June 30, 2020 21:39

audit audience logging

7e01dfb

corrected logs according to changes

1070b0f

optimizely-codereview-bot assigned mnoman09 Jul 1, 2020

mnoman09 added 2 commits July 1, 2020 21:07

Updated header

5e90af7

test fix

e95ec2d

mnoman09 and others added 6 commits July 7, 2020 13:36

changed name of function isUserInExperiment to doesUserMeetAudienceCo…

5c0df79

…nditions

evaluating audiences fix

1cef659

Updated logs

7112762

Merge branch 'master' into mnoman/audienceLogAudit

b7e064c

Updated test and feature variable logs

46de92d

Merge branch 'master' into mnoman/audienceLogAudit

40b565f

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

msohailhussain reviewed Jul 8, 2020

View reviewed changes

core-api/src/main/java/com/optimizely/ab/Optimizely.java Show resolved Hide resolved

msohailhussain reviewed Jul 8, 2020

View reviewed changes

core-api/src/main/java/com/optimizely/ab/Optimizely.java Outdated Show resolved Hide resolved

msohailhussain reviewed Jul 8, 2020

View reviewed changes

mnoman09 and others added 2 commits July 9, 2020 19:44

Resolved comment and added additional logs

d916aa5

Merge branch 'master' into mnoman/audienceLogAudit

36a0b48

mnoman09 requested a review from msohailhussain July 10, 2020 14:11

mnoman09 removed their assignment Jul 10, 2020

Merge branch 'master' into mnoman/audienceLogAudit

be289b1

mnoman09 marked this pull request as ready for review July 14, 2020 10:09

mnoman09 requested a review from a team as a code owner July 14, 2020 10:09

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

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

Merge branch 'master' into mnoman/audienceLogAudit

56b36d4

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

77aba54

mnoman09 requested a review from aliabbasrizvi July 22, 2020 07:45

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

aliabbasrizvi reviewed Jul 24, 2020

View reviewed changes

aliabbasrizvi approved these changes Jul 24, 2020

View reviewed changes

Log changed

238bfeb

mikeproeng37 merged commit acf8cb9 into master Jul 27, 2020

mikeproeng37 deleted the mnoman/audienceLogAudit branch July 27, 2020 18:29

refact(audience-logs): Added and refactored audience and feature variable evaluation logs #380

refact(audience-logs): Added and refactored audience and feature variable evaluation logs #380

Uh oh!

Conversation

mnoman09 commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coveralls commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1467

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mnoman09 commented Jul 1, 2020 •

edited

Loading

coveralls commented Jul 6, 2020 •

edited

Loading