Repository navigation
[AI-FSSDK] [FSSDK-12958] Continue rule evaluation after local holdout hit - #460
alexjoeyyong wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
Mat001
left a comment
There was a problem hiding this comment.
Reviewed against the canonical cross-SDK implementations of this spec (python-sdk#525, java-sdk#640) and the two in-flight siblings (javascript-sdk#1181, swift-sdk#669). The core of this PR is solid — record-and-continue in both the experiment and rollout loops, one impression per hit gated by DisableDecisionEvent (not sendFlagDecisions), decision-error suppression via the early return in decide(), and an additive change to the public decision.FeatureDecision (nice — Go sidesteps the public-API break that java-sdk#640 had to walk back).
Two divergences from the canonical implementations I'd like to resolve before approving, plus one non-blocking heads-up.
1. Legacy APIs should not emit holdout impressions (please change)
IsFeatureEnabled and GetDetailedFeatureDecisionUnsafe now call sendHoldoutImpression(...). In the canonical SDKs, the legacy single-flag path sends no holdout impression at all — including for the excludeTargetedDeliveries bypass. Only the decide / OptimizelyDecision path dispatches holdout impressions.
- Python (#525):
is_feature_enabled/get_feature_variable_*go throughget_variation_for_featureand never iterateholdout_decisions; only_create_optimizely_decision(decide path) dispatches. - Java (#640):
isFeatureEnabled/getFeatureVariable*consume only the finalFeatureDecisionand never iterateholdoutDecisions; onlycreateOptimizelyDecisiondispatches.
The spec is explicit: "Legacy (non-decide) APIs such as isFeatureEnabled / getFeatureVariable skip a locally held-out rule without sending a holdout event, matching how they already treat global holdouts bypassed by excludeTargetedDeliveries."
Suggest removing the sendHoldoutImpression(...) calls from IsFeatureEnabled and GetDetailedFeatureDecisionUnsafe so the Go legacy path matches the others. If that sendHoldoutImpression-from-legacy behavior was inherited from the FSSDK-12735 excludeTD work rather than intentional here, worth confirming either way.
2. Consider unifying the two holdout representations into one ordered list (design)
This PR carries two parallel mechanisms:
LocalHoldoutDecisions []FeatureDecisionfor local hits (dispatched by the loop indispatchDecisionEvents), andHoldoutExperiment/HoldoutVariation+sendHoldoutImpressionfor the globalexcludeTargetedDeliveriesbypass.
All four reference SDKs deliberately collapse these into a single ordered holdout list — the global excludeTD hit prepended first, then experiment-rule hits, then delivery-rule hits — handled by one dispatch loop. Python's PR body states it directly: "Local hits share the existing mechanism used for global holdouts bypassed by excludeTargetedDeliveries." Java prepends the global hit at index 0 of holdoutDecisions.
Since HoldoutExperiment / HoldoutVariation are new (unreleased) fields, folding them into LocalHoldoutDecisions needs no deprecation and would: match the canonical shape, give consistent impression ordering, and delete the separate sendHoldoutImpression path. Not a correctness blocker on its own, but it's the one place Go's design is the outlier across all five SDKs.
3. Non-blocking heads-up: FSC
The cross-SDK compatibility suite still encodes the old "return immediately" behavior; the new scenarios are tracked in FSSDK-13294 and not merged yet. Expect FSC red until those land — worth coordinating the merge so this isn't mistaken for a regression.
Happy to pair on any of these.
Mat001
left a comment
There was a problem hiding this comment.
Requesting changes for item 1:
- Legacy APIs should not emit holdout impressions
Summary
Previously, a local holdout hit returned the holdout variation immediately and skipped all remaining rules. Now a local holdout hit excludes the user from that rule only, sends a holdout impression, and evaluation continues to the next experiment or delivery rule.
Changes
Jira Ticket
FSSDK-12958