-
Notifications
You must be signed in to change notification settings - Fork 19
[FSSDK-11589] Add go-sdk logic to support agent for cmab #412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
* Fix CMAB error handling to properly propagate error reasons in Decision objects * add cmab cache options to getAllOptions * fix failing fsc tests * add cmab errors file * adjust lowercase * add test * fix error message propagation in resons * add error handling to feature experiment servvice * Add more error handling to feature exper and composite feature service * nil back to err * add reasons message to composite feature service GetDecision * use AddError for reasons * Trigger PR check * remove implicit error handling - PR feedback * use nil instead of err for legacy * fix error format * Fix lint issue with fsc error * Rename error var, lint stuttering issue
func TestOptimizelyClient_handleDecisionServiceError(t *testing.T) { | ||
// Create the client | ||
client := &OptimizelyClient{ | ||
// MockCmabService for testing CMAB functionality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be no change to this file
@@ -78,12 +77,6 @@ type Response struct { | |||
type RetryConfig struct { | |||
// MaxRetries is the maximum number of retry attempts | |||
MaxRetries int | |||
// InitialBackoff is the initial backoff duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can keep the client configurable as we already have it. What I meant is we can remove these arguments from the top level cmabConfig in config.go to keep the api simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep this file unchanged
@@ -200,10 +200,7 @@ func TestDefaultCmabClient_FetchDecision_WithRetry(t *testing.T) { | |||
Timeout: 5 * time.Second, | |||
}, | |||
RetryConfig: &RetryConfig{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this file as well
@@ -14,105 +14,104 @@ | |||
* limitations under the License. * | |||
***************************************************************************/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we keep this file unchanged?
} | ||
} | ||
|
||
// WithCmabCacheSize sets only the CMAB cache size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need these. User can just set one field in cmab.Config and use WithCmabConfig
// CompositeExperimentService bridges together the various experiment decision services that ship by default with the SDK | ||
type CompositeExperimentService struct { | ||
experimentServices []ExperimentService | ||
overrideStore ExperimentOverrideStore | ||
userProfileService UserProfileService | ||
cmabConfig cmab.Config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be better to use a pointer
@@ -40,11 +44,70 @@ func WithOverrideStore(overrideStore ExperimentOverrideStore) CESOptionFunc { | |||
} | |||
} | |||
|
|||
// WithCmabConfig merges the provided CMAB configuration with defaults. | |||
// Only non-zero values from the provided config will override defaults. | |||
func WithCmabConfig(config cmab.Config) CESOptionFunc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move the merging of config to NewExperimentCmabService? NewExperimentCmabService already does some default handling. It would be better to have that in one place. For this method, we can change the parameter to *cmab.Config and just do f.cmabConfig = config
@@ -61,7 +124,10 @@ func NewCompositeExperimentService(sdkKey string, options ...CESOptionFunc) *Com | |||
// 2. Whitelist | |||
// 3. CMAB (always created) | |||
// 4. Bucketing (with User profile integration if supplied) | |||
compositeExperimentService := &CompositeExperimentService{logger: logging.GetLogger(sdkKey, "CompositeExperimentService")} | |||
compositeExperimentService := &CompositeExperimentService{ | |||
cmabConfig: cmab.NewDefaultConfig(), // Initialize with defaults |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove this default initialization and handle defaults in NewExperimentCmabService as mentioned above
InitialBackoff: cmab.DefaultInitialBackoff, | ||
MaxBackoff: cmab.DefaultMaxBackoff, | ||
BackoffMultiplier: cmab.DefaultBackoffMultiplier, | ||
func NewExperimentCmabService(sdkKey string, config cmab.Config) *ExperimentCmabService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please change the config parameter to a pointer. Also we should handle defaults in this function as follows:
- If the passed in config pointer is nil, we use default values for all config.
- If the passed pointer is not nil, then we consider zero values as unconfigured and use the default value. Otherwise we use the value from the config pointer
Added CMAB (Contextual Multi-Armed Bandit) support to go-sdk for Agent integration:
- Added cmabService field to OptimizelyClient
- Added WithCmabService() factory option for dependency injection
- Modified Decide() method to call CMAB service for CMAB-enabled experiments
We need CMAB support for agent in go-sdk because:
method
This is how CMAB data flows between go-sdk and agent:
Key points:
https://jira.sso.episerver.net/browse/FSSDK-11589