Report prerequisite exposures and add an opt-in deferred tracking buffer - #135
Conversation
Confidence Score: 5/5The PR appears safe to merge; no outstanding or newly introduced actionable failures remain. The changes since the previous review are empty, no repository-rule violations were identified, and the only previous finding was resolved after Greptile accepted that concurrent reuse falls outside the synchronous client's documented single-user contract. Reviews (2): Last reviewed commit: "trim call overhead on the no-telemetry e..." | Re-trigger Greptile |
|
Re the summary above: the single P1 was discussed and resolved in the inline thread — the scenario (one sync instance shared across concurrent requests) is outside the sync client's single-user contract (instance-level attributes already race there), the README prescribes per-request instances, and the concurrent multi-user case is the async client's caller-owned per-request |
Closes #133
Summary
Two related changes:
eval_prereqsdropped every callback, so users were bucketed into real experiments that never showed up in analytics.setDeferredTrackingCalls()+fireDeferredTrackingCalls(). This is the missing piece for SSR and remote-evaluation setups.Replaces #133 with a different structure: callbacks live on the
EvaluationContext(like the JS SDK's global context), so nested evaluations — prerequisites included — inherit them automatically instead of depending on every call site to pass them along. The deferred buffer is owned by the caller rather than stored onUserContext.What changes for users
Prerequisite telemetry:
on_experiment_viewedfires for prerequisite experiment assignments, including when the gate ultimately fails (matches the JS SDK).on_feature_usagefires for every feature an evaluation touches, prerequisites included — once per key per evaluation.subscribe()andget_all_results()now see prerequisite experiments.run(). It has no per-user change-detection, so firing on every eval would repeat every subscriber callback on every request. (The JS multi-user client has no eval-time subscriptions either.)Deferred tracking:
GrowthBook(defer_tracking=True), thenget_deferred_tracking_calls()andclear_deferred_tracking_calls(). One instance per request when serving multiple users, as the README already prescribes.TrackingBufferto the eval methods. The caller owns the buffer, so concurrent requests cannot mix.on_experiment_viewed— both fire (same semantics as the Go SDK, Track passthrough and prerequisite exposures; deferred tracking; user context on ExperimentCallback (breaking) growthbook-golang#84).json.dumps-ready. An exposure carrying a non-JSON value (say, adatetimeattribute) is dropped with a log line; the rest of the batch is unaffected.usercontext requires JS SDK 1.7.0+.Smaller fixes that fell out of review:
core.eval_feature/core.run_experimentstill accept the oldtracking_cb/callback_subscriptionkeyword arguments. They are deprecated, emit aDeprecationWarning, and apply only to that call.Experiment.to_dict()no longer turns an explicitcoverageof0into1.Performance
Measured with
python tests/scripts/benchmark_eval_overhead.py(500k sequential in-memory evals, no callbacks configured, best of 5):The cheapest path costs about 0.13 us more per evaluation; a realistic experiment path is within ~1%. The remaining cost is the
EvaluationContextcarrying the callback fields — the mechanism that fixes the dropped prerequisite exposures — and is accepted deliberately (there is a Performance note in the CHANGELOG). Usage bookkeeping and the deprecated-kwarg shim are skipped entirely when unused.Test plan
pytest tests/ -q --ignore=tests/test_live.py— 972 passed. (Twotest_typing.pymypy failures are a local urllib3-stub environment issue; they fail identically onmainand pass in CI.)trackingCallssection in cases.json — 5 cases run by both the sync and async suites. It is a Python-local extension listed in the freshness checker, so it gets drift-checked automatically if the JS corpus adopts it.tests/scripts/simulate_deferred_tracking.pyruns SSR and concurrent-API scenarios against the published@growthbook/growthbook1.7.0 in Node. 60 requests, 148 exposures fired with the exposure-time user context; expected values are derived from the SDK's own assignments, not hard-coded. Injected regressions (a dropped prerequisite exposure, a corrupted user) are caught.tests/test_tracking_telemetry.pycovers the buffer API: snapshot and JSON-drop semantics, dedupe collisions, legacy-kwarg scoping, async per-request isolation.Follow-ups
trackingCallssection upstream in the JS SDK's shared cases.json — no SDK's spec asserts tracking today, which is how this bug stayed invisible everywhere.Clientwith clone-sharing, toward an eval-scoped collector like this design.eventLoggersilently disables deferral.