-
Notifications
You must be signed in to change notification settings - Fork 85
fix!: disable VirtualPageLoadTimer by default #739
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
Changes from 4 commits
fb33ffa
7d1c6f1
36e6089
c188919
91789e2
4c2d687
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,6 +98,7 @@ export const defaultConfig = (cookieAttributes: CookieAttributes): Config => { | |
| useBeacon: true, | ||
| userIdRetentionDays: 30, | ||
| enableW3CTraceId: false, | ||
| legacySPASupport: false, // deprecated | ||
| ...internalConfigOverrides | ||
| }; | ||
| }; | ||
|
|
@@ -174,6 +175,9 @@ export interface Config { | |
| alias?: string; | ||
| headers?: HeaderBag; | ||
| enableW3CTraceId: boolean; | ||
|
|
||
| // Deprecated | ||
|
||
| legacySPASupport: boolean; | ||
| } | ||
|
|
||
| export interface PartialConfig | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,7 +50,7 @@ export class PageManager { | |
| private page: Page | undefined; | ||
| private resumed: boolean; | ||
| private attributes: Attributes | undefined; | ||
| private virtualPageLoadTimer: VirtualPageLoadTimer; | ||
| private virtualPageLoadTimer: VirtualPageLoadTimer | undefined; | ||
| private TIMEOUT = 1000; | ||
| private timeOnParentPage: number | undefined; | ||
|
|
||
|
|
@@ -68,11 +68,18 @@ export class PageManager { | |
| this.page = undefined; | ||
| this.resumed = false; | ||
| this.recordInteraction = false; | ||
| this.virtualPageLoadTimer = new VirtualPageLoadTimer( | ||
| this, | ||
| config, | ||
| record | ||
| ); | ||
| if (config.legacySPASupport) { | ||
| if (config.debug) { | ||
| InternalLogger.warn( | ||
| 'VirtualPageLoadTiming (deprecated) is enabled. Please use with caution after reviewing https://github.com/aws-observability/aws-rum-web/issues/723' | ||
|
||
| ); | ||
| } | ||
| this.virtualPageLoadTimer = new VirtualPageLoadTimer( | ||
| this, | ||
| config, | ||
| record | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| public getPage(): Page | undefined { | ||
|
|
@@ -99,28 +106,38 @@ export class PageManager { | |
| pageId = payload.pageId; | ||
| } | ||
|
|
||
| if (this.config.debug) { | ||
| InternalLogger.info(`recordPageView called with pageId: ${pageId}`); | ||
| } | ||
|
|
||
| if (this.useCookies()) { | ||
| this.recordInteraction = true; | ||
| } | ||
|
|
||
| if (!this.page) { | ||
| this.createLandingPage(pageId); | ||
| if (this.config.debug) { | ||
| InternalLogger.info(`Landing page recorded: ${pageId}`); | ||
| InternalLogger.info(`Landing page created: ${pageId}`); | ||
| } | ||
| } else if (this.page.pageId !== pageId) { | ||
| this.createNextPage(this.page, pageId); | ||
| if (this.config.debug) { | ||
| InternalLogger.info(`Navigation to page: ${pageId}`); | ||
| InternalLogger.info(`Navigation to new page: ${pageId}`); | ||
| } | ||
| } else if (this.resumed) { | ||
| if (this.config.debug) { | ||
| InternalLogger.info(`Resumed session for page: ${pageId}`); | ||
| } | ||
| // Update attributes state in PageManager for event metadata | ||
| this.collectAttributes( | ||
| this.page as Page, | ||
| typeof payload === 'object' ? payload : undefined | ||
| ); | ||
| return; | ||
| } else { | ||
| if (this.config.debug) { | ||
| InternalLogger.info(`No page change detected for: ${pageId}`); | ||
| } | ||
| // The view has not changed. | ||
| return; | ||
| } | ||
|
|
@@ -139,7 +156,14 @@ export class PageManager { | |
|
|
||
| private createNextPage(currentPage: Page, pageId: string) { | ||
| let startTime = Date.now(); | ||
| const interactionTime = this.virtualPageLoadTimer.latestInteractionTime; | ||
|
|
||
| if (this.config.debug) { | ||
| InternalLogger.info( | ||
| `Creating next page ${pageId}, interaction: ${ | ||
| currentPage.interaction + 1 | ||
| }` | ||
| ); | ||
| } | ||
|
|
||
| // The latest interaction time (latest) is not guaranteed to be the | ||
| // interaction that triggered the route change (actual). There are two | ||
|
|
@@ -160,9 +184,24 @@ export class PageManager { | |
| // | ||
| // We do not believe that case (2) has a high risk of skewing route | ||
| // change timing, and therefore ignore case (2). | ||
| if (!this.resumed && startTime - interactionTime <= this.TIMEOUT) { | ||
| startTime = interactionTime; | ||
| this.virtualPageLoadTimer.startTiming(); | ||
| if (this.config.legacySPASupport && this.virtualPageLoadTimer) { | ||
| const interactionTime = | ||
| this.virtualPageLoadTimer.latestInteractionTime; | ||
| if (!this.resumed && startTime - interactionTime <= this.TIMEOUT) { | ||
| startTime = interactionTime; | ||
| this.virtualPageLoadTimer.startTiming(); | ||
| if (this.config.debug) { | ||
| InternalLogger.info( | ||
| `Started virtual page load timing for ${pageId}` | ||
| ); | ||
| } | ||
| } else if (this.config.debug) { | ||
| InternalLogger.info( | ||
| `Skipped virtual page load timing for ${pageId} (resumed: ${ | ||
| this.resumed | ||
| }, timeDiff: ${startTime - interactionTime}ms)` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| this.timeOnParentPage = startTime - currentPage.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.
suggestion: we can mention the issues with SPA page load timing #723
Also might be good to mention that when
legacySPASupportis false, web client does not sendperformance_navigation_eventfor SPA