Skip to content
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

refactor: Changed the way reasons are being returned from decide APIs #279

Merged
merged 17 commits into from
Jan 7, 2021

Conversation

zashraf1985
Copy link
Contributor

@zashraf1985 zashraf1985 commented Dec 15, 2020

Summary

Reasons were previously being collected by sending a reasons object down the stack to all the related functions which would mutate it by pushing their own reasons. This is now changed. Every related function now returns the reasons array along with the actual value.

Test plan

  • Manually tested thoroughly.
  • Fixed all the affected unit test.
  • FSC passing

@coveralls
Copy link

coveralls commented Dec 15, 2020

Coverage Status

Coverage increased (+0.01%) to 99.47% when pulling 87894b3 on zeeshan/decide-reasons-fix into f718a18 on master.

@zashraf1985 zashraf1985 marked this pull request as ready for review December 16, 2020 23:36
@zashraf1985 zashraf1985 requested a review from a team as a code owner December 16, 2020 23:36
@zashraf1985 zashraf1985 removed their assignment Dec 17, 2020
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!
It'll be great if we add a few more tests validating reasons collecting works ok for several hierarchical paths. No need for validating all messages.


whitelisted_variation_id = project_config.get_variation_id_from_key(experiment_key, whitelisted_variation_key)

unless whitelisted_variation_id
message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}', which is not in the datafile."
@logger.log(Logger::INFO, message)
decide_reasons&.push(message)
return nil
return nil, message
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ok to return a string instead of a string array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if you notice, i am using splat operator * when pushing into the array. it flattens the messages if its an array or uses it as is if its a single message.

decide_reasons&.push(message)
return saved_variation_id
decide_reasons.push(message)
return saved_variation_id, decide_reasons
end
end

# Check audience conditions
unless Audience.user_meets_audience_conditions?(project_config, experiment, attributes, @logger)
Copy link
Contributor

Choose a reason for hiding this comment

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

We miss reasons from "user_meets_audience_conditions". Can you change it to merge the reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@zashraf1985 zashraf1985 removed their assignment Dec 19, 2020
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's a lot of works for validating collections of reasons! They look good!
Suggesting 2-3 types of messages removed from reasons. See my comments.

)
message = format(logs_hash['EVALUATING_AUDIENCES_COMBINED'], logging_key, audience_conditions)
logger.log(Logger::DEBUG, message)
decide_reasons.push(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

This message should not be in reasons - no helpful info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)
message = format(logs_hash['EVALUATING_AUDIENCE'], audience_id, audience_conditions)
logger.log(Logger::DEBUG, message)
decide_reasons.push(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good! I think this message should BE in reasons since it shows "audience_conditions".

user_meets_audience_conditions, reasons = Optimizely::Audience.user_meets_audience_conditions?(config, experiment, user_attributes, spy_logger)
expect(user_meets_audience_conditions).to be false
expect(reasons).to eq([
"Evaluating audiences for experiment 'test_experiment_with_audience': [\"11154\"].",
Copy link
Contributor

Choose a reason for hiding this comment

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

This debugging message should be removed from reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 74 to 75
"User 'test_user' is not in the forced variation map.",
"Evaluating audiences for experiment 'test_experiment': [].",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove these two messages from reasons?
The first one (forced variation) should be added into reasons only when "is" in the forced variation map.
The 2nd one is not helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"User 'test_user' is not in the forced variation map.",
"Evaluating audiences for experiment 'test_experiment': [].",
"Audiences for experiment 'test_experiment' collectively evaluated to TRUE.",
"Assigned bucket 4577 to user 'test_user' with bucketing ID: 'test_user'.",
Copy link
Contributor

Choose a reason for hiding this comment

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

This message is also not needed for reasons - bucket id assignment info is not helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

expect(variation_received).to eq('111128')
expect(reasons).to eq([
"User 'test_user' is not in the forced variation map.",
"User '' was previously bucketed into variation ID '111111' for experiment '111127', but no matching variation was found. Re-bucketing user.",
Copy link
Contributor

Choose a reason for hiding this comment

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

UserID is missing in the message?

Copy link
Contributor Author

@zashraf1985 zashraf1985 Dec 22, 2020

Choose a reason for hiding this comment

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

Looked like an old bug. Fixed the log

@zashraf1985 zashraf1985 removed their assignment Dec 22, 2020
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

@zashraf1985 zashraf1985 changed the title Fixed the way reasons are being passed refactor: Changed the way reasons are being returned from decide APIs Jan 7, 2021
@zashraf1985 zashraf1985 merged commit 75fc548 into master Jan 7, 2021
@zashraf1985 zashraf1985 deleted the zeeshan/decide-reasons-fix branch January 7, 2021 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants