feat(ForcedDecisions): add forced-decisions APIs to OptimizelyUserContext #324

yasirfolio3 · 2021-09-28T12:37:33Z

Summary

Add a set of new APIs for forced-decisions to OptimizelyUserContext:

SetForcedDecision
GetForcedDecision
RemoveForcedDecision
RemoveAllForcedDecisions

Test plan

unit tests for the new APIs
FSC tests with new test cases

FSC Result

https://app.travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/builds/239804154

msohailhussain

partial review.

msohailhussain · 2021-09-28T21:57:11Z

pkg/client/client.go

@@ -28,6 +28,7 @@ import (
 	"github.com/optimizely/go-sdk/pkg/config"
 	"github.com/optimizely/go-sdk/pkg/decide"
 	"github.com/optimizely/go-sdk/pkg/decision"
+	pkgReasons "github.com/optimizely/go-sdk/pkg/decision/reasons"


any reason for this named package?

This is because there are several properties named reasons throughout the code.

msohailhussain · 2021-09-28T22:12:42Z

pkg/client/client.go

@@ -52,7 +53,7 @@ type OptimizelyClient struct {
 // CreateUserContext creates a context of the user for which decision APIs will be called.
 // A user context will be created successfully even when the SDK is not fully configured yet.
 func (o *OptimizelyClient) CreateUserContext(userID string, attributes map[string]interface{}) OptimizelyUserContext {
-	return newOptimizelyUserContext(o, userID, attributes)
+	return newOptimizelyUserContext(o, userID, attributes, nil)


why passing nil?

Since we don't have any instance of forcedDecisionService to pass since a new userContext is being created. In this case, newOptimizelyUserContext will create a forcedDecisionService internally by itself .

msohailhussain · 2021-09-28T22:18:10Z

pkg/client/client.go


-	featureDecision, reasons, err := o.DecisionService.GetFeatureDecision(decisionContext, usrContext, &allOptions)
+	// check forced-decisions first
+	variation, reasons, err := userContext.forcedDecisionService.FindValidatedForcedDecision(projectConfig, key, "", &allOptions)


add a comment, passing empty because checking mapping with flagKey only.

msohailhussain · 2021-09-28T22:26:14Z

pkg/decision/forced_decision_service.go

+// ForcedDecisionService defines user contexts that the SDK will use to make decisions for.
+type ForcedDecisionService struct {
+	UserID          string
+	forcedDecisions map[forcedDecision]string


I will recommend to use key instead of struct as key. Just think what is the json representation of the object structure.
@jaeopt @The-inside-man any comments on it?

Structs can be used as keys to a map if they hold comparable values which in our case is true since we have string values inside forcedDecision . https://go.dev/blog/maps
I have benchmarked both map of maps and the above approach. Both approaches take almost equal time to read and write.

@msohailhussain Since go allows for the use of structs as keys then I think this is fine. My assumption is that the struct becomes some hash value in the background and that is what allows for quick lookups, same as if it were a single value. The only thing we would need to be careful of is if later we extend the forced decision struct to contain more data, as if we then update that object, the key will be different and take more than one place in the map if we are not careful and removing the older instance. This can also lead to a memory leak possibly if we dont clean up old objects that "get lost in the map". I am not 100% sure the Go implementation of this, but in Java that is an issue and why it should be used very cautiously. @jaeopt Do we see any extension of the forcedDecisions in the future? If so I would say avoid this method, otherwise - it should be alright for now.

OK I checked it and struct is much faster than conventional mapOfMap.

2021/09/29 11:50:43 mapOfMaps took 545.791µs 2021/09/29 11:50:43 mapOfForcedDecisions took 270.387µs 2021/09/29 11:50:43 mapOfMaps took 460.888µs 2021/09/29 11:50:43 mapOfForcedDecisions took 294.126µs 2021/09/29 11:50:43 mapOfMaps took 498.434µs 2021/09/29 11:50:43 mapOfForcedDecisions took 240.871µs

Insertion is bit slower in struct keys.

2021/09/29 12:20:50 mapOfMaps took 914.17801ms 2021/09/29 12:20:53 forcedDecision took 2.856786962s

It's not likely to extend FD in future more than what we have, but cannot be guaranteed. I think it'll be safe as long as we have good test cases. Performance-wise, I don't care much since this is for testing environment only :) @msohailhussain @The-inside-man

msohailhussain · 2021-09-29T05:22:26Z

pkg/client/optimizely_user_context.go

 	// store a copy of the provided attributes so it isn't affected by changes made afterwards.
 	if attributes == nil {
 		attributes = map[string]interface{}{}
 	}
+	if forcedDecisionService == nil {
+		forcedDecisionService = decision.NewForcedDecisionService(userID)


do we need to pass userId?

userID is required here for logging purposes.

msohailhussain · 2021-09-29T05:27:00Z

pkg/config/datafileprojectconfig/mappers/forced_decision.go

+
+import (
+	datafileprojectconfig "github.com/optimizely/go-sdk/pkg/config/datafileprojectconfig/entities"
+	"github.com/optimizely/go-sdk/pkg/entities"


use named pkg, quite confusing.

since we are importing two entities files here, one has not be named.

msohailhussain · 2021-09-29T05:27:57Z

pkg/config/datafileprojectconfig/mappers/forced_decision.go

+		}
+	}
+
+	return flagRulesMap


don't need to return.

jaeopt

It overall looks great. See some suggestions and clarifications.

pkg/client/client.go

pkg/client/optimizely_user_context.go

pkg/config/datafileprojectconfig/mappers/forced_decision.go

pkg/decision/rollout_service.go

jaeopt · 2021-09-30T00:02:22Z

pkg/decision/forced_decision_service.go

+// ForcedDecisionService defines user contexts that the SDK will use to make decisions for.
+type ForcedDecisionService struct {
+	UserID          string
+	forcedDecisions map[forcedDecision]string


It's not likely to extend FD in future more than what we have, but cannot be guaranteed. I think it'll be safe as long as we have good test cases. Performance-wise, I don't care much since this is for testing environment only :) @msohailhussain @The-inside-man

pkg/decision/forced_decision_service.go

pkg/client/optimizely_user_context_test.go

pkg/decision/forced_decision_service_test.go

jaeopt

All changes look great!
I still see copying forcedDecision missing. Also consider optimization for creating forced-decision-service on the first setForcedDecision call. If not used for forced decision, it should be nil for normal decision flows.

yasirfolio3 · 2021-10-01T12:45:37Z

All changes look great! I still see copying forcedDecision missing. Also consider optimization for creating forced-decision-service on the first setForcedDecision call. If not used for forced decision, it should be nil for normal decision flows.

I have made the suggested changes but this is leading to all forcedDecision API's returning false or empty string until setForcedDecision is called once. We might have to update FSC tests accordingly too as currently, one of the tests expects remove_all_forced_decisions to return true in every case.

jaeopt

Looks good. A couple of changes suggested.

jaeopt · 2021-10-01T15:51:12Z

pkg/client/optimizely_user_context.go

@@ -85,21 +89,21 @@ func (o *OptimizelyUserContext) SetAttribute(key string, value interface{}) {
 // all data required to deliver the flag or experiment.
 func (o *OptimizelyUserContext) Decide(key string, options []decide.OptimizelyDecideOptions) OptimizelyDecision {
 	// use a copy of the user context so that any changes to the original context are not reflected inside the decision
-	userContextCopy := newOptimizelyUserContext(o.GetOptimizely(), o.GetUserID(), o.GetUserAttributes(), o.forcedDecisionService)
+	userContextCopy := newOptimizelyUserContext(o.GetOptimizely(), o.GetUserID(), o.GetUserAttributes(), o.getForcedDecisionService())


Shouldn't we use "o.forcedDecisionService" here? We do not want to CreateCopy until SetForcedDecision is called.

My bad. It looks good as is :)

jaeopt · 2021-10-01T15:53:51Z

pkg/client/optimizely_user_context.go

@@ -147,6 +160,9 @@ func (o *OptimizelyUserContext) RemoveAllForcedDecisions() bool {
 		o.optimizely.logger.Error("Optimizely instance is not valid, failing removeForcedDecision call.", err)
 		return false
 	}
+	if o.forcedDecisionService == nil {
+		return false


I also see this can be either "true" or "false". Logically I'd like to have "false" here, but can be a burden for FSC. @msohailhussain

Let's change this to "true". Logically false, but it should not change behavior depending on implementation. It still can be considered removed all successfully.

jaeopt

LGTM

jaeopt

Changes look good. Can we also remove the deprecation note from the PR summary?

jaeopt

LGTM

yasirfolio3 added 8 commits September 9, 2021 17:26

Initial commit for forced decision.

8588797

Adding new force_decision file.

3c6362e

nits fixed.

69f56e9

forced-decision implementations.

dbfba69

Implementations.

5271838

fixes.

32600af

Added unit tests.

856909c

nits fixed.

3f0d19c

yasirfolio3 requested a review from msohailhussain September 28, 2021 12:37

optimizely-codereview-bot assigned yasirfolio3 Sep 28, 2021

msohailhussain reviewed Sep 29, 2021

View reviewed changes

yasirfolio3 and others added 2 commits September 29, 2021 14:54

fixes.

3b036df

Merge branch 'master' into yasir/forced-decision

638783a

msohailhussain marked this pull request as ready for review September 29, 2021 18:53

msohailhussain requested a review from a team as a code owner September 29, 2021 18:53

jaeopt requested changes Sep 30, 2021

View reviewed changes

yasirfolio3 closed this Sep 30, 2021

yasirfolio3 deleted the yasir/forced-decision branch September 30, 2021 07:01

yasirfolio3 restored the yasir/forced-decision branch September 30, 2021 07:04

yasirfolio3 reopened this Sep 30, 2021

suggested changes made.

feb7647

yasirfolio3 removed their assignment Sep 30, 2021

optimizely-codereview-bot requested review from jaeopt, msohailhussain and The-inside-man September 30, 2021 16:54

jaeopt requested changes Oct 1, 2021

View reviewed changes

optimizely-codereview-bot assigned yasirfolio3 Oct 1, 2021

suggested changes made.

7cd2554

yasirfolio3 removed their assignment Oct 1, 2021

optimizely-codereview-bot requested a review from jaeopt October 1, 2021 11:52

suggested changes made.

4bad798

jaeopt requested changes Oct 1, 2021

View reviewed changes

optimizely-codereview-bot assigned yasirfolio3 Oct 1, 2021

suggested changes made.

db8fc1d

yasirfolio3 requested a review from jaeopt October 4, 2021 12:37

jaeopt approved these changes Oct 4, 2021

View reviewed changes

yasirfolio3 added 2 commits October 6, 2021 12:09

fixing unit tests.

a22ee99

removed deprication warnings.

5f5d52b

jaeopt requested changes Oct 7, 2021

View reviewed changes

jaeopt approved these changes Oct 11, 2021

View reviewed changes

fixing certificates issue for go version 1.10.x.

c3fac23

jaeopt approved these changes Oct 13, 2021

View reviewed changes

yasirfolio3 added 3 commits October 14, 2021 11:01

Updating forced-decision log messages to match with swift.

4d033d3

Adding unit tests to improve coverage.

808356e

Added new tests to assert forced decision logs.

6ccaf0d

msohailhussain closed this Oct 19, 2021

msohailhussain reopened this Oct 19, 2021

msohailhussain closed this Oct 20, 2021

msohailhussain reopened this Oct 20, 2021

msohailhussain merged commit 5cbe8e7 into master Oct 20, 2021

msohailhussain deleted the yasir/forced-decision branch October 20, 2021 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ForcedDecisions): add forced-decisions APIs to OptimizelyUserContext #324

feat(ForcedDecisions): add forced-decisions APIs to OptimizelyUserContext #324

yasirfolio3 commented Sep 28, 2021 •

edited

Loading

msohailhussain left a comment

msohailhussain Sep 28, 2021

yasirfolio3 Sep 29, 2021

msohailhussain Sep 28, 2021

yasirfolio3 Sep 29, 2021

msohailhussain Sep 28, 2021

msohailhussain Sep 28, 2021

yasirfolio3 Sep 29, 2021

The-inside-man Sep 29, 2021

msohailhussain Sep 29, 2021

msohailhussain Sep 29, 2021

msohailhussain Sep 29, 2021

jaeopt Sep 30, 2021

msohailhussain Sep 29, 2021

yasirfolio3 Sep 29, 2021

msohailhussain Sep 29, 2021

yasirfolio3 Sep 29, 2021

msohailhussain Sep 29, 2021

jaeopt left a comment

jaeopt Sep 30, 2021

jaeopt left a comment

yasirfolio3 commented Oct 1, 2021

jaeopt left a comment

jaeopt Oct 1, 2021

jaeopt Oct 1, 2021

jaeopt Oct 1, 2021

jaeopt Oct 1, 2021

jaeopt left a comment

jaeopt left a comment

jaeopt left a comment

feat(ForcedDecisions): add forced-decisions APIs to OptimizelyUserContext #324

feat(ForcedDecisions): add forced-decisions APIs to OptimizelyUserContext #324

Conversation

yasirfolio3 commented Sep 28, 2021 • edited Loading

Summary

Test plan

FSC Result

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

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

jaeopt left a comment

Choose a reason for hiding this comment

yasirfolio3 commented Oct 1, 2021

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

jaeopt left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

yasirfolio3 commented Sep 28, 2021 •

edited

Loading