fix(decide): change to return reasons as a part of tuple in decision hierarchy #415
Conversation
Pull Request Test Coverage Report for Build 1644
💛 - Coveralls |
| @@ -545,17 +546,18 @@ public boolean setForcedVariation(@Nonnull Experiment experiment, | |||
| * | |||
| * @param experiment The experiment forced. | |||
| * @param userId The user ID to be used for bucketing. | |||
| * @param reasons Decision log messages | |||
| * @return The variation the user was bucketed into. This value can be null if the | |||
There was a problem hiding this comment.
Should update comments to return DecisionResponse
| @@ -344,15 +368,14 @@ Variation getWhitelistedVariation(@Nonnull Experiment experiment, | |||
| * @param experiment {@link Experiment} in which the user was bucketed. | |||
| * @param userProfile {@link UserProfile} of the user. | |||
| * @param projectConfig The current projectConfig | |||
| * @param reasons Decision log messages | |||
| * @return null if the {@link UserProfileService} implementation is null or the user was not previously bucketed. | |||
There was a problem hiding this comment.
Should update comments with DecisionResponse
| @@ -427,7 +423,7 @@ private Boolean isFeatureEnabled(@Nonnull ProjectConfig projectConfig, | |||
|
|
|||
| Map<String, ?> copiedAttributes = copyAttributes(attributes); | |||
| FeatureDecision.DecisionSource decisionSource = FeatureDecision.DecisionSource.ROLLOUT; | |||
| FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig); | |||
| FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig).getResult(); | |||
There was a problem hiding this comment.
The decision service methods are all marked as nullable return value.
There was a problem hiding this comment.
A good catch. I'll change them all @nonnull.
| @Nonnull Map<String, ?> filteredAttributes, | ||
| @Nonnull ProjectConfig projectConfig) { | ||
| return getVariation(experiment, userId, filteredAttributes, projectConfig, Collections.emptyList(), DefaultDecisionReasons.newInstance()); | ||
| public DecisionResponse<Variation> getVariation(@Nonnull Experiment experiment, |
There was a problem hiding this comment.
These methods are all marked as nullable return value. However, the usage does not test for null:
Variation variation = decisionService.getVariation(experiment, userId, copiedAttributes, projectConfig).getResult();
There was a problem hiding this comment.
I'll change them all @nonnull.
| } catch (NullPointerException e) { | ||
| String message = reasons.addInfo("attribute or value null for match %s", match != null ? match : "legacy condition"); | ||
| logger.error(message, e); | ||
| logger.error("attribute or value null for match {}", match != null ? match : "legacy condition", e); |
There was a problem hiding this comment.
if the only reason to have reasons is for debugging, then, not having these doesn't make sense. also, we need to make sure we are doing the same for other sdks. We were including attribute failures before.
There was a problem hiding this comment.
I agree it's a good to have in reasons, so was added before. Found that this is the only meaningful message from conditions. It's quite expensive to apply the unused reasons to all Conditions, that's why I decided to drop this from reasons. We provide other relevant reasons like "does not meet audience condition", ... and they can take a look at log messages for investigation if needed.
Summary
Test plan