-
Notifications
You must be signed in to change notification settings - Fork 28
feat(Decide): Add Decide API #274
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
Conversation
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.
It looks good!
I see a few remaining issues. See comments.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
Missing mutex lock for attributes reading. Other than that, all changes look good!
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
i just fixed it
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.
LGTM
This looks good. I'll create a JIRA to make sure other sdks cover this sync-userContext issue.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why passing clone
, we already cloning user_attributes
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.
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
OptimizelyUserContext
class throughcreate_user_context
class api. This creates an optimizely instance with memoized user context and exposes the following APIsset_attribute
decide
decide_all
decide_for_keys
track_event
Test plan