feat(Decide): Add Decide API #274
Conversation
lib/optimizely.rb
Outdated
| return {} | ||
| end | ||
|
|
||
| enabled_flags_only = !decide_options.nil? && (decide_options.include? OptimizelyDecideOption::ENABLED_FLAGS_ONLY) |
There was a problem hiding this comment.
I see default_decide_options are merged in "decide()" function, but not in "decide_for_keys()".
| end | ||
|
|
||
| def set_attribute(attribute_key, attribute_value) | ||
| @user_attributes[attribute_key] = attribute_value |
There was a problem hiding this comment.
I do not see mutex protection for attributes read/write. Is it ok in ruby?
| @@ -3689,4 +3823,120 @@ def callback(_args); end | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| describe '#decide_all' do | |||
There was a problem hiding this comment.
My bad. I didn't see checking the count of enabled flags.
| end | ||
| end | ||
|
|
||
| describe 'DISABLE_DECISION_EVENT' do |
There was a problem hiding this comment.
This default_decide_option testing misses a test with "ENABLED_FLAGS_ONLY", with which we could have detected the bug in "decide_for_keys" not checking default_decide_options :)
| variation_key: 'control' | ||
| ) | ||
| end | ||
| end |
There was a problem hiding this comment.
They look good. One thing I'd like to add is testing with IGNORE_USER_PROFILE_SERVICE, which skips "saving" as well as "reading" UPS.
| end | ||
|
|
||
| def decide_all(options = nil) | ||
| @optimizely_client&.decide_all(self, options) |
There was a problem hiding this comment.
don't you want to check if optmizely_client is nil and log a message or pass an error?
There was a problem hiding this comment.
Good Point! I thought about it and i believe that ruby being a dynamic language, this is possible if someone is determined to produce this scenario but this is not possible in the intended usage. The only way we expect users to obtain user_context instance is by calling a get_user_context method on the optimizely instance itself, which means the optimizely_instance cant be null. I believe it would be overkill to add a check and a log message.
If we still want to do that, i am a little worried about where would i get logger from. I will probably need to pass in the logger in the user_context constructor along with the optimizely_client which is doable but i really dont think its necessary because if a user has generated user_context through a method on optimizely instance, it cant simply be null.
What do you think?
| @optimizely_client = optimizely_client | ||
| @user_id = user_id | ||
| @user_attributes = user_attributes.nil? ? {} : user_attributes.clone | ||
| end | ||
|
|
||
| def set_attribute(attribute_key, attribute_value) | ||
| @user_attributes[attribute_key] = attribute_value | ||
| @set_attr_mutex.synchronize { @user_attributes[attribute_key] = attribute_value } |
There was a problem hiding this comment.
We need the same lock protection for reading attributes as well.
Also can we return a snapshot copy of attributes when reading (so concurrent setAttribute won't change the attributes for ongoing decision)?
C# has the same issue being addressed (optimizely/csharp-sdk#253).
There was a problem hiding this comment.
if we need to make sure its not modified while a decision is in progress, we will need to wrap all the userContext api methods in synchronize blocks. This will result in some slowdown on heavy use. I am not sure how making a copy could resolve this.. userContext is passes a few levels down the stack before user attributes are fetched. What do you think
There was a problem hiding this comment.
We do not need protect entire decide methods. The only possible conflict is to read/update attributes.
You can see the charp-sdk PR linked above as a good reference.
| @@ -50,7 +58,7 @@ def set_attribute(attribute_key, attribute_value) | |||
| # @return [OptimizelyDecision] A decision result | |||
|
|
|||
| def decide(key, options = nil) | |||
| @optimizely_client&.decide(self, key, options) | |||
| @optimizely_client&.decide(clone, key, options) | |||
There was a problem hiding this comment.
why passing clone, we already cloning user_attributes
There was a problem hiding this comment.
These are two separate things.
- Clone is used to pass a full clone of userContext so that subsequect updates to attributes has no impact on in flight decisions and the final userContext object which is referred to in decision object has the same attribute information that was used for the decision.
- user_attributes cloning is for the scenario where some access user_attributes for some other use from out side. We want to make sure that the copy that they read is not mutated by setAttribute called somewherre later in the code.
Summary
Added new Apis to support the decide feature. Introduced a new
OptimizelyUserContextclass throughcreate_user_contextclass api. This creates an optimizely instance with memoized user context and exposes the following APIsset_attributedecidedecide_alldecide_for_keystrack_eventTest plan