feat(ups): Added UserProfileService Support. #326

yasirfolio3 · 2021-12-24T06:45:38Z

Summary

Provided 2 out of the box user profile services including in-memory, rest and redis.
Added lifo and fifo ordering in in-memory UPS.
Added support for custom user profile services

Build Size

Without UPS: 12.1 mb
With UPS: 12.7 mb

Build Time

Without UPS: 7.384s
With UPS: 7.387s

Performance

Without UPS: 1000 decide all calls in 580ms
With UPS (disabled by default): 1000 decide all calls in 580ms

UserProfileService EndPoints

To lookup a user profile, use agent's POST /v1/lookup:

In the request `application/json` body, include the `userId`. The full request looks like this:

```curl
curl --location --request POST 'http://localhost:8080/v1/lookup' \
--header 'X-Optimizely-SDK-Key: YOUR_SDK_KEY' \
--header 'Accept: text/event-stream' \
--header 'Content-Type: application/json' \
--data-raw '{
  "userId": "string"
}'

To save a user profile, use agent's POST /v1/save:

In the request `application/json` body, include the `userId` and `experimentBucketMap`. The full request looks like this:

```curl
curl --location --request POST 'http://localhost:8080/v1/save' \
--header 'X-Optimizely-SDK-Key: YOUR_SDK_KEY' \
--header 'Accept: text/event-stream' \
--header 'Content-Type: application/json' \
--data-raw '{
    "userId": "string",
    "experimentBucketMap": {
        "experiment_id_to_save": {
            "variation_id": "variation_id_to_save"
        }
    }
}'

Out of Box UserProfileService Usage

To use the in-memory UserProfileService, update the config.yaml as shown below:

## configure optional User profile service
userProfileService:
      default: "in-memory"
      services:
        in-memory: 
          ## 0 means no limit on capacity
          capacity: 0
          ## supports lifo/fifo
          storageStrategy: "fifo"

To use the redis UserProfileService, update the config.yaml as shown below:

## configure optional User profile service
userProfileService:
      default: "redis"
      services:
        redis: 
          host: "your_host"
          password: "your_password"
          database: 0 ## your database

To use the rest UserProfileService, update the config.yaml as shown below:

## configure optional User profile service
userProfileService:
      default: "rest"
      services:
        rest:
          host: "your_host"
          lookupPath: "/lookup_endpoint"
          lookupMethod: "POST"
          savePath: "/save_endpoint"
          saveMethod: "POST"
          userIDKey: "user_id"
          headers: 
            "header_key": "header_value"

Implement 2 POST api's /lookup_endpoint and /save_endpoint on your host. Api methods will be POST by default but can be updated through lookupMethod and saveMethod properties. Similarly, request parameter key for user_id can also be updated using userIDKey property.

lookup_endpoint should accept user_id in its json body or query (depending upon the method type) and if successful, return the status code 200 with json response (keep in mind that when sending response, user_id should be substituted with value of userIDKey from config.yaml):

{
  "experiment_bucket_map": {
    "saved_experiment_id": {
      "variation_id": "saved_variation_id"
    }
  },
  "user_id": "saved_user_id"
}

save_endpoint should accept the following parameters in its json body or query (depending upon the method type) and return the status code 200 if successful (keep in mind that user_id should be substituted with the value ofuserIDKey from config.yaml):

{
  "experiment_bucket_map": {
    "experiment_id_to_save": {
      "variation_id": "variation_id_to_save"
    }
  },
  "user_id": "user_id_to_save"
}

Custom UserProfileService Implementation

To implement a custom user profile service, followings steps need to be taken:

Create a struct that implements the decision.UserProfileService interface in plugins/userprofileservice/services.
Add a init method inside your UserProfileService file as shown below:

func init() {
	myUPSCreator := func() decision.UserProfileService {
		return &yourUserProfileServiceStruct{
		}
	}
	userprofileservice.Add("my_ups_name", myUPSCreator)
}

Update the config.yaml file with your UserProfileService config as shown below:

## configure optional User profile service
userProfileServices:
    default: "my_ups_name"
    services:
        my_ups_name: 
           ## Add those parameters here that need to be mapped to the UserProfileService
           ## For example, if the UPS struct has a json mappable property called `host`
           ## it can updated with value `abc.com` as shown
           host: “abc.com”

If a user has created multiple UserProfileServices and wants to override the default UserProfileService for a specifc sdkKey, they can do so by providing the UserProfileService name in the request Header X-Optimizely-UPS-Name.

Tests

Unit tests added.
FSC should pass.

msohailhussain · 2021-12-28T22:30:54Z

pkg/optimizely/cache.go

@@ -114,6 +119,11 @@ func (c *OptlyCache) UpdateConfigs(sdkKey string) {
 	}
 }

+// SetUserProfileService sets maps userProfileService against the given sdkKey
+func (c *OptlyCache) SetUserProfileService(sdkKey, userProfileService string) {


Please add lock here.

we are using userProfileServiceMap cmap.ConcurrentMap here which handles concurrency itself.

I think we should still add the mutex here. For reading the ConcurrentMap should be fine but when setting, I believe the mutex should be used to ensure no race condition, even with the ConcurrentMap, just to be safe.. I could be wrong though.

I will need to check this part again.

anvth · 2022-01-06T14:51:40Z

Hey team, this is amazing news and something my team was considering contributing! 💟

Just a question, in the example config we see there is an option to specify custom URL for User Profile Service. However, reviewing PR I could not see an implementation for that. Is there a plan to support that any time soon? Even the PR description does not cover this.

Thank you :-)

yasirfolio3 · 2022-01-07T10:24:35Z

Hey team, this is amazing news and something my team was considering contributing! 💟

Just a question, in the example config we see there is an option to specify custom URL for User Profile Service. However, reviewing PR I could not see an implementation for that. Is there a plan to support that any time soon? Even the PR description does not cover this.

Thank you :-)

Hi @anvth , We have updated the PR description, please have a look and let us know if there's anything else we can help with, Thanks.

The-inside-man

LGTM! Just the one comment around mutex as @msohailhussain suggested.

204Constie · 2022-01-10T15:07:20Z

Hey team, this is amazing news for us as we were planning to implement this. 😸
A significant question here that we would have would be about the custom UPS implementation. Contrary to how in memory and redis are implemented there is no way of implementing it by simply providing certain options in the config file.
Did you consider an option to make it less flexible but not requiring changes in the code ?
What about having an out of the box option for microservice UPS ?

thank you 😸

jaeopt

I have a suggestion and a clarification.
Other than those, it looks great to me as far as I can follow with go :)

jaeopt · 2022-01-20T01:27:19Z

plugins/userprofileservice/services/in_memory_ups.go

+			case "lifo":
+				u.lifoOrderedProfiles = append(u.lifoOrderedProfiles, profile.ID)
+			default:
+				// fifo by default
+				u.fifoOrderedProfiles <- profile.ID


The benefit of supporting fifo/lifo is not clear to me. Can we just implement the default LRU cache unless we see clear reasons they prefer fifo/lifo?

jaeopt · 2022-01-20T01:32:04Z

plugins/userprofileservice/services/rest_ups.go

+// Lookup is used to retrieve past bucketing decisions for users
+func (r *RestUserProfileService) Lookup(userID string) (profile decision.UserProfile) {
+	if userID == "" {
+		return


Is it ok not returning profile in go?

yes, if we don't do it here, it will just return empty profile struct.

lookupMethod, saveMethod, userIDKey

204Constie · 2022-01-28T10:17:53Z

hello! 😸 all of the updates to rest UPS look wonderful 😻 all configurable via config.yml file 😻 great work!
i have a small question. i can read that both lookup and save methods expect the rest endpoints to return the profile on success. this is for sure necessary in case of lookup but is it really required in case of save method ? does it use the response of the save method or expect to use it in the future ? or it actually only require the response of status code 2XX and would in truth behave the same in case of 200 and 204 ?

yasirfolio3 · 2022-01-28T10:23:37Z

hello! 😸 all of the updates to rest UPS look wonderful 😻 all configurable via config.yml file 😻 great work! i have a small question. i can read that both lookup and save methods expect the rest endpoints to return the profile on success. this is for sure necessary in case of lookup but is it really required in case of save method ? does it use the response of the save method or expect to use it in the future ? or it actually only require the response of status code 2XX and would in truth behave the same in case of 200 and 204 ?

Hi @204Constie , In case of save, only 200 status code needs to be sent if successful (204 is not supported). The documentation is actually stating the payload required for the save request 👍

204Constie · 2022-01-28T11:27:41Z

hello! 😸 all of the updates to rest UPS look wonderful 😻 all configurable via config.yml file 😻 great work! i have a small question. i can read that both lookup and save methods expect the rest endpoints to return the profile on success. this is for sure necessary in case of lookup but is it really required in case of save method ? does it use the response of the save method or expect to use it in the future ? or it actually only require the response of status code 2XX and would in truth behave the same in case of 200 and 204 ?

Hi @204Constie , In case of save, only 200 status code needs to be sent if successful (204 is not supported). The documentation is actually stating the payload required for the save request 👍

😸 this is perfectly fine, i was just wondering why is this the case. is the response used in any way ? is there some other reason ? i'm simply curious

yasirfolio3 · 2022-01-28T12:04:15Z

hello! 😸 all of the updates to rest UPS look wonderful 😻 all configurable via config.yml file 😻 great work! i have a small question. i can read that both lookup and save methods expect the rest endpoints to return the profile on success. this is for sure necessary in case of lookup but is it really required in case of save method ? does it use the response of the save method or expect to use it in the future ? or it actually only require the response of status code 2XX and would in truth behave the same in case of 200 and 204 ?

Hi @204Constie , In case of save, only 200 status code needs to be sent if successful (204 is not supported). The documentation is actually stating the payload required for the save request 👍

😸 this is perfectly fine, i was just wondering why is this the case. is the response used in any way ? is there some other reason ? i'm simply curious

So for save API, no response is required. Even if a response is sent, it will be ignored by agent. Agent only checks for status code 200.

msohailhussain

please address

msohailhussain · 2022-01-25T21:10:03Z

.travis.yml

@@ -8,6 +8,9 @@ env:
    # may also want to run `go mod edit -go=1.13` to fix go.mod as well
    - GIMME_GO_VERSION=1.13.x GIMME_OS=linux GIMME_ARCH=amd64

+services:


what's the purpose of this service, please add as a comment.

msohailhussain · 2022-01-25T21:12:19Z

README.md

@@ -95,6 +95,7 @@ Below is a comprehensive list of available configuration properties.
 |client.pollingInterval|OPTIMIZELY_CLIENT_POLLINGINTERVAL|The time between successive polls for updated project configuration. Default: 1m|
 |client.queueSize|OPTIMIZELY_CLIENT_QUEUESIZE|The max number of events pending dispatch. Default: 1000|
 |client.sdkKeyRegex|OPTIMIZELY_CLIENT_SDKKEYREGEX|Regex to validate SDK keys provided in request header. Default: ^\\w+(:\\w+)?$|
+|client.userProfileService|OPTIMIZELY_CLIENT_USERPROFILESERVICE| Property used to enable and set UserProfileServices. Default: ./config.yaml|


i think this needs to be revised.

copied from interceptors: https://github.com/optimizely/agent/pull/326/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5L110

msohailhussain · 2022-01-25T21:13:01Z

cmd/optimizely/main.go

@@ -84,6 +87,11 @@ func loadConfig(v *viper.Viper) *config.AgentConfig {
 		conf.Server.Interceptors = interceptors
 	}

+	// Check if JSON string was set using OPTIMIZELY_CLIENT_USERPROFILESERVICE


OPTIMIZELY_CLIENT_USERPROFILESERVICE what does it mean?

msohailhussain · 2022-01-25T21:16:44Z

config/config.go

@@ -78,6 +78,10 @@ func NewDefaultConfig() *AgentConfig {
 			EventURL:            "https://logx.optimizely.com/v1/events",
 			// https://github.com/google/re2/wiki/Syntax
 			SdkKeyRegex: "^\\w+(:\\w+)?$",
+			UserProfileService: UserProfileServiceConfigs{


space before curly braces.

Suggested by linter, Golang follows the same convention for initialising new properties.

msohailhussain · 2022-01-25T21:39:10Z

go.sum

+github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7JYyY=
+github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1ybHNo=
+github.com/onsi/gomega v1.16.0 h1:6gjqkI8iiRHMvdccRJM8rVKjCWk6ZIm6FTm3ddIe4/c=
+github.com/onsi/gomega v1.16.0/go.mod h1:HnhC7FXeEQY45zxNK3PPoIUhzk/80Xly9PcubAlGdZY=


so many packages are added, we will need to check if we can ship without redis.

Have updated the PR description with build size with redis.

msohailhussain · 2022-02-02T06:29:43Z

pkg/optimizely/cache.go

+
+	createAndReturnUPSWithName := func(upsName string) decision.UserProfileService {
+		if clientConfigUPSMap, ok := conf.UserProfileService["services"].(map[string]interface{}); ok {
+			if defaultUserProfileServiceMap, ok := clientConfigUPSMap[upsName].(map[string]interface{}); ok {


userprofileserviceconfig should be better name.

msohailhussain · 2022-02-02T06:31:01Z

pkg/optimizely/cache.go

+		return nil
+	}
+
+	// Check if ups type was provided in the request headers


msohailhussain · 2022-02-02T06:32:42Z

pkg/optimizely/cache.go

+	// Check if ups type was provided in the request headers
+	if ups, ok := userProfileServiceMap.Get(sdkKey); ok {
+		if upsNameStr, ok := ups.(string); ok && upsNameStr != "" {
+			return createAndReturnUPSWithName(upsNameStr)


initialize or New

msohailhussain · 2022-02-02T06:33:18Z

pkg/optimizely/interface.go

@@ -25,6 +25,7 @@ import (
 type Cache interface {
 	GetClient(sdkKey string) (*OptlyClient, error)
 	UpdateConfigs(sdkKey string)
+	SetUserProfileService(sdkKey, userProfileService string)


add description, why we need to add here.

msohailhussain · 2022-02-02T06:36:39Z

plugins/userprofileservice/services/in_memory_ups.go

+	fifoOrderedProfiles chan string
+	lifoOrderedProfiles []string
+	lock                sync.RWMutex
+	isReady             bool


why we need isReady?

since we need to initialize fifoOrderedProfiles with capacity and this can only be done when first save is called, isReady is used to this purpose. All other required initialisations also take place if isReady is false.

u.fifoOrderedProfiles = make(chan string, u.Capacity)

msohailhussain

lgtm

jaeopt

LGTM

yasirfolio3 added 11 commits December 7, 2021 17:29

Initial commit.

4b74918

Further improvements.

4ffb17f

Suggested changes made.

38e3055

Working on unit tests.

d44aee6

Unit tests added.

5a3d6e1

Suggested changes made.

5dc1708

suggested fixes made.

5d01ae3

Added further unit tests.

2678466

Implementation added for in-memory capacity.

d791a10

order by lifo implemented in in_memory ups.

3b4edb4

new tests added.

5e74cf7

yasirfolio3 requested a review from msohailhussain December 24, 2021 06:45

yasirfolio3 added 2 commits December 27, 2021 16:13

suggested changes made.

87aa661

suggested changes made.

8fd8efb

msohailhussain reviewed Dec 28, 2021

View reviewed changes

yasirfolio3 added 2 commits January 4, 2022 12:50

headers updated with 2022.

402a0de

Unit tests added.

c7fd962

yasirfolio3 marked this pull request as ready for review January 5, 2022 07:45

yasirfolio3 requested a review from msohailhussain January 5, 2022 07:45

The-inside-man approved these changes Jan 7, 2022

View reviewed changes

Readme added.

984268b

msohailhussain requested a review from jaeopt January 10, 2022 19:43

yasirfolio3 added 4 commits January 11, 2022 14:08

Log added for successful UPS creation.

54d421c

fixes.

debf220

Rest API UPS added.

10d64ef

fixes.

02ac80d

jaeopt requested changes Jan 20, 2022

View reviewed changes

nit fixed.

abc6e4a

yasirfolio3 closed this Jan 24, 2022

yasirfolio3 reopened this Jan 24, 2022

Additional options added to rest ups:

6ccf3b3

lookupMethod, saveMethod, userIDKey

msohailhussain suggested changes Feb 2, 2022

View reviewed changes

fixes.

7abddfc

yasirfolio3 force-pushed the yasir/inmemory-ups branch from 7c0a1a0 to 7abddfc Compare February 2, 2022 09:09

yasirfolio3 added 2 commits February 2, 2022 14:10

removing redis from travis.

b3486b6

documentation updated.

0879cbb

msohailhussain approved these changes Feb 10, 2022

View reviewed changes

msohailhussain requested a review from jaeopt February 10, 2022 06:03

jaeopt approved these changes Feb 10, 2022

View reviewed changes

Merge branch 'master' into yasir/inmemory-ups

eea30d2

msohailhussain merged commit 5da4ab3 into master Feb 11, 2022

msohailhussain deleted the yasir/inmemory-ups branch February 11, 2022 06:37

anvth mentioned this pull request Mar 15, 2022

Looking forward to adopt UPS integration #335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ups): Added UserProfileService Support. #326

feat(ups): Added UserProfileService Support. #326

yasirfolio3 commented Dec 24, 2021 •

edited

Loading

msohailhussain Dec 28, 2021

yasirfolio3 Dec 29, 2021

The-inside-man Jan 7, 2022

msohailhussain Jan 25, 2022

anvth commented Jan 6, 2022

yasirfolio3 commented Jan 7, 2022

The-inside-man left a comment

204Constie commented Jan 10, 2022

jaeopt left a comment

jaeopt Jan 20, 2022

jaeopt Jan 20, 2022

yasirfolio3 Jan 20, 2022

204Constie commented Jan 28, 2022

yasirfolio3 commented Jan 28, 2022

204Constie commented Jan 28, 2022

yasirfolio3 commented Jan 28, 2022

msohailhussain left a comment

msohailhussain Jan 25, 2022

msohailhussain Jan 25, 2022

yasirfolio3 Feb 2, 2022

msohailhussain Jan 25, 2022

msohailhussain Jan 25, 2022

yasirfolio3 Feb 2, 2022

msohailhussain Jan 25, 2022

yasirfolio3 Feb 2, 2022

msohailhussain Feb 2, 2022

msohailhussain Feb 2, 2022

msohailhussain Feb 2, 2022

msohailhussain Feb 2, 2022

msohailhussain Feb 2, 2022

yasirfolio3 Feb 2, 2022

msohailhussain left a comment

jaeopt left a comment

feat(ups): Added UserProfileService Support. #326

feat(ups): Added UserProfileService Support. #326

Conversation

yasirfolio3 commented Dec 24, 2021 • edited Loading

Summary

Build Size

Build Time

Performance

UserProfileService EndPoints

Out of Box UserProfileService Usage

Custom UserProfileService Implementation

Tests

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

anvth commented Jan 6, 2022

yasirfolio3 commented Jan 7, 2022

The-inside-man left a comment

Choose a reason for hiding this comment

204Constie commented Jan 10, 2022

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

204Constie commented Jan 28, 2022

yasirfolio3 commented Jan 28, 2022

204Constie commented Jan 28, 2022

yasirfolio3 commented Jan 28, 2022

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

msohailhussain left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

yasirfolio3 commented Dec 24, 2021 •

edited

Loading