use sendBeacon for exporting when page is hidden #4619
Conversation
There was a problem hiding this comment.
Generally looks good % nit, likely needs a test update though.
pinging @MSNev, I seem to remember a discussion about this change a long while ago. I would appreciate your input 🙂
| @@ -69,20 +64,22 @@ export abstract class OTLPExporterBrowserBase< | |||
| const body = JSON.stringify(serviceRequest); | |||
|
|
|||
| const promise = new Promise<void>((resolve, reject) => { | |||
| if (this._useXHR) { | |||
| sendWithXhr( | |||
| if (!this._alwaysUseXhr && document.hidden && body.length < 64 * 1024) { | |||
There was a problem hiding this comment.
can we move the 64 * 1024 to a constant?
| sendWithXhr( | ||
| if (!this._alwaysUseXhr && document.hidden && body.length < 64 * 1024) { | ||
| // sendBeacon must be used when the page is being hidden, as per: https://w3c.github.io/beacon/. | ||
| // Note that headers are ignored in this case, if this is a problem, configuration option `alwaysUseXhr` must be set to true. |
There was a problem hiding this comment.
Hmm is this enough to add && Object.keys(this._headers).length == 0 to the if condition above? (Since it's not possible to know if a request header is required (eg. to pass auth tokens*) or optional)
(Alternatively could provide an option for user to pass their own conditional check so and have this+header len as sane default)
* - yeah there's better alternatives but sometimes people make bad decisions
There was a problem hiding this comment.
Additionally, "if" the page is unloading then you should use (if available) the fetch API with the keep-alive flag enabled (this still has the 64kb limit), but you can pass headers.
There was a problem hiding this comment.
fetch API with the keep-alive flag enabled
| @@ -69,20 +64,22 @@ export abstract class OTLPExporterBrowserBase< | |||
| const body = JSON.stringify(serviceRequest); | |||
|
|
|||
| const promise = new Promise<void>((resolve, reject) => { | |||
| if (this._useXHR) { | |||
| sendWithXhr( | |||
| if (!this._alwaysUseXhr && document.hidden && body.length < 64 * 1024) { | |||
There was a problem hiding this comment.
document.hidden doesn't always mean that the page is being navigated away from. Firefox will return true if the page is "covered" with a non-transparent window.
IMHO, this (should) use a combination of the page hide events visibilitystate, pagehide, beforeunload and (optionally) unload so that it can keep it's own state across all browsers. You could also use the document.visibilityState but this is also not supported on all browsers.
If your listening to events then we would also need to handle the "show" events to reverse the value.
| @@ -52,4 +52,5 @@ export interface OTLPExporterConfigBase { | |||
| /** Maximum time the OTLP exporter will wait for each batch export. | |||
| * The default value is 10000ms. */ | |||
| timeoutMillis?: number; | |||
| alwaysUseXhr?: boolean; | |||
There was a problem hiding this comment.
rather than expose a new API property called always (when it can't), I would prefer this to be an array of "transport" preferences...
As you may not be able to use XHR and you might want to use fetch instead (like in a worker).
Suggestion: Something like what we do in Application Insights
With the "available" transports here
This allows the user to "request" their preferred transport (sendBeacon, XHR, fetch) and the SDK will use the most appropriate transport for the given scenario. The difference between the transports and unloadTransports that we have is to allow the user to have a different preference once we detect that the page is navigating away (we listen only to the events)
Which problem is this PR solving?
Currently, the option to use xhr vs sendBeacon is left to the app developer when setting up the sdk. However, when a page is hidden we must use sendBeacon or otherwise risk losing the data. This PR makes this change, while still giving an option to the app developer to override and use xhr all the time.
Type of change
Please delete options that are not relevant.
Checklist: