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

feat: Duplicate experiment key issue with multi feature flag #226

Merged
merged 5 commits into from
Aug 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Optimizely/Bucketer.php
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ public function bucket(ProjectConfigInterface $config, Experiment $experiment, $
list($variationId, $reasons) = $this->findBucket($bucketingId, $userId, $experiment->getId(), $experiment->getTrafficAllocation());
$decideReasons = array_merge($decideReasons, $reasons);
if (!empty($variationId)) {
$variation = $config->getVariationFromId($experiment->getKey(), $variationId);
$variation = $config->getVariationFromIdByExperimentId($experiment->getId(), $variationId);

return [ $variation, $decideReasons ];
}
Expand Down
94 changes: 86 additions & 8 deletions src/Optimizely/Config/DatafileProjectConfig.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2019-2020, Optimizely
* Copyright 2019-2021, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -123,6 +123,16 @@ class DatafileProjectConfig implements ProjectConfigInterface
*/
private $_variationIdMap;

/**
* @var array Associative array of experiment id to associative array of variation ID to variations.
*/
private $_variationIdMapByExperimentId;

/**
* @var array Associative array of experiment id to associative array of variation key to variations.
*/
private $_variationKeyMapByExperimentId;

/**
* @var array Associative array of event key to Event(s) in the datafile.
*/
Expand Down Expand Up @@ -247,7 +257,7 @@ public function __construct($datafile, $logger, $errorHandler)
}

$this->_groupIdMap = ConfigParser::generateMap($groups, 'id', Group::class);
$this->_experimentKeyMap = ConfigParser::generateMap($experiments, 'key', Experiment::class);
$this->_experimentIdMap = ConfigParser::generateMap($experiments, 'id', Experiment::class);
$this->_eventKeyMap = ConfigParser::generateMap($events, 'key', Event::class);
$this->_attributeKeyMap = ConfigParser::generateMap($attributes, 'key', Attribute::class);
$typedAudienceIdMap = ConfigParser::generateMap($typedAudiences, 'id', Audience::class);
Expand All @@ -256,32 +266,38 @@ public function __construct($datafile, $logger, $errorHandler)
$this->_featureFlags = ConfigParser::generateMap($featureFlags, null, FeatureFlag::class);

foreach (array_values($this->_groupIdMap) as $group) {
$experimentsInGroup = ConfigParser::generateMap($group->getExperiments(), 'key', Experiment::class);
$experimentsInGroup = ConfigParser::generateMap($group->getExperiments(), 'id', Experiment::class);
foreach (array_values($experimentsInGroup) as $experiment) {
$experiment->setGroupId($group->getId());
$experiment->setGroupPolicy($group->getPolicy());
}
$this->_experimentKeyMap = $this->_experimentKeyMap + $experimentsInGroup;
$this->_experimentIdMap = $this->_experimentIdMap + $experimentsInGroup;
}

foreach ($this->_rollouts as $rollout) {
foreach ($rollout->getExperiments() as $experiment) {
$this->_experimentKeyMap[$experiment->getKey()] = $experiment;
$this->_experimentIdMap[$experiment->getId()] = $experiment;
}
}

$this->_variationKeyMap = [];
$this->_variationIdMap = [];
$this->_experimentIdMap = [];
$this->_variationKeyMapByExperimentId = [];
$this->_variationIdMapByExperimentId = [];
$this->_experimentKeyMap = [];

foreach (array_values($this->_experimentKeyMap) as $experiment) {
foreach (array_values($this->_experimentIdMap) as $experiment) {
$this->_variationKeyMap[$experiment->getKey()] = [];
$this->_variationIdMap[$experiment->getKey()] = [];
$this->_experimentIdMap[$experiment->getId()] = $experiment;
$this->_variationIdMapByExperimentId[$experiment->getId()] = [];
$this->_variationKeyMapByExperimentId[$experiment->getId()] = [];
$this->_experimentKeyMap[$experiment->getKey()] = $experiment;

foreach ($experiment->getVariations() as $variation) {
$this->_variationKeyMap[$experiment->getKey()][$variation->getKey()] = $variation;
$this->_variationIdMap[$experiment->getKey()][$variation->getId()] = $variation;
$this->_variationKeyMapByExperimentId[$experiment->getId()][$variation->getKey()] = $variation;
$this->_variationIdMapByExperimentId[$experiment->getId()][$variation->getId()] = $variation;
}
}

Expand All @@ -300,24 +316,32 @@ public function __construct($datafile, $logger, $errorHandler)

$rolloutVariationIdMap = [];
$rolloutVariationKeyMap = [];
$rolloutVariationIdMapByExperimentId = [];
$rolloutVariationKeyMapByExperimentId = [];
foreach ($this->_rollouts as $rollout) {
$this->_rolloutIdMap[$rollout->getId()] = $rollout;

foreach ($rollout->getExperiments() as $rule) {
$rolloutVariationIdMap[$rule->getKey()] = [];
$rolloutVariationKeyMap[$rule->getKey()] = [];
$rolloutVariationIdMapByExperimentId[$rule->getId()] = [];
$rolloutVariationKeyMapByExperimentId[$rule->getId()] = [];

$variations = $rule->getVariations();
foreach ($variations as $variation) {
$rolloutVariationIdMap[$rule->getKey()][$variation->getId()] = $variation;
$rolloutVariationKeyMap[$rule->getKey()][$variation->getKey()] = $variation;
$rolloutVariationIdMapByExperimentId[$rule->getId()][$variation->getId()] = $variation;
$rolloutVariationKeyMapByExperimentId[$rule->getId()][$variation->getKey()] = $variation;
}
}
}

// Add variations for rollout experiments to variationIdMap and variationKeyMap
$this->_variationIdMap = $this->_variationIdMap + $rolloutVariationIdMap;
$this->_variationKeyMap = $this->_variationKeyMap + $rolloutVariationKeyMap;
$this->_variationIdMapByExperimentId = $this->_variationIdMapByExperimentId + $rolloutVariationIdMapByExperimentId;
$this->_variationKeyMapByExperimentId = $this->_variationKeyMapByExperimentId + $rolloutVariationKeyMapByExperimentId;

foreach (array_values($this->_featureFlags) as $featureFlag) {
$this->_featureKeyMap[$featureFlag->getKey()] = $featureFlag;
Expand Down Expand Up @@ -655,6 +679,60 @@ public function getVariationFromId($experimentKey, $variationId)
return new Variation();
}

/**
* @param $experimentId string ID for experiment.
* @param $variationId string ID for variation.
*
* @return Variation Entity corresponding to the provided experiment ID and variation ID.
* Dummy entity is returned if key or ID is invalid.
*/
public function getVariationFromIdByExperimentId($experimentId, $variationId)
{
if (isset($this->_variationIdMapByExperimentId[$experimentId])
&& isset($this->_variationIdMapByExperimentId[$experimentId][$variationId])
) {
return $this->_variationIdMapByExperimentId[$experimentId][$variationId];
}

$this->_logger->log(
Logger::ERROR,
sprintf(
'No variation ID "%s" defined in datafile for experiment "%s".',
$variationId,
$experimentId
)
);
$this->_errorHandler->handleError(new InvalidVariationException('Provided variation is not in datafile.'));
return new Variation();
}

/**
* @param $experimentId string ID for experiment.
* @param $variationKey string Key for variation.
*
* @return Variation Entity corresponding to the provided experiment ID and variation Key.
* Dummy entity is returned if key or ID is invalid.
*/
public function getVariationFromKeyByExperimentId($experimentId, $variationKey)
{
if (isset($this->_variationKeyMapByExperimentId[$experimentId])
&& isset($this->_variationKeyMapByExperimentId[$experimentId][$variationKey])
) {
return $this->_variationKeyMapByExperimentId[$experimentId][$variationKey];
}

$this->_logger->log(
Logger::ERROR,
sprintf(
'No variation Key "%s" defined in datafile for experiment "%s".',
$variationKey,
$experimentId
)
);
$this->_errorHandler->handleError(new InvalidVariationException('Provided variation is not in datafile.'));
return new Variation();
}

/**
* Gets the feature variable instance given feature flag key and variable key
*
Expand Down
20 changes: 19 additions & 1 deletion src/Optimizely/Config/ProjectConfigInterface.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2016, 2018-2020 Optimizely
* Copyright 2016, 2018-2021 Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -141,6 +141,24 @@ public function getVariationFromKey($experimentKey, $variationKey);
*/
public function getVariationFromId($experimentKey, $variationId);

/**
* @param $experimentId string ID for experiment.
* @param $variationId string ID for variation.
*
* @return Variation Entity corresponding to the provided experiment ID and variation ID.
* Dummy entity is returned if key or ID is invalid.
*/
public function getVariationFromIdByExperimentId($experimentId, $variationId);

/**
* @param $experimentId string ID for experiment.
* @param $variationKey string Key for variation.
*
* @return Variation Entity corresponding to the provided experiment ID and variation Key.
* Dummy entity is returned if key or ID is invalid.
*/
public function getVariationFromKeyByExperimentId($experimentId, $variationKey);

/**
* Gets the feature variable instance given feature flag key and variable key
*
Expand Down
2 changes: 1 addition & 1 deletion src/Optimizely/DecisionService/DecisionService.php
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ private function getWhitelistedVariation(ProjectConfigInterface $projectConfig,
$forcedVariations = $experiment->getForcedVariations();
if (!is_null($forcedVariations) && isset($forcedVariations[$userId])) {
$variationKey = $forcedVariations[$userId];
$variation = $projectConfig->getVariationFromKey($experiment->getKey(), $variationKey);
$variation = $projectConfig->getVariationFromKeyByExperimentId($experiment->getId(), $variationKey);
if ($variationKey && !empty($variation->getKey())) {
$message = sprintf('User "%s" is forced in variation "%s" of experiment "%s".', $userId, $variationKey, $experiment->getKey());

Expand Down
10 changes: 5 additions & 5 deletions src/Optimizely/Event/Builder/EventBuilder.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2016-2020, Optimizely
* Copyright 2016-2021, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -222,19 +222,19 @@ private function getConversionParams($eventEntity, $eventTags)
* Create impression event to be sent to the logging endpoint.
*
* @param $config ProjectConfigInterface Configuration for the project.
* @param $experimentKey Experiment Experiment being activated.
* @param $experimentId Experiment Experiment being activated.
* @param $variationKey string Variation user
* @param $userId string ID of user.
* @param $attributes array Attributes of the user.
*
* @return LogEvent Event object to be sent to dispatcher.
*/
public function createImpressionEvent($config, $experimentKey, $variationKey, $flagKey, $ruleKey, $ruleType, $enabled, $userId, $attributes)
public function createImpressionEvent($config, $experimentId, $variationKey, $flagKey, $ruleKey, $ruleType, $enabled, $userId, $attributes)
{
$eventParams = $this->getCommonParams($config, $userId, $attributes);

$experiment = $config->getExperimentFromKey($experimentKey);
$variation = $config->getVariationFromKey($experimentKey, $variationKey);
$experiment = $config->getExperimentFromId($experimentId);
$variation = $config->getVariationFromKeyByExperimentId($experimentId, $variationKey);
$impressionParams = $this->getImpressionParams($experiment, $variation, $flagKey, $ruleKey, $ruleType, $enabled);

$eventParams[VISITORS][0][SNAPSHOTS][] = $impressionParams;
Expand Down
25 changes: 16 additions & 9 deletions src/Optimizely/Optimizely.php
Original file line number Diff line number Diff line change
Expand Up @@ -201,16 +201,17 @@ private function validateUserInputs($attributes, $eventTags = null)
}

/**
* @param string Experiment key
* @param string Experiment ID
* @param string Variation key
* @param string User ID
* @param array Associative array of user attributes
* @param DatafileProjectConfig DatafileProjectConfig instance
*/
protected function sendImpressionEvent($config, $experimentKey, $variationKey, $flagKey, $ruleKey, $ruleType, $enabled, $userId, $attributes)
protected function sendImpressionEvent($config, $experimentId, $variationKey, $flagKey, $ruleKey, $ruleType, $enabled, $userId, $attributes)
{
$experimentKey = $config->getExperimentFromId($experimentId)->getKey();
$impressionEvent = $this->_eventBuilder
->createImpressionEvent($config, $experimentKey, $variationKey, $flagKey, $ruleKey, $ruleType, $enabled, $userId, $attributes);
->createImpressionEvent($config, $experimentId, $variationKey, $flagKey, $ruleKey, $ruleType, $enabled, $userId, $attributes);
$this->_logger->log(Logger::INFO, sprintf('Activating user "%s" in experiment "%s".', $userId, $experimentKey));
$this->_logger->log(
Logger::DEBUG,
Expand Down Expand Up @@ -244,10 +245,10 @@ protected function sendImpressionEvent($config, $experimentKey, $variationKey, $
$this->notificationCenter->sendNotifications(
NotificationType::ACTIVATE,
array(
$config->getExperimentFromKey($experimentKey),
$config->getExperimentFromId($experimentId),
$userId,
$attributes,
$config->getVariationFromKey($experimentKey, $variationKey),
$config->getVariationFromKeyByExperimentId($experimentId, $variationKey),
$impressionEvent
)
);
Expand Down Expand Up @@ -340,6 +341,7 @@ public function decide(OptimizelyUserContext $userContext, $key, array $decideOp
$variationKey = null;
$featureEnabled = false;
$ruleKey = null;
$experimentId = null;
$flagKey = $key;
$allVariables = [];
$decisionEventDispatched = false;
Expand All @@ -360,9 +362,11 @@ public function decide(OptimizelyUserContext $userContext, $key, array $decideOp
$variationKey = $variation->getKey();
$featureEnabled = $variation->getFeatureEnabled();
$ruleKey = $decision->getExperiment()->getKey();
$experimentId = $decision->getExperiment()->getId();
} else {
$variationKey = null;
$ruleKey = null;
$experimentId = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks redundant?

}

// send impression only if decide options do not contain DISABLE_DECISION_EVENT
Expand All @@ -374,7 +378,7 @@ public function decide(OptimizelyUserContext $userContext, $key, array $decideOp
if ($source == FeatureDecision::DECISION_SOURCE_FEATURE_TEST || $sendFlagDecisions) {
$this->sendImpressionEvent(
$config,
$ruleKey,
$experimentId,
$variationKey === null ? '' : $variationKey,
$flagKey,
$ruleKey === null ? '' : $ruleKey,
Expand Down Expand Up @@ -531,8 +535,9 @@ public function activate($experimentKey, $userId, $attributes = null)
$this->_logger->log(Logger::INFO, sprintf('Not activating user "%s".', $userId));
return null;
}
$experimentId = $config->getExperimentFromKey($experimentKey)->getId();

$this->sendImpressionEvent($config, $experimentKey, $variationKey, '', $experimentKey, FeatureDecision::DECISION_SOURCE_EXPERIMENT, true, $userId, $attributes);
$this->sendImpressionEvent($config, $experimentId, $variationKey, '', $experimentKey, FeatureDecision::DECISION_SOURCE_EXPERIMENT, true, $userId, $attributes);

return $variationKey;
}
Expand Down Expand Up @@ -818,19 +823,21 @@ public function isFeatureEnabled($featureFlagKey, $userId, $attributes = null)
$featureEnabled = $variation->getFeatureEnabled();
}
$ruleKey = $decision->getExperiment() ? $decision->getExperiment()->getKey() : '';
$this->sendImpressionEvent($config, $ruleKey, $variation ? $variation->getKey() : '', $featureFlagKey, $ruleKey, $decision->getSource(), $featureEnabled, $userId, $attributes);
$experimentId = $decision->getExperiment() ? $decision->getExperiment()->getId() : '';
$this->sendImpressionEvent($config, $experimentId, $variation ? $variation->getKey() : '', $featureFlagKey, $ruleKey, $decision->getSource(), $featureEnabled, $userId, $attributes);
}

if ($variation) {
$experimentKey = $decision->getExperiment()->getKey();
$experimentId = $decision->getExperiment()->getId();
$featureEnabled = $variation->getFeatureEnabled();
if ($decision->getSource() == FeatureDecision::DECISION_SOURCE_FEATURE_TEST) {
$sourceInfo = (object) array(
'experimentKey'=> $experimentKey,
'variationKey'=> $variation->getKey()
);

$this->sendImpressionEvent($config, $experimentKey, $variation->getKey(), $featureFlagKey, $experimentKey, $decision->getSource(), $featureEnabled, $userId, $attributes);
$this->sendImpressionEvent($config, $experimentId, $variation->getKey(), $featureFlagKey, $experimentKey, $decision->getSource(), $featureEnabled, $userId, $attributes);
} else {
$this->_logger->log(Logger::INFO, "The user '{$userId}' is not being experimented on Feature Flag '{$featureFlagKey}'.");
}
Expand Down
Loading