fix(experiments): match frontend breakdownFilter wrapper for saved metric cache#60412
Merged
Merged
Conversation
…metric cache The frontend's sharedMetricsToExperimentMetrics merges link.metadata.breakdowns into a breakdownFilter on every saved metric before posting to /query, even when breakdowns are empty. The Temporal activity was using the raw saved_metric.query, producing a different cache key. Result: the warmed entry was never read.
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
posthog/temporal/experiments/test_cache_warming.py:105-195
**Test only covers empty breakdowns path**
The test wires up `metadata={"type": "primary"}` (no `breakdowns` key), so `saved_metric_metadata.get("breakdowns") or []` always produces `[]`. A saved metric whose link carries real breakdowns (e.g. `metadata={"type": "primary", "breakdowns": [{"property": "plan", "type": "person"}]}`) takes a slightly different code path — the cache key now includes those breakdown values and the activity must produce the same serialisation as the frontend. That path has no coverage, so a regression there would be silent.
Reviews (1): Last reviewed commit: "fix(experiments): match frontend's break..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+105
to
+195
| @freeze_time("2020-01-10T12:00:00Z") | ||
| def test_temporal_activity_warms_query_cache_for_saved_metric(self): | ||
| """ | ||
| Saved metrics take a different path: the frontend merges | ||
| ExperimentToSavedMetric.metadata.breakdowns into a breakdownFilter on | ||
| the metric before posting to /query. The activity must apply the same | ||
| merge or the cache key diverges. This test covers that path. | ||
| """ | ||
| feature_flag = self.create_feature_flag() | ||
| experiment = self.create_experiment( | ||
| feature_flag=feature_flag, | ||
| start_date=datetime(2020, 1, 1, 0, 0, 0), | ||
| ) | ||
|
|
||
| metric = ExperimentMeanMetric(uuid=str(uuid4()), source=EventsNode(event="purchase")) | ||
| metric_dict = metric.model_dump(mode="json") | ||
|
|
||
| saved_metric = ExperimentSavedMetric.objects.create( | ||
| name="test saved metric", | ||
| team=self.team, | ||
| query=metric_dict, | ||
| created_by=self.user, | ||
| ) | ||
| # No breakdowns on the link metadata — mirrors the production scenario | ||
| # where the frontend still wraps the metric with breakdownFilter: {breakdowns: []}. | ||
| ExperimentToSavedMetric.objects.create( | ||
| experiment=experiment, | ||
| saved_metric=saved_metric, | ||
| metadata={"type": "primary"}, | ||
| ) | ||
|
|
||
| feature_flag_property = f"$feature/{feature_flag.key}" | ||
| for variant in ("control", "test"): | ||
| for i in range(5): | ||
| distinct_id = f"user_{variant}_{i}" | ||
| _create_person(distinct_ids=[distinct_id], team_id=self.team.pk) | ||
| _create_event( | ||
| team=self.team, | ||
| event="$feature_flag_called", | ||
| distinct_id=distinct_id, | ||
| timestamp="2020-01-02T12:00:00Z", | ||
| properties={ | ||
| feature_flag_property: variant, | ||
| "$feature_flag_response": variant, | ||
| "$feature_flag": feature_flag.key, | ||
| }, | ||
| ) | ||
| _create_event( | ||
| team=self.team, | ||
| event="purchase", | ||
| distinct_id=distinct_id, | ||
| timestamp="2020-01-02T12:01:00Z", | ||
| properties={feature_flag_property: variant}, | ||
| ) | ||
| flush_persons_and_events() | ||
|
|
||
| # Build the metric the way the frontend does for saved metrics: | ||
| # wrap with breakdownFilter merged from link.metadata.breakdowns | ||
| # (see sharedMetricsToExperimentMetrics in experimentLogic.tsx). | ||
| frontend_metric_dict = { | ||
| **metric_dict, | ||
| "breakdownFilter": {"breakdowns": []}, | ||
| } | ||
| frontend_query = ExperimentQuery.model_validate( | ||
| { | ||
| "kind": "ExperimentQuery", | ||
| "experiment_id": experiment.id, | ||
| "metric": frontend_metric_dict, | ||
| } | ||
| ) | ||
|
|
||
| cache.clear() | ||
|
|
||
| fingerprint = compute_metric_fingerprint( | ||
| metric_dict, | ||
| experiment.start_date, | ||
| get_experiment_stats_method(experiment), | ||
| experiment.exposure_criteria, | ||
| only_count_matured_users=experiment.only_count_matured_users, | ||
| ) | ||
| with patch("posthog.temporal.experiments.activities.close_old_connections"): | ||
| activity_result = _calculate_experiment_saved_metric_sync.func( # type: ignore[attr-defined] | ||
| experiment.id, metric_dict["uuid"], fingerprint | ||
| ) | ||
| self.assertTrue(activity_result.success, msg=activity_result.error_message) | ||
|
|
||
| warm_response = ExperimentQueryRunner(query=frontend_query, team=self.team).run( | ||
| execution_mode=ExecutionMode.RECENT_CACHE_CALCULATE_BLOCKING_IF_STALE, | ||
| ) | ||
| assert isinstance(warm_response, CachedExperimentQueryResponse) | ||
| self.assertTrue(warm_response.is_cached) |
Contributor
There was a problem hiding this comment.
The test wires up metadata={"type": "primary"} (no breakdowns key), so saved_metric_metadata.get("breakdowns") or [] always produces []. A saved metric whose link carries real breakdowns (e.g. metadata={"type": "primary", "breakdowns": [{"property": "plan", "type": "person"}]}) takes a slightly different code path — the cache key now includes those breakdown values and the activity must produce the same serialisation as the frontend. That path has no coverage, so a regression there would be silent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/temporal/experiments/test_cache_warming.py
Line: 105-195
Comment:
**Test only covers empty breakdowns path**
The test wires up `metadata={"type": "primary"}` (no `breakdowns` key), so `saved_metric_metadata.get("breakdowns") or []` always produces `[]`. A saved metric whose link carries real breakdowns (e.g. `metadata={"type": "primary", "breakdowns": [{"property": "plan", "type": "person"}]}`) takes a slightly different code path — the cache key now includes those breakdown values and the activity must produce the same serialisation as the frontend. That path has no coverage, so a regression there would be silent.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
mp-hog
approved these changes
May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
#60200 warmed the response cache from the daily Temporal recalc. For experiments using saved metrics, the warmed entries were never read: the frontend wraps every saved metric with a
breakdownFilterfield before posting to/query, and the activity wasn't doing the same wrap. Different wrap → different cache key.Changes
In
_calculate_experiment_saved_metric_sync, apply the samebreakdownFilterwrap the frontend applies. Cache keys now match.How did you test this code?