refact(audience-logs): Added and refactored audience evaluation logs. #336

yasirfolio3 · 2020-06-29T12:27:20Z

Summary

Added and updated missing audience evaluation logs.

Issues

https://optimizely.atlassian.net/browse/OASIS-6545

coveralls · 2020-06-29T14:18:24Z

Coverage decreased (-0.003%) to 98.797% when pulling ed1315c on yasir/audit-audience-evaluation into bc78734 on master.

msohailhussain · 2020-06-30T19:07:39Z

I have done my first pass, please address.

msohailhussain

please address.

msohailhussain · 2020-06-30T18:43:53Z

Sources/Utils/Utils.swift

@@ -21,6 +21,8 @@ class Utils {
    // from auto-generated variable OPTIMIZELYSDKVERSION
    static var sdkVersion: String = OPTIMIZELYSDKVERSION

+    private static var jsonEncoder = JSONEncoder()


I am not comfortable to define here, while it's part of only one method.

msohailhussain · 2020-06-30T19:01:41Z

Sources/Implementation/DefaultDecisionService.swift

@@ -80,12 +80,23 @@ class DefaultDecisionService: OPTDecisionService {
        return bucketedVariation
    }

-    func isInExperiment(config: ProjectConfig, experiment: Experiment, userId: String, attributes: OptimizelyAttributes) -> Bool {
+    func isInExperiment(config: ProjectConfig, experiment: Experiment, userId: String, attributes: OptimizelyAttributes, isRule: Bool? = nil, loggingKey: String? = nil) -> Bool {


isRule doesn't make sense? I think if you are copying from python, then it's an enum which indiciats either its evaluating for rolloutRule or experiment.

Please revise.

msohailhussain · 2020-06-30T19:04:34Z

Sources/Data Model/Project.swift

+    let logger = OPTLoggerFactory.getLogger()
+
+    // Required since logger in not decodable
+    enum CodingKeys: String, CodingKey {


Where it's needed?

Adding let logger = OPTLoggerFactory.getLogger() caused the struct to throw error since logger does not confirm to Codable and Equatable protocols. The solution is to tell the struct explicitly which properties will confirm to Codable and Equatable.

msohailhussain · 2020-06-30T19:05:32Z

Sources/Data Model/Audience/AttributeValue.swift

@@ -231,7 +256,7 @@ extension AttributeValue {

        // valid range: [-2^53, 2^53]
        if abs(num) > pow(2, 53) {
-            throw OptimizelyError.evaluateAttributeValueOutOfRange(prettySrc(caller, target: number))
+            throw OptimizelyError.evaluateAttributeValueOutOfRange(condition, name)


Why removed prettySrc

Updated it as per python since it also doesn't log the source of the error.

msohailhussain · 2020-06-30T19:06:11Z

Sources/Data Model/Audience/AttributeValue.swift

@@ -214,7 +229,17 @@ extension AttributeValue {
        }
    }

-    func checkValidAttributeNumber(_ number: Any?, caller: String = #function) throws {
+    func isValidForExactMatcher() -> Bool {


Please move this method after checkvalidattributenumber

msohailhussain · 2020-07-02T00:11:42Z

Sources/Utils/Utils.swift

@@ -128,7 +126,7 @@ class Utils {
    }

    static func getConditionString<T: Encodable>(conditions: T) -> String {
-        if let jsonData = try? self.jsonEncoder.encode(conditions), let jsonString = String(data: jsonData, encoding: .utf8) {
+        if let jsonData = try? JSONEncoder().encode(conditions), let jsonString = String(data: jsonData, encoding: .utf8) {


This is expensive to initialize JSONEncoder on everycall, I think you may keep static inside the func. @jaeopt do you have any suggestion?

declaring static property inside a method throws this error: Static properties may only be declared on a type, solution to this is declaring private static let jsonEncoder = JSONEncoder() inside the class like i had implemented earlier.

declaring static property inside a method throws this error: Static properties may only be declared on a type, Workaround to this is declaring the property inside the class, similar to how it was done in the first commit.

msohailhussain · 2020-07-02T00:13:46Z

Sources/Implementation/DefaultDecisionService.swift

@@ -18,6 +18,11 @@ import Foundation

 class DefaultDecisionService: OPTDecisionService {

+    enum evaluationLogType: String {


Don't we have separate file for enums?

jaeopt

Looks good with a lot of details! A few errors should be fixed.
We also need more attention to avoid logging overhead for non-debug mode.

jaeopt · 2020-07-02T00:07:34Z

Sources/Data Model/Project.swift

@@ -42,6 +42,23 @@ struct Project: Codable, Equatable {
    var typedAudiences: [Audience]?
    var featureFlags: [FeatureFlag]
    var botFiltering: Bool?
+    let logger = OPTLoggerFactory.getLogger()


Can you add a space above to make it clear from the other JSON fields?

jaeopt · 2020-07-02T00:10:18Z

Sources/Implementation/DefaultDecisionService.swift

@@ -18,6 +18,11 @@ import Foundation

 class DefaultDecisionService: OPTDecisionService {

+    enum evaluationLogType: String {


Suggested change

enum evaluationLogType: String {

enum EvaluationLogType: String {

jaeopt · 2020-07-02T00:12:17Z

Sources/Implementation/DefaultDecisionService.swift

+        var finalLoggingKey = experiment.key
+        if let key = loggingKey {
+            finalLoggingKey = key
+        }


Suggested change

var finalLoggingKey = experiment.key

if let key = loggingKey {

finalLoggingKey = key

}

let finalLoggingKey = loggingKey ?? experiment.key

jaeopt · 2020-07-02T00:18:18Z

Sources/Implementation/DefaultDecisionService.swift


        do {
            if let conditions = experiment.audienceConditions {
+                logger.d(.evaluatingAudiencesCombined(evType, finalLoggingKey, Utils.getConditionString(conditions: conditions)))


Suggested change

logger.d(.evaluatingAudiencesCombined(evType, finalLoggingKey, Utils.getConditionString(conditions: conditions)))

logger.d {

return .evaluatingAudiencesCombined(evType, finalLoggingKey, Utils.getConditionString(conditions: conditions)))

}

"getConditionsString" will be expensive. Let's avoid this for normal flow (logLevel < .debug).

swift-sdk/Sources/Customization/Protocols/OPTLogger.swift

Line 51 in bc78734

func d(_ message: () -> String) {

jaeopt · 2020-07-02T00:19:07Z

Sources/Implementation/DefaultDecisionService.swift

@@ -100,23 +112,23 @@ class DefaultDecisionService: OPTDecisionService {
                    result = true
                }
            }
-            // backward compatibility with audiencIds list
+                // backward compatibility with audiencIds list


nit: fix aligment

jaeopt · 2020-07-02T00:19:45Z

Sources/Implementation/DefaultDecisionService.swift

            else if experiment.audienceIds.count > 0 {
                var holder = [ConditionHolder]()
                holder.append(.logicalOp(.or))
                for id in experiment.audienceIds {
                    holder.append(.leaf(.audienceId(id)))
                }
-
+                logger.d(.evaluatingAudiencesCombined(evType, finalLoggingKey, Utils.getConditionString(conditions: holder)))


Fix to logger.d{} for all non-trivial messages.

jaeopt · 2020-07-02T00:49:37Z

Sources/Data Model/Audience/UserAttribute.swift

@@ -90,49 +90,55 @@ extension UserAttribute {

    func evaluate(attributes: OptimizelyAttributes?) throws -> Bool {

+        let conditionString = Utils.getConditionString(conditions: self)


This conditionString buidling is expensive. Let's remove this from normal flow (non-error).
We can build it in each error case.

jaeopt · 2020-07-02T00:52:14Z

Sources/Optimizely/OptimizelyError.swift

+    case userAttributeNilValue(_ condition: String)
+    case nilAttributeValue(_ condition: String, _ key: String)


Do we need both of these? Looks like redundant. If so, let's use "userAttributeNilValue" to be consistent.

both serve different purposes:
userAttributeNilValue = Audience condition (\(condition)) evaluated to UNKNOWN because of null value.
nilAttributeValue = Audience condition (\(condition)) evaluated to UNKNOWN because a null value was passed for user attribute (\(key)).

jaeopt · 2020-07-02T00:56:50Z

Sources/Data Model/Audience/UserAttribute.swift

            }
        }

        switch matchFinal {
        case .exists:
            return !(rawAttributeValue is NSNull || rawAttributeValue == nil)
        case .exact:
-            return try value!.isExactMatch(with: rawAttributeValue!)
+            return try value!.isExactMatch(with: rawAttributeValue!, condition: conditionString, name: nameFinal)


This case is tough. How can we avoid "conditionString" buiding overhead?

Tried several ideas to evaluate conditionString just once for each UserAttribute. The problem i faced is that we are dealing with a struct which is a value type and to have a lazy or mutable property inside it, we would have to mark evaluate method as mutating.

One possible fix is to create "conditionsString" when UserAttribute is instantiated by datafile parsing (just like we did for "FeatureVariable.swift" for JSON type conversion. Then we can avoid "Utils.getConditionString".

jaeopt

A couple of fixes suggested.
Can you add test cases for all decision error messages?
Those tests will be a good spec for decision-error messages.

jaeopt · 2020-07-02T16:24:11Z

Sources/Optimizely/OptimizelyError.swift

+        case .nilAttributeValue(let condition, let key):    message = "Audience condition (\(condition)) evaluated to UNKNOWN because a null value was passed for user attribute (\(key))."
+        case .userAttributeInvalidName(let condition):      message = "Audience condition (\(condition)) evaluated to UNKNOWN because of invalid attribute name."


I see we need both. Can we change the order of these two lines so "userAttribute" and "attribute" errors can be grouped?

jaeopt · 2020-07-02T16:31:14Z

Sources/Data Model/Audience/UserAttribute.swift

            }
        }

        switch matchFinal {
        case .exists:
            return !(rawAttributeValue is NSNull || rawAttributeValue == nil)
        case .exact:
-            return try value!.isExactMatch(with: rawAttributeValue!)
+            return try value!.isExactMatch(with: rawAttributeValue!, condition: conditionString, name: nameFinal)


One possible fix is to create "conditionsString" when UserAttribute is instantiated by datafile parsing (just like we did for "FeatureVariable.swift" for JSON type conversion. Then we can avoid "Utils.getConditionString".

jaeopt

Can we add test cases for decision-logging?
Other changes look good. A couple of small changes suggested.

jaeopt · 2020-07-02T21:53:22Z

Sources/Data Model/Audience/UserAttribute.swift

@@ -24,6 +24,7 @@ struct UserAttribute: Codable, Equatable {
    var type: String?
    var match: String?
    var value: AttributeValue?
+    var stringRepresentation: String


nit: add a space to differentiate from JSON fields

Suggested change

var stringRepresentation: String

var stringRepresentation: String = ""

jaeopt · 2020-07-02T21:57:42Z

Sources/Data Model/Audience/UserAttribute.swift

+            // initializing with empty value before using self to avoid constructor warnings
+            self.stringRepresentation = ""


Let's remove these by setting default value above

jaeopt · 2020-07-02T21:57:57Z

Sources/Data Model/Audience/UserAttribute.swift

+        // initializing with empty value before using self to avoid constructor warnings
+        self.stringRepresentation = ""


Remove these too

jaeopt

We need test cases that validate the expected log messages printed out (instead of validating error types). Can you fix the test cases. See my comments for details.

jaeopt · 2020-07-06T22:40:30Z

Tests/OptimizelyTests-DataModel/AttributeValueTests.swift

        } catch {
            tmpError = error
        }
-        XCTAssertTrue(tmpError != nil)
+        XCTAssertEqual("[Optimizely][Error] " + tmpError!.localizedDescription, OptimizelyError.evaluateAttributeInvalidType(conditionString, invalidValue, attributeKey).localizedDescription)


These tests validate expected error "type"s, but not testing if the log messages are actually printed out.
We throw errors. At some point, SDK is expected to catch it and prints out the log message.
We need validate that the expected error messages are actually logged.

Hi @jaeopt , I have added new tests in decision service to verify if thrown errors are logged properly using MockLogger. i have also kept the newly added tests for UserAttribute and AttributeValue to test error handling but have removed assertion of log messages from them.

jaeopt

Error logging validation tests look great.
I suggest some changes for error-type checking.

jaeopt · 2020-07-08T16:13:53Z

Tests/OptimizelyTests-Common/DecisionServiceTests_Experiments.swift

+        HandlerRegistryService.shared.binders.property?.removeAll()
+        let binder: Binder = Binder<OPTLogger>(service: OPTLogger.self).to { () -> OPTLogger? in
+            return self.mockLogger
+        }
+        HandlerRegistryService.shared.registerBinding(binder: binder)


We can remove these lines.

Jae, DataStore binder sets its own logger which is causing problem here. All logs go there that's why reset it here.

jaeopt · 2020-07-08T18:16:39Z

Tests/OptimizelyTests-DataModel/AttributeValueTests.swift

        var tmpError: Error?
        do {
-            _ = try attr.isExactMatch(with: invalidValue, condition: conditionString, name: attributeKey)
+            _ = try attr.isExactMatch(with: invalidValue)
        } catch {
            tmpError = error
        }
-        XCTAssertEqual("[Optimizely][Error] " + tmpError!.localizedDescription, OptimizelyError.evaluateAttributeInvalidCondition(conditionString).localizedDescription)
+        XCTAssertNotNil(tmpError)


Can we change these non-nil checking to specific error type checking?

var tmpError: OptimizelyError? ... } catch { tmpError = error as? OptimizelyError } ... if case . evaluateAttributeInvalidCondition = tmpError { XCTAssert(true) } else { XCTAssert(false) }

I removed specific error type checking here since we are doing the same in the new decisionService tests.

I understand they're redundant. but it's better to have type checking here. Without them, these tests look incomplete.
Recovering your old XCTAssertEqual will also work.

jaeopt

It now all looks good!
Thanks for taking care of all the detailed fixes

msohailhussain

lgtm.

yasirfolio3 added 3 commits June 26, 2020 19:44

Initial commit.

8c5cb8a

minor fix.

2de57f0

fixes.

1c79eca

yasirfolio3 added the wip label Jun 29, 2020

yasirfolio3 assigned msohailhussain Jun 29, 2020

yasirfolio3 and others added 3 commits June 29, 2020 18:15

fixes.

dba9c96

fixes.

6f4f2e7

Merge branch 'master' into yasir/audit-audience-evaluation

fca7b01

yasirfolio3 mentioned this pull request Jun 30, 2020

fix(logs): Fixing log messages for Targeted Rollouts and feature variable evaluation. #337

Merged

msohailhussain requested a review from jaeopt June 30, 2020 19:06

msohailhussain changed the title ~~refact(audience-logs): Added and refactored audience evaluation logs. (WIP)~~ refact(audience-logs): Added and refactored audience evaluation logs. Jun 30, 2020

msohailhussain removed their assignment Jun 30, 2020

msohailhussain removed the wip label Jun 30, 2020

msohailhussain marked this pull request as ready for review June 30, 2020 19:07

msohailhussain requested a review from a team as a code owner June 30, 2020 19:07

optimizely-codereview-bot added 🤖 8 hrs 🤖 12 hrs and removed 🤖 8 hrs labels Jun 30, 2020

msohailhussain suggested changes Jul 1, 2020

View reviewed changes

yasirfolio3 self-assigned this Jul 1, 2020

optimizely-codereview-bot removed the 🤖 12 hrs label Jul 1, 2020

yasirfolio3 added 2 commits July 1, 2020 12:51

Addressed review.

ddd16c8

fixes.

6b09583

yasirfolio3 removed their assignment Jul 1, 2020

optimizely-codereview-bot added the 🤖 4 hrs label Jul 1, 2020

msohailhussain suggested changes Jul 2, 2020

View reviewed changes

jaeopt requested changes Jul 2, 2020

View reviewed changes

optimizely-codereview-bot removed the 🤖 4 hrs label Jul 2, 2020

optimizely-codereview-bot assigned yasirfolio3 Jul 2, 2020

Suggested changes made.

aaae35f

yasirfolio3 removed their assignment Jul 2, 2020

optimizely-codereview-bot requested review from jaeopt and msohailhussain July 2, 2020 10:38

jaeopt requested changes Jul 2, 2020

View reviewed changes

optimizely-codereview-bot assigned yasirfolio3 Jul 2, 2020

Addressed review

6189ef2

jaeopt requested changes Jul 2, 2020

View reviewed changes

Added tests and made suggested changes.

a9ca059

yasirfolio3 removed their assignment Jul 6, 2020

optimizely-codereview-bot requested a review from jaeopt July 6, 2020 10:27

optimizely-codereview-bot added the 🤖 4 hrs label Jul 6, 2020

jaeopt requested changes Jul 6, 2020

View reviewed changes

optimizely-codereview-bot removed the 🤖 4 hrs label Jul 6, 2020

optimizely-codereview-bot assigned yasirfolio3 Jul 6, 2020

yasirfolio3 added 3 commits July 7, 2020 20:06

Added new unit tests to verify if thrown errors are caught and logged.

27ea03a

bug fixed.

8b3e711

fixes.

4322a83

yasirfolio3 removed their assignment Jul 8, 2020

optimizely-codereview-bot requested a review from jaeopt July 8, 2020 09:55

fixes.

a7e7ece

jaeopt requested changes Jul 8, 2020

View reviewed changes

optimizely-codereview-bot assigned yasirfolio3 Jul 8, 2020

reverting change to AttributeValueTests.

ed1315c

jaeopt approved these changes Jul 8, 2020

View reviewed changes

msohailhussain approved these changes Jul 8, 2020

View reviewed changes

jaeopt merged commit 4ccb419 into master Jul 8, 2020

jaeopt deleted the yasir/audit-audience-evaluation branch July 8, 2020 21:20

		@@ -18,6 +18,11 @@ import Foundation

		class DefaultDecisionService: OPTDecisionService {

		enum evaluationLogType: String {

-                logger.d(.evaluatingAudiencesCombined(evType, finalLoggingKey, Utils.getConditionString(conditions: conditions)))
+                logger.d {
+                   return .evaluatingAudiencesCombined(evType, finalLoggingKey, Utils.getConditionString(conditions: conditions)))
+                }

		@@ -90,49 +90,55 @@ extension UserAttribute {

		func evaluate(attributes: OptimizelyAttributes?) throws -> Bool {

		let conditionString = Utils.getConditionString(conditions: self)

		case userAttributeNilValue(_ condition: String)
		case nilAttributeValue(_ condition: String, _ key: String)

		case .nilAttributeValue(let condition, let key): message = "Audience condition (\(condition)) evaluated to UNKNOWN because a null value was passed for user attribute (\(key))."
		case .userAttributeInvalidName(let condition): message = "Audience condition (\(condition)) evaluated to UNKNOWN because of invalid attribute name."

	var stringRepresentation: String
	var stringRepresentation: String = ""

		// initializing with empty value before using self to avoid constructor warnings
		self.stringRepresentation = ""

refact(audience-logs): Added and refactored audience evaluation logs. #336

refact(audience-logs): Added and refactored audience evaluation logs. #336

Uh oh!

Conversation

yasirfolio3 commented Jun 29, 2020

Summary

Issues

Uh oh!

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

Uh oh!

Uh oh!

msohailhussain commented Jun 30, 2020

Uh oh!

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

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!

jaeopt 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!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 29, 2020 •

edited

Loading

yasirfolio3 Jul 2, 2020 •

edited

Loading

jaeopt left a comment •

edited

Loading

yasirfolio3 Jul 8, 2020 •

edited

Loading