feat(ats): add ODP integration. #355

yasirfolio3 · 2022-11-24T09:59:53Z

Summary

This PR adds a support for Optimizely Data Platform (ODP) integration to Full Stack. With this extension, clients may not need to pre-determine and include user segments in attributes. SDK can fetch user segments from the ODP server for the current user.

Add a new public API to OptimizelyUserContext (fetchQualifiedSegments).
Add a new public API to OptimizelyClient (SendOdpEvent).

Test plan

Tests for OptimizelyUserContext new APIs.
Tests for OptimizelyClient new APIs.
Tests for odpManager.

Issues

FSSDK-8516

msohailhussain

few minor comments, will review again.

msohailhussain · 2022-11-30T07:36:41Z

pkg/client/client.go

 // 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 {
 	// Passing qualified segments as nil initially since they will be fetched later
+	if o.identify {


is the comment above is valid for this line?

msohailhussain · 2022-11-30T07:38:07Z

pkg/client/client.go

 	return o.decideForKeys(userContext, allFlagKeys, options)
 }

+func (o *OptimizelyClient) fetchQualifiedSegments(userContext *OptimizelyUserContext, options []pkgOdpSegment.OptimizelySegmentOption, callback func(segments []string, err error)) {


documentation?

msohailhussain · 2022-11-30T07:38:59Z

pkg/client/client.go

+	userContext.SetQualifiedSegments(nil)
+
+	_, err = o.getProjectConfig()
+	if err != nil {


can't we combine these two conditions above & below.

msohailhussain · 2022-11-30T07:41:54Z

pkg/client/client.go

+	// Async
+	if callback != nil {
+		go func() {
+			callback(fetchAndSetSegments())


there may be inconsistency / race condition issue when you call routine. thoughts?

All internal implementations are using locks where required so this should not be a problem.

msohailhussain · 2022-11-30T07:42:41Z

pkg/client/factory.go

-	userProfileService   decision.UserProfileService
-	overrideStore        decision.ExperimentOverrideStore
-	metricsRegistry      metrics.Registry
+	identify              *bool


alphabetize please.

pkg/client/factory.go

msohailhussain · 2022-11-30T07:45:58Z

pkg/config/datafileprojectconfig/config.go

 	var hostForODP, publicKeyForODP string
 	for _, integration := range datafile.Integrations {
-		if integration.Key == "odp" {
+		if integration.Key == nil {


What about if it's empty.

empty key is allowed.

msohailhussain · 2022-11-30T07:47:12Z

pkg/config/datafileprojectconfig/entities/entities.go

-	Key       string `json:"key"`
-	Host      string `json:"host"`
-	PublicKey string `json:"publicKey"`
+	Key       *string `json:"key"`


take it as type

do you mean we should rename it to type? pointer to the string is required here since string type in golang cannot be nil by itself.

discussed offline.

jaeopt

It looks good overall. It's a quite big PR. A few top-level suggestions first.

jaeopt · 2022-11-28T18:39:38Z

pkg/client/client.go

 // 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 {
 	// Passing qualified segments as nil initially since they will be fetched later
+	if o.identify {


Can't we use odp.Manager == null for identify event instead of this separate flag? Or we can call send event and odpManager can decide if to send or not.

jaeopt · 2022-11-29T19:11:50Z

pkg/client/optimizely_user_context.go

+// FetchQualifiedSegments fetches all qualified segments for the user context.
+func (o *OptimizelyUserContext) FetchQualifiedSegments(options []pkgOdpSegment.OptimizelySegmentOption, callback func(segments []string, err error)) {
+	o.optimizely.fetchQualifiedSegments(o, options, callback)
+}
+


Can we split into 2 apis (sync and async)? Sync api returns a boolean (success/failure) and async callback is called with the boolean. https://github.com/optimizely/java-sdk/blob/b16dff5f7d958ea36b94e1cfd8d0bdd0b2aae7f8/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java#L316

if (FetchQualifiedSegments()) { Decide() }

FetchQualifiedSegments(callback: (status) -> { if(status) Decide() })

jaeopt · 2022-11-29T19:23:22Z

pkg/config/datafileprojectconfig/entities/entities.go

+	Host      string  `json:"host"`
+	PublicKey string  `json:"publicKey"`


Wondering why Key is a pointer while Host/PublicKey are not. They are all optional.

This is because in golang string cannot be null, it is always empty if nothing is assigned to it. In our case we need to make sure that Key is provided with every integration in the datafile, if not then optimizely-client should throw an error. This is a internal struct and user will not be able to access it.

jaeopt · 2022-11-30T17:13:56Z

pkg/client/client.go

+	// on failure, qualifiedSegments should be reset if a previous value exists.
+	userContext.SetQualifiedSegments(nil)
+
+	if _, err = o.getProjectConfig(); err != nil {


This guard is good to have. Wondering if there is any case when a user context is created before ProjectConfig is not ready. If so, identify event will be discarded (ODP key is not ready yet) for those user contexts.

In case user provides an sdkKey, go-sdk allows user to create client as well as userContext while datafile is being downloaded in the background. As for discarding events, the check to ignore events if odp is not integrated is there in the odp manager.

jaeopt · 2022-11-30T17:19:55Z

pkg/client/factory.go

+}
+
+// WithOdpUserIdentification confirms whether to call IdentifyUser API on userContext initialization.
+func WithOdpUserIdentification(identify bool) OptionFunc {


It's expected that identify event will be sent if odpManager is enabled and integration is set in datafile. Wondering what this extra control flag is for. We can remove it for now and consider if we need this configurability for all SDKs.

jaeopt · 2022-11-30T18:00:19Z

pkg/client/factory.go

+		f.optimizelySDKSettings = &OptimizelySdkSettings{
+			SegmentsCacheSize:          pkgOdpUtils.DefaultSegmentsCacheSize,
+			SegmentsCacheTimeoutInSecs: pkgOdpUtils.DefaultSegmentsCacheTimeout,
+		}


What is this structuring for? I see optimizelySDKSettings is used in this function only.

jaeopt · 2022-11-30T18:17:49Z

pkg/client/factory.go

+func WithOptimizelySdkSettings(optimizelySdkSettings *OptimizelySdkSettings) OptionFunc {
+	return func(f *OptimizelyFactory) {
+		f.optimizelySDKSettings = optimizelySdkSettings
+	}
+}
+
+// WithOdpManager sets odp manager on a client.
+func WithOdpManager(odpManager odp.Manager) OptionFunc {
+	return func(f *OptimizelyFactory) {
+		f.odpManager = odpManager
+	}
+}


It looks like we use combinations of config files (cacheSize, cacheTimeout, enabled) and builder method for ODP configuration. Can we consider removing config files and adding them ODP Builder (withSegmentCacheSize, withSegmentCacheTimeout) just like other ODP configuration?

jaeopt · 2022-11-30T18:31:25Z

pkg/client/factory.go

+	// Create new odp manaager with latest config
+	if f.odpManager == nil {
+		options := []odp.OMOptionFunc{}
+		if isProjectConfigAvailable {
+			// Add odp Config with latest config
+			options = append(options, odp.WithOdpConfig(pkgOdpConfig.NewConfig(projectConfig.GetPublicKeyForODP(), projectConfig.GetHostForODP(), projectConfig.GetSegmentList())))
+		}
+		// Create ODP Manager
+		appClient.OdpManager = odp.NewOdpManager(f.SDKKey, f.optimizelySDKSettings.DisableOdp, f.optimizelySDKSettings.SegmentsCacheSize, f.optimizelySDKSettings.SegmentsCacheTimeoutInSecs, options...)
+		return
+	}
+
+	// Update user given odp manager with latest config
+	if f.odpManager != nil {
+		appClient.OdpManager = f.odpManager
+		// Update odp config with latest config
+		if isProjectConfigAvailable {
+			appClient.OdpManager.Update(projectConfig.GetPublicKeyForODP(), projectConfig.GetHostForODP(), projectConfig.GetSegmentList())
+		}
+	}
+}


Can we drop OMOption for OdpConfig?

if odpManager == nil { odpManager = odp.NewOdpManager(f.SDKKey, ...) } if isProjectConfigAvailable { appClient.OdpManager.Update(projectConfig.GetPublicKeyForODP(), ...) }

jaeopt · 2022-11-30T18:44:05Z

pkg/odp/odp_manager.go

+func WithOdpConfig(odpConfig config.Config) OMOptionFunc {
+	return func(om *DefaultOdpManager) {
+		om.OdpConfig = odpConfig
+	}
+}


We may drop this?

jaeopt · 2022-11-30T18:45:27Z

pkg/odp/odp_manager.go

+		om.segmentsCache = segmentsCache
+	}
+}
+


We can add withSegmentCacheSize and withSegmentCacheTimeout here.

msohailhussain

lgtm. please address jae's comments.

msohailhussain · 2022-11-30T21:01:35Z

pkg/client/factory.go

+
+	projectConfig, err := appClient.ConfigManager.GetConfig()
+	// For cases when project config is not fetched yet
+	isProjectConfigAvailable := err == nil && projectConfig != nil


I think return in case projectConfig not available.

msohailhussain · 2022-11-30T21:04:30Z

pkg/config/datafileprojectconfig/entities/entities.go

-	Key       string `json:"key"`
-	Host      string `json:"host"`
-	PublicKey string `json:"publicKey"`
+	Key       *string `json:"key"`


discussed offline.

…ent fetch api manager.

jaeopt

LGTM with a nit.

jaeopt · 2022-12-16T00:27:09Z

pkg/client/optimizely_user_context.go

+}
+
+// FetchQualifiedSegmentsAsync fetches all qualified segments aysnchronously for the user context.
+func (o *OptimizelyUserContext) FetchQualifiedSegmentsAsync(callback func(success bool), options []pkgOdpSegment.OptimizelySegmentOption) {


Wondering if this order is a common practice in golang -

Suggested change

func (o *OptimizelyUserContext) FetchQualifiedSegmentsAsync(callback func(success bool), options []pkgOdpSegment.OptimizelySegmentOption) {

func (o *OptimizelyUserContext) FetchQualifiedSegmentsAsync(options []pkgOdpSegment.OptimizelySegmentOption, callback func(success bool)) {

it's not, might've copied it while looking into java, thank's for pointing out 👍

yasirfolio3 added 12 commits November 16, 2022 14:55

ODP Event manager implementations

03be067

Live test added.

3556bd6

fixes.

6d301b7

fixes.

936011f

suggested changes made.

78fe698

ODP Manager implementations.

38f7ea7

Added unit tests.

54207f6

Fixes and unit tests added.

5c4acbd

fixes.

5aa440a

Merging master.

de8378a

Unit tests added for fetchQualifiedSegments

e20881d

fixes.

3922198

yasirfolio3 requested a review from a team as a code owner November 24, 2022 09:59

optimizely-codereview-bot assigned yasirfolio3 Nov 24, 2022

yasirfolio3 added 3 commits November 24, 2022 16:21

fixes.

b59b2c6

should return decode error if odp key is nil.

2917d3b

fixing unit tests.

6d0e7f2

yasirfolio3 force-pushed the yasir/odp-manager branch from fe7f328 to 6d0e7f2 Compare November 29, 2022 10:19

msohailhussain suggested changes Nov 30, 2022

View reviewed changes

suggested changes made.

e3a59d8

jaeopt requested changes Nov 30, 2022

View reviewed changes

msohailhussain approved these changes Nov 30, 2022

View reviewed changes

fixes.

274cd89

yasirfolio3 requested a review from jaeopt December 1, 2022 15:30

Fixes

9c45e0b

yasirfolio3 force-pushed the yasir/odp-manager branch from b60957a to 9c45e0b Compare December 1, 2022 17:03

yasirfolio3 added 4 commits December 6, 2022 11:56

fixed cache timeout type and added default timeout for event and segm…

f3603a9

…ent fetch api manager.

fixes.

d1d4232

removed odp config dependency from segment manager and event manager

810c19a

nit fixes.

1a1740d

jaeopt approved these changes Dec 16, 2022

View reviewed changes

made suggested changes.

e0fbb44

msohailhussain merged commit a4246e6 into master Dec 19, 2022

msohailhussain deleted the yasir/odp-manager branch December 19, 2022 06:08

		Host string `json:"host"`
		PublicKey string `json:"publicKey"`

	func (o *OptimizelyUserContext) FetchQualifiedSegmentsAsync(callback func(success bool), options []pkgOdpSegment.OptimizelySegmentOption) {
	func (o *OptimizelyUserContext) FetchQualifiedSegmentsAsync(options []pkgOdpSegment.OptimizelySegmentOption, callback func(success bool)) {

feat(ats): add ODP integration. #355

feat(ats): add ODP integration. #355

Uh oh!

Conversation

yasirfolio3 commented Nov 24, 2022

Summary

Test plan

Issues

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!

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!

jaeopt Nov 28, 2022 • 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!

yasirfolio3 Dec 1, 2022 • 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!

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!

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!

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!

jaeopt Nov 28, 2022 •

edited

Loading

yasirfolio3 Dec 1, 2022 •

edited

Loading