Skip to content

add beacon transport #1498

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

Merged
merged 1 commit into from
Aug 31, 2018
Merged

add beacon transport #1498

merged 1 commit into from
Aug 31, 2018

Conversation

tvanier
Copy link
Contributor

@tvanier tvanier commented Aug 30, 2018

Provide a new transport based on the browser Beacon API, supported in all major browsers except IE (caniuse). Using navigator.sendBeacon instead of fetch or XMLHttpRequest could help sending reports at unload time, and possibly allow for bigger reports to be sent.
This pull request adds BeaconTransport to the browser package, leaving it as an opt-in:

import { init, Transports } from '@sentry/browser';

init({
  dsn: '__DSN__',
  transport: Transports.BeaconTransport
});

If working good, beacon transport could be the default in the future? In packages/browser/src/backend.ts:

import { supportsBeacon, supportsFetch } from '@sentry/utils/supports';
import { BeaconTransport, FetchTransport, XHRTransport } from './transports';

const transportOptions = {
  dsn: this.options.dsn,
  ...this.options.transportOptions
};

let transportClass;
if (this.options.transport)
  { transportClass = this.options.transport; }
else if (supportsBeacon())
  { transportClass = BeaconTransport; }
else if (supportsFetch())
  { transportClass = FetchTransport; }
else
  { transportClass = XHRTransport; }

const transport = new transportClass(transportOptions);

BTW does transport needs to be instantiated at every sendEvent?

@tvanier tvanier requested a review from kamilogorek as a code owner August 30, 2018 23:11
@codecov-io
Copy link

codecov-io commented Aug 30, 2018

Codecov Report

Merging #1498 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1498      +/-   ##
==========================================
+ Coverage   72.55%   72.61%   +0.06%     
==========================================
  Files          41       41              
  Lines        1246     1249       +3     
  Branches      238      239       +1     
==========================================
+ Hits          904      907       +3     
  Misses        333      333              
  Partials        9        9
Impacted Files Coverage Δ
packages/utils/src/supports.ts 46.34% <100%> (+4.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f3bdca...f98481b. Read the comment docs.

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

TIL Beacon API 😅 🙈

This is super nice, thanks for that.
Also, you are right, no need creating a new transport instance every time, I will change that.

@HazAT HazAT merged commit d54c762 into getsentry:master Aug 31, 2018
@tvanier
Copy link
Contributor Author

tvanier commented Sep 4, 2018

thanks @HazAT , do we know if this will be included in the next 4.0.0 (beta) release? Any tentative date?

@tvanier tvanier deleted the beacon-transport branch September 4, 2018 17:25
@HazAT
Copy link
Member

HazAT commented Sep 5, 2018

@tvanier Preparing an RC release today with beacon as the default transport 😄

@yangdan8
Copy link

Why did you cancel beacon from version 5.0?

@kamilogorek
Copy link
Contributor

Because it was partially broken, see: #1952

Starting in Chrome 59, this method cannot send a Blob whose type is not CORS safelisted. This is a temporary change until a mitigation can be found for the security issues that this creates. For more information see Chrome bug 720283.

But I see it has been fixed somewhere in 2020, so we may think again about reintroducing it (which is still not trivial, as old chrome versions would still break anyway).

@tvanier
Copy link
Contributor Author

tvanier commented Aug 27, 2021

It has been a while since I worked with the Beacon API, I was wondering if, knowing the Chrome change was likely to be temporary, the code for beacon transport could have been kept, changing just supportsBeacon? something like (untested)

export function supportsBeacon(): boolean {
  const global = getGlobalObject();
  if ('navigator' in global && 'sendBeacon' in global.navigator) {
    try {
      const blob = new Blob([JSON.stringify('{}')], {type : 'application/json'});
      global.navigator.sendBeacon('/', blob); // or any better url
      // no exception
      return true;
    } catch {
      // exception in Chrome 59
    }
  }
  return false;
}

Or just return false until we figure out?

@kamilogorek
Copy link
Contributor

We'll rethink the beacon transport once we get sessions, tracing and performance measurements sorted out for sure, as it'll be really useful there. For now, we don't want to add more bundle size for features that only few people might make use of.

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.

5 participants