DEVPROD-22466 Add analytics indicating how long a user looked at a page.#1205
DEVPROD-22466 Add analytics indicating how long a user looked at a page.#1205khelif96 merged 34 commits intoevergreen-ci:mainfrom
Conversation
| }); | ||
| } | ||
| }; | ||
| }, [enabled, sendEvent, trackSession, minDuration]); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| document.removeEventListener("visibilitychange", handleVisibilityChange); | ||
|
|
||
| // Track session end | ||
| if (trackSession && stateChangeCount.current > 0) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| * }); | ||
| * ``` | ||
| */ | ||
| export const usePageVisibilityAnalytics = ({ |
There was a problem hiding this comment.
I think it would be preferable if we could use this hook without instantiating it on every page; it seems like it would be easy to forget to add this in the future. Do you think there's any way of doing that?
Also, leveraging the route_name param instead of the analytics object may be more helpful from a usability perspective. Is there any reason we must use identifier? At the very least, maybe omitting it would allow instantiating tracking in one parent component.
There was a problem hiding this comment.
I removed it and its now based on route names and only instantiated at the root. We do however lose page specific metadata by only instantiating once. Since we can't inject things like project identifier.
…ross multiple components for consistency and improved tracking.
| stateChangeCount.current = 0; | ||
| lastVisibilityState.current = null; | ||
| }; | ||
| }, [enabled, sendEvent, trackSession, minDuration, pathname]); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if (duration < minDuration) { | ||
| visibilityStartTime.current = currentTime; | ||
| return; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| "visibility.timestamp": string; | ||
| }; | ||
|
|
||
| interface UsePageVisibilityAnalyticsOptions { |
There was a problem hiding this comment.
[nit] Preference to omit use from the typename
| interface UsePageVisibilityAnalyticsOptions { | |
| interface PageVisibilityAnalyticsOptions { |
Could you also remove the field comments?
| * Helps avoid tracking rapid tab switches | ||
| * @default 1000 | ||
| */ | ||
| minDuration?: number; |
There was a problem hiding this comment.
[nit] Prefer minDurationMs
| "visibility.total_visible_ms": number; | ||
| "visibility.total_hidden_ms": number; | ||
| "visibility.state_changes": number; | ||
| "visibility.timestamp": string; |
There was a problem hiding this comment.
I don't think we should include timestamp on any of these since Honeycomb already attaches that data to a trace
|
|
||
| type PageVisibilityAction = | ||
| | { | ||
| name: "System Event page became visible"; |
There was a problem hiding this comment.
Naming nit... what's the idea behind "System Event" prefixing these?
| lastVisibilityState.current = null; | ||
| }; | ||
| }, [enabled, sendEvent, trackSession, minDuration, pathname]); | ||
|
|
There was a problem hiding this comment.
There was a problem hiding this comment.
The query param issue is gone, but I'm seeing double logging now because of things like a redirect from root -> user patches -> my user patches. Here's a good timeline. I imagine that could be kind of tricky to solve, but I do think it decreases the usefulness
| /** | ||
| * Whether to track the visibility changes | ||
| * @default true | ||
| */ | ||
| enabled?: boolean; | ||
| /** | ||
| * Whether to track session start/end events | ||
| * @default true | ||
| */ | ||
| trackSession?: boolean; |
There was a problem hiding this comment.
What are the use cases for these?
There was a problem hiding this comment.
Removed track session
| name: "System Event page visibility session ended"; | ||
| "visibility.total_visible_ms": number; | ||
| "visibility.total_hidden_ms": number; | ||
| "visibility.state_changes": number; |
There was a problem hiding this comment.
I might prefer to call this visibility_changes to clarify it's not referring to page navigation
| "visibility.timestamp": string; | ||
| } | ||
| | { | ||
| name: "System Event page visibility session started"; |
There was a problem hiding this comment.
[nit] Could call this "Session started" since it's in the PageVisibility analytics object, your call
| "visibility.timestamp": string; | ||
| } | ||
| | { | ||
| name: "System Event page visibility session ended"; |
| } | ||
|
|
||
| try { | ||
| sendEvent({ |
There was a problem hiding this comment.
Would it be possible to send all of these as spans within a trace 😳 I think that would be wayyyy more useful
There was a problem hiding this comment.
Theoretically its possible but we would end up with a ginormous trace which may be less useful. (Thinking potential multi hour traces.)
There was a problem hiding this comment.
Good point—I just found this timeline view (linked by session ID) which totally paints the same picture I was hoping for https://ui.honeycomb.io/mongodb-4b/environments/staging/session-timeline-experiment/f3c2231532c0bb2ed5492a608151770e 👍
| lastVisibilityState.current = null; | ||
| }; | ||
| }, [enabled, sendEvent, trackSession, minDuration, pathname]); | ||
|
|
There was a problem hiding this comment.
The query param issue is gone, but I'm seeing double logging now because of things like a redirect from root -> user patches -> my user patches. Here's a good timeline. I imagine that could be kind of tricky to solve, but I do think it decreases the usefulness
| "visibility.duration_ms": number; | ||
| } | ||
| | { | ||
| name: "System Event page hidden"; |
There was a problem hiding this comment.
Just to be clear, navigating to a new page should fire a page hidden event and then a new page visible event, right? That's what I'm observing
There was a problem hiding this comment.
Good catch fixed, It should have also fixed the other issue
|
|
||
| try { | ||
| sendEvent({ | ||
| name: "System Event session ended", |
There was a problem hiding this comment.
I fear I haven't been able to produce this event during this iteration, but I was on the previous review 🙁 honeycomb
There was a problem hiding this comment.
Apparently this isn't super reliable and browsers don't consistently allow enough time for the component to unmount do you have any thoughts on how we should handle this?
There was a problem hiding this comment.
I think removing pathname from the useEffect is the reason this no longer works (but that's still the right move imo). Claude had these two suggestions:
- Use visibilitychange + navigator.sendBeacon() (most reliable)
The visibilitychange event with state "hidden" fires reliably on page unload in all modern browsers (it's part of the Page Lifecycle API). Combine it with navigator.sendBeacon(), which is specifically designed to guarantee delivery during page teardown. This would mean sending the session-ended telemetry outside of the OTel span pipeline, directly as a beacon to the Honeycomb endpoint.- Use pagehide event as a session-end signal
Register a pagehide listener (in the effect setup, not in cleanup) that sends the session summary. pagehide fires more reliably than relying on React unmount.
The default parameter `attributes = {}` created a new object on every
render, causing useCallback to return a new sendEvent each cycle. This
triggered spurious session-ended/session-started analytics events in
usePageVisibilityAnalytics. Use a module-level constant instead.
Co-Authored-By: Claude Code <noreply@anthropic.com>
|
evergreen retry |
|
|
||
| try { | ||
| sendEvent({ | ||
| name: "System Event session ended", |
There was a problem hiding this comment.
I think removing pathname from the useEffect is the reason this no longer works (but that's still the right move imo). Claude had these two suggestions:
- Use visibilitychange + navigator.sendBeacon() (most reliable)
The visibilitychange event with state "hidden" fires reliably on page unload in all modern browsers (it's part of the Page Lifecycle API). Combine it with navigator.sendBeacon(), which is specifically designed to guarantee delivery during page teardown. This would mean sending the session-ended telemetry outside of the OTel span pipeline, directly as a beacon to the Honeycomb endpoint.- Use pagehide event as a session-end signal
Register a pagehide listener (in the effect setup, not in cleanup) that sends the session summary. pagehide fires more reliably than relying on React unmount.


DEVPROD-22466
Description
In an effort to measure how long people spend performing investigations this pr add some new analytics events which track how long a user spends on an individual page.