-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(experiments): Avoid displaying results on draft experiments #27374
base: master
Are you sure you want to change the base?
Conversation
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 👍 This has confused me a couple of times.
@@ -763,8 +763,13 @@ export const experimentLogic = kea<experimentLogicType>([ | |||
}, | |||
updateExperimentSuccess: async ({ experiment }) => { | |||
actions.updateExperiments(experiment) | |||
actions.loadMetricResults() | |||
actions.loadSecondaryMetricResults() | |||
if (experiment.start_date) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think do the same at setExperimentStatsVersion
? Not sure if that can only happen after the experiment starts running though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think do the same at
setExperimentStatsVersion
? Not sure if that can only happen after the experiment starts running though
Yes, correct, and no, it can be used whenever. Good catch. setExperimentStatsVersion
is only exposed for a few customers right now though, but for the sake of completeness: d1bc2a2
actions.loadMetricResults() | ||
actions.loadSecondaryMetricResults() | ||
} else { | ||
actions.loadMetricResultsSuccess([]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we depend on other follow up listeners running here? Or can we just not fire the actions at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we depend on other follow up listeners running here? Or can we just not fire the actions at all?
I think this falls under "general refactor of experimentLogic".
Another oddity is that addSharedMetricToExperiment
calls closePrimarySharedMetricModal
, closeSecondarySharedMetricModal
, and loadExperiment
:
posthog/frontend/src/scenes/experiments/experimentLogic.tsx
Lines 904 to 918 in 5c0e29c
addSharedMetricToExperiment: async ({ sharedMetricId, metadata }) => { | |
const sharedMetricsIds = values.experiment.saved_metrics.map((sharedMetric) => ({ | |
id: sharedMetric.saved_metric, | |
metadata, | |
})) | |
sharedMetricsIds.push({ id: sharedMetricId, metadata }) | |
await api.update(`api/projects/${values.currentProjectId}/experiments/${values.experimentId}`, { | |
saved_metrics_ids: sharedMetricsIds, | |
}) | |
actions.closePrimarySharedMetricModal() | |
actions.closeSecondarySharedMetricModal() | |
actions.loadExperiment() | |
}, |
However, closePrimarySharedMetricModal
and closeSecondarySharedMetricModal
each call loadExperiment
too:
posthog/frontend/src/scenes/experiments/experimentLogic.tsx
Lines 753 to 758 in 5c0e29c
closePrimarySharedMetricModal: () => { | |
actions.loadExperiment() | |
}, | |
closeSecondarySharedMetricModal: () => { | |
actions.loadExperiment() | |
}, |
I think @jurajmajerik might have some ideas in mind, so I'd rather wait until he's back.
See #27014
Changes
Prevents results from displaying on a draft experiment when you add or remove a metric.
Previously, adding or removing a metric would call
updateExperiment()
which progresses along to callloadMetricResults()
andloadSecondaryMetricResults()
, regardless of whether the experiment is started. The experiment results then erroneously displayed until you refreshed the page.This new logic only fetches metric results when the experiment is started, and noops them otherwise.
Before
CleanShot.2025-01-08.at.13.16.21.mp4
After
CleanShot.2025-01-08.at.13.17.26.mp4
How did you test this code?
I verified experiment results no longer appear unexpectedly when adding or removing a metric on a draft experiment.
I also verified experiment results appear as expected when adding or removing a metric on a launched experiment.