fix(logs): Fixing log messages for Targeted Rollouts #268
Conversation
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.
aliabbasrizvi
force-pushed
the
ali/update_logs_for_tr
branch
from
040472a to
0e69287
Compare
| @@ -131,10 +132,6 @@ def bucket(self, project_config, experiment, user_id, bucketing_id): | |||
| 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.
aliabbasrizvi
force-pushed
the
ali/update_logs_for_tr
branch
from
712ddea to
55a7261
Compare
| 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.