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

feat:Time Spent on Page #341

Merged
merged 2 commits into from
Feb 1, 2023
Merged

feat:Time Spent on Page #341

merged 2 commits into from
Feb 1, 2023

Conversation

ps863
Copy link
Member

@ps863 ps863 commented Jan 12, 2023

Currently, AWS CloudWatch RUM does not record the time spent on a certain page. This change records the time spent on a page as part of the parentTimeSpentOnPage field in the page view event details. This field records the time spent on the parent page to the page view event it is recorded in.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -165,6 +167,7 @@ export class PageManager {
pageId,
parentPageId: currentPage.pageId,
interaction: currentPage.interaction + 1,
parentTimeSpentOnPage: timeSpentOnParentPage,
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: parentTimeSpentOnPage -> timeSpentOnParentPage

@@ -8,6 +8,7 @@ export type Page = {
pageId: string;
interaction: number;
parentPageId?: string;
parentTimeSpentOnPage?: number;
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: parentTimeSpentOnPage -> timeSpentOnParentPage

Copy link
Member Author

Choose a reason for hiding this comment

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

agree. Added to the revision

@adebayor123
Copy link
Member

thought: Since we cannot retrieve the last page's duration and we decided to not create a new form of page view, what about sending a page view event with pageId: EndVisit or something similar to indicate that this was the last page to visit? I believe we have beacon to send events in the case of browser closing (feel free to correct me if I'm wrong), and if we can detect a user closing the browser/page and utilize the beacon, we would capture the last page's duration.

@ps863
Copy link
Member Author

ps863 commented Jan 13, 2023

thought: Since we cannot retrieve the last page's duration and we decided to not create a new form of page view, what about sending a page view event with pageId: EndVisit or something similar to indicate that this was the last page to visit? I believe we have beacon to send events in the case of browser closing (feel free to correct me if I'm wrong), and if we can detect a user closing the browser/page and utilize the beacon, we would capture the last page's duration.

Had thought about this. From what I know, I see three main issues with this approach:

  1. In order to send this last page view event on user closing browser, we would need to listen for the beforeunload or unload event. Listening for both events is known to be unreliable (not supported by certain browsers) and can cause performance issues.

  2. Following (1), since the existing page view event is recorded on route change and unload event is fired whenever user navigates away from a page, we would not be able to distinguish and would end up duplicating page view events recorded (as a unload event occurs with every navigation between pages and so does a route change).
    We could have transition to capturing page view event exclusively when the page is navigated away from/closed by relying on the unload event, but this requires sendBeacon to work reliably. (refer to (3) on why it does not)

  3. To flush the event cache reliably on the user closing the page, we need to use sendBeacon. However, as noted here, we noticed ad blockers were preventing sendBeacon from functioning correctly. Therefore, users have an option to configure web client to send events using fetch or sendBeacon (both approaches have their downsides).
    If fetch is used, a situation can arise wherein the page has not fully unloaded before the fetch is completed and events are dispatched to the dataplane. As such any page view event that relies on listening for page unload may not get dispatched.

@adebayor123
Copy link
Member

question: How do we handle the case where the session is picked up from the cookie? For example, if user left the browser at page B, the web client may hold the "page.start" value in the cache, and when you move to subsequent pages, it may return an incorrect timeSpendOnParentPage information. If page.start is not stored in the cookie, what if the page manager's page.start is undefined? How do we handle this case?

@@ -136,6 +137,7 @@ export class PageManager {

private createNextPage(currentPage: Page, pageId: string) {
let startTime = Date.now();
const timeOnParentPage = startTime - currentPage.start;
Copy link
Member

@qhanam qhanam Jan 24, 2023

Choose a reason for hiding this comment

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

suggestion: timeOnParentPage is more concise. Use timeOnParentPage in the event schema instead of timeSpentOnParentPage.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated in revision and schemas

@@ -165,6 +167,7 @@ export class PageManager {
pageId,
parentPageId: currentPage.pageId,
interaction: currentPage.interaction + 1,
timeSpentOnParentPage: timeOnParentPage,
Copy link
Member

Choose a reason for hiding this comment

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

question: What happens when the page is resumed?

We may want to change the behavior so that page view events are not recorded for resumed pages. RUM users have been confused by seeing users navigate from "PageA" to "PageA" in user journeys.

Copy link
Member Author

Choose a reason for hiding this comment

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

Handled this case in revision. Page view events are no longer recorded for resumed pages if page ID is same. Start time from the Session cookie is used to compute the time on page if resumed page is different than when it was when session stopped.

@@ -8,6 +8,7 @@ export type Page = {
pageId: string;
interaction: number;
parentPageId?: string;
timeSpentOnParentPage?: number;
Copy link
Member

Choose a reason for hiding this comment

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

question: Is there a way we can omit timeOnParentPage from this structure? This is stored on browser storage, so if we don't need it, we should omit it.

I feel like it's something we can compute during recordPageView().

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in revision. Now its done during recordPageView

@ps863
Copy link
Member Author

ps863 commented Jan 25, 2023

question: How do we handle the case where the session is picked up from the cookie? For example, if user left the browser at page B, the web client may hold the "page.start" value in the cache, and when you move to subsequent pages, it may return an incorrect timeSpendOnParentPage information. If page.start is not stored in the cookie, what if the page manager's page.start is undefined? How do we handle this case?

Handled this in latest revision. The following change should handle the concerns you had:

  • If the resumed page Id is same as the page id stored when session temporarily stopped, then we don't dispatch a new Page View Event. Only the PageManager state is updated. The start time is updated as the time stored in the cookie (ie the start time is not reset like it was previously, and therefore the correct start time is recorded). Therefore when user moves to a new page, we will use the correct start time to compute time on page.

@ps863 ps863 merged commit d1c3b17 into aws-observability:main Feb 1, 2023
@ps863 ps863 mentioned this pull request Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants