feat(experimentation): experiment exposures read and refresh endpoints#7754
feat(experimentation): experiment exposures read and refresh endpoints#7754gagantrivedi wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7754 +/- ##
==========================================
+ Coverage 98.55% 98.57% +0.01%
==========================================
Files 1454 1460 +6
Lines 56000 56489 +489
==========================================
+ Hits 55193 55682 +489
Misses 807 807 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to retrieve and refresh experiment exposures, adding a new refresh_requested_at field, a serializer, a background task for computing exposures, and API endpoints with rate-limiting. The feedback highlights a potential race condition in api/experimentation/views.py where enqueuing the background task immediately with delay() inside a database transaction could cause an IntegrityError if the worker executes before the transaction commits; using transaction.on_commit is recommended to resolve this.
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
Zaimwa9
left a comment
There was a problem hiding this comment.
Looks good but i'd appreciate logging refreshing errors and potentially a guard against refreshing the exposure data if an experiment has been running past the 90 days TTL?
| except Exception: | ||
| exposures.record_failure() | ||
| return |
There was a problem hiding this comment.
Can we log an error here or within record_failure ?
| def is_final(self) -> bool: | ||
| # Recomputing a final row can only lose data: warehouse events expire. | ||
| ended_at = self.experiment.ended_at | ||
| return ( |
There was a problem hiding this comment.
Can we also return true when an experiment has been running for more than 90 days and the first buckets data have expired ?
docs/if required so people know about the feature. (deferred — flag-gated; docs land with the UI.)Changes
Contributes to the experimentation exposures panel (v0.1). Follows #7740; UI lands next.
GET …/experiments/{id}/exposures/— returns the stored row ({"exposures": null}before the first compute); never hits ClickHouse.POST …/experiments/{id}/exposures/refresh/— stampsrefresh_requested_at(migration 0007) and enqueues the compute task; 202. Guards: 400 before start, 400 once final, 429 +Retry-AfterwithinEXPOSURES_REFRESH_MIN_INTERVAL(5 min).compute_experiment_exposures(experiment_id)— full-window recompute (started_at→ended_at or now); warehouse errors →record_failure, preserving the last good payload; skips deleted, never-started and final rows.ExperimentExposures.is_final— finality (as_of >= ended_at) defined once on the model and enforced in the task, so no caller can recompute a final row after the warehouse's 90-day event TTL and lose data.tasksnow importsmodels/services, not the reverse; theWarehouseConnectionhooks defer their task imports to the enqueue site.Both endpoints sit behind the existing
ExperimentPermission.How did you test this code?
Retry-After, finalising refresh allowed vs final row rejected, failure preserves payload, deleted/never-started skips, 403s.pytest tests/unit/experimentation/— 275 passed;ruff+mypystrict clean.