[Alex Yong (alexjoeyyong)]

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

• A local holdout hit on an experiment or delivery rule skips that rule and evaluation continues with the next rule; it no longer ends the flag decision
• Each local holdout hit is recorded on the feature decision, and the decide path sends one holdout impression per hit, regardless of sendFlagDecisions
• The DISABLE_DECISION_EVENT decide option suppresses local holdout impressions
• Legacy (non-decide) APIs skip a locally held-out rule without sending a holdout event
• Forced decisions, global holdouts, and the final decision and its events are unchanged

Jira Ticket

FSSDK-12958

[datadog-optimizely-experimentation]

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: e22b181 | Docs | Give us feedback!}

[Matjaz Pirnovar (Mat001)]

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 through get_variation_for_feature and never iterate holdout_decisions; only _create_optimizely_decision (decide path) dispatches.
• Java (#640): isFeatureEnabled / getFeatureVariable* consume only the final FeatureDecision and never iterate holdoutDecisions; only createOptimizelyDecision dispatches.

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 []FeatureDecision for local hits (dispatched by the loop in dispatchDecisionEvents), and
• HoldoutExperiment / HoldoutVariation + sendHoldoutImpression for the global excludeTargetedDeliveries bypass.

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.

[Matjaz Pirnovar (Mat001)]

Requesting changes for item 1:

1. Legacy APIs should not emit holdout impressions

[Alex Yong (alexjoeyyong)]

Thanks for the review — addressed in e22b181 via the shared spec (FSSDK-12958):

1. Legacy APIs now send no holdout impressions for local hits or the excludeTD bypass. Those sendHoldoutImpression calls came from FSSDK-12735 and shipped in v2.6.0, so this changes released behavior; the spec now pins it for all SDKs.
2. Unified into one ordered HoldoutDecisions list (global excludeTD first, then experiment, then delivery hits) dispatched by one loop. HoldoutExperiment/HoldoutVariation actually shipped in v2.6.0, so they're kept as deprecated fields mirroring the first entry rather than removed.
3. Noted on FSC; FSSDK-13294 tracks it.
   Ready for another look.

[Matjaz Pirnovar (Mat001)]

my requests for change were addressed. Approving.