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

yasirfolio3 · 2020-10-01T11:52:35Z

Summary

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

Test summary

Added unit tests.

coveralls · 2020-10-01T12:09:33Z

Coverage increased (+0.04%) to 98.831% when pulling 397a51d on yasir/flag-decisions into b1d0962 on master.

msohailhussain · 2020-10-05T21:55:15Z

Sources/Data Model/Project.swift

@@ -42,6 +42,7 @@ struct Project: Codable, Equatable {
    var typedAudiences: [Audience]?
    var featureFlags: [FeatureFlag]
    var botFiltering: Bool?
+    var sendFlagDecisions: Bool?


It's not nullable.

Since Project struct is marked codable, for every value that might not exist, we have to mark it as optional, otherwise decoding a datafile with no sendFlagDecisions into Project struct will cause a crash.

msohailhussain

please address.

msohailhussain · 2020-10-05T21:56:07Z

Sources/Data Model/ProjectConfig.swift

@@ -177,6 +177,13 @@ extension ProjectConfig {

 extension ProjectConfig {

+    /**
+     * Determines whether impressions events are sent for ALL decision types.


Get sendFlagDecisions values...

msohailhussain · 2020-10-05T21:57:44Z

Sources/Implementation/DefaultDecisionService.swift

        //Evaluate in this order:

        //1. Attempt to bucket user into experiment using feature flag.
        // Check if the feature flag is under an experiment and the the user is bucketed into one of these experiments
        if let pair = getVariationForFeatureExperiment(config: config, featureFlag: featureFlag, userId: userId, attributes: attributes) {
-            return pair
+            return (pair.experiment, pair.variation, Constants.DecisionSource.featureTest.rawValue)


why not returning pair? discuss offline.

msohailhussain · 2020-10-05T22:00:40Z

Sources/Implementation/Events/BatchEventBuilder.swift

+
+        var variationId = ""
+        var variationKey = ""
+        if let tmpVariation = variation {


why using it?
variation?.Id should be fine.

Value of optional type 'String?' must be unwrapped to a value of type 'String' before assigning it into the struct. This change is there since variation can now be nil too.

msohailhussain · 2020-10-05T22:01:25Z

Sources/Optimizely/OptimizelyClient.swift

            sendDecisionNotification(decisionType: .feature,
                                     userId: userId,
                                     attributes: attributes,
+                                     source: Constants.DecisionSource.rollout.rawValue,


need to check.

msohailhussain · 2020-10-05T22:02:27Z

Sources/Optimizely/OptimizelyClient.swift

-                                          attributes: attributes,
-                                          event: event,
-                                          async: false)
+            if let tmpVariation = variation {


Why need to add this condition?

Variation was non-optional before so we didn't have to unwrap it, but since we have allowed impression event's to be sent with nil variation, this is necessary!

pawels-optimizely

lgtm

2. Added tests for rule_key in metadata.

jaeopt

Overall looks good. See my comments for change requests.

jaeopt · 2020-10-09T17:39:37Z

Sources/Data Model/Project.swift

@@ -52,13 +53,13 @@ struct Project: Codable, Equatable {
        // V3
        case anonymizeIP
        // V4
-        case rollouts, typedAudiences, featureFlags, botFiltering
+        case rollouts, typedAudiences, featureFlags, botFiltering, sendFlagDecisions
    }

    // Required since logger in not equatable


Suggested change

// Required since logger in not equatable

// Required since logger is not equatable

jaeopt · 2020-10-09T17:42:57Z

Sources/Data Model/ProjectConfig.swift

+    func sendFlagDecisions() -> Bool {
+        return project.sendFlagDecisions ?? false
+    }


The function name misleads as an action. Let's change it to comp variable.

Suggested change

func sendFlagDecisions() -> Bool {

return project.sendFlagDecisions ?? false

}

var sendFlagDecisions: Bool {

return project.sendFlagDecisions ?? false

}

jaeopt · 2020-10-09T17:56:47Z

Sources/Implementation/DefaultDecisionService.swift

    }

    func getVariationForFeatureExperiment(config: ProjectConfig,
                                          featureFlag: FeatureFlag,
                                          userId: String,
-                                          attributes: OptimizelyAttributes) -> (experiment: Experiment?, variation: Variation?)? {
+                                          attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation?, source: String)? {


I think we can remove option from Variation.

Suggested change

attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation?, source: String)? {

attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation, source: String)? {

jaeopt · 2020-10-09T17:57:17Z

Sources/Implementation/DefaultDecisionService.swift

@@ -174,7 +173,7 @@ class DefaultDecisionService: OPTDecisionService {
    func getVariationForFeatureRollout(config: ProjectConfig,
                                       featureFlag: FeatureFlag,
                                       userId: String,
-                                       attributes: OptimizelyAttributes) -> Variation? {
+                                       attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation?, source: String)? {


I think we can remove option from Variation.

Suggested change

attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation?, source: String)? {

attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation, source: String)? {

jaeopt · 2020-10-09T17:57:38Z

Sources/Implementation/DefaultDecisionService.swift

@@ -131,7 +131,7 @@ class DefaultDecisionService: OPTDecisionService {
        return result
    }

-    func getVariationForFeature(config: ProjectConfig, featureFlag: FeatureFlag, userId: String, attributes: OptimizelyAttributes) -> (experiment: Experiment?, variation: Variation?)? {
+    func getVariationForFeature(config: ProjectConfig, featureFlag: FeatureFlag, userId: String, attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation?, source: String)? {


I think we can remove option from Variation.

Suggested change

func getVariationForFeature(config: ProjectConfig, featureFlag: FeatureFlag, userId: String, attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation?, source: String)? {

func getVariationForFeature(config: ProjectConfig, featureFlag: FeatureFlag, userId: String, attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation, source: String)? {

jaeopt · 2020-10-09T18:26:56Z

Sources/Implementation/Events/BatchEventBuilder.swift

+        var variationKey = ""
+        if let tmpVariation = variation {
+            variationKey = tmpVariation.key
+            variationId = tmpVariation.id


Do we send a decision event when variation is nil?

yes we do now.

jaeopt · 2020-10-09T20:05:08Z

Sources/Protocols/OPTDecisionService.swift

@@ -44,6 +44,6 @@ protocol OPTDecisionService {
     - Parameter attributes: User attributes
     - Returns: The variation assigned to the specified user ID for a feature flag.
     */
-    func getVariationForFeature(config: ProjectConfig, featureFlag: FeatureFlag, userId: String, attributes: OptimizelyAttributes) -> (experiment: Experiment?, variation: Variation?)?
+    func getVariationForFeature(config: ProjectConfig, featureFlag: FeatureFlag, userId: String, attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation?, source: String)?


same as above - can we remove option for variationKey?

jaeopt · 2020-10-09T20:07:47Z

Sources/Optimizely/OptimizelyClient.swift

@@ -372,13 +374,10 @@ open class OptimizelyClient: NSObject {
            return false
        }

-        let pair = decisionService.getVariationForFeature(config: config,
+        guard let pair = decisionService.getVariationForFeature(config: config,


Even when pair is nil (rollout decision fails even after the last rule), we still need to send a decision event. Check with @msohailhussain

jaeopt · 2020-10-09T21:09:30Z

Tests/OptimizelyTests-Common/DecisionServiceTests_Features.swift

                                                                           featureFlag: featureFlag,
                                                                           userId: kUserId,
                                                                           attributes: kAttributesRolloutAge2Match)
-        XCTAssertNil(variation)
+        XCTAssertNil(pair)


Can we add test cases for decision event sent for this nil pair cases?

jaeopt · 2020-10-09T21:18:03Z

Tests/OptimizelyTests-Common/BatchEventBuilderTests_Events.swift

+        let metaData = decision["metadata"] as! Dictionary<String, Any>
+        XCTAssertEqual(metaData["rule_type"] as! String, "experiment")
+        XCTAssertEqual(metaData["rule_key"] as! String, "ab_running_exp_audience_combo_exact_foo_or_true__and__42_or_4_2")
+        XCTAssertEqual(metaData["flag_key"] as! String, "")


Does this test pass? I understand "flag_key" set to same as "rule_key" for activate.

jaeopt

Refactor for non-optional variation look great!
A couple of changes requested as the spec was clarified in the meeting today.

it looks like a dummy event still not sent out when a variation is decided to nil (we also need a test case for this)
activate should send an event with a valid flag-key for feature-test.

jaeopt · 2020-10-12T18:14:02Z

Sources/Optimizely/OptimizelyClient.swift

@@ -385,8 +385,7 @@ open class OptimizelyClient: NSObject {
                                     featureEnabled: false)
            return false


We still need to send an impression event with a dummy metadata even when pair is null. These lines should be refactored to support it.

We also need to change the impression-event from activate call. When the experimentKey is for feature-test (not ab-test) we should send a valid flag-key (instead of copying experiment-key).

msohailhussain · 2020-10-13T17:08:55Z

Sources/Implementation/DefaultDecisionService.swift

@@ -41,7 +41,7 @@ class DefaultDecisionService: OPTDecisionService {

        // ---- check if the user is forced into a variation ----
        if let variationId = config.getForcedVariation(experimentKey: experiment.key, userId: userId)?.id,
-            let variation = experiment.getVariation(id: variationId) {
+           let variation = experiment.getVariation(id: variationId) {


please fix indentation.

msohailhussain

look good, but please fix indentation issues, empty spaces and i saw some other spacing issues.

msohailhussain

lgtm. @jaeopt please check indentation rules are changed in xcode 12.

jaeopt

A couple of small changes suggested. Other than that, it looks good!

Also I keep getting Swiftlint error on this line ("line is too long"), wondering why you do not see it.

swift-sdk/Sources/Data Model/Project.swift

Line 60 in b1d0962

    
           return lhs.version == rhs.version && lhs.projectId == rhs.projectId && lhs.experiments == rhs.experiments &&

jaeopt · 2020-10-14T00:11:08Z

Sources/Implementation/DefaultDecisionService.swift

@@ -147,6 +147,7 @@ class DefaultDecisionService: OPTDecisionService {
        }

        return nil
+


Remove this empty line

jaeopt · 2020-10-14T00:21:57Z

Sources/Optimizely/OptimizelyClient.swift

@@ -259,7 +259,7 @@ open class OptimizelyClient: NSObject {
                            variation: variation,
                            userId: userId,
                            attributes: attributes,
-                            flagKey: experimentKey,
+                            flagKey: "",
                            ruleType: "experiment")


Do we have this new ruleType "experiment"? If so, can we add it to DecisionSource enum?

jaeopt · 2020-10-14T00:40:41Z

Tests/OptimizelyTests-Common/DecisionListenerTests.swift

+
+        let event = getFirstEventJSON()!
+        let visitor = (event["visitors"] as! Array<Dictionary<String, Any>>)[0]
+        let snapshot = (visitor["snapshots"] as! Array<Dictionary<String, Any>>)[0]
+        let decision = (snapshot["decisions"]  as! Array<Dictionary<String, Any>>)[0]
+
+        let metaData = decision["metadata"] as! Dictionary<String, Any>
+        XCTAssertEqual(metaData["rule_type"] as! String, Constants.DecisionSource.rollout.rawValue)
+        XCTAssertEqual(metaData["rule_key"] as! String, "")
+        XCTAssertEqual(metaData["flag_key"] as! String, "feature_1")
+        XCTAssertEqual(metaData["variation_key"] as! String, "")
+
+        self.optimizely.config!.project!.sendFlagDecisions = nil


This test looks good, but looks like in a wrong place, not related to Notification. Can we move this part to other like BathEventTests?

jaeopt

LGTM - nit: fix indent

jaeopt · 2020-10-14T16:27:36Z

Tests/OptimizelyTests-Common/BatchEventBuilderTests_Events.swift

+        _ = fakeOptimizelyManager.isFeatureEnabled(featureKey: featureKey, userId: userId)
+
+        let result = XCTWaiter.wait(for: [exp], timeout: 0.1)
+         if result == XCTWaiter.Result.timedOut {


Fix indentation in all three tests

pawels-optimizely · 2020-10-16T23:36:20Z

Tests/OptimizelyTests-APIs/OptimizelyClientTests_Others.swift

@@ -290,7 +290,7 @@ class OptimizelyClientTests_Others: XCTestCase {
        // set invalid (infinity) to attribute values, which will cause JSONEncoder.encode exception
        let attributes = ["testvar": Double.infinity]

-        optimizely.sendImpressionEvent(experiment: experiment, variation: variation, userId: kUserId, attributes: attributes)
+        optimizely.sendImpressionEvent(experiment: experiment, variation: variation, userId: kUserId, attributes: attributes, flagKey: experiment.key, ruleType: Constants.DecisionSource.rollout.rawValue)


don't assign experiment.key to flagKey even in tests

pawels-optimizely

looks good, one nit comment, I hope it ran through FSC tests.

yasirfolio3 · 2020-10-19T06:11:20Z

looks good, one nit comment, I hope it ran through FSC tests.

suggested changes made, Have also verified it with fsc flag-decision tests.

msohailhussain

lgtm

pawels-optimizely

lgtm

jaeopt

LGTM

yasirfolio3 added 2 commits September 30, 2020 17:25

Initial commit.

5bdd20b

Tests added.

7a935f7

yasirfolio3 added the wip label Oct 1, 2020

yasirfolio3 requested a review from msohailhussain October 1, 2020 11:52

msohailhussain reviewed Oct 5, 2020

View reviewed changes

msohailhussain requested changes Oct 5, 2020

View reviewed changes

msohailhussain marked this pull request as ready for review October 5, 2020 22:03

msohailhussain requested a review from a team as a code owner October 5, 2020 22:03

msohailhussain changed the title ~~DO NOT REVIEW: feat(flag-decisions): Add support for sending flag decisions along with decision metadata.~~ feat(flag-decisions): Add support for sending flag decisions along with decision metadata. Oct 5, 2020

fixes.

116a6fb

yasirfolio3 requested a review from msohailhussain October 6, 2020 12:37

msohailhussain removed the wip label Oct 6, 2020

msohailhussain requested review from jaeopt and pawels-optimizely October 6, 2020 18:00

optimizely-codereview-bot added the 🤖 24 hrs label Oct 6, 2020

pawels-optimizely previously approved these changes Oct 6, 2020

View reviewed changes

pawels-optimizely and others added 2 commits October 6, 2020 12:29

Merge branch 'master' into yasir/flag-decisions

69d6e22

1. Updated flag_type to rule_type.

9712a60

2. Added tests for rule_key in metadata.

yasirfolio3 dismissed pawels-optimizely’s stale review via 9712a60 October 8, 2020 07:40

fixes.

974f4d9

jaeopt requested changes Oct 9, 2020

View reviewed changes

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

optimizely-codereview-bot assigned yasirfolio3 Oct 9, 2020

yasirfolio3 added 2 commits October 12, 2020 14:55

suggested changes made.

0b501a3

fixed by reverting some changes.

1bd30c8

jaeopt requested changes Oct 12, 2020

View reviewed changes

yasirfolio3 added 2 commits October 13, 2020 13:13

fixes as per discussion.

79a9241

nit fixed.

08d2abe

yasirfolio3 removed their assignment Oct 13, 2020

msohailhussain reviewed Oct 13, 2020

View reviewed changes

msohailhussain requested changes Oct 13, 2020

View reviewed changes

yasirfolio3 added 2 commits October 13, 2020 22:57

fixing indentation issues.

4753176

indentation fixes.

1dc28ac

yasirfolio3 requested a review from msohailhussain October 13, 2020 18:27

msohailhussain previously approved these changes Oct 13, 2020

View reviewed changes

optimizely-codereview-bot added the 🤖 4 hrs label Oct 13, 2020

jaeopt requested changes Oct 14, 2020

View reviewed changes

optimizely-codereview-bot removed the 🤖 4 hrs label Oct 14, 2020

optimizely-codereview-bot assigned yasirfolio3 Oct 14, 2020

suggested changes made.

4f3f06f

yasirfolio3 dismissed msohailhussain’s stale review via 4f3f06f October 14, 2020 07:50

yasirfolio3 requested a review from jaeopt October 14, 2020 07:50

jaeopt previously approved these changes Oct 14, 2020

View reviewed changes

indentation fixed.

7c0f6ef

yasirfolio3 dismissed jaeopt’s stale review via 7c0f6ef October 16, 2020 13:58

pawels-optimizely reviewed Oct 16, 2020

View reviewed changes

pawels-optimizely previously approved these changes Oct 16, 2020

View reviewed changes

nit fixed.

397a51d

yasirfolio3 dismissed pawels-optimizely’s stale review via 397a51d October 19, 2020 05:37

yasirfolio3 requested a review from pawels-optimizely October 19, 2020 06:11

yasirfolio3 removed their assignment Oct 19, 2020

optimizely-codereview-bot requested review from jaeopt and msohailhussain October 19, 2020 06:11

msohailhussain approved these changes Oct 20, 2020

View reviewed changes

pawels-optimizely approved these changes Oct 20, 2020

View reviewed changes

jaeopt approved these changes Oct 20, 2020

View reviewed changes

pawels-optimizely merged commit 3190808 into master Oct 20, 2020

pawels-optimizely deleted the yasir/flag-decisions branch October 20, 2020 21:10

	// Required since logger in not equatable
	// Required since logger is not equatable

	attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation?, source: String)? {
	attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation, source: String)? {

	func getVariationForFeature(config: ProjectConfig, featureFlag: FeatureFlag, userId: String, attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation?, source: String)? {
	func getVariationForFeature(config: ProjectConfig, featureFlag: FeatureFlag, userId: String, attributes: OptimizelyAttributes) -> (experiment: Experiment, variation: Variation, source: String)? {

		@@ -385,8 +385,7 @@ open class OptimizelyClient: NSObject {
		featureEnabled: false)
		return false

		@@ -147,6 +147,7 @@ class DefaultDecisionService: OPTDecisionService {
		}

		return nil

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

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

Conversation

yasirfolio3 commented Oct 1, 2020

Summary

Test summary

coveralls commented Oct 1, 2020 • edited Loading

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

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

pawels-optimizely 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

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

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

Choose a reason for hiding this comment

msohailhussain 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 • 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

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

pawels-optimizely left a comment

Choose a reason for hiding this comment

yasirfolio3 commented Oct 19, 2020

msohailhussain left a comment

Choose a reason for hiding this comment

pawels-optimizely left a 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

jaeopt left a comment •

edited

Loading