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.

Sign up for GitHub

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

Jump to bottom

use sendBeacon for exporting when page is hidden #4619

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

use sendBeacon for exporting when page is hidden #4619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 18 additions & 21 deletions experimental/packages/otlp-exporter-base/src/platform/browser/OTLPExporterBrowserBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,27 +29,22 @@ export abstract class OTLPExporterBrowserBase<
ExportItem,
ServiceRequest,
> extends OTLPExporterBase<OTLPExporterConfigBase, ExportItem, ServiceRequest> {
protected _headers: Record<string, string>;
private _useXHR: boolean = false;
protected _headers: Record<string, string> = {};
private _alwaysUseXhr: boolean = false;

/**
* @param config
*/
constructor(config: OTLPExporterConfigBase = {}) {
super(config);
this._useXHR =
!!config.headers || typeof navigator.sendBeacon !== 'function';
if (this._useXHR) {
this._headers = Object.assign(
{},
parseHeaders(config.headers),
baggageUtils.parseKeyPairsIntoRecord(
getEnv().OTEL_EXPORTER_OTLP_HEADERS
)
);
} else {
this._headers = {};
}
this._alwaysUseXhr =
config.alwaysUseXhr || typeof navigator.sendBeacon !== 'function';

this._headers = Object.assign(
{},
parseHeaders(config.headers),
baggageUtils.parseKeyPairsIntoRecord(getEnv().OTEL_EXPORTER_OTLP_HEADERS)
);
}

onInit(): void {}
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

can we move the 64 * 1024 to a constant?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

fetch API with the keep-alive flag enabled

MDN says that's not supported by firefox

sendWithBeacon(
body,
this.url,
this._headers,
this.timeoutMillis,
{ type: 'application/json' },
resolve,
reject
);
} else {
sendWithBeacon(
sendWithXhr(
body,
this.url,
{ type: 'application/json' },
this._headers,
this.timeoutMillis,
resolve,
reject
);
Expand Down
1 change: 1 addition & 0 deletions experimental/packages/otlp-exporter-base/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

}