Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,18 @@
value: value,
}))

export const ExperimentReloadAction = ({

Check warning on line 101 in frontend/src/scenes/experiments/ExperimentView/ExperimentReloadAction.tsx

View workflow job for this annotation

GitHub Actions / Frontend formatting

lint:complexity

`ExperimentReloadAction` has cyclomatic complexity 11 (warn >10)
isRefreshing,
lastRefresh,
onClick,
progress,
queuedHint,
}: {
isRefreshing: boolean
lastRefresh: string | null
onClick: () => void
progress?: { completed: number; total: number }
queuedHint?: string
}): JSX.Element => {
const { autoRefresh, experiment } = useValues(experimentLogic)
const { setAutoRefresh, setPageVisibility, stopAutoRefreshInterval } = useActions(experimentLogic)
Expand Down Expand Up @@ -158,7 +160,7 @@
size="xsmall"
icon={isRefreshing ? <Spinner textColored /> : <IconRefresh />}
data-attr="refresh-experiment"
disabledReason={isRefreshing ? 'Loading...' : null}
disabledReason={isRefreshing ? (queuedHint ?? 'Loading...') : null}
sideAction={
ended
? undefined
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export function ExperimentReloadActionContainer({

function RecalculationReloadAction({ experiment }: { experiment: Experiment }): JSX.Element {
const metricsLogic = experimentMetricsLogic({ experiment })
const { isRecalculating, recalculationProgress, lastRefresh } = useValues(metricsLogic)
const { isRecalculating, recalculationProgress, lastRefresh, queuedRerun } = useValues(metricsLogic)
const { triggerRecalculation } = useActions(metricsLogic)
const { autoRefresh, currentRefresh } = useValues(experimentLogic)
const { refreshExperimentResults, reportExperimentMetricsRefreshed } = useActions(experimentLogic)
Expand All @@ -47,6 +47,7 @@ function RecalculationReloadAction({ experiment }: { experiment: Experiment }):
isRefreshing={isRecalculating}
lastRefresh={lastRefresh}
progress={recalculationProgress}
queuedHint={queuedRerun ? 'Changes will apply after the current recalculation finishes' : undefined}
onClick={() => {
reportExperimentMetricsRefreshed(experiment, true, {
triggered_by: 'manual',
Expand Down
177 changes: 177 additions & 0 deletions frontend/src/scenes/experiments/experimentMetricsLogic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,54 @@ describe('experimentMetricsLogic', () => {
}).toDispatchActions(['setCurrentRecalculation'])
expect(createMock).toHaveBeenCalled()
})

describe('queuing', () => {
it('queues instead of posting when a run is active', async () => {
const createMock = jest.fn(() => [201, pendingRecalculation])
useMocks({
get: {
'/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [
200,
inProgressRecalculation,
],
},
post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock },
})
mountLogic()
await expectLogic(logic).toDispatchActions(['setCurrentRecalculation'])
expect(logic.values.isRecalculating).toBe(true)

await expectLogic(logic, () => {
logic.actions.triggerRecalculation('metric_config_change')
}).toDispatchActions(['setQueuedRerun'])

expect(logic.values.queuedRerun).toBe('metric_config_change')
expect(createMock).not.toHaveBeenCalled()
})

it('does not queue a cold_run (cold_run always starts fresh)', async () => {
const createMock = jest.fn(() => [201, pendingRecalculation])
useMocks({
get: {
'/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [
200,
inProgressRecalculation,
],
},
post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock },
})
mountLogic()
await expectLogic(logic).toDispatchActions(['setCurrentRecalculation'])
expect(logic.values.isRecalculating).toBe(true)

await expectLogic(logic, () => {
logic.actions.triggerRecalculation('cold_run')
}).toDispatchActions(['setCurrentRecalculation'])

expect(logic.values.queuedRerun).toBeNull()
expect(createMock).toHaveBeenCalled()
})
})
})

describe('loading + progress selectors', () => {
Expand Down Expand Up @@ -743,6 +791,8 @@ describe('experimentMetricsLogic', () => {

// Flush afterMount → 404 → trigger → poll(create), then drive enough ticks to exhaust retries.
await jest.advanceTimersByTimeAsync(0)
// A config change queued a rerun while the run was in flight.
logic.actions.setQueuedRerun('metric_config_change')
for (let i = 0; i < MAX_POLL_RETRIES + 2; i++) {
await jest.advanceTimersByTimeAsync(POLL_INTERVAL_MS)
}
Expand All @@ -752,6 +802,9 @@ describe('experimentMetricsLogic', () => {
expect(lemonToast.error).toHaveBeenCalledWith(
'Failed to load recalculation results. Please reload to try again.'
)
// The bail clears the queued rerun so it can't strand: the terminal branch (its only consumer)
// never runs on a give-up path.
expect(logic.values.queuedRerun).toBeNull()
})

it('on a cold_run, applies partial results mid-flight before the run is terminal', async () => {
Expand Down Expand Up @@ -805,6 +858,111 @@ describe('experimentMetricsLogic', () => {
expect(logic.values.currentRecalculation).toEqual(expect.objectContaining({ status: 'in_progress' }))
expect(logic.values.primaryMetricsResults[0]).toEqual(primaryResult)
})

it('fires the queued rerun when the current run reaches terminal', async () => {
const createMock = jest.fn(() => [201, pendingRecalculation])
useMocks({
get: {
'/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [404, {}],
'/api/projects/:team_id/experiments/:id/metrics_recalculation/:recalc_id/': () => [
200,
inProgressRecalculation,
],
},
post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock },
})
jest.useFakeTimers()
mountLogic()

// Flush the afterMount → 404 → cold-run trigger → poll(create) chain before queuing.
await jest.advanceTimersByTimeAsync(0)

// A config change arrives while the cold run is active: it queues instead of posting.
logic.actions.triggerRecalculation('metric_config_change')
expect(logic.values.queuedRerun).toBe('metric_config_change')

// Now let the active run reach terminal; the retrieve mock always answers 'in_progress', so
// repoint it to 'completed' for the tick that follows.
useMocks({
get: {
'/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [404, {}],
'/api/projects/:team_id/experiments/:id/metrics_recalculation/:recalc_id/': () => [
200,
completedRecalculation2,
],
},
post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock },
})

await expectLogic(logic, async () => {
await jest.advanceTimersByTimeAsync(POLL_INTERVAL_MS)
}).toDispatchActions([
(a) => a.type === logic.actionTypes.setQueuedRerun && a.payload.trigger === null,
(a) =>
a.type === logic.actionTypes.triggerRecalculation && a.payload.trigger === 'metric_config_change',
])

expect(logic.values.queuedRerun).toBeNull()
// The rerun fired as a fresh run (not re-queued), since the prior run was terminal by then.
await jest.advanceTimersByTimeAsync(0)
expect(createMock).toHaveBeenCalledTimes(2)
})

it('coalesces multiple queued config changes into a single rerun at the stronger scope', async () => {
const createMock = jest.fn(() => [201, pendingRecalculation])
useMocks({
get: {
'/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [404, {}],
'/api/projects/:team_id/experiments/:id/metrics_recalculation/:recalc_id/': () => [
200,
inProgressRecalculation,
],
},
post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock },
})
jest.useFakeTimers()
mountLogic()

// Flush the afterMount → 404 → cold-run trigger → poll(create) chain before queuing.
await jest.advanceTimersByTimeAsync(0)

// Two config changes arrive while the cold run is still active: they coalesce to the
// stronger scope instead of posting twice, so exactly one rerun fires later.
logic.actions.triggerRecalculation('metric_config_change')
expect(logic.values.queuedRerun).toBe('metric_config_change')
logic.actions.triggerRecalculation('experiment_config_change')
expect(logic.values.queuedRerun).toBe('experiment_config_change')

// Still one active run: neither queued change posted a create.
expect(createMock).toHaveBeenCalledTimes(1)

// Now let the active run reach terminal; the retrieve mock always answers 'in_progress', so
// repoint it to 'completed' for the tick that follows.
useMocks({
get: {
'/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [404, {}],
'/api/projects/:team_id/experiments/:id/metrics_recalculation/:recalc_id/': () => [
200,
completedRecalculation2,
],
},
post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock },
})

await expectLogic(logic, async () => {
await jest.advanceTimersByTimeAsync(POLL_INTERVAL_MS)
}).toDispatchActions([
(a) => a.type === logic.actionTypes.setQueuedRerun && a.payload.trigger === null,
(a) =>
a.type === logic.actionTypes.triggerRecalculation &&
a.payload.trigger === 'experiment_config_change',
])

expect(logic.values.queuedRerun).toBeNull()
// Exactly one fresh rerun fired, coalesced to the stronger scope.
await jest.advanceTimersByTimeAsync(0)
expect(createMock).toHaveBeenCalledTimes(2)
})
})

describe('liveRowsProgress', () => {
Expand Down Expand Up @@ -949,4 +1107,23 @@ describe('experimentMetricsLogic', () => {
expect(createMock).not.toHaveBeenCalled()
})
})

describe('queuedRerun reducer', () => {
beforeEach(() => {
mountLogic()
})
it('coalesces to the stronger scope', () => {
logic.actions.setQueuedRerun('metric_config_change')
expect(logic.values.queuedRerun).toBe('metric_config_change')
logic.actions.setQueuedRerun('experiment_config_change')
expect(logic.values.queuedRerun).toBe('experiment_config_change')
logic.actions.setQueuedRerun('metric_config_change')
expect(logic.values.queuedRerun).toBe('experiment_config_change')
})
it('clears on null', () => {
logic.actions.setQueuedRerun('metric_config_change')
logic.actions.setQueuedRerun(null)
expect(logic.values.queuedRerun).toBeNull()
})
})
})
45 changes: 45 additions & 0 deletions frontend/src/scenes/experiments/experimentMetricsLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@
nextRetryAt: string | null
primaryMetricsResults: CachedNewExperimentQueryResponse[]
primaryMetricsResultsErrors: (unknown | null)[]
queuedRerun: ExperimentMetricsRecalculationTriggerEnumApi | null
recalculatingMetricUuids: string[]
recalculationDisplayState: 'cold' | 'initial' | 'partial' | 'refreshing' | 'resting'
recalculationLoading: boolean
Expand Down Expand Up @@ -248,6 +249,9 @@
setPrimaryMetricsResultsErrors: (errors: (unknown | null)[]) => {
errors: unknown[]
}
setQueuedRerun: (trigger: ExperimentMetricsRecalculationTriggerEnumApi | null) => {
trigger: ExperimentMetricsRecalculationTriggerEnumApi | null
}
setRecalculatingMetricUuids: (uuids: string[]) => {
uuids: string[]
}
Expand Down Expand Up @@ -318,6 +322,7 @@
setRecalculationLoading: (loading: boolean) => ({ loading }),
// The metrics still showing a stale value while a non-cold recalc refreshes them in place.
setRecalculatingMetricUuids: (uuids: string[]) => ({ uuids }),
setQueuedRerun: (trigger: ExperimentMetricsRecalculationTriggerEnumApi | null) => ({ trigger }),
}),
reducers({
currentRecalculation: [
Expand All @@ -334,6 +339,16 @@
setCurrentRecalculation: () => false,
},
],
queuedRerun: [
null as ExperimentMetricsRecalculationTriggerEnumApi | null,
{
/**
* If the state is `experiment_config_change`, it sticks.
*/
setQueuedRerun: (state, { trigger }) =>
trigger === null ? null : state === 'experiment_config_change' ? state : trigger,
},
],
primaryMetricsResults: [
[] as CachedNewExperimentQueryResponse[],
{
Expand Down Expand Up @@ -684,7 +699,7 @@
actions.setRecalculationLoading(false)
}
},
triggerRecalculation: async ({ trigger }) => {

Check warning on line 702 in frontend/src/scenes/experiments/experimentMetricsLogic.ts

View workflow job for this annotation

GitHub Actions / Frontend formatting

lint:complexity

`triggerRecalculation` has cyclomatic complexity 14 (warn >10)
/**
* bail if feature not enabled
*/
Expand All @@ -698,6 +713,17 @@
if (!experimentIsLaunched()) {
return
}
/**
* If we have a recalculation in flight, and the user has changed the experiment or the metrics configuration,
* we schedule a new recalculation with the corresponding trigger.
*/
if (
(trigger === 'experiment_config_change' || trigger === 'metric_config_change') &&
values.isRecalculating
) {
actions.setQueuedRerun(trigger)
return
}
/**
* Dim the metrics that already show something (a value or an error) so they read as
* "refreshing" until the new result streams in. Cold runs have nothing prior, so nothing to dim.
Expand Down Expand Up @@ -794,7 +820,7 @@
* free poll-vs-poll supersession, no interval to clear. The gap: a terminal-on-create run applies
* results without dispatching pollRecalculation, so cache.activeRecalculationId covers that case.
*/
pollRecalculation: async ({ recalculationId }, breakpoint) => {

Check warning on line 823 in frontend/src/scenes/experiments/experimentMetricsLogic.ts

View workflow job for this annotation

GitHub Actions / Frontend formatting

lint:complexity

`pollRecalculation` has cyclomatic complexity 18 (warn >10)
if (!flagEnabled()) {
return
}
Expand All @@ -810,8 +836,14 @@
}
cache.activeRecalculationId = recalculationId

/**
* if for any reason the recalculation workflow never reaches terminal state, we need to
* stop polling, and clear recalculation state. This includes any queued rerun, that will never
* be fired.
*/
if (Date.now() - (cache.recalcStartMs ?? Date.now()) > MAX_POLL_DURATION_MS) {
actions.setRecalculatingMetricUuids([])
actions.setQueuedRerun(null)
lemonToast.error('Recalculation appears stuck. Please reload to try again.')
return
}
Expand All @@ -830,9 +862,11 @@
/**
* Retry on transient errors, but give up after MAX_POLL_RETRIES consecutive failures so a
* persistently failing endpoint can't poll forever.
* If we reach the retry cap, we also need to clear queued reruns that never fire.
*/
cache.pollRetryCount = (cache.pollRetryCount ?? 0) + 1
if (cache.pollRetryCount >= MAX_POLL_RETRIES) {
actions.setQueuedRerun(null)
lemonToast.error('Failed to load recalculation results. Please reload to try again.')
return
}
Expand Down Expand Up @@ -893,6 +927,17 @@
`${recalculation.failed_metrics} of ${recalculation.total_metrics} metrics failed to load`
)
}

/**
* The user has changed the experiment or the metrics configuration while a recalculation was in flight.
* We clear the flag an trigger a new recalculation to reflect the changes. Cache resolution is handled by the
* durable execution backend, so we'll try to reuse the cached results.
*/
const queued = values.queuedRerun
if (queued) {
actions.setQueuedRerun(null)
actions.triggerRecalculation(queued)
Comment thread
rodrigoi marked this conversation as resolved.
Resolved
}
Comment thread
rodrigoi marked this conversation as resolved.
Resolved
},
}
}),
Expand Down
Loading