-
Notifications
You must be signed in to change notification settings - Fork 2
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: track user resetting flow #2307
Conversation
- Update `analytics_log` record for node which was reset on reset confirmation - Change flow_direction to "reset" to store this user action - Ammend update permission to allow flow_direction to be changed
- On click of a reset button on a notice update analytics_log to `{flow_direction: "reset"}`
🤖 Hasura Change Summary compared a subset of table metadata including permissions: Updated Tables (1)
|
Removed vultr server and associated DNS entries |
@@ -199,6 +202,31 @@ export const AnalyticsProvider: React.FC<{ children: React.ReactNode }> = ({ | |||
} | |||
} | |||
|
|||
// Comment to trigger new pizza build. |
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.
Vultr isn't behaving itself still 😅
I logged in and killed the instances and DNS records for this PR i.e. 2307
but it seems to be taking a very long time.
I'll remove this comment and troubleshoot further next week.
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.
👀 pizza is looking okay now to me, is this resolved or do you have any outstanding vultr bugs/questions?
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's resolved 😅
The situation was:
- Pizza failed to build
- I deleted the instances and DNS records for it on Vultr
- The pizza built but the vultr health check failed.
- I pushed a commit
- The vultr health check passed 🤷
I'm pretty confident the PR behaves as expected.
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.
If you're happy to review @jessicamcinchak I'll merge / update as per feedback 👍
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.
A couple tips for future Vultru debugging:
- If only the healthcheck fails, Vultr might just be slow to warm up - wait a couple minutes and try re-running just the failed GH action job to save yourself commits (it usually passes on the next go!)
- Vultr fails to build entirely for a couple common reasons: timeouts (usually around 10m mark), commits pushed close together and an "update" trys to run before the "create" is finished. In these cases, again first trying to re-run just the failed GH action job before doing a full nuke can save a lot of time!
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.
Working as expected for me and database structure looks good 👍
Super easy to understand, really nice analytics-value to number-of-line code change ratio here I think 🙂
@@ -6,7 +6,7 @@ import React, { createContext, useContext, useEffect, useState } from "react"; | |||
|
|||
import { Store, useStore } from "./store"; | |||
|
|||
export type AnalyticsType = "init" | "resume"; | |||
export type AnalyticsType = "init" | "resume" | "reset" |
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.
Hey @jessicamcinchak something I hadn't realised is that this AnalyticsType
is related to the analytics
model which tracks the overarching analytics session? It stores whether the user's session was an init
or a resume
?
I believe the log direction only applies to analytics_log
model instances which are tied together with an instance of the analytics
model?
Based on the way the "reset"
is being recorded against an analytics_log
if it make more sense to think of it as a log direction i.e. an extreme case of going backwards
where you go back to the the start?
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'm not entirely sure I'm following this one yet, but my instinct here is that "reset" ends the given session and therefore, at least in my mind, it's an analytics type rather than pure "direction" (eg directions like "forwards" and "backwards" are always within a session). But also totally trust your judgement on this!
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 that's a really good point although in this PR the "reset"
is only ever applied within a session as a flow_direction
As in:
- The last
analytics_log
for thatanalytic
session would have aflow_direction
of"reset"
. - The
type
of the aforementionedanalytic
session would remain whatevertype
it had been i.e."init"
or"resume"
- The new
analytic
session would default to"init"
So currently the AnalyticType
is only applied as a flow_direction
.
Does that make sense? I'm maybe over focussing on the semantics but I'm just trying to get my head around who things have been working and what we're looking for with this new functionality.
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.
Thanks for this clarification, this makes a lot more sense now.
I'm thinking:
analytic
sessiontype
captures how the session starts, it's never captured how it "ends" before now- therefore continuing to just capture
reset
as aflow_direction
onanalytics_log
seems consistent?
There's an existing chart in metabase about "Where applicants drop-off" - and if we're considering "reset" as similar to "drop-off" - maybe looking at that underlying SQL query might offter confirmation if this approach will be able to be aggregated/summarised/charted similarly or be difficult without carrying over "reset" as a new analytic.type
?
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.
That's a very helpful framing of the situation, thanks Jess!
I'll have a look at "Where applicants drop-off" and see how it's being approached there and if there are any implications with this change.
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 had a wee look at the SQL query:
SELECT date_trunc('week', analytics_logs.created_at) as "Week", count(*) as "Exits"
FROM analytics_logs
JOIN analytics ON (analytics.id = analytics_logs.analytics_id)
WHERE analytics_logs.user_exit = true
AND analytics_logs.node_type = 3
AND analytics.flow_id = {{flow_id}}::uuid
GROUP BY "Week"
ORDER BY "Week"
The drop-off
uses the user_exit
property which to my understanding is marked as true
when a user stops viewing the page:
const onPageExit = () => { | |
if (lastAnalyticsLogId && shouldTrackAnalytics) { | |
if (document.visibilityState === "hidden") { | |
send( | |
`${ | |
process.env.REACT_APP_API_URL | |
}/analytics/log-user-exit?analyticsLogId=${lastAnalyticsLogId.toString()}`, | |
); | |
} | |
if (document.visibilityState === "visible") { | |
send( | |
`${ | |
process.env.REACT_APP_API_URL | |
}/analytics/log-user-resume?analyticsLogId=${lastAnalyticsLogId?.toString()}`, | |
); | |
} | |
} | |
}; | |
useEffect(() => { | |
if (shouldTrackAnalytics) | |
document.addEventListener("visibilitychange", onPageExit); | |
return () => { | |
if (shouldTrackAnalytics) | |
document.removeEventListener("visibilitychange", onPageExit); | |
}; | |
}, []); |
I had thought of a user deciding to reset
as not actually exiting the page and therefore not being a user_exit: true
.
Although in the context of drop off I'm not so sure 🤔
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.
Actually on second thoughts I think we'll be able to handle the reset
state in a similar fashion as the query above so we should be okay I think 👍
- Currently, the new "reset" type will only be applied to the flow_direction property - Hence moving to the type for this property i.e. AnalyticsLogDirection - This can be easily changed in the future but it's more consistent with current behaviour - Auto-linting changes
What
flow_direction
of ananalytics_log
to a new typereset
.Notice
with a reset.Why
{flow_direction: "reset"}
will show when a user has decided to restart.Screen Recording
Shows:
reset
when resetting via the header.Notice
button press.reset
Notice
button press.Screen.Recording.2023-10-13.at.15.49.12.mov
Supersedes
Note