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

fix: Handle missing PageId in metadata when page is resumed #388

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

ps863
Copy link
Member

@ps863 ps863 commented Mar 16, 2023

This change contains a patch based on customer reported issue, wherein, pageId was missing from the metadata of certain events.

In a previous change we changed the behavior of when a Page View event is recorded. In cases where page views resume (this includes resumed sessions and page being reloaded) but page ID is same were changed from being recorded as a separate page view event for each reload/resume to only creating a new page view event only when there was a change in the pageId. However, the change failed to address the following:

  1. In the case when the PageManager's state is restored after being resumed (using the page object stored in cookie), the cookie does not contain PageManager managed attributes which are added as metadata to all events. This means, the PageManager's attributes are not updated from what they were previously and remain null. Therefore, after a page is restored or reloaded, subsequent events don't contain the pageId in the metadata (since pageId is obtained from the PageManager). The pageId is restored in metadata only after a new page view event is generated as that forces the PageManager to update the metadata attributes.

This change addresses this by:

  1. Updating the PageManager's attributes when a page is resumed as well.
  2. Adding Unit tests to capture this behavior

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

@@ -108,6 +108,13 @@ export class PageManager {
} else if (this.page.pageId !== pageId) {
this.createNextPage(this.page, pageId);
} else {
Copy link
Member

@adebayor123 adebayor123 Mar 16, 2023

Choose a reason for hiding this comment

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

Nevermind - it seems we want to short circuit

);
});

test('when session resumes and page is manually recorded with custom page attributes then custom page attributes are restored', async () => {
Copy link
Member

@adebayor123 adebayor123 Mar 16, 2023

Choose a reason for hiding this comment

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

Synced offline

src/sessions/PageManager.ts Outdated Show resolved Hide resolved
Co-authored-by: Quinn Hanam <qhanam@gmail.com>
@ps863 ps863 merged commit f81bcf2 into aws-observability:main Mar 16, 2023
ps863 added a commit to ps863/aws-rum-web that referenced this pull request Mar 17, 2023
…rvability#388)



Co-authored-by: Quinn Hanam <qhanam@gmail.com>

---------

Co-authored-by: Quinn Hanam <qhanam@gmail.com>
ps863 added a commit that referenced this pull request Mar 17, 2023

Co-authored-by: Quinn Hanam <qhanam@gmail.com>

---------

Co-authored-by: Quinn Hanam <qhanam@gmail.com>
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.

3 participants