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
chore: instrument time insights are marked as loading #11222
Conversation
loadDashboardItemsSuccess: () => { | ||
// I didn't want any parameters but kea does :) | ||
sharedListeners.reportLoadTiming(null, async () => {}, {} as { type: string; payload: any }, null) |
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 you can do something like this instead:
loadDashboardItemsSuccess: () => { | |
// I didn't want any parameters but kea does :) | |
sharedListeners.reportLoadTiming(null, async () => {}, {} as { type: string; payload: any }, null) | |
loadDashboardItemsSuccess: (...args) => { | |
sharedListeners.reportLoadTiming(...args) |
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.
Ah, cool 🙌
@@ -2,7 +2,7 @@ import { isBreakpoint, kea } from 'kea' | |||
import api from 'lib/api' | |||
import { dashboardsModel } from '~/models/dashboardsModel' | |||
import { router } from 'kea-router' | |||
import { dayjs, now } from 'lib/dayjs' | |||
import { Dayjs, dayjs, now } from 'lib/dayjs' |
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.
kea-typegen
gets confused because of Dayjs
being imported for some reason… But also Day.js seems overkill for this, Date should be able to do the job too
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 something forgot to be removed. Once done I think this is good
@@ -27,6 +26,7 @@ import { teamLogic } from '../teamLogic' | |||
import { urls } from 'scenes/urls' | |||
import { userLogic } from 'scenes/userLogic' | |||
import { mergeWithDashboardTile } from 'scenes/insights/utils/dashboardTiles' | |||
import { dayjs, now } from 'lib/dayjs' |
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 these were forgotten to be removed?
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.
Interestingly. We use dayjs elsewhere in the file already but we never return the Dayjs
type. So kea typegen never has to deal with it.
I thought I'd not rewrite existing functionality since there's a good chance we leave this in for a few days to get a snapshot of data and then remove it
Problem
We don't know how long people see Insight loading skeletons for
And want to know that to inform design work
Changes
dashboard loading time
event. Which is a (reasonable) proxy for the time someone would be seeing skeleton loaders on the dashboardinsight refresh time
. Which is a reasonable proxy for the time someone would be seeing loaders over the insight cards on a dashboardHow did you test this code?
running it locally to see the capture calls made