feat(decision): User profile service #163
Conversation
mikeproeng37
requested review from
mjc1283,
thomaszurkan-optimizely,
pawels-optimizely and
mikecdavis
| func WithUserProfileService(userProfileService UserProfileService) PESOptionFunc { | ||
| return func(f *PersistingExperimentService) { | ||
| f.userProfileService = userProfileService | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this optional? Seems like it should be required.
| @@ -34,14 +34,16 @@ type CompositeExperimentService struct { | |||
|
|
|||
| // NewCompositeExperimentService creates a new instance of the CompositeExperimentService | |||
| func NewCompositeExperimentService() *CompositeExperimentService { | |||
| persistingExperimentService := NewPersistingExperimentService(NewExperimentBucketerService()) | |||
There was a problem hiding this comment.
Should we only apply the persisting wrapper when they use an option func and actually provide a user profile service? Otherwise, most of the time, this persisting experiment service is running, but not doing anything interesting.
There was a problem hiding this comment.
Yeah you are right. Let me fix that.
| // GetDecision returns the decision with the variation the user is bucketed into | ||
| func (p PersistingExperimentService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) { | ||
| var experimentDecision ExperimentDecision | ||
| var err error |
There was a problem hiding this comment.
var experimentDecision ExperimentDecision
var err error -- these could be defined on the return level
mikeproeng37
changed the title
pkg/decision/entities.go
Outdated
| type UserDecisionKey struct { | ||
| ExperimentID string | ||
| } |
There was a problem hiding this comment.
Why use a struct here, as opposed to just a string?
There was a problem hiding this comment.
Good catch. In our current user profile service implementation we use a combination of experimentID and the key variation_id (which I forgot to add in this PR). I don't remember why we decided to make that value a map instead of pointing from experimentID to variationID and I added the struct so we could maintain that relationship and maintain consistency with the other SDKs.
pkg/decision/interface.go
Outdated
|
|
||
| // UserProfileService is used to save and retrieve past bucketing decisions for users | ||
| type UserProfileService interface { | ||
| Lookup(string) (UserProfile, error) |
There was a problem hiding this comment.
Do we need Lookup to return an error? The SDK internals don't have to care whether there was an error - that seems more like a concern of the implementation. Whoever implements this could track and handle errors on their end. Do they really need to route an error through the SDK logger like that?
From the SDK's perspective, it just needs a user profile for a given user id. We could require that a UserProfile is always returned, and say that when there is no saved decision for that user id (for whatever reason, error or not), then the implementation should return a UserProfile containing an empty ExperimentBucketMap.
There was a problem hiding this comment.
This is a good point. I'll remove the errors since we don't actually care about them.
pkg/decision/interface.go
Outdated
| // UserProfileService is used to save and retrieve past bucketing decisions for users | ||
| type UserProfileService interface { | ||
| Lookup(string) (UserProfile, error) | ||
| Save(UserProfile) error |
There was a problem hiding this comment.
Same comment above applies to returning an error here. Why does the SDK care?
Summary
CompositeExperimentServicewill be done in a follow-up PR