Skip to content
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

Add analytics events for browser preview #1984

Merged
merged 7 commits into from
Nov 1, 2024

Conversation

daveajrussell
Copy link
Contributor

@daveajrussell daveajrussell commented Oct 25, 2024

Adds a number of events to track page loads and UI switch actions for the browser preview.

When the browser app is loaded, we track a page load event with properties { previewUI: boolean, hasTriedPreviewUI: boolean }

hasTriedPreviewUI is stored when we first switch to the preview UI.

When we use the welcome tile to switch to the preview, we track an additional switched ui event with properties { switchedTo: 'browser' | 'preview'; timeSinceLastSwitch: number }.

We track timeSinceLastSwitch using a timestamp stored in localstorage, which we reset when switching between UIs.

@daveajrussell daveajrussell force-pushed the preview-analytics-events branch from 42371f9 to 83eb87f Compare October 30, 2024 16:58
Comment on lines +329 to +337
export const trackPreviewEpic: Epic<Action, GlobalState> = action$ => {
return action$.ofType(PREVIEW_EVENT).map((action: any) => {
return metricsEvent({
category: 'preview',
label: action.label,
data: action.data
})
})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding this epic for tracking preview events here so that we can consume them in the existing subscriber in the App and dispatch to segment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this previewDuck module to create redux actions for our preview switch and page load events, containing logic for setting properties of each action

@daveajrussell daveajrussell marked this pull request as ready for review October 30, 2024 17:43
useEffect(() => {
const pageLoadAction = trackPageLoad()
props.bus && props.bus.send(pageLoadAction.type, pageLoadAction)
}, [props.bus, props.telemetrySettings.allowUserStats])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it shouldn't be a problem, but I still worry a little about this effect running extra times and giving us bad stats. I think we could be certain to avoid that if we run dispatch in dbMetaEpics instead, in the section where the server configuration finishes. There's a comment: // side-effects where it'd fit well

Copy link
Contributor Author

@daveajrussell daveajrussell Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! I'll take a look

it seems that when dispatching from the dbMetaEpic, props.telemetrySettings.allowUserStats in App.tsx is still false :(

I'll see what we can do here

}

export default withBus(
connect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just pass null here instead of a placeholder function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that works, thanks

@@ -546,6 +547,12 @@ export const serverConfigEpic = (some$: any, store: any) =>
store.dispatch(triggerCredentialsTimeout())
}

setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting that it works even with a timeout of 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed - it's where I start to get a bit iffy with the eventloop/callback queue - but I guess the callback just going to the back of the queue is enough for react's internals to finish and resolve props in the app

@daveajrussell daveajrussell merged commit 8dd931d into neo4j:master Nov 1, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants