-
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 dashboard queries when filters change #13178
Conversation
abortQuery: ({ queryId }) => { | ||
const { currentTeamId } = values | ||
if (values.featureFlags[FEATURE_FLAGS.CANCEL_RUNNING_QUERIES]) { | ||
api.create(`api/projects/${currentTeamId}/insights/cancel`, { client_query_id: queryId }) |
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.
Missing await?
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.
We don't await in the insightLogic either. I think it's OK here to be completely fire and forget.
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 it is good hygene to always await. PTSD from untracked promises causing trouble in plugin-server land.
@@ -1210,6 +1242,12 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |||
actions.setShouldReportOnAPILoad(true) | |||
} | |||
}, | |||
abortQuery: ({ queryId }) => { |
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.
TODO for future: making sure that these reach time-to-see-data dataset :)
actions.setRefreshStatuses( | ||
insights.map((item) => item.short_id), | ||
true | ||
) | ||
|
||
// If a query is in progress, kill that query |
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.
Non-blocking note: It feels like there's a missing abstraction here that would make this code a lot less unwieldy. Missing kea feature?
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.
Let's make sure to await
and let's get this live.
Pretty sure this might break n months down the line, but we'll deal with this when we deal with it 😅
It makes sense to await when capturing to metrics so will cover that in #13182 (which hopefully can follow very quickly) |
Problem
See #13041
We cancel queries in insight logic when filters change.
Changes
Let's cancel them on dashboards too
Implements the cancellation without refactoring (make it work -> make it right -> make it fast)
How did you test this code?