fix(analytics): distinguish fallback reasons + forward backend error message (SDK-79, SDK-83)#180
fix(analytics): distinguish fallback reasons + forward backend error message (SDK-79, SDK-83)#180tylerjroach wants to merge 6 commits into
Conversation
tylerjroach
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #180 +/- ##
==========================================
+ Coverage 95.79% 95.96% +0.16%
==========================================
Files 13 13
Lines 2259 2401 +142
Branches 129 136 +7
==========================================
+ Hits 2164 2304 +140
- Misses 62 63 +1
- Partials 33 34 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SelectedVariant now carries two source fields: `variant_source` (local | remote | fallback) and `fallback_reason` (FLAG_NOT_FOUND | MISSING_CONTEXT_KEY | NO_ROLLOUT_MATCH | BACKEND_ERROR | NOT_READY, set only when source is fallback). Three behaviorally distinct outcomes — flag-not-found, no-rollout-match, and missing-context-key — previously all returned the bare fallback. The OpenFeature wrapper collapsed them to FLAG_NOT_FOUND, sending callers chasing the flag name when the real cause was usually a rule miss or absent context. The wrapper now dispatches on fallback_reason and maps each to the spec-correct OpenFeature response. Most notably, NO_ROLLOUT_MATCH becomes `reason: DEFAULT` with no error code instead of FLAG_NOT_FOUND. Constant names align with mixpanel-php for consistency across SDKs. Linear: SDK-79 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
tylerjroach
force-pushed
the
fix/sdk-79-variant-source-fallback-reason
branch
from
d07720e to
4d8ba39
Compare
…end message (SDK-79, SDK-83)
SDK-79 made fallback_reason a PHP-aligned string constant on every
returned fallback variant. That covers the local-eval cases (flag
not found, no rollout match, missing context key) but couldn't carry
detail — when the remote /flags endpoint returned an error response,
the SDK still had nowhere to attach the backend's message.
Upgrade FallbackReason from a class of string constants to a frozen
Pydantic value object with `kind` (the discriminator) and optional
`message`. Frozen singletons for the no-detail reasons; factory
methods for the ones that carry detail.
SDK-83: RemoteFeatureFlagsProvider's except branches now tag the
fallback with FallbackReason.backend_error(message). For httpx
HTTPStatusError specifically, the response body is extracted via
_describe_backend_error so the actionable detail ("distinct_id must
be provided in evalContext as a string") reaches the wrapper —
str(exc) alone only yields httpx's standardized status line.
Wrapper dispatches on reason.kind, forwards reason.message into
FlagResolutionDetails.error_message for backend_error and
missing_context_key.
Linear: SDK-79, SDK-83
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
tylerjroach
changed the title
The wrapper short-circuits to PROVIDER_NOT_READY at the top of resolve when areFlagsReady() is false (see _are_flags_ready), so no producer ever constructs a FallbackReason with kind NOT_READY — the case was dead, same pattern Swift PR #745 and Android PR #981 just cleaned up. Remove the kind from the Literal type, drop the not_ready() factory and _NOT_READY singleton, and drop the NOT_READY dispatch arm from the wrapper's error_mapping. The provider_not_ready short-circuit test path is unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comments were explaining the absence of a case to a hypothetical cross-SDK reader. The absence is self-explanatory. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the Mixpanel Python SDK’s feature-flag “fallback” plumbing so callers (especially via the OpenFeature provider) can distinguish why a fallback occurred and, for remote evaluation errors, see the backend’s error message rather than an undifferentiated “flag not found”.
Changes:
- Introduces
VariantSourceand a structuredFallbackReason(kind, message)onSelectedVariant, with helper methods to tag successful variants vs. fallbacks. - Tags local and remote flag evaluation results with
variant_source/fallback_reason, including propagating remote backend error bodies intoFallbackReason.message. - Updates the OpenFeature provider to map each fallback kind to the spec-appropriate
FlagResolutionDetails(error code/reason/message), and adds/extends tests for the new mapping behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| openfeature-provider/tests/test_provider.py | Extends wrapper tests to cover new fallback kinds and forwarded backend/missing-key messages. |
| openfeature-provider/src/mixpanel_openfeature/provider.py | Maps SDK fallback reasons to OpenFeature responses and forwards error messages. |
| mixpanel/flags/types.py | Adds VariantSource, FallbackReason, and SelectedVariant helpers for tagging evaluation outcomes. |
| mixpanel/flags/test_remote_feature_flags.py | Adds test ensuring backend HTTP error text is preserved through FallbackReason.message. |
| mixpanel/flags/test_local_feature_flags.py | Adds tests validating local provider tags match/fallback cases with correct reason/message. |
| mixpanel/flags/remote_feature_flags.py | Tags remote results as REMOTE or FALLBACK, and captures backend error bodies for fallbacks. |
| mixpanel/flags/local_feature_flags.py | Tags local results as LOCAL or FALLBACK with specific fallback kinds. |
| def __init__( | ||
| self, | ||
| flags_provider: typing.Any, | ||
| mixpanel_instance: Optional[Mixpanel] = None, | ||
| mixpanel_instance: Mixpanel | None = None, | ||
| ) -> None: |
|
|
||
| @property | ||
| def mixpanel(self) -> Optional[Mixpanel]: | ||
| def mixpanel(self) -> Mixpanel | None: |
| flag_key: str, | ||
| default_value: bool, | ||
| evaluation_context: typing.Optional[EvaluationContext] = None, | ||
| evaluation_context: EvaluationContext | None = None, |
| flag_key: str, | ||
| default_value: str, | ||
| evaluation_context: typing.Optional[EvaluationContext] = None, | ||
| evaluation_context: EvaluationContext | None = None, |
| flag_key: str, | ||
| default_value: int, | ||
| evaluation_context: typing.Optional[EvaluationContext] = None, | ||
| evaluation_context: EvaluationContext | None = None, |
| default_value: typing.Any, | ||
| expected_type: typing.Optional[type], | ||
| evaluation_context: typing.Optional[EvaluationContext] = None, | ||
| expected_type: type | None, | ||
| evaluation_context: EvaluationContext | None = None, | ||
| ) -> FlagResolutionDetails: |
| def _fallback_details( | ||
| fallback_reason: FallbackReason | None, default_value: typing.Any | ||
| ) -> FlagResolutionDetails | None: |
| fallback_details = self._fallback_details( | ||
| result.fallback_reason, default_value | ||
| ) | ||
| if fallback_details is not None: | ||
| return fallback_details |
| Set by the providers on every returned variant — coarse-grained | ||
| (local / remote / fallback). For the specific reason behind a fallback, | ||
| see FallbackReason. |
| kind: Literal[ | ||
| "FLAG_NOT_FOUND", | ||
| "MISSING_CONTEXT_KEY", | ||
| "NO_ROLLOUT_MATCH", | ||
| "BACKEND_ERROR", | ||
| ] |
ketanmixpanel
left a comment
ketanmixpanel
left a comment
There was a problem hiding this comment.
@tylerjroach Take a look at the copilot comments, apart from that looks good to me.
…ndling
Two review-driven fixes:
1. RemoteFeatureFlagsProvider.{get,aget}_all_variants now stamp
variant_source=REMOTE on every returned variant, matching the
docstring's promise that VariantSource is set on "every returned
variant". Previously these methods returned the raw decoded
SelectedVariants and undercut the wrapper's source-based dispatch.
2. The OpenFeature wrapper now treats `result is fallback &&
fallback_reason is None` as FLAG_NOT_FOUND. Defends against custom
flags providers written against the pre-SDK-79 contract that return
the fallback object unchanged — otherwise the wrapper would
incorrectly report a successful targeting match.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks @ketanmixpanel — pushed 1. 2. Wrapper treating bare fallback as a successful match (provider.py:172 — Copilot ID 3501288498): Real regression risk for any custom flags provider written against the pre-SDK-79 contract that returns the fallback object unchanged. Added a defensive 3. NOT_READY mention in PR description (types.py:98 — Copilot ID 3501288546): The kind was removed entirely in 4. PEP 604 union syntax warnings on Python 3.9 (multiple Copilot threads on Tests: 148 passed, ruff clean. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def missing_context_key(cls, key: Optional[str] = None) -> "FallbackReason": | ||
| return cls(kind="MISSING_CONTEXT_KEY", message=key) |
There was a problem hiding this comment.
missing_context_key accepts None key silently
The key parameter defaults to None, so FallbackReason.missing_context_key() produces a MISSING_CONTEXT_KEY reason with message=None. The OpenFeature wrapper then forwards None as error_message, giving callers no indication of which attribute is missing — defeating the purpose of SDK-79's richer error propagation. All current call sites pass the key correctly, but the signature invites misuse. Consider removing the default or asserting key is not None.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| """ | ||
| if fallback_reason is None: | ||
| return None | ||
| # Flag exists, user just didn't match any rollout — per the | ||
| # OpenFeature spec this is `reason: DEFAULT` with no error. | ||
| if fallback_reason.kind == "NO_ROLLOUT_MATCH": | ||
| return FlagResolutionDetails(value=default_value, reason=Reason.DEFAULT) | ||
| error_mapping = { | ||
| "FLAG_NOT_FOUND": (ErrorCode.FLAG_NOT_FOUND, Reason.DEFAULT), | ||
| "MISSING_CONTEXT_KEY": (ErrorCode.TARGETING_KEY_MISSING, Reason.ERROR), | ||
| "BACKEND_ERROR": (ErrorCode.GENERAL, Reason.ERROR), | ||
| } | ||
| if fallback_reason.kind in error_mapping: | ||
| error_code, reason = error_mapping[fallback_reason.kind] | ||
| return FlagResolutionDetails( | ||
| value=default_value, | ||
| error_code=error_code, | ||
| error_message=fallback_reason.message, | ||
| reason=reason, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
fallback_reason.kind
If fallback_reason is set (not None) but its kind isn't NO_ROLLOUT_MATCH and isn't in error_mapping, _fallback_details returns None. The caller in _resolve then falls through to the success path and returns reason=TARGETING_MATCH with the fallback value — a false positive. All four current Literal kinds are handled, so this doesn't fire today, but adding a new kind to the Literal without updating this mapping would silently misreport it as a successful evaluation. A final else branch returning a GENERAL error (or an assert_never) would make the exhaustiveness explicit.
Summary
Bundles two related fixes. Both touch the same
fallback_reasonplumbing so they merge as one PR.SDK-79 — distinguish fallback reasons (local eval)
Three local-evaluation outcomes — flag-not-found, no-rollout-match, and missing-context-key — previously all returned the bare developer fallback. The OpenFeature wrapper collapsed them to
FLAG_NOT_FOUND, sending callers chasing the flag name when the real cause was usually a rule miss or absent context.SelectedVariantnow carries two source fields:variant_source(local/remote/fallback) andfallback_reason(set only when source isfallback). The wrapper dispatches onfallback_reasonand maps each to the spec-correct OpenFeature response — most notably,NO_ROLLOUT_MATCHbecomesreason: DEFAULTwith no error code instead ofFLAG_NOT_FOUND.SDK-83 — forward the backend's error message (remote eval)
When the remote
/flagsendpoint returned an error response (e.g. HTTP 400 with "distinct_id must be provided in evalContext as a string"), the catch block returned the fallback variant without attaching the cause. The wrapper saw a fallback and translated toFLAG_NOT_FOUND— indistinguishable from a genuinely missing flag. Go propagates these asGENERALwith the full backend message; the goal here is to match that.To carry the message,
FallbackReasonis upgraded from a bare string constant to a small value object withkind(the PHP-aligned discriminator) and optionalmessage(set onBACKEND_ERRORandMISSING_CONTEXT_KEY). The remote provider's catch branches tag the fallback withFallbackReason.backend_error(message); the wrapper forwardsmessageinto OpenFeature'serror_message/errorMessage.Fix pattern
FallbackReasonkinds (PHP-aligned)FLAG_NOT_FOUND/flagsresponseMISSING_CONTEXT_KEYNO_ROLLOUT_MATCHBACKEND_ERROR/flagserror responseOpenFeature mapping
FLAG_NOT_FOUNDerror_code: FLAG_NOT_FOUND,reason: DEFAULTMISSING_CONTEXT_KEYerror_code: TARGETING_KEY_MISSING,reason: ERROR,error_message: <attribute name>NO_ROLLOUT_MATCHreason: DEFAULT, no errorBACKEND_ERRORerror_code: GENERAL,reason: ERROR,error_message: <backend body>PROVIDER_NOT_READYis handled by the wrapper's_are_flags_readyshort-circuit before invoking the underlying provider, so no producer ever stamps aNOT_READYkind.Test plan
BACKEND_ERRORproducer-side test verifies the backend response body propagates through tofallback_reason.messageerror_message/errorMessageforwards the backend message forBACKEND_ERRORand the missing-attribute name forMISSING_CONTEXT_KEY🤖 Generated with Claude Code