feat(experimentation): add Experiment base model and CRUD endpoints#7591
feat(experimentation): add Experiment base model and CRUD endpoints#7591Zaimwa9 wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
github-actions
Bot
added
api
Docker builds report
|
github-actions
Bot
added
feature
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new experimentation framework, including the Experiment model, associated API endpoints, and audit logging. Key additions include status transition logic, permissions based on feature flags, and comprehensive unit tests. Review feedback suggests moving timestamp management to model lifecycle hooks for better consistency, optimizing environment lookups in permission classes by utilizing cached data, and updating the error response status code to 409 when an active experiment already exists to align with existing patterns and handle potential race conditions.
| class Experiment(LifecycleModelMixin, SoftDeleteExportableModel): # type: ignore[misc] | ||
| environment = models.ForeignKey( | ||
| Environment, | ||
| on_delete=models.CASCADE, | ||
| related_name="experiments", | ||
| ) | ||
| feature = models.ForeignKey( | ||
| "features.Feature", | ||
| on_delete=models.CASCADE, | ||
| related_name="experiments", | ||
| ) | ||
| name = models.CharField(max_length=255) | ||
| hypothesis = models.TextField() | ||
| status = models.CharField( | ||
| max_length=50, | ||
| choices=ExperimentStatus.choices, | ||
| default=ExperimentStatus.CREATED, | ||
| ) | ||
| created_at = models.DateTimeField(auto_now_add=True) | ||
| updated_at = models.DateTimeField(auto_now=True) | ||
| started_at = models.DateTimeField(null=True, blank=True) | ||
| ended_at = models.DateTimeField(null=True, blank=True) |
There was a problem hiding this comment.
The logic for automatically setting started_at and ended_at timestamps based on status transitions is currently implemented in the serializer's update method. This logic is better suited for the model layer using django-lifecycle hooks, which are already being used in this model. This ensures data consistency regardless of how the model is updated (e.g., via the Django shell, admin, or background tasks).
class Experiment(LifecycleModelMixin, SoftDeleteExportableModel): # type: ignore[misc]
environment = models.ForeignKey(
Environment,
on_delete=models.CASCADE,
related_name="experiments",
)
feature = models.ForeignKey(
"features.Feature",
on_delete=models.CASCADE,
related_name="experiments",
)
name = models.CharField(max_length=255)
hypothesis = models.TextField()
status = models.CharField(
max_length=50,
choices=ExperimentStatus.choices,
default=ExperimentStatus.CREATED,
)
created_at = models.DateTimeField(auto_now_add=True)
updated_at = models.DateTimeField(auto_now=True)
started_at = models.DateTimeField(null=True, blank=True)
ended_at = models.DateTimeField(null=True, blank=True)
@hook(
BEFORE_UPDATE,
when="status",
was=ExperimentStatus.CREATED,
is_now=ExperimentStatus.RUNNING,
)
def set_started_at(self) -> None:
if not self.started_at:
self.started_at = timezone.now()
@hook(BEFORE_UPDATE, when="status", is_now=ExperimentStatus.COMPLETED)
def set_ended_at(self) -> None:
self.ended_at = timezone.now()| try: | ||
| environment = Environment.objects.get( | ||
| api_key=view.kwargs.get("environment_api_key") | ||
| ) |
There was a problem hiding this comment.
This permission class performs a redundant database query to fetch the environment. Since ExperimentViewSet inherits from NestedEnvironmentViewSet, the environment is already fetched and cached by the view. You can reuse it by calling view._get_environment().
try:
environment = view._get_environment()
There was a problem hiding this comment.
Bad bot! Don't access private method
| if ( | ||
| Experiment.objects.filter( | ||
| feature=feature, | ||
| environment=environment, | ||
| ) | ||
| .exclude(status=ExperimentStatus.COMPLETED) | ||
| .exists() | ||
| ): | ||
| return Response( | ||
| {"detail": "An active experiment already exists for this feature."}, | ||
| status=400, | ||
| ) |
There was a problem hiding this comment.
This manual check for an existing active experiment is prone to a race condition. While there is a database-level UniqueConstraint, a concurrent request could pass this check and then cause an IntegrityError during perform_create, resulting in a 500 response. It is safer to catch the IntegrityError or use a 409 Conflict status code for consistency with other similar endpoints in this file (like WarehouseConnectionViewSet).
| if ( | |
| Experiment.objects.filter( | |
| feature=feature, | |
| environment=environment, | |
| ) | |
| .exclude(status=ExperimentStatus.COMPLETED) | |
| .exists() | |
| ): | |
| return Response( | |
| {"detail": "An active experiment already exists for this feature."}, | |
| status=400, | |
| ) | |
| if ( | |
| Experiment.objects.filter( | |
| feature=feature, | |
| environment=environment, | |
| ) | |
| .exclude(status=ExperimentStatus.COMPLETED) | |
| .exists() | |
| ): | |
| return Response( | |
| {"detail": "An active experiment already exists for this feature."}, | |
| status=409, | |
| ) |
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-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
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-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
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-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Failed testsfirefox › tests/environment-permission-test.pw.ts › Environment Permission Tests › Environment-level permissions control access to features, identities, and segments @enterprise
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
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-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7591 +/- ##
========================================
Coverage 98.51% 98.51%
========================================
Files 1436 1439 +3
Lines 54363 54646 +283
========================================
+ Hits 53553 53836 +283
Misses 810 810 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nd use hooks to set timestamps
github-actions
Bot
added
feature
github-actions
Bot
added
feature
github-actions
Bot
added
feature
| ) | ||
| view = self.context.get("view") | ||
| if view: | ||
| environment: Environment = view._get_environment() |
There was a problem hiding this comment.
We can pass environment explicitly via get_serializer_context in the view: context["environment"] = self.get_environment_from_request(), then read self.context["environment"] in the serializer.
|
|
||
| def validate_status(self, status: str) -> str: | ||
| if self.instance is None: | ||
| if status != ExperimentStatus.CREATED: |
There was a problem hiding this comment.
We should keep this field private and add actions on the view to act on experiment, e.g: starting, pausing and ending(mostly because we will need to add more logic to those endpoints)
github-actions
Bot
added
feature
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #7590
Adds the foundation
Experimentmodel and CRUD endpoints to theexperimentationapp, enough to wire up the frontend wizard's Step 1 (Setup) and the experiments list page.Model:
Experimentwith FK toEnvironmentandFeature, fields:name,hypothesis,status,created_at,updated_at,started_at,ended_atExperimentStatusenum:created,running,paused,completedUniqueConstrainton(feature, environment)excluding completed — one active experiment per flag per environmentSoftDeleteExportableModelEndpoints (nested under
/environments/{env_key}/experiments/):GET /— list, filterable by?status=POST /— create (multivariate flags only)GET /{id}/— retrievePATCH /{id}/— update name, hypothesis, statusValidations:
started_atauto-set on first transition torunning,ended_atauto-set oncompletedOther:
ExperimentPermissiongated behindexperimental_flagsfeature flag + environment adminRelatedObjectType.EXPERIMENTHow did you test this code?
New tests