Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions ee/clickhouse/views/experiment_saved_metrics.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from django.db.models import QuerySet
from django.db.models.functions import Lower

from drf_spectacular.utils import extend_schema
from drf_spectacular.utils import OpenApiParameter, extend_schema, extend_schema_view
from rest_framework import filters, serializers, viewsets

from posthog.api.routing import TeamAndOrgViewSetMixin
Expand All @@ -9,7 +10,7 @@
from posthog.rbac.user_access_control import UserAccessControlSerializerMixin

from products.experiments.backend.experiment_saved_metric_service import ExperimentSavedMetricService
from products.experiments.backend.metric_utils import refresh_action_names_in_metric
from products.experiments.backend.metric_utils import filter_metric_group_ids_by_event, refresh_action_names_in_metric
from products.experiments.backend.models.experiment import ExperimentSavedMetric, ExperimentToSavedMetric

from ee.api.rbac.access_control import AccessControlViewSetMixin
Expand Down Expand Up @@ -137,14 +138,50 @@ def _build_service(self) -> ExperimentSavedMetricService:
return ExperimentSavedMetricService(team=self.context["get_team"](), user=request.user)


@extend_schema_view(
list=extend_schema(
parameters=[
OpenApiParameter(
name="event",
type=str,
location=OpenApiParameter.QUERY,
required=False,
description="Filter to shared metrics whose query references this event name. Matches events "
"used directly in metric queries as well as events behind any actions those metrics reference. "
"Use this for reuse discovery (find a metric by what it measures); distinct from 'search', "
"which matches the metric's own name/description/tags.",
),
],
),
)
@extend_schema(extensions={"x-swagger-tag": "experiment_saved_metrics", "x-product": "experiments"})
class ExperimentSavedMetricViewSet(TeamAndOrgViewSetMixin, AccessControlViewSetMixin, viewsets.ModelViewSet):
scope_object = "experiment_saved_metric"
queryset = ExperimentSavedMetric.objects.prefetch_related("created_by").order_by(Lower("name")).distinct()
serializer_class = ExperimentSavedMetricSerializer
filter_backends = [filters.SearchFilter]
# `search` matches the metric's own name/description/tags, while `event` looks in metrics
search_fields = ["name", "description", "tagged_items__tag__name"]

def safely_get_queryset(self, queryset: QuerySet) -> QuerySet:
# `?event=` matches metrics whose query references the event directly (EventsNode) or via an action's
# step events (ActionsNode resolved live) — mirrors the experiments-list filter. Scoped to `list` so it
# never narrows a detail/update/destroy lookup. Rows are read team-scoped (safely_get_queryset runs
# before the mixin's team filter), then matched in Python since event references live deep in the JSON.
if self.action != "list":
return queryset
event = self.request.query_params.get("event")
if event:
# One group per saved metric: (pk, [its query]). `.order_by()` drops the class-level
# ordering for this internal fetch — the result is matched in Python, so ordering is
# irrelevant, and it avoids a needless sort (and the DISTINCT+ORDER BY column injection).
groups = [
(pk, [query] if query else [])
for pk, query in queryset.filter(team_id=self.team.pk).order_by().values_list("pk", "query")
]
Comment thread
mp-hog marked this conversation as resolved.
queryset = queryset.filter(pk__in=filter_metric_group_ids_by_event(groups, event, self.team))
return queryset

def perform_destroy(self, instance: ExperimentSavedMetric) -> None:
service = ExperimentSavedMetricService(team=self.team, user=self.request.user)
service.delete_saved_metric(instance)
73 changes: 72 additions & 1 deletion ee/clickhouse/views/test/test_experiment_saved_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@
from parameterized import parameterized
from rest_framework import status

from products.actions.backend.models.action import Action
from products.experiments.backend.experiment_saved_metric_service import ExperimentSavedMetricService
from products.experiments.backend.models.experiment import Experiment, ExperimentToSavedMetric
from products.experiments.backend.models.experiment import Experiment, ExperimentSavedMetric, ExperimentToSavedMetric

from ee.api.test.base import APILicensedTest

Expand Down Expand Up @@ -981,3 +982,73 @@ def test_list_paginates_with_limit_and_offset(self) -> None:
ids_page1 = {row["id"] for row in page1["results"]}
ids_page2 = {row["id"] for row in page2["results"]}
self.assertEqual(ids_page1 & ids_page2, set())

def _seed_metrics_for_event_filter(self) -> None:
# Created via the ORM so the stored query JSON is exactly what the filter inspects. The action fires
# on the "signup" event, so its metric is discoverable by that event — not by the action's label.
signup_action = Action.objects.create(team=self.team, name="Completed signup", steps_json=[{"event": "signup"}])
for name, source in [
("Pageview mean", {"kind": "EventsNode", "event": "$pageview"}),
("Purchase mean", {"kind": "EventsNode", "event": "purchase"}),
("Signup via action", {"kind": "ActionsNode", "id": signup_action.id, "name": "Completed signup"}),
]:
ExperimentSavedMetric.objects.create(
team=self.team,
created_by=self.user,
name=name,
query={"kind": "ExperimentMetric", "metric_type": "mean", "source": source},
)

@parameterized.expand(
[
# `event` matches the events a metric references — directly, or via an action's step events.
("direct_event", "$pageview", {"Pageview mean"}),
("another_direct_event", "purchase", {"Purchase mean"}),
("event_behind_an_action", "signup", {"Signup via action"}),
# It matches events, not the action's label, nor query structure/type tokens.
("action_label_not_matched", "Completed signup", set()),
("metric_type_token_not_matched", "mean", set()),
("node_kind_token_not_matched", "EventsNode", set()),
("no_match", "not_an_event", set()),
]
)
def test_event_filter_matches_referenced_events_only(
self, _name: str, event: str, expected_names: set[str]
) -> None:
self._seed_metrics_for_event_filter()

response = self.client.get(f"/api/projects/{self.team.id}/experiment_saved_metrics/", data={"event": event})

self.assertEqual(response.status_code, status.HTTP_200_OK)
returned = {row["name"] for row in response.json()["results"]}
self.assertEqual(returned, expected_names)

def test_event_filter_composes_with_search(self) -> None:
# `event` (references) and `search` (name/description/tags) apply through different mechanisms;
# both must narrow the result set together (AND), not clobber each other.
purchase_query = {
"kind": "ExperimentMetric",
"metric_type": "mean",
"source": {"kind": "EventsNode", "event": "purchase"},
}
for name in ["Alpha purchase", "Beta purchase"]:
ExperimentSavedMetric.objects.create(team=self.team, created_by=self.user, name=name, query=purchase_query)

response = self.client.get(
f"/api/projects/{self.team.id}/experiment_saved_metrics/?event=purchase&search=Alpha"
)

self.assertEqual(response.status_code, status.HTTP_200_OK)
returned = {row["name"] for row in response.json()["results"]}
self.assertEqual(returned, {"Alpha purchase"})

def test_event_param_does_not_filter_detail_retrieve(self) -> None:
# `event` is a list-only filter; it must never narrow a detail lookup into a 404.
metric_id = self._create_saved_metric("Pageview mean")

response = self.client.get(
f"/api/projects/{self.team.id}/experiment_saved_metrics/{metric_id}?event=not_an_event"
)

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(response.json()["id"], metric_id)
32 changes: 9 additions & 23 deletions products/experiments/backend/experiment_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
from products.experiments.backend.hogql_queries.base_query_utils import is_threshold_supported_math
from products.experiments.backend.hogql_queries.experiment_metric_fingerprint import compute_metric_fingerprint
from products.experiments.backend.hogql_queries.funnel_validation import FunnelDWValidator
from products.experiments.backend.metric_utils import collect_metric_events_and_action_ids, resolve_action_events
from products.experiments.backend.metric_utils import filter_metric_group_ids_by_event
from products.experiments.backend.models.experiment import (
LEGACY_METRIC_KINDS,
Experiment,
Expand Down Expand Up @@ -2658,28 +2658,14 @@ def _experiments_matching_event(self, queryset: QuerySet[Experiment], event: str
if query:
saved_queries_by_experiment[experiment_id].append(query)

per_experiment: list[tuple[int, set[str], set[int]]] = []
all_action_ids: set[int] = set()
for pk, metrics, metrics_secondary in inline_metrics:
combined: list[dict[str, Any]] = [
*(metrics or []),
*(metrics_secondary or []),
*saved_queries_by_experiment.get(pk, []),
]
events, action_ids = collect_metric_events_and_action_ids(combined)
per_experiment.append((pk, events, action_ids))
all_action_ids |= action_ids

action_events = resolve_action_events(all_action_ids, self.team)

matching_ids: list[int] = []
for pk, events, action_ids in per_experiment:
resolved = set(events)
for action_id in action_ids:
resolved |= action_events.get(action_id, set())
if event in resolved:
matching_ids.append(pk)
return matching_ids
metric_groups: list[tuple[int, list[dict[str, Any]]]] = [
(
pk,
[*(metrics or []), *(metrics_secondary or []), *saved_queries_by_experiment.get(pk, [])],
)
for pk, metrics, metrics_secondary in inline_metrics
]
return filter_metric_group_ids_by_event(metric_groups, event, self.team)

def filter_experiments_queryset(
self,
Expand Down
29 changes: 29 additions & 0 deletions products/experiments/backend/metric_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,3 +202,32 @@ def resolve_action_events(action_ids: set[int], team: Team) -> dict[int, set[str
action.id: {event for event in action.get_step_events() if event}
for action in Action.objects.filter(id__in=action_ids, team=team, deleted=False)
}


def filter_metric_group_ids_by_event(
metric_groups: list[tuple[int, list[dict[str, Any]]]], event: str, team: Team
) -> list[int]:
"""Return the ids of metric groups whose queries reference ``event``, directly or via an action.

Each group is ``(id, [metric_query, ...])`` — a single query for one saved metric, or an experiment's
inline + secondary + attached saved-metric queries. Every referenced action is resolved to its step
events in one batched query across all groups, so an action-based metric is matched by the events its
action fires on (not by the action's stored — possibly stale — name).
"""
per_group: list[tuple[int, set[str], set[int]]] = []
all_action_ids: set[int] = set()
for group_id, metrics in metric_groups:
events, action_ids = collect_metric_events_and_action_ids(metrics)
per_group.append((group_id, events, action_ids))
all_action_ids |= action_ids

action_events = resolve_action_events(all_action_ids, team)

matching_ids: list[int] = []
for group_id, events, action_ids in per_group:
resolved = set(events)
for action_id in action_ids:
resolved |= action_events.get(action_id, set())
if event in resolved:
matching_ids.append(group_id)
return matching_ids
4 changes: 4 additions & 0 deletions products/experiments/frontend/generated/api.schemas.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 12 additions & 2 deletions products/experiments/mcp/tools.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -712,13 +712,23 @@ tools:
title: List shared experiment metrics
description: >
List shared metrics in the current project. Returns paginated results with id, name, description, query,
created_at, updated_at, tags. Use experiment-saved-metrics-retrieve for a single metric by ID, or this
listing to resolve by name.
created_at, updated_at, tags.


To REUSE a metric by what it measures (rather than by its name): pass ?event=<the event you're measuring> to
find metrics that reference it — directly or via an action — then compare each candidate's 'query'
(metric_type + math) against the metric you'd otherwise build. Use 'search' (name/description/tags) only
when the user named a specific metric to attach.
param_overrides:
limit:
cast: string-int
offset:
cast: string-int
event:
description: >
Filter to shared metrics whose query references this event name — matched directly (an EventsNode)
or via the step events of any action the metric references. For finding a reusable metric by what it
measures; then confirm the match against each row's 'query'.
response:
include:
- id
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,18 @@ compare on what each metric measures (its `query`), never on its title.
(mean / funnel / ratio / retention) before searching — see Step 2 to confirm the event exists via
`read-data-schema`. You can only recognize a duplicate once you know the concrete event/action,
so this check runs _after_ you've pinned down the event, not before.
2. **List the library and compare each candidate's `query`.** Call `experiment-saved-metrics-list`
and inspect every result's **`query`** field (not just `name`/`description`). A saved metric is a
reuse match when its `query` measures the **same event or action with the same `metric_type`**
(and compatible `math`) as the metric you'd otherwise build — even if its name is
different.
- **Match locally, not via `search`.** `search` matches only `name` / `description` / tags —
never the underlying event or action — so it cannot find a definition match, and an empty
result means nothing here. Page through the full library with `limit`/`offset` and compare each
row's `query` yourself. (Use `search` only when the user names a specific saved metric to
attach — that's name resolution, not a definition match.)
2. **Search by the event, then compare each candidate's `query`.** Call `experiment-saved-metrics-list`
with `?event=<the event you're measuring>` to find metrics that reference it — matched directly (an
`EventsNode`) **or** via the step events of any action a metric references, so action-based metrics are
found by the event their action fires on. Then for each returned row, inspect its **`query`** (not the
`name`/`description`): a saved metric is a reuse match when its `query` measures the **same event or
action with the same `metric_type`** (and compatible `math`) as the metric you'd otherwise build, even
if its name is different.
- **Match on the event, not the action's name.** An action-based metric is discoverable by the event
the action fires on — pass that event, not the action's label.
- **Do not use `search` for this.** `search` matches only the metric's own `name` / `description` / tags —
never the underlying event or action — so it cannot find a definition match. Use `search` only when the
user names a specific saved metric to attach (name resolution, not a definition match).
3. **If a saved metric matches the definition** — confirm the match with the user by name/description,
then attach it instead of building a new one:
- Call `experiment-get` to read the experiment's current `saved_metrics`.
Expand Down
2 changes: 1 addition & 1 deletion services/mcp/schema/generated-tool-definitions.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion services/mcp/schema/tool-definitions-all.json
Original file line number Diff line number Diff line change
Expand Up @@ -3297,7 +3297,7 @@
}
},
"experiment-saved-metrics-list": {
"description": "List shared metrics in the current project. Returns paginated results with id, name, description, query, created_at, updated_at, tags. Use experiment-saved-metrics-retrieve for a single metric by ID, or this listing to resolve by name.",
"description": "List shared metrics in the current project. Returns paginated results with id, name, description, query, created_at, updated_at, tags.\n\nTo REUSE a metric by what it measures (rather than by its name): pass ?event=<the event you're measuring> to find metrics that reference it — directly or via an action — then compare each candidate's 'query' (metric_type + math) against the metric you'd otherwise build. Use 'search' (name/description/tags) only when the user named a specific metric to attach.",
"category": "Experiments",
"feature": "experiments",
"summary": "List shared experiment metrics",
Expand Down
4 changes: 4 additions & 0 deletions services/mcp/src/api/generated.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions services/mcp/src/generated/experiments/api.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading