-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: cancel queries on navigation-ish #13414
Changes from all commits
8ecb703
7ae4f46
6ad60b3
5d44ab0
fe2c3a5
e480015
4989b1a
1064aba
b3c5e2f
09234ce
22c6fc6
139a446
e4d2a70
63c8259
3950a5c
f971799
4328195
c71bf97
706f9d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,178 @@ | ||
import { expectLogic, partial } from 'kea-test-utils' | ||
import { initKeaTests } from '~/test/init' | ||
import { dashboardLogic } from 'scenes/dashboard/dashboardLogic' | ||
import { dashboardsModel } from '~/models/dashboardsModel' | ||
import { insightsModel } from '~/models/insightsModel' | ||
import { eventUsageLogic } from 'lib/utils/eventUsageLogic' | ||
import { DashboardType, InsightModel, InsightShortId } from '~/types' | ||
import { useMocks } from '~/mocks/jest' | ||
import { now } from 'lib/dayjs' | ||
import { MOCK_TEAM_ID } from 'lib/api.mock' | ||
import api from 'lib/api' | ||
import { boxToString, dashboardResult, insightOnDashboard, tileFromInsight } from 'scenes/dashboard/dashboardLogic.test' | ||
|
||
const seenQueryIDs: string[] = [] | ||
|
||
describe('dashboardLogic query cancellation', () => { | ||
let logic: ReturnType<typeof dashboardLogic.build> | ||
|
||
let dashboards: Record<number, DashboardType> = {} | ||
|
||
let dashboardTwelveInsightLoadedCount = 0 | ||
|
||
beforeEach(() => { | ||
jest.spyOn(api, 'update') | ||
|
||
const insights: Record<number, InsightModel> = { | ||
2040: { | ||
...insightOnDashboard(2040, [12]), | ||
id: 2040, | ||
short_id: '2040' as InsightShortId, | ||
last_refresh: now().toISOString(), | ||
}, | ||
} | ||
dashboards = { | ||
12: { | ||
...dashboardResult(12, [tileFromInsight(insights['2040'])]), | ||
}, | ||
} | ||
useMocks({ | ||
get: { | ||
'/api/projects/:team/dashboards/12/': { ...dashboards['12'] }, | ||
'/api/projects/:team/dashboards/': { | ||
count: 1, | ||
next: null, | ||
previous: null, | ||
results: [{ ...dashboards['12'] }], | ||
}, | ||
'/api/projects/:team/insights/:id/': (req) => { | ||
const clientQueryId = req.url.searchParams.get('client_query_id') | ||
if (clientQueryId !== null) { | ||
seenQueryIDs.push(clientQueryId) | ||
} | ||
|
||
const dashboard = req.url.searchParams.get('from_dashboard') | ||
if (!dashboard) { | ||
throw new Error('the logic must always add this param') | ||
} | ||
const matched = insights[boxToString(req.params['id'])] | ||
if (!matched) { | ||
return [404, null] | ||
} | ||
if (dashboard === '12') { | ||
dashboardTwelveInsightLoadedCount++ | ||
// delay for 2 seconds before response without pausing | ||
// but only the first time that dashboard 12 refreshes its results | ||
if (dashboardTwelveInsightLoadedCount === 1) { | ||
return new Promise((resolve) => | ||
setTimeout(() => { | ||
resolve([200, matched]) | ||
}, 2000) | ||
) | ||
} | ||
} | ||
return [200, matched] | ||
}, | ||
}, | ||
post: { | ||
'/api/projects/:team/insights/cancel/': [201], | ||
}, | ||
patch: { | ||
'/api/projects/:team/dashboards/:id/': async (req) => { | ||
const dashboardId = typeof req.params['id'] === 'string' ? req.params['id'] : req.params['id'][0] | ||
const payload = await req.json() | ||
return [200, { ...dashboards[dashboardId], ...payload }] | ||
}, | ||
'/api/projects/:team/insights/:id/': async (req) => { | ||
try { | ||
const updates = await req.json() | ||
if (typeof updates !== 'object') { | ||
return [500, `this update should receive an object body not ${JSON.stringify(updates)}`] | ||
} | ||
const insightId = boxToString(req.params.id) | ||
|
||
const starting: InsightModel = insights[insightId] | ||
insights[insightId] = { | ||
...starting, | ||
...updates, | ||
} | ||
|
||
starting.dashboards?.forEach((dashboardId) => { | ||
// remove this insight from any dashboard it is already on | ||
dashboards[dashboardId].tiles = dashboards[dashboardId].tiles.filter( | ||
(t) => !!t.insight && t.insight.id !== starting.id | ||
) | ||
}) | ||
|
||
insights[insightId].dashboards?.forEach((dashboardId: number) => { | ||
// then add it to any it new references | ||
dashboards[dashboardId].tiles.push(tileFromInsight(insights[insightId])) | ||
}) | ||
|
||
return [200, insights[insightId]] | ||
} catch (e) { | ||
return [500, e] | ||
} | ||
}, | ||
}, | ||
}) | ||
initKeaTests() | ||
dashboardsModel.mount() | ||
insightsModel.mount() | ||
}) | ||
|
||
describe('cancelling queries', () => { | ||
beforeEach(async () => { | ||
logic = dashboardLogic({ id: 12 }) | ||
logic.mount() | ||
await expectLogic(logic).toFinishAllListeners() | ||
}) | ||
|
||
it('cancels a running query', async () => { | ||
jest.spyOn(api, 'create') | ||
|
||
setTimeout(() => { | ||
// this change of filters will dispatch cancellation on the first query | ||
// will run while the -180d query is still running | ||
logic.actions.setDates('-90d', null) | ||
}, 200) | ||
// dispatches an artificially slow data request | ||
// takes 3000 milliseconds to return | ||
logic.actions.setDates('-180d', null) | ||
|
||
await expectLogic(logic) | ||
.toDispatchActions([ | ||
'setDates', | ||
'updateFilters', | ||
'abortAnyRunningQuery', | ||
'refreshAllDashboardItems', | ||
'abortAnyRunningQuery', | ||
'setDates', | ||
'updateFilters', | ||
'abortAnyRunningQuery', | ||
'abortQuery', | ||
'refreshAllDashboardItems', | ||
eventUsageLogic.actionTypes.reportDashboardRefreshed, | ||
]) | ||
.toMatchValues({ | ||
filters: partial({ date_from: '-90d' }), | ||
}) | ||
|
||
const mockCreateCalls = (api.create as jest.Mock).mock.calls | ||
// there will be at least two used client query ids | ||
// the most recent has not been cancelled | ||
// the one before that has been | ||
// the query IDs are made of `${dashboard query ID}::{insight query ID}` | ||
// only the dashboard query ID is sent to the API | ||
const cancelledQueryID = seenQueryIDs[seenQueryIDs.length - 2].split('::')[0] | ||
expect(mockCreateCalls).toEqual([ | ||
[ | ||
`api/projects/${MOCK_TEAM_ID}/insights/cancel`, | ||
{ | ||
client_query_id: cancelledQueryID, | ||
}, | ||
], | ||
]) | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,6 @@ import { insightsModel } from '~/models/insightsModel' | |
import { | ||
AUTO_REFRESH_DASHBOARD_THRESHOLD_HOURS, | ||
DashboardPrivilegeLevel, | ||
FEATURE_FLAGS, | ||
OrganizationMembershipLevel, | ||
} from 'lib/constants' | ||
import { DashboardEventSource, eventUsageLogic } from 'lib/utils/eventUsageLogic' | ||
|
@@ -187,9 +186,10 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |
loadingDashboardItemsStarted: (action: string, dashboardQueryId: string) => ({ action, dashboardQueryId }), | ||
setInitialLoadResponseBytes: (responseBytes: number) => ({ responseBytes }), | ||
abortQuery: (payload: { dashboardQueryId: string; queryId: string; queryStartTime: number }) => payload, | ||
abortAnyRunningQuery: true, | ||
}), | ||
|
||
loaders(({ actions, props, values, cache }) => ({ | ||
loaders(({ actions, props, values }) => ({ | ||
// TODO this is a terrible name... it is "dashboard" but there's a "dashboard" reducer ¯\_(ツ)_/¯ | ||
allItems: [ | ||
null as DashboardType | null, | ||
|
@@ -225,10 +225,7 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |
return values.allItems | ||
} | ||
|
||
if (cache.abortController) { | ||
cache.abortController.abort() | ||
} | ||
cache.abortController = null | ||
actions.abortAnyRunningQuery() | ||
|
||
try { | ||
return await api.update(`api/projects/${values.currentTeamId}/dashboards/${props.id}`, { | ||
|
@@ -778,7 +775,6 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |
} | ||
}, | ||
beforeUnmount: () => { | ||
cache.abortController?.abort() | ||
if (cache.autoRefreshInterval) { | ||
window.clearInterval(cache.autoRefreshInterval) | ||
cache.autoRefreshInterval = null | ||
|
@@ -926,11 +922,8 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |
true | ||
) | ||
|
||
// If a query is in progress, kill that query | ||
// we will use one abort controller for all insight queries for this dashboard | ||
if (cache.abortController) { | ||
cache.abortController.abort() | ||
} | ||
actions.abortAnyRunningQuery() | ||
cache.abortController = new AbortController() | ||
const methodOptions: ApiMethodOptions = { | ||
signal: cache.abortController.signal, | ||
|
@@ -980,7 +973,6 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |
}) | ||
totalResponseBytes += getResponseBytes(refreshedInsightResponse) | ||
} catch (e: any) { | ||
console.error(e) | ||
if (isBreakpoint(e)) { | ||
cancelled = true | ||
} else if (e.name === 'AbortError' || e.message?.name === 'AbortError') { | ||
|
@@ -995,10 +987,7 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |
} | ||
|
||
refreshesFinished += 1 | ||
if (refreshesFinished === insights.length) { | ||
breakpoint() | ||
cache.abortController = null | ||
|
||
if (!cancelled && refreshesFinished === insights.length) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @macobo one last thing to check now I have the tests passing I've had to add a I think it's OK not to report time_to_see data for the dashboard load here since we'd be reporting on the cancellation but.... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the heads up. |
||
const payload: TimeToSeeDataPayload = { | ||
type: 'dashboard_load', | ||
context: 'dashboard', | ||
|
@@ -1162,25 +1151,30 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |
actions.setShouldReportOnAPILoad(true) | ||
} | ||
}, | ||
abortAnyRunningQuery: () => { | ||
if (cache.abortController) { | ||
cache.abortController.abort() | ||
cache.abortController = null | ||
} | ||
}, | ||
abortQuery: async ({ dashboardQueryId, queryId, queryStartTime }) => { | ||
const { currentTeamId } = values | ||
if (values.featureFlags[FEATURE_FLAGS.CANCEL_RUNNING_QUERIES]) { | ||
await api.create(`api/projects/${currentTeamId}/insights/cancel`, { client_query_id: dashboardQueryId }) | ||
|
||
// TRICKY: we cancel just once using the dashboard query id. | ||
// we can record the queryId that happened to capture the AbortError exception | ||
// and request the cancellation, but it is probably not particularly relevant | ||
await captureTimeToSeeData(values.currentTeamId, { | ||
type: 'insight_load', | ||
context: 'dashboard', | ||
dashboard_query_id: dashboardQueryId, | ||
query_id: queryId, | ||
status: 'cancelled', | ||
time_to_see_data_ms: Math.floor(performance.now() - queryStartTime), | ||
insights_fetched: 0, | ||
insights_fetched_cached: 0, | ||
}) | ||
} | ||
|
||
await api.create(`api/projects/${currentTeamId}/insights/cancel`, { client_query_id: dashboardQueryId }) | ||
|
||
// TRICKY: we cancel just once using the dashboard query id. | ||
// we can record the queryId that happened to capture the AbortError exception | ||
// and request the cancellation, but it is probably not particularly relevant | ||
await captureTimeToSeeData(values.currentTeamId, { | ||
type: 'insight_load', | ||
context: 'dashboard', | ||
dashboard_query_id: dashboardQueryId, | ||
query_id: queryId, | ||
status: 'cancelled', | ||
time_to_see_data_ms: Math.floor(performance.now() - queryStartTime), | ||
insights_fetched: 0, | ||
insights_fetched_cached: 0, | ||
}) | ||
}, | ||
})), | ||
|
||
|
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.
Suggestion: create a
useQueryCancellation(fn)
wrapping useEffect.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.
The problem with wrapping (or at least the problem I've not figured out the solution to) is the insight and dashboard cancellation cases are still really different