Skip to content

Commit

Permalink
Merge branch 'master' into olly_better_cdp_fetch_handling
Browse files Browse the repository at this point in the history
  • Loading branch information
benjackwhite committed Oct 29, 2024
2 parents d5e6ebe + ded1262 commit 6ba05d0
Show file tree
Hide file tree
Showing 649 changed files with 18,065 additions and 6,230 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ jobs:

- name: Run clippy
if: needs.changes.outputs.rust == 'true'
run: cargo clippy -- -D warnings
run: cargo clippy --all-targets --all-features -- -D warnings

- name: Run cargo check
if: needs.changes.outputs.rust == 'true'
Expand Down
13 changes: 12 additions & 1 deletion cypress/e2e/notebooks-insights.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,18 @@ describe('Notebooks', () => {
cy.clickNavMenu('notebooks')
cy.location('pathname').should('include', '/notebooks')
})
;['SQL', 'TRENDS', 'FUNNELS', 'RETENTION', 'PATHS', 'STICKINESS', 'LIFECYCLE'].forEach((insightType) => {

it(`Can add a HogQL insight`, () => {
savedInsights.createNewInsightOfType('SQL')
insight.editName('SQL Insight')
insight.save()
cy.get('[data-attr="notebooks-add-button"]').click()
cy.get('[data-attr="notebooks-select-button-create"]').click()
cy.get('.ErrorBoundary').should('not.exist')
// Detect if table settings are present. They shouldn't appear in the block, but rather on side.
cy.get('[data-attr="notebook-node-query"]').get('[data-attr="export-button"]').should('not.exist')
})
;['TRENDS', 'FUNNELS', 'RETENTION', 'PATHS', 'STICKINESS', 'LIFECYCLE'].forEach((insightType) => {
it(`Can add a ${insightType} insight`, () => {
savedInsights.createNewInsightOfType(insightType)
insight.editName(`${insightType} Insight`)
Expand Down
14 changes: 7 additions & 7 deletions docker/clickhouse/user_defined_function.xml
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@
<function>
<type>executable_pool</type>
<name>aggregate_funnel_trends</name>
<return_type>Array(Tuple(UInt64, Int8, Nullable(String)))</return_type>
<return_type>Array(Tuple(UInt64, Int8, Nullable(String), UUID))</return_type>
<return_name>result</return_name>
<argument>
<type>UInt8</type>
Expand Down Expand Up @@ -169,7 +169,7 @@
<name>prop_vals</name>
</argument>
<argument>
<type>Array(Tuple(Nullable(Float64), UInt64, Nullable(String), Array(Int8)))</type>
<type>Array(Tuple(Nullable(Float64), UInt64, UUID, Nullable(String), Array(Int8)))</type>
<name>value</name>
</argument>
<format>JSONEachRow</format>
Expand All @@ -181,7 +181,7 @@
<type>executable_pool</type>
<name>aggregate_funnel_array_trends</name>
<!-- Return type for trends is a start interval time, a success flag (1 or -1), and a breakdown value -->
<return_type>Array(Tuple(UInt64, Int8, Array(String)))</return_type>
<return_type>Array(Tuple(UInt64, Int8, Array(String), UUID))</return_type>
<return_name>result</return_name>
<argument>
<type>UInt8</type>
Expand All @@ -208,7 +208,7 @@
<name>prop_vals</name>
</argument>
<argument>
<type>Array(Tuple(Nullable(Float64), UInt64, Array(String), Array(Int8)))</type>
<type>Array(Tuple(Nullable(Float64), UInt64, UUID, Array(String), Array(Int8)))</type>
<name>value</name>
</argument>
<format>JSONEachRow</format>
Expand All @@ -220,7 +220,7 @@
<type>executable_pool</type>
<name>aggregate_funnel_cohort_trends</name>
<!-- Return type for trends is a start interval time, a success flag (1 or -1), and a breakdown value -->
<return_type>Array(Tuple(UInt64, Int8, UInt64))</return_type>
<return_type>Array(Tuple(UInt64, Int8, UInt64, UUID))</return_type>
<return_name>result</return_name>
<argument>
<type>UInt8</type>
Expand All @@ -247,7 +247,7 @@
<name>prop_vals</name>
</argument>
<argument>
<type>Array(Tuple(Nullable(Float64), UInt64, UInt64, Array(Int8)))</type>
<type>Array(Tuple(Nullable(Float64), UInt64, UUID, UInt64, Array(Int8)))</type>
<name>value</name>
</argument>
<format>JSONEachRow</format>
Expand Down Expand Up @@ -285,7 +285,7 @@
<name>prop_vals</name>
</argument>
<argument>
<type>Array(Tuple(Nullable(Float64), UInt64, Array(String), Array(Int8)))</type>
<type>Array(Tuple(Nullable(Float64), UInt64, UUID, Array(String), Array(Int8)))</type>
<name>value</name>
</argument>
<format>JSONEachRow</format>
Expand Down
4 changes: 4 additions & 0 deletions ee/clickhouse/queries/experiments/funnel_experiment_result.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
calculate_credible_intervals,
calculate_probabilities,
)
from posthog.models.experiment import ExperimentHoldout
from posthog.models.feature_flag import FeatureFlag
from posthog.models.filters.filter import Filter
from posthog.models.team import Team
Expand Down Expand Up @@ -54,10 +55,13 @@ def __init__(
feature_flag: FeatureFlag,
experiment_start_date: datetime,
experiment_end_date: Optional[datetime] = None,
holdout: Optional[ExperimentHoldout] = None,
funnel_class: type[ClickhouseFunnel] = ClickhouseFunnel,
):
breakdown_key = f"$feature/{feature_flag.key}"
self.variants = [variant["key"] for variant in feature_flag.variants]
if holdout:
self.variants.append(f"holdout-{holdout.id}")

# our filters assume that the given time ranges are in the project timezone.
# while start and end date are in UTC.
Expand Down
4 changes: 4 additions & 0 deletions ee/clickhouse/queries/experiments/trend_experiment_result.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
calculate_credible_intervals,
calculate_probabilities,
)
from posthog.models.experiment import ExperimentHoldout
from posthog.models.feature_flag import FeatureFlag
from posthog.models.filters.filter import Filter
from posthog.models.team import Team
Expand Down Expand Up @@ -81,9 +82,12 @@ def __init__(
experiment_end_date: Optional[datetime] = None,
trend_class: type[Trends] = Trends,
custom_exposure_filter: Optional[Filter] = None,
holdout: Optional[ExperimentHoldout] = None,
):
breakdown_key = f"$feature/{feature_flag.key}"
self.variants = [variant["key"] for variant in feature_flag.variants]
if holdout:
self.variants.append(f"holdout-{holdout.id}")

# our filters assume that the given time ranges are in the project timezone.
# while start and end date are in UTC.
Expand Down
110 changes: 110 additions & 0 deletions ee/clickhouse/views/experiment_holdouts.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
from typing import Any
from rest_framework import serializers, viewsets
from rest_framework.exceptions import ValidationError
from rest_framework.request import Request
from rest_framework.response import Response
from django.db import transaction


from posthog.api.feature_flag import FeatureFlagSerializer
from posthog.api.routing import TeamAndOrgViewSetMixin
from posthog.api.shared import UserBasicSerializer
from posthog.models.experiment import ExperimentHoldout


class ExperimentHoldoutSerializer(serializers.ModelSerializer):
created_by = UserBasicSerializer(read_only=True)

class Meta:
model = ExperimentHoldout
fields = [
"id",
"name",
"description",
"filters",
"created_by",
"created_at",
"updated_at",
]
read_only_fields = [
"id",
"created_by",
"created_at",
"updated_at",
]

def _get_filters_with_holdout_id(self, id: int, filters: list) -> list:
variant_key = f"holdout-{id}"
updated_filters = []
for filter in filters:
updated_filters.append(
{
**filter,
"variant": variant_key,
}
)
return updated_filters

def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> ExperimentHoldout:
request = self.context["request"]
validated_data["created_by"] = request.user
validated_data["team_id"] = self.context["team_id"]

if not validated_data.get("filters"):
raise ValidationError("Filters are required to create an holdout group")

instance = super().create(validated_data)
instance.filters = self._get_filters_with_holdout_id(instance.id, instance.filters)
instance.save()
return instance

def update(self, instance: ExperimentHoldout, validated_data):
filters = validated_data.get("filters")
if filters and instance.filters != filters:
# update flags on all experiments in this holdout group
new_filters = self._get_filters_with_holdout_id(instance.id, filters)
validated_data["filters"] = new_filters
with transaction.atomic():
for experiment in instance.experiment_set.all():
flag = experiment.feature_flag
existing_flag_serializer = FeatureFlagSerializer(
flag,
data={
"filters": {**flag.filters, "holdout_groups": validated_data["filters"]},
},
partial=True,
context=self.context,
)
existing_flag_serializer.is_valid(raise_exception=True)
existing_flag_serializer.save()

return super().update(instance, validated_data)


class ExperimentHoldoutViewSet(TeamAndOrgViewSetMixin, viewsets.ModelViewSet):
scope_object = "experiment"
queryset = ExperimentHoldout.objects.prefetch_related("created_by").all()
serializer_class = ExperimentHoldoutSerializer
ordering = "-created_at"

def destroy(self, request: Request, *args: Any, **kwargs: Any) -> Response:
instance = self.get_object()

with transaction.atomic():
for experiment in instance.experiment_set.all():
flag = experiment.feature_flag
existing_flag_serializer = FeatureFlagSerializer(
flag,
data={
"filters": {
**flag.filters,
"holdout_groups": None,
}
},
partial=True,
context={"request": request, "team": self.team, "team_id": self.team_id},
)
existing_flag_serializer.is_valid(raise_exception=True)
existing_flag_serializer.save()

return super().destroy(request, *args, **kwargs)
42 changes: 38 additions & 4 deletions ee/clickhouse/views/experiments.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
ClickhouseTrendExperimentResult,
)
from ee.clickhouse.queries.experiments.utils import requires_flag_warning
from ee.clickhouse.views.experiment_holdouts import ExperimentHoldoutSerializer
from posthog.api.cohort import CohortSerializer
from posthog.api.feature_flag import FeatureFlagSerializer, MinimalFeatureFlagSerializer
from posthog.api.routing import TeamAndOrgViewSetMixin
Expand All @@ -27,7 +28,7 @@
from posthog.caching.insight_cache import update_cached_state
from posthog.clickhouse.query_tagging import tag_queries
from posthog.constants import INSIGHT_TRENDS
from posthog.models.experiment import Experiment
from posthog.models.experiment import Experiment, ExperimentHoldout
from posthog.models.filters.filter import Filter
from posthog.utils import generate_cache_key, get_safe_cache

Expand All @@ -50,6 +51,7 @@ def _calculate_experiment_results(experiment: Experiment, refresh: bool = False)
experiment.feature_flag,
experiment.start_date,
experiment.end_date,
holdout=experiment.holdout,
custom_exposure_filter=exposure_filter,
).get_results()
else:
Expand All @@ -59,6 +61,7 @@ def _calculate_experiment_results(experiment: Experiment, refresh: bool = False)
experiment.feature_flag,
experiment.start_date,
experiment.end_date,
holdout=experiment.holdout,
).get_results()

return _experiment_results_cached(
Expand Down Expand Up @@ -156,6 +159,10 @@ class ExperimentSerializer(serializers.ModelSerializer):
feature_flag_key = serializers.CharField(source="get_feature_flag_key")
created_by = UserBasicSerializer(read_only=True)
feature_flag = MinimalFeatureFlagSerializer(read_only=True)
holdout = ExperimentHoldoutSerializer(read_only=True)
holdout_id = serializers.PrimaryKeyRelatedField(
queryset=ExperimentHoldout.objects.all(), source="holdout", required=False, allow_null=True
)

class Meta:
model = Experiment
Expand All @@ -167,6 +174,8 @@ class Meta:
"end_date",
"feature_flag_key",
"feature_flag",
"holdout",
"holdout_id",
"exposure_cohort",
"parameters",
"secondary_metrics",
Expand All @@ -183,6 +192,7 @@ class Meta:
"updated_at",
"feature_flag",
"exposure_cohort",
"holdout",
]

def validate_parameters(self, value):
Expand Down Expand Up @@ -221,6 +231,10 @@ def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Experiment:
if properties:
raise ValidationError("Experiments do not support global filter properties")

holdout_groups = None
if validated_data.get("holdout"):
holdout_groups = validated_data["holdout"].filters

default_variants = [
{"key": "control", "name": "Control Group", "rollout_percentage": 50},
{"key": "test", "name": "Test Variant", "rollout_percentage": 50},
Expand All @@ -230,6 +244,7 @@ def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Experiment:
"groups": [{"properties": properties, "rollout_percentage": 100}],
"multivariate": {"variants": variants or default_variants},
"aggregation_group_type_index": aggregation_group_type_index,
"holdout_groups": holdout_groups,
}

feature_flag_serializer = FeatureFlagSerializer(
Expand Down Expand Up @@ -263,6 +278,7 @@ def update(self, instance: Experiment, validated_data: dict, *args: Any, **kwarg
"parameters",
"archived",
"secondary_metrics",
"holdout",
}
given_keys = set(validated_data.keys())
extra_keys = given_keys - expected_keys
Expand All @@ -273,7 +289,7 @@ def update(self, instance: Experiment, validated_data: dict, *args: Any, **kwarg
if extra_keys:
raise ValidationError(f"Can't update keys: {', '.join(sorted(extra_keys))} on Experiment")

# if an experiment has launched, we cannot edit its variants anymore.
# if an experiment has launched, we cannot edit its variants or holdout anymore.
if not instance.is_draft:
if "feature_flag_variants" in validated_data.get("parameters", {}):
if len(validated_data["parameters"]["feature_flag_variants"]) != len(feature_flag.variants):
Expand All @@ -285,13 +301,19 @@ def update(self, instance: Experiment, validated_data: dict, *args: Any, **kwarg
!= 1
):
raise ValidationError("Can't update feature_flag_variants on Experiment")
if "holdout" in validated_data and validated_data["holdout"] != instance.holdout:
raise ValidationError("Can't update holdout on running Experiment")

properties = validated_data.get("filters", {}).get("properties")
if properties:
raise ValidationError("Experiments do not support global filter properties")

if instance.is_draft:
# if feature flag variants have changed, update the feature flag.
# if feature flag variants or holdout have changed, update the feature flag.
holdout_groups = instance.holdout.filters if instance.holdout else None
if "holdout" in validated_data:
holdout_groups = validated_data["holdout"].filters if validated_data["holdout"] else None

if validated_data.get("parameters"):
variants = validated_data["parameters"].get("feature_flag_variants", [])
aggregation_group_type_index = validated_data["parameters"].get("aggregation_group_type_index")
Expand All @@ -312,6 +334,7 @@ def update(self, instance: Experiment, validated_data: dict, *args: Any, **kwarg
"groups": [{"properties": properties, "rollout_percentage": 100}],
"multivariate": {"variants": variants or default_variants},
"aggregation_group_type_index": aggregation_group_type_index,
"holdout_groups": holdout_groups,
}

existing_flag_serializer = FeatureFlagSerializer(
Expand All @@ -322,6 +345,17 @@ def update(self, instance: Experiment, validated_data: dict, *args: Any, **kwarg
)
existing_flag_serializer.is_valid(raise_exception=True)
existing_flag_serializer.save()
else:
# no parameters provided, just update the holdout if necessary
if "holdout" in validated_data:
existing_flag_serializer = FeatureFlagSerializer(
feature_flag,
data={"filters": {**feature_flag.filters, "holdout_groups": holdout_groups}},
partial=True,
context=self.context,
)
existing_flag_serializer.is_valid(raise_exception=True)
existing_flag_serializer.save()

if instance.is_draft and has_start_date:
feature_flag.active = True
Expand All @@ -336,7 +370,7 @@ def update(self, instance: Experiment, validated_data: dict, *args: Any, **kwarg
class EnterpriseExperimentsViewSet(TeamAndOrgViewSetMixin, viewsets.ModelViewSet):
scope_object = "experiment"
serializer_class = ExperimentSerializer
queryset = Experiment.objects.prefetch_related("feature_flag", "created_by").all()
queryset = Experiment.objects.prefetch_related("feature_flag", "created_by", "holdout").all()
ordering = "-created_at"

# ******************************************
Expand Down
Loading

0 comments on commit 6ba05d0

Please sign in to comment.