fix!: disable VirtualPageLoadTimer by default#739
fix!: disable VirtualPageLoadTimer by default#739williazz merged 6 commits intoaws-observability:mainfrom
Conversation
docs/configuration.md
Outdated
| | eventCacheSize | Number | `1000` | The maximum number of events the cache can contain before dropping events. Each event is typically 1KB in size, so we recommend a default limit of 1000 events -> 1 MB to balance performance against capturing all observed events. If necessary, feel free to enable debug mode to get detailed logs on how to optimize cache size depending on your application behavior. | | ||
| | sessionLengthSeconds | Number | `1800` | The duration of a session (in seconds). | | ||
| | headers | Object | `{}` | The **headers** configuration is optional and allows you to include custom headers in an HTTP request. For example, you can use it to pass `Authorization` and `x-api-key` headers.<br/><br/>For more details, see: [MDN - Request Headers](https://developer.mozilla.org/en-US/docs/Glossary/Request_header). | | ||
| | legacySPASupport | Boolean | `false` | When this field is `true`, the web client will use legacy virtual page load timing for single page applications. This feature tracks HTTP requests and DOM mutations to determine when a route change is complete. **Warning:** This is a legacy feature that is no longer supported. Please enable with caution in debugging mode to see if it is relevant for your web application. | |
There was a problem hiding this comment.
suggestion: we can mention the issues with SPA page load timing #723
Also might be good to mention that when legacySPASupport is false, web client does not send performance_navigation_event for SPA
| | eventCacheSize | Number | `1000` | The maximum number of events the cache can contain before dropping events. Each event is typically 1KB in size, so we recommend a default limit of 1000 events -> 1 MB to balance performance against capturing all observed events. If necessary, feel free to enable debug mode to get detailed logs on how to optimize cache size depending on your application behavior. | | ||
| | sessionLengthSeconds | Number | `1800` | The duration of a session (in seconds). | | ||
| | headers | Object | `{}` | The **headers** configuration is optional and allows you to include custom headers in an HTTP request. For example, you can use it to pass `Authorization` and `x-api-key` headers.<br/><br/>For more details, see: [MDN - Request Headers](https://developer.mozilla.org/en-US/docs/Glossary/Request_header). | | ||
| | legacySPASupport | Boolean | `false` | When this field is `true`, then legacy NavigationEvents with initiatorType=route_change will be recorded for single page applications. **Warning:** This is a legacy feature that is no longer supported. Please enable with caution in debugging mode first to see if it is relevant for your web application before enabling it in prod. See [#723](https://github.com/aws-observability/aws-rum-web/issues/723) for more details. | |
There was a problem hiding this comment.
question: Right now we record navigations in SPA out of the box with no config to guard it. When we do implement SPA navigation recording correctly, is the plan to put a config (something like spaSupport) and default it to true? In that scenario, we will just need to make sure that it overrides the legacy setting you have here and logs a warning if the user sets both to true.
There was a problem hiding this comment.
Right now, we record page views when pushState or replaceState are used AND the relative page path has changed. When we migrate to framework-specific SDKs, then we can disable all legacy SPA support from PageViewPlugin, and only allow MPA support.
src/sessions/PageManager.ts
Outdated
| 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' |
There was a problem hiding this comment.
comment: Should this message be a bit more explicit about the implication of it being enabled?
For example, "Keeping this enabled will result in inaccurate page load timing with single page applications"
I know you have linked the github issue, but perhaps something more explicit can make it clearer to the users of the implication of this and get them to disable it.
src/orchestration/Orchestration.ts
Outdated
| headers?: HeaderBag; | ||
| enableW3CTraceId: boolean; | ||
|
|
||
| // Deprecated |
There was a problem hiding this comment.
nit: Not sure if the comment here is necessary. The class is deprecated, not the config
There was a problem hiding this comment.
i think it's worth to describe that the feature itself is deprecated. clarified the wording
|
comment: We will need to be very clear about this change in the change log given that users might be surprised to see what they think is missing telemetry after a minor version bump |
3 participants
BREAKING CHANGE: we are disabling the VirtualPageLoadTimer by default because it is irrelevant for many single page applications.
Summary
Resolves #723
The VirtualPageLoadTimer makes the PerformanceNavigationDuration metric unusable for many single page applications. The strategy to look for quiet windows makes a few big assumptions.
If all assumptions are correct, then it will record a decent virtual page load time that does not include DOM processing time. But if any assumption is incorrect, then it is likely to report irrelevant information and pollute the PerformanceNavigationDuration metric.
Implementation
Hide VirtualPageLoadTimer behind new configuration
legacySPASupport, which is disabled by default. When we migrate to full framework-specific SDKS, then we can put other SPA related logic behind this configuration as well.Backwards compatibility
This change does not affect PerformanceNavigationDuration for Multi-Page Applications.
Future
We need to make framework-specific page load timers based on view lifecycles.
Testing
Updated unit tests and locally tested.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.