feat(flag-decisions): Add support for sending flag decisions along with decision metadata. #405

mnoman09 · 2020-10-01T10:56:34Z

Summary

Add experiment/feature flag key, variation key and type to impression events.
Send events for ALL decision types.
Add Metadata field to EventBatch.Decisions to capture flag type, key and variation key.

Test plan

All FSC and unit tests should pass

- Added unit tests

coveralls · 2020-10-01T11:03:45Z

Pull Request Test Coverage Report for Build 1580

91 of 98 (92.86%) changed or added relevant lines in 11 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.03%) to 89.735%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/Optimizely.java	10	11	90.91%
core-api/src/main/java/com/optimizely/ab/event/internal/payload/Decision.java	6	7	85.71%
core-api/src/main/java/com/optimizely/ab/event/internal/payload/DecisionMetadata.java	29	34	85.29%

Files with Coverage Reduction	New Missed Lines	%
core-api/src/main/java/com/optimizely/ab/event/internal/UserEventFactory.java	1	97.92%

Totals
Change from base Build 1565:	0.03%
Covered Lines:	4126
Relevant Lines:	4598

💛 - Coveralls

msohailhussain

please address.

msohailhussain · 2020-10-01T20:45:06Z

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

@@ -216,7 +216,7 @@ private Variation activate(@Nullable ProjectConfig projectConfig,
            return null;
        }

-        sendImpression(projectConfig, experiment, userId, copiedAttributes, variation);
+        sendImpression(projectConfig, experiment, userId, copiedAttributes, variation, experiment.getKey(), "experiment");


please make experiment const or enum

msohailhussain · 2020-10-01T20:46:45Z

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

-            logger.info("Experiment has \"Launched\" status so not dispatching event during activation.");
-            return;
-        }
+                                @Nonnull Variation variation,


I will appreciate if you can add header doc because of no. of args.

msohailhussain · 2020-10-01T20:47:02Z

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

-            return;
-        }
+                                @Nonnull Variation variation,
+                                @Nonnull String flagKey,


it should be featureFlagKey or featureKey

we are calling it flagKey and flagType in all other sdks. So should I change it in java-sdk?

I like flagKey which we use in Decide-API

msohailhussain · 2020-10-01T20:47:34Z

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

-        }
+                                @Nonnull Variation variation,
+                                @Nonnull String flagKey,
+                                @Nonnull String flagType) {


It should be featureSource,

msohailhussain · 2020-10-01T20:49:20Z

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

@@ -386,16 +390,22 @@ private Boolean isFeatureEnabled(@Nonnull ProjectConfig projectConfig,
        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig);
        Boolean featureEnabled = false;
        SourceInfo sourceInfo = new RolloutSourceInfo();
+        if (featureDecision.decisionSource != null) {


in which case, decisionSource can be null.

when experiment and variation is null.
the cases are

When The feature flag is not used in a rollout

The rollout with id was not found in the datafile for feature flag

added documentation of sendImpression

yasirfolio3

Can we have a test checking if impression event's meta data has null variationKey when variation is null.

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

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

yasirfolio3 · 2020-10-05T07:54:45Z

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

+        String layerID = null;
+        String experimentId = null;
+        String experimentKey = null;
+        if (activatedExperiment != null) {


This check for experiment shouldn't be required since it is already checked by the caller?

core-api/src/main/java/com/optimizely/ab/event/internal/payload/DecisionMetadata.java

set flagKey to empty in activate case

pawels-optimizely

lgtm

msohailhussain

lgtm

jaeopt

Overall it looks good. Need some changes in tests and comments.

jaeopt · 2020-10-20T16:09:47Z

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

-            return;
-        }
+                                @Nonnull Variation variation,
+                                @Nonnull String flagKey,


I like flagKey which we use in Decide-API

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

core-api/src/test/java/com/optimizely/ab/event/internal/UserEventFactoryTest.java

jaeopt · 2020-10-20T17:39:10Z

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

        Variation variation = optimizely.activate(launchedExperiment.getKey(), testUserId);
        assertNotNull(variation);
        assertThat(variation.getKey(), is(expectedVariation.getKey()));
+        eventHandler.expectImpression(launchedExperiment.getId(), expectedVariation.getId(), testUserId);


Not sure if I miss it, but I do not see validation of new metadata in expected events.

Updated userEventFactory create impression test to avoid sending experiment key as flag key in case of ruletype experiment

jaeopt

LGTM

mnoman09 added 2 commits September 30, 2020 23:16

Added Flag decision support

b558918

- Added unit tests

Rollout to string fix

ca32f1f

optimizely-codereview-bot assigned mnoman09 Oct 1, 2020

Merge branch 'master' into mnoman/DecisionFlag

9cee565

msohailhussain suggested changes Oct 1, 2020

View reviewed changes

Made experiment and variation nullable in createImpression

fdc1afb

added documentation of sendImpression

yasirfolio3 reviewed Oct 5, 2020

View reviewed changes

mnoman09 added 3 commits October 12, 2020 20:19

Renamed flag_type to rule_type and added rule_key

95409f0

nit fix

029cfd5

set ruletype to rollout when decision source is null

42d7165

set flagKey to empty in activate case

mnoman09 closed this Oct 13, 2020

mnoman09 reopened this Oct 13, 2020

mnoman09 closed this Oct 14, 2020

mnoman09 reopened this Oct 14, 2020

msohailhussain requested review from thomaszurkan-optimizely and jaeopt October 14, 2020 22:59

msohailhussain unassigned mnoman09 Oct 14, 2020

optimizely-codereview-bot requested review from msohailhussain and yasirfolio3 October 14, 2020 22:59

msohailhussain marked this pull request as ready for review October 14, 2020 22:59

msohailhussain requested a review from a team as a code owner October 14, 2020 22:59

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

optimizely-codereview-bot removed the 🤖 16 hrs label Oct 19, 2020

msohailhussain requested a review from pawels-optimizely October 19, 2020 22:12

optimizely-codereview-bot added 🤖 24 hrs and removed 🤖 20 hrs labels Oct 19, 2020

pawels-optimizely approved these changes Oct 19, 2020

View reviewed changes

msohailhussain approved these changes Oct 20, 2020

View reviewed changes

jaeopt requested changes Oct 20, 2020

View reviewed changes

optimizely-codereview-bot removed the 🤖 24 hrs label Oct 20, 2020

optimizely-codereview-bot assigned mnoman09 Oct 20, 2020

Comments resolved

799e65d

Updated userEventFactory create impression test to avoid sending experiment key as flag key in case of ruletype experiment

jaeopt approved these changes Oct 23, 2020

View reviewed changes

pawels-optimizely merged commit dcbf9b0 into master Oct 23, 2020

pawels-optimizely deleted the mnoman/DecisionFlag branch October 23, 2020 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(flag-decisions): Add support for sending flag decisions along with decision metadata. #405

feat(flag-decisions): Add support for sending flag decisions along with decision metadata. #405

mnoman09 commented Oct 1, 2020

coveralls commented Oct 1, 2020 •

edited

Loading

msohailhussain left a comment

msohailhussain Oct 1, 2020

msohailhussain Oct 1, 2020

msohailhussain Oct 1, 2020

mnoman09 Oct 1, 2020

jaeopt Oct 20, 2020

msohailhussain Oct 1, 2020

msohailhussain Oct 1, 2020

mnoman09 Oct 1, 2020

yasirfolio3 left a comment

yasirfolio3 Oct 5, 2020

pawels-optimizely left a comment

msohailhussain left a comment

jaeopt left a comment

jaeopt Oct 20, 2020

jaeopt Oct 20, 2020

jaeopt left a comment

feat(flag-decisions): Add support for sending flag decisions along with decision metadata. #405

feat(flag-decisions): Add support for sending flag decisions along with decision metadata. #405

Conversation

mnoman09 commented Oct 1, 2020

Summary

Test plan

coveralls commented Oct 1, 2020 • edited Loading

Pull Request Test Coverage Report for Build 1580

💛 - Coveralls

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

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

yasirfolio3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawels-optimizely left a comment

Choose a reason for hiding this comment

msohailhussain left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

coveralls commented Oct 1, 2020 •

edited

Loading