Feat(AudienceEvalLogging): Added logging in Audience evaluation and its unit tests #251

mnoman09 · 2019-01-02T10:44:03Z

Summary

Added logging in audience evaluation code
Added its unit tests to verify logging

coveralls · 2019-01-02T14:11:25Z

Pull Request Test Coverage Report for Build 813

28 of 29 (96.55%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 90.01%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java	12	13	92.31%

Totals
Change from base Build 807:	0.2%
Covered Lines:	2721
Relevant Lines:	3023

💛 - Coveralls

coveralls · 2019-01-02T14:11:25Z

Pull Request Test Coverage Report for Build 858

35 of 37 (94.59%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 89.709%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/config/audience/AudienceIdCondition.java	4	5	80.0%
core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java	15	16	93.75%

Totals
Change from base Build 857:	0.3%
Covered Lines:	2711
Relevant Lines:	3022

💛 - Coveralls

loganlinn

This isn't specific to this PR, but is there a reason why we use String.format when we log? The Logger class can format more efficiently using the info(String format, Object... arguments) methods. @mnoman09, I know you were following the existing conventions; this is more pointed at @optimizely/fullstack-devs.

loganlinn · 2019-01-02T18:37:24Z

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

+            Match matchType = MatchType.getMatchType(match, value, this).getMatcher();
+            Boolean result = matchType.eval(userAttributeValue);
+
+            if (!matchType.getClass().toString().contains("Null") && result == null) {


substring matching the class name is a very nasty hack 👎
any logging specific to that class is better off being inside that class, i.e. the eval method of NullMatch in this case

loganlinn · 2019-01-02T18:53:17Z

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

        if (!"custom_attribute".equals(type)) {
-            MatchType.logger.error(String.format("condition type not equal to `custom_attribute` %s", type));
+            logger.error(String.format("Audience condition \"%s\" has an unknown condition type: %s", this.toString(), type));
+            logger.error(String.format("condition type not equal to `custom_attribute` %s", type));


single log line per error is highly preferred -- this is not a sufficient error message by itself. If we want additional information in the message, modify the existing logger.error() call, otherwise you can log "supplemental" information like this with a different log level here.

loganlinn · 2019-01-02T18:54:07Z

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

+                    //Missing attribute value
+                    logger.debug(String.format("Audience condition \"%s\" evaluated as UNKNOWN because no value was passed for user attribute \"%s\"", this.toString(), name));
+                } else {
+                    logger.warn(


not justified as warn level IMO

@loganlinn, to clarify, are you saying that this message is redundant with the warning that we would have logged in castToValueType?

As of commit 2be5a31, we're no longer logging a warning in castToValueType

loganlinn · 2019-01-02T19:01:40Z

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

                return new MatchType(matchType, new NullMatch());
        }

+        logger.warn(String.format("Audience condition \"%s\" condition value \"%s\" data type is inapplicable", userAttribute.toString(), conditionValue));


not justified as warn level IMO.

this would be more informative if we logged matchType.
also, think we shouldn't be logging conditionValue object -- it is potentially very expensive (depends on it's toString() impl) and has potential to log undesired data (PII).

not justified as warn level IMO.

I dunno, a customer probably won't expect an audience condition to become inactive for this reason, and therefore probably ought to be warned. If it helps, the updated log message suggests that the customer upgrade their SDK.

this would be more informative if we logged matchType.

It's included (along with type, name, and value) in our logging of the audience condition (the userAttribute variable).

also, think we shouldn't be logging conditionValue object -- it is potentially very expensive (depends on it's toString() impl) and has potential to log undesired data (PII).

Good feedback. We're not logging it any more!

loganlinn · 2019-01-02T19:05:22Z

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

    private Match matcher;

-    public static MatchType getMatchType(String matchType, Object conditionValue) {
+    public static MatchType getMatchType(String matchType, Object conditionValue, UserAttribute userAttribute) {


👎 to adding an argument to the signature just for logging

@loganlinn Do you have any suggestion to deal it, where we need to log with userAttributes. I am thinking to add some enum types for NullMatch, when NullMatch is returned, we'll also set NullMatchReasonType in the object. So In two or many more different cases we will check it using ReasonType under NullMatch object.

loganlinn · 2019-01-02T19:08:23Z

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

        try {
-            return conditions.evaluate(projectConfig, attributes);
+            Boolean result = conditions.evaluate(projectConfig, attributes);
+            logger.info(String.format("Audiences for experiment %s collectively evaluated as " + result, experiment.getKey()));


not justified as info level IMO.

I would rather leave it up to the user of SDK to log this on the caller side than to put here.

msohailhussain · 2019-01-03T20:50:20Z

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

        // check user attribute value is equal
        try {
-            return MatchType.getMatchType(match, value).getMatcher().eval(userAttributeValue);
+            Match matchType = MatchType.getMatchType(match, value, this.toString()).getMatcher();


please see my comments below for this position.

msohailhussain · 2019-01-03T20:50:47Z

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

    private Match matcher;

-    public static MatchType getMatchType(String matchType, Object conditionValue) {
+    public static MatchType getMatchType(String matchType, Object conditionValue, String audienceCondition) {


remove arg from here.

# Conflicts: # core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java # core-api/src/main/java/com/optimizely/ab/config/audience/match/AttributeMatch.java # core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java # core-api/src/test/java/com/optimizely/ab/internal/ExperimentUtilsTest.java

…nce condition

nchilada

Sorry for the late response! I left a bunch of comments; hope they're not too crazy. @msohailhussain @loganlinn I'd be happy to walk through these offline, if that helps

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

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

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

nchilada · 2019-01-15T08:19:13Z

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

+            return result;
        } catch (NullPointerException np) {
-            MatchType.logger.error(String.format("attribute or value null for match %s", match != null ? match : "legacy condition"), np);
+            logger.error(String.format("attribute or value null for match %s", match != null ? match : "legacy condition"), np);


Nit: This is a somewhat cryptic log message. @loganlinn @thomaszurkan-optimizely maybe we can audit our code to include all necessary null checks and then get rid of this NullPointerException catcher? Tho maybe it's not worthwhile if we don't expect to get here in practice...

nchilada · 2019-01-15T08:23:15Z

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

+                    //Missing attribute value
+                    logger.debug(String.format("Audience condition \"%s\" evaluated as UNKNOWN because no value was passed for user attribute \"%s\"", this.toString(), name));
+                } else {
+                    logger.warn(


@loganlinn, to clarify, are you saying that this message is redundant with the warning that we would have logged in castToValueType?

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

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

nchilada

Thanks for the updates! Lemme see if I can get another code review from someone who's more familiar with this.

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

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

nchilada

Thanks for the updates! And thanks for writing unit tests for this, too. It's looking good apart from a couple suggestions!

core-api/src/test/java/com/optimizely/ab/config/audience/AudienceConditionEvaluationTest.java

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

nchilada

One remaining request for the test cases!

core-api/src/test/java/com/optimizely/ab/config/audience/AudienceConditionEvaluationTest.java

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

loganlinn · 2019-01-17T19:00:32Z

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

+            Match matchType = MatchType.getMatchType(match, value).getMatcher();
+            Boolean result = matchType.eval(userAttributeValue);
+
+            if (!(matchType instanceof NullMatch) && result == null) {


I'm opposed to this approach. The instanceof and numerous logic branches are hard to follow and logging makes assumptions about the implementation details of matcher. This may be correct right now, but not guaranteed in the future. It smells out of being out of place and not a proper utilization of MatchType/Matcher/etc

Okay, smells a little funky to me too.

The alternative is logging from the matcher itself. Given the type of log message that we want, that means passing down the entire audience condition to the matcher, which I think you'd been unsure about. And we'd also want to pass an additional boolean if we want to preserve distinct messages for "no value" and "a null value".

How should we proceed?

My opinion (which I will also mention in other SDK's PRs) is that it's okay to pass additional data for logging purposes. @msohailhussain @mikeng13 FYI

Sorry, I'm so late to the party. But, I agree: It stinks. 2 questions on approach:

What's wrong with logging the null messages on the match type and remove the if instanceof?

Why not just create two exceptions and catch for those and log that way? It is way cleaner than this.

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

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

nchilada

Thanks for all the updates! The log messages look good to me, but will wait to hear @loganlinn's feedback on how the mismatch logs are instrumented.

thomaszurkan-optimizely

Can we change the NullMatchType errors to exceptions we catch with the error string already there?

thomaszurkan-optimizely · 2019-01-23T03:03:28Z

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

+            Match matchType = MatchType.getMatchType(match, value).getMatcher();
+            Boolean result = matchType.eval(userAttributeValue);
+
+            if (!(matchType instanceof NullMatch) && result == null) {


Sorry, I'm so late to the party. But, I agree: It stinks. 2 questions on approach:

What's wrong with logging the null messages on the match type and remove the if instanceof?

Why not just create two exceptions and catch for those and log that way? It is way cleaner than this.

thomaszurkan-optimizely · 2019-01-23T03:04:40Z

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

+                    this);
+            }
+            return result;
        } catch (NullPointerException np) {


Add a couple of new exception type instead of matchType instanceof NullMatch?

thomaszurkan-optimizely · 2019-01-23T20:10:49Z

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

-            MatchType.logger.error(String.format("attribute or value null for match %s", match != null ? match : "legacy condition"), np);
+            logger.error("attribute or value null for match {}", match != null ? match : "legacy condition", np);
+            return null;
+        } catch (NullMatch nm) {


This should be an exception. There should be one per error.

thomaszurkan-optimizely · 2019-01-23T20:12:09Z

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

+
+package com.optimizely.ab.config.audience.match;
+
+public enum NullMatchTypeErrors {


get rid of this class

create two exceptions UnknownMatchTypeException and UnexpectedValueTypeException.

thomaszurkan-optimizely · 2019-01-23T20:13:46Z

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

-
-    protected NullMatch() {
-        this.value = null;
+    protected NullMatch(NullMatchTypeErrors nullMatchTypeErrors) {


get rid of this constructor.

thomaszurkan-optimizely · 2019-01-23T20:16:24Z

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

 package com.optimizely.ab.config.audience.match;

-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+public class NullMatch extends Throwable {


create two new exceptions with the suffix Exception and set the appropriate errorDescription

thomaszurkan-optimizely

LGTM. Thanks!

Resolved

mnoman09 added 4 commits December 28, 2018 19:29

Refact(AudienceLogging): Added logging in audience evaluation

35c25c8

Updated headers

2664e1a

changed missing attribute log level to debug from warn

01ee254

potential bug fixes and added null test in isuserinexperiment tests

5da3ac8

loganlinn previously requested changes Jan 2, 2019

View reviewed changes

mnoman09 added 2 commits January 3, 2019 01:36

Refact and changed log message of missing attribute value

8fa2710

Code refact

1de986a

msohailhussain reviewed Jan 3, 2019

View reviewed changes

mnoman09 added 2 commits January 8, 2019 16:20

Added matchType Error enum to get match type error and log with audie…

5b35599

…nce condition

nchilada suggested changes Jan 15, 2019

View reviewed changes

Updated logging with latest changes in audience logging docs

5957ef1

nchilada suggested changes Jan 16, 2019

View reviewed changes

core-api/src/main/java/com/optimizely/ab/config/audience/match/MatchTypeError.java Outdated Show resolved Hide resolved

core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java Outdated Show resolved Hide resolved

mnoman09 and others added 2 commits January 16, 2019 10:47

Merge branch 'master' into mnoman/audienceEvalLog

cb2e00e

Refactored code and edited some logs

2be5a31

nchilada suggested changes Jan 17, 2019

View reviewed changes

nit changes

e7c8ae7

nchilada suggested changes Jan 17, 2019

View reviewed changes

core-api/src/test/java/com/optimizely/ab/config/audience/AudienceConditionEvaluationTest.java Outdated Show resolved Hide resolved

loganlinn reviewed Jan 17, 2019

View reviewed changes

core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java Outdated Show resolved Hide resolved

loganlinn reviewed Jan 17, 2019

View reviewed changes

thomaszurkan-optimizely suggested changes Jan 17, 2019

View reviewed changes

core-api/src/main/java/com/optimizely/ab/config/audience/match/MatchTypeError.java Outdated Show resolved Hide resolved

core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java Outdated Show resolved Hide resolved

mnoman09 added 2 commits January 18, 2019 16:35

made expected logging inline

e01c9b5

header update

8b8c4a7

nchilada approved these changes Jan 18, 2019

View reviewed changes

thomaszurkan-optimizely suggested changes Jan 23, 2019

View reviewed changes

thomaszurkan-optimizely and others added 2 commits January 23, 2019 11:59

Merge branch 'master' into mnoman/audienceEvalLog

cb15130

requested changes - on behalf of Noman

4244ebf

thomaszurkan-optimizely suggested changes Jan 23, 2019

View reviewed changes

msohailhussain and others added 2 commits January 24, 2019 11:35

tom commits

f8d2d87

Merge branch 'master' into mnoman/audienceEvalLog

e7310a1

thomaszurkan-optimizely approved these changes Jan 24, 2019

View reviewed changes

thomaszurkan-optimizely merged commit 28097f0 into master Jan 24, 2019

nchilada deleted the mnoman/audienceEvalLog branch January 24, 2019 22:08

loganlinn mentioned this pull request Jan 28, 2019

Rework attribute audience conditions #259

Closed


		package com.optimizely.ab.config.audience.match;

		public enum NullMatchTypeErrors {

Feat(AudienceEvalLogging): Added logging in Audience evaluation and its unit tests #251

Feat(AudienceEvalLogging): Added logging in Audience evaluation and its unit tests #251

Uh oh!

Conversation

mnoman09 commented Jan 2, 2019

Summary

Uh oh!

coveralls commented Jan 2, 2019

Pull Request Test Coverage Report for Build 813

💛 - Coveralls

Uh oh!

coveralls commented Jan 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 858

💛 - Coveralls

Uh oh!

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

nchilada left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nchilada left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nchilada left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nchilada left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

coveralls commented Jan 2, 2019 •

edited

Loading

nchilada Jan 18, 2019 •

edited

Loading