feat: cancel queries on navigation-ish #13414
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
| 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, | ||
| }, | ||
| ], | ||
| ]) | ||
| }) | ||
| }) | ||
| }) |
This file contains 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
This file contains 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
| 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, | ||
| 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 }) => ({ | ||
| // 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 | ||
| } | ||
|
|
||
| actions.abortAnyRunningQuery() | ||
|
|
||
| try { | ||
| return await api.update(`api/projects/${values.currentTeamId}/dashboards/${props.id}`, { | ||
|
|
@@ -778,7 +775,6 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |
| } | ||
| }, | ||
| beforeUnmount: () => { | ||
| if (cache.autoRefreshInterval) { | ||
| window.clearInterval(cache.autoRefreshInterval) | ||
| cache.autoRefreshInterval = null | ||
|
|
@@ -926,11 +922,8 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |
| true | ||
| ) | ||
|
|
||
| // we will use one abort controller for all insight queries for this dashboard | ||
| 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) { | ||
| 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 (!cancelled && refreshesFinished === insights.length) { | ||
|
There was a problem hiding this comment. @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.... |
||
| 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 | ||
|
|
||
| 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, | ||
| }) | ||
| }, | ||
| })), | ||
|
|
||
|
|
||
Oops, something went wrong.
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.
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