fix(logs): Fixing log messages for Targeted Rollouts#268
fix(logs): Fixing log messages for Targeted Rollouts#268aliabbasrizvi merged 11 commits intomasterfrom
Conversation
mjc1283
left a comment
There was a problem hiding this comment.
There are conflicts with master that need to be resolved.
optimizely/helpers/audience.py
Outdated
| def does_user_meet_audience_conditions( | ||
| config, audience_conditions, experiment_or_rollout_rule, logging_key, attributes, logger): |
There was a problem hiding this comment.
There are a lot of arguments - it might be easier to read at the call site if some or all were keyword arguments.
040472a to
0e69287
Compare
| variation_id = self.find_bucket(project_config, bucketing_id, experiment.id, experiment.trafficAllocation) | ||
| if variation_id: | ||
| variation = project_config.get_variation_from_id(experiment.key, variation_id) | ||
| project_config.logger.info( |
There was a problem hiding this comment.
These log messages have been moved to the call site, akin to how log messages are generated for rollouts.
712ddea to
55a7261
Compare
mjc1283
left a comment
There was a problem hiding this comment.
One request before merging - a few changed files did not get 2020 added in their license header years. Otherwise LGTM.
Added a non-blocking comment. I am fine with the implementation as-is but wanted to share this as a potential follow up.
| bucketing_id: ID to be used for bucketing the user. | ||
| Args: | ||
| project_config: Instance of ProjectConfig. | ||
| experiment: Object representing the experiment or rollout rule in which user is to be bucketed. |
There was a problem hiding this comment.
Nit: maybe rename experiment to experiment_or_rule?
There was a problem hiding this comment.
I think I will keep it like this for now, given it is the Experiment entity.
optimizely/helpers/audience.py
Outdated
| Args: | ||
| config: project_config.ProjectConfig object representing the project. | ||
| audience_conditions: Audience conditions corresponding to the experiment or rollout rule. | ||
| experiment_or_rollout_rule: String representing whether entity being evaluated is experiment or rollout rule. |
There was a problem hiding this comment.
I think this could be cleaner if:
audience_logswas an argument, rather thanexperiment_or_rollout_rule- The association between rollout rule/experiment and the corresponding logs was abstracted by the
Experimententity class, and internally represented by a dict, or class property/method (maybe make rollout rule its own entity class?)
There was a problem hiding this comment.
I will adopt the audience_logs change now.
maybe make rollout rule its own entity class
I was thinking about that, but in light of more incoming product changes where this terminology will change a lot, I am hoping that we make the transition then. Let me know if that sounds ok.
With this change we are introducing targeted rollout specific log messages.
Since, rules right now do not have a "name", I am proposing to use rule's index in the log message.