diff --git a/ee/clickhouse/views/experiment_saved_metrics.py b/ee/clickhouse/views/experiment_saved_metrics.py index 05595371c5c8..84575993388b 100644 --- a/ee/clickhouse/views/experiment_saved_metrics.py +++ b/ee/clickhouse/views/experiment_saved_metrics.py @@ -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 @@ -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 @@ -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") + ] + 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) diff --git a/ee/clickhouse/views/test/test_experiment_saved_metrics.py b/ee/clickhouse/views/test/test_experiment_saved_metrics.py index 8c2af49e426b..85a6c47d764f 100644 --- a/ee/clickhouse/views/test/test_experiment_saved_metrics.py +++ b/ee/clickhouse/views/test/test_experiment_saved_metrics.py @@ -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 @@ -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) diff --git a/products/experiments/backend/experiment_service.py b/products/experiments/backend/experiment_service.py index 28bc02588521..3e42d4e6b6a6 100644 --- a/products/experiments/backend/experiment_service.py +++ b/products/experiments/backend/experiment_service.py @@ -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, @@ -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, diff --git a/products/experiments/backend/metric_utils.py b/products/experiments/backend/metric_utils.py index 40d944bdd659..5a763989057d 100644 --- a/products/experiments/backend/metric_utils.py +++ b/products/experiments/backend/metric_utils.py @@ -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 diff --git a/products/experiments/frontend/generated/api.schemas.ts b/products/experiments/frontend/generated/api.schemas.ts index 1beb137bc7f3..511daaaea453 100644 --- a/products/experiments/frontend/generated/api.schemas.ts +++ b/products/experiments/frontend/generated/api.schemas.ts @@ -1648,6 +1648,10 @@ export type ExperimentHoldoutsListParams = { } export type ExperimentSavedMetricsListParams = { + /** + * 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. + */ + event?: string /** * Number of results to return per page. */ diff --git a/products/experiments/mcp/tools.yaml b/products/experiments/mcp/tools.yaml index f260b5c1833d..cb6fd1c23924 100644 --- a/products/experiments/mcp/tools.yaml +++ b/products/experiments/mcp/tools.yaml @@ -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= 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 diff --git a/products/experiments/skills/configuring-experiment-analytics/SKILL.md b/products/experiments/skills/configuring-experiment-analytics/SKILL.md index 46a2eadcc732..04a14a48ab6f 100644 --- a/products/experiments/skills/configuring-experiment-analytics/SKILL.md +++ b/products/experiments/skills/configuring-experiment-analytics/SKILL.md @@ -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=` 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`. diff --git a/services/mcp/schema/generated-tool-definitions.json b/services/mcp/schema/generated-tool-definitions.json index f579d1574469..23aea2ae1c92 100644 --- a/services/mcp/schema/generated-tool-definitions.json +++ b/services/mcp/schema/generated-tool-definitions.json @@ -3099,7 +3099,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= 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", diff --git a/services/mcp/schema/tool-definitions-all.json b/services/mcp/schema/tool-definitions-all.json index 793b6e0968b6..b901e57189f3 100644 --- a/services/mcp/schema/tool-definitions-all.json +++ b/services/mcp/schema/tool-definitions-all.json @@ -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= 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", diff --git a/services/mcp/src/api/generated.ts b/services/mcp/src/api/generated.ts index cb42c9afeae5..dc92101838e1 100644 --- a/services/mcp/src/api/generated.ts +++ b/services/mcp/src/api/generated.ts @@ -62068,6 +62068,10 @@ export namespace Schemas { }; export type ExperimentSavedMetricsListParams = { + /** + * 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. + */ + event?: string; /** * Number of results to return per page. */ diff --git a/services/mcp/src/generated/experiments/api.ts b/services/mcp/src/generated/experiments/api.ts index ba4f808bc2e3..3a889138e9e3 100644 --- a/services/mcp/src/generated/experiments/api.ts +++ b/services/mcp/src/generated/experiments/api.ts @@ -582,6 +582,12 @@ export const ExperimentSavedMetricsListParams = /* @__PURE__ */ zod.object({ }) export const ExperimentSavedMetricsListQueryParams = /* @__PURE__ */ zod.object({ + event: zod + .string() + .optional() + .describe( + "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." + ), limit: zod.number().optional().describe('Number of results to return per page.'), offset: zod.number().optional().describe('The initial index from which to return the results.'), search: zod.string().optional().describe('A search term.'), diff --git a/services/mcp/src/tools/generated/experiments.ts b/services/mcp/src/tools/generated/experiments.ts index c83455b8c0d3..f985d929ca7c 100644 --- a/services/mcp/src/tools/generated/experiments.ts +++ b/services/mcp/src/tools/generated/experiments.ts @@ -696,6 +696,9 @@ const experimentSavedMetricsDestroy = (): ToolBase