feat(decide): Adding support for decide options and decision reasons. #298

yasirfolio3 · 2020-10-22T09:58:54Z

Summary

A part of multiple PRs for Decide API:

added DecideOptions.
added DecisionReasons.
Refactored interfaces with additional parameter of reasons and options.
Fixed unit tests throughout the sdk.

Tests

Unit tests added.

jaeopt

I understand that this PR extends existing interface to support changes for new decide APIs. It looks good in that sense.
Not sure if you plan to add them later, but I do not see collecting log messages into DecisionReasons.

jaeopt · 2020-11-06T18:04:12Z

pkg/decide/default_decision_reasons.go

+
+// AddError appends given message to the error list.
+func (o *DefaultDecisionReasons) AddError(format string, arguments ...interface{}) {
+	o.mutex.Lock()


Decision reasons should be managed per decision, so no need for concurrency control.

jaeopt · 2020-11-06T18:12:58Z

pkg/decision/composite_feature_service.go

 	"github.com/optimizely/go-sdk/pkg/entities"
 	"github.com/optimizely/go-sdk/pkg/logging"
 )

 // CompositeFeatureService is the default out-of-the-box feature decision service
 type CompositeFeatureService struct {
 	featureServices []FeatureService
-	logger logging.OptimizelyLogProducer
+	logger          logging.OptimizelyLogProducer


remove extra spaces

this indentation fix is on purpose, it should be aligned with the text above.

jaeopt · 2020-11-06T18:20:50Z

pkg/decision/evaluator/condition.go

 	"github.com/optimizely/go-sdk/pkg/decision/evaluator/matchers"
 	"github.com/optimizely/go-sdk/pkg/entities"
 	"github.com/optimizely/go-sdk/pkg/logging"
 )

 // ItemEvaluator evaluates a condition against the given user's attributes
 type ItemEvaluator interface {
-	Evaluate(interface{}, *entities.TreeParameters) (bool, error)
+	Evaluate(interface{}, *entities.TreeParameters, decide.OptimizelyDecideOptions, decide.DecisionReasons) (bool, error)


No need for OptimizelyDecideOptions here

jaeopt · 2020-11-06T18:22:01Z

pkg/decision/evaluator/condition.go

@@ -61,7 +61,7 @@ func (c CustomAttributeConditionEvaluator) Evaluate(condition entities.Condition
 		return false, fmt.Errorf(`invalid Condition matcher "%s"`, condition.Match)


Don't we need this to reasons? Basically, we should add all error/warning/info (and some of debug) log messages into reasons when they're decision-related.

@msohailhussain

we can probably put it within the reasons

jaeopt · 2020-11-06T18:22:21Z

pkg/decision/evaluator/condition.go

 	// We should only be evaluating custom attributes

 	if condition.Type != customAttributeType {
-
 		c.logger.Warning(fmt.Sprintf(logging.UnknownConditionType.String(), condition.StringRepresentation))
 		return false, fmt.Errorf(`unable to evaluate condition of type "%s"`, condition.Type)


Don't we need add this message to reasons?

@msohailhussain

jaeopt · 2020-11-06T23:31:11Z

pkg/decide/default_decision_reasons.go

+
+// ToReport returns reasons to be reported.
+func (o *DefaultDecisionReasons) ToReport() []string {
+	o.mutex.RLock()


Same. no need for mutex control here.

why do we need mutex here ?

jaeopt · 2020-11-07T00:09:35Z

pkg/decision/persisting_experiment_service_test.go

-	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext).Return(s.testComputedDecision, nil)
+	s.options = decide.OptimizelyDecideOptions{}
+	s.reasons = decide.NewDecisionReasons(s.options)
+	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext, s.options, s.reasons).Return(s.testComputedDecision, nil)
 }

 func (s *PersistingExperimentServiceTestSuite) TestNilUserProfileService() {


Not sure if it's planned to add later, but we need test cases to validate "IgnoreUPS" option here.

yes these tests are included in the succeeding PR. #299

yasirfolio3 · 2020-11-09T06:27:52Z

I understand that this PR extends existing interface to support changes for new decide APIs. It looks good in that sense.
Not sure if you plan to add them later, but I do not see collecting log messages into DecisionReasons.

yes i was waiting for one of the PR's on other sdk's to finalize so that i can add messages to decision reasons, will do that in another PR.

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

pkg/decision/experiment_bucketer_service.go

 		s.logger.Debug(fmt.Sprintf(logging.ExperimentAudiencesEvaluatedTo.String(), experiment.Key, evalResult))
 		if !evalResult {
 			s.logger.Debug(fmt.Sprintf(logging.UserNotInExperiment.String(), userContext.ID, experiment.Key))
-			experimentDecision.Reason = reasons.FailedAudienceTargeting
+			experimentDecision.Reason = pkgReasons.FailedAudienceTargeting


This comment was originally added by @jaeopt but got accidentally deleted:

This message should go into new DecisionReasons as well.
Wondering how we can merge this existing "Reason" with new DecisionReasons.

@msohailhussain

yeah, these make sense

jaeopt

LGTM - I expect all reasons collecting in new PRs

pawels-optimizely

reasons are passed to Evaluator, condition and Matchers, but I don't think they are used.
Please either clean them up (remove them) or use them.

pawels-optimizely · 2020-12-10T21:11:29Z

pkg/decide/decision_reasons_test.go

+		ExcludeVariables:         true,
+	}
+	reasons := NewDecisionReasons(options)
+	reasons.AddError("error message")


can you add a test with arguments ?

yasirfolio3 · 2020-12-11T06:26:50Z

reasons are passed to Evaluator, condition and Matchers, but I don't think they are used.
Please either clean them up (remove them) or use them.

We will be using them in a separate PR just dedicated to reasons with all the unit tests. 👍

msohailhussain

lgtm

msohailhussain · 2020-11-06T00:11:22Z

pkg/client/client.go

@@ -49,6 +49,8 @@ type OptimizelyClient struct {
 	defaultDecideOptions decide.OptimizelyDecideOptions
 }

+// Decide API


Please remove

yasirfolio3 and others added 13 commits October 14, 2020 16:18

initial commit.

2f52077

Adding unit tests.

e568bac

linter warnings fixed.

5792f2c

nit fixed.

26a25ec

fixes.

d98e193

fixes.

c7a0fd9

Merge branch 'master' into yasir/user-context-2

431f97a

Internal changes to accomodate decide api.

b223b56

Merge branch 'yasir/user-context-2' into yasir/decide-api

d8c4f49

linter warnings fixed.

6157c71

linter warnings fixed.

880fb08

removing added reasons, to be added in a separate PR.

91c1beb

linter warnings fixed.

8fc5bc5

yasirfolio3 requested a review from msohailhussain October 22, 2020 09:58

optimizely-codereview-bot assigned yasirfolio3 Oct 22, 2020

yasirfolio3 added 15 commits October 23, 2020 10:39

removing redundant info method.

564679e

suggested changes made.

f257a20

fixes.

4ab5403

Merge branch 'yasir/user-context-2' into yasir/decide-api-2

7d688c1

fixes.

f08ac01

Merge branch 'yasir/user-context-2' into yasir/decide-api-2

23cb870

Suggested changes made.

b841442

Merge branch 'yasir/user-context-2' into yasir/decide-api-2

9b7153a

removing unnecessary code.

d413572

nit fixed.

55dec9b

Updating decision reasons to a pointer.

e28ed7c

nits fixed.

f63b7f5

Merge branch 'yasir/user-context-2' into yasir/decide-api-2

0b6d43a

fixes.

ea619fa

Merge branch 'yasir/user-context-2' into yasir/decide-api-2.

bd34bd7

yasirfolio3 and others added 3 commits November 3, 2020 11:50

suggested changes made.

ef47d2a

Merge branch 'master' into yasir/user-context-2

6482bda

Updates and fixes.

1ffd78c

jaeopt requested changes Nov 7, 2020

View reviewed changes

optimizely deleted a comment from jaeopt Nov 9, 2020

yasirfolio3 added 2 commits November 9, 2020 11:28

fixes.

5f4a517

linter warnings fixed.

319dfd2

yasirfolio3 commented Nov 10, 2020

View reviewed changes

msohailhussain marked this pull request as ready for review November 10, 2020 22:34

msohailhussain requested a review from a team as a code owner November 10, 2020 22:34

jaeopt approved these changes Nov 10, 2020

View reviewed changes

yasirfolio3 added 2 commits November 24, 2020 14:05

Merge branch 'master' into yasir/user-context-2

b460a9a

Merge branch 'yasir/user-context-2' into yasir/decide-api-2

e113a70

Base automatically changed from yasir/user-context-2 to master December 10, 2020 19:16

Merge branch 'master' into yasir/decide-api-2

7f4b735

msohailhussain unassigned yasirfolio3 Dec 10, 2020

msohailhussain requested a review from pawels-optimizely December 10, 2020 19:19

pawels-optimizely suggested changes Dec 10, 2020

View reviewed changes

optimizely-codereview-bot assigned yasirfolio3 Dec 10, 2020

Added test for reasons with arguments.

5fc5e9f

yasirfolio3 requested a review from pawels-optimizely December 11, 2020 06:32

suggested changes made.

5084ef8

pawels-optimizely approved these changes Dec 15, 2020

View reviewed changes

msohailhussain approved these changes Dec 15, 2020

View reviewed changes

jaeopt merged commit 273fcd9 into master Dec 15, 2020

jaeopt deleted the yasir/decide-api-2 branch December 15, 2020 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(decide): Adding support for decide options and decision reasons. #298

feat(decide): Adding support for decide options and decision reasons. #298

yasirfolio3 commented Oct 22, 2020 •

edited

Loading

jaeopt left a comment

jaeopt Nov 6, 2020

jaeopt Nov 6, 2020

yasirfolio3 Nov 9, 2020

jaeopt Nov 6, 2020

jaeopt Nov 6, 2020

yasirfolio3 Nov 9, 2020

pawels-optimizely Dec 11, 2020

jaeopt Nov 6, 2020

yasirfolio3 Nov 9, 2020

jaeopt Nov 6, 2020

pawels-optimizely Dec 11, 2020

jaeopt Nov 7, 2020

yasirfolio3 Nov 9, 2020

yasirfolio3 commented Nov 9, 2020

yasirfolio3 Nov 10, 2020 •

edited

Loading

yasirfolio3 Nov 10, 2020

pawels-optimizely Dec 11, 2020

jaeopt left a comment

pawels-optimizely left a comment

pawels-optimizely Dec 10, 2020

yasirfolio3 Dec 11, 2020

yasirfolio3 commented Dec 11, 2020

msohailhussain left a comment

msohailhussain Nov 6, 2020

		@@ -61,7 +61,7 @@ func (c CustomAttributeConditionEvaluator) Evaluate(condition entities.Condition
		return false, fmt.Errorf(`invalid Condition matcher "%s"`, condition.Match)

feat(decide): Adding support for decide options and decision reasons. #298

feat(decide): Adding support for decide options and decision reasons. #298

Conversation

yasirfolio3 commented Oct 22, 2020 • edited Loading

Summary

Tests

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasirfolio3 commented Nov 9, 2020

yasirfolio3 Nov 10, 2020 • edited Loading

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

pawels-optimizely left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasirfolio3 commented Dec 11, 2020

msohailhussain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasirfolio3 commented Oct 22, 2020 •

edited

Loading

yasirfolio3 Nov 10, 2020 •

edited

Loading