feat!: implement experimentation tracking - #37
Conversation
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
|
@themis-blindfold review |
⚖️ Themis review: ✅ Ship itSolid, well-guarded experimentation surface. The tracking/exposure logic is correct against the SDK's
📝 Walkthrough
🧪 How to verify
Automate: all covered by Product take: This closes the experimentation gap between the JS and Python OpenFeature providers. Users can now record experiment exposures via the hook (recommended), explicit 🧭 Assumptions & unverified claims
Clean experimentation surface, correct guards, thorough tests — the rare PR where the reviewer's job is to confirm rather than correct. · reviewed at 2682781 |
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
4012ab6 to
d070cb4
Compare
|
|
||
| def __init__( | ||
| self, | ||
| provider: "FlagsmithProvider", |
There was a problem hiding this comment.
As discussed: the hook takes the provider, not the Flagsmith client.
So it can call provider.track(EXPOSURE_TRACKING_EVENT, …) instead of calling client.track_exposure_event directly.
It's the hook which emits the same event as an explicit of_client.track call so all the mapping context x identifier/traits and other checks can live in the same place.
Trade-off is one extra layer versus hitting the SDK directly from the hook. Wdyt ?
There was a problem hiding this comment.
Also worth noting that in the SDK's current implementation, track_exposure_event raises in some scenarios (e.g enable_events false).
If uncaught in an after hook, OpenFeature turns that into ERROR + default value on every hooked evaluation so this way we cna keep the errors contained in the hook
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
| dedupe_key = json.dumps([targeting_key, details.flag_key, variant]) | ||
| with self._lock: | ||
| if dedupe_key in self._seen: | ||
| self._seen.move_to_end(dedupe_key) | ||
| return | ||
| self._seen[dedupe_key] = None | ||
| while len(self._seen) > self._max_dedupe_entries: | ||
| self._seen.popitem(last=False) |
There was a problem hiding this comment.
I think this belongs in the SDK. See Flagsmith/flagsmith-python-client#233 — that PR doesn't replicate the LRU logic in here, but will protect against bursts / extra requests nonetheless.
The problem with a dedup layer in the hook is that it will not work without additional caching on the consuming side. You cannot attach hooks to a provider instance (singleton), only to a client instance (never a singleton, created on-demand).
|
|
||
| test-sdk-floor: | ||
| runs-on: ubuntu-latest | ||
| name: Pytest against the minimum supported flagsmith version |
There was a problem hiding this comment.
nit: We should really have a matrix analogous to what we have in the Python SDK repo. Can be a follow-up issue/PR.
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Changes
Ports the experimentation surface of the JS OpenFeature provider (open-feature/js-sdk-contrib#1591) to the Python provider.
reason,variantandflag_metadata(enabled,featureId, andexperiment.arm/experiment.active/experiment.unitfor multivariate assignments). Multivariate percentage-split assignments resolve with reasonSPLIT, aligned with the engine's reason taxonomy.feature_flag.exposureevent records experiment exposures, with an explicit variant, or variant-less (the provider resolves the flag for the context's targeting key and applies the same guards as the SDK'sget_experiment_flag).FlagsmithExposureHook, an opt-in after-hook that records an exposure when an evaluation resolves with a variant and reasonSPLIT, deduped per identity/flag/variant in a bounded thread-safe LRU. Attaching the hook to a call site is the experiment declaration; nothing auto-exposes."transient": Truecontext attribute.track()crashing with an uncaughtTypeErroron flagsmith >=5.4 (the SDK renamedtrack_event(identity_identifier=)toidentifier=).How did you test this code?
BREAKING CHANGE
The minimum supported flagsmith version is now 5.5. track() previously crashed with a TypeError on flagsmith >=5.4 due to the SDK's renamed track_event signature; it now calls the current signature and sends TrackingEventDetails.value as the first-class Flagsmith event value instead of metadata["value"].