8000 feat(Decide): Add Decide API by oakbani · Pull Request #274 · optimizely/ruby-sdk · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 49 commits into from
Dec 14, 2020
Merged

feat(Decide): Add Decide API #274

merged 49 commits into from
Dec 14, 2020

Conversation

oakbani
Copy link
Contributor
@oakbani oakbani commented Oct 23, 2020

Summary

Added new Apis to support the decide feature. Introduced a new OptimizelyUserContext class through create_user_context class api. This creates an optimizely instance with memoized user context and exposes the following APIs

  1. set_attribute
  2. decide
  3. decide_all
  4. decide_for_keys
  5. track_event

Test plan

  1. Manually tested thoroughly.
  2. Added unit tests to cover new functionality.
  3. All new and existing FSC tests pass.

@zashraf1985 zashraf1985 changed the base branch from oakbani/decide/user-context to master November 6, 2020 02:56
@zashraf1985 zashraf1985 closed this Nov 6, 2020
@zashraf1985 zashraf1985 reopened this Nov 6, 2020
@coveralls
Copy link
coveralls commented Nov 6, 2020

Coverage Status

Coverage decreased (-0.1%) to 99.457% when pulling f58d8bd on oakbani/decide-internal into 9da6a58 on master.

@zashraf1985 zashraf1985 marked this pull request as ready for review November 6, 2020 18:12
@zashraf1985 zashraf1985 requested a review from a team as a code owner November 6, 2020 18:12
Copy link
Contributor
@jaeopt jaeopt left a 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.

return {}
end

enabled_flags_only = !decide_options.nil? && (decide_options.include? OptimizelyDecideOption::ENABLED_FLAGS_ONLY)
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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)
Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Contributor
@jaeopt jaeopt left a 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 }
Copy link
Contributor

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).

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just fixed it

Copy link
Contributor
@jaeopt jaeopt left a 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)
Copy link
Contributor

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

Copy link
Contributor

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.

  1. 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.
  2. 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.

@zashraf1985 zashraf1985 merged commit f718a18 into master Dec 14, 2020
@zashraf1985 zashraf1985 deleted the oakbani/decide-internal branch December 14, 2020 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
0