getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.9k stars 1.56k forks source link

Asynchronous Loading and Capturing Errors #169

Closed karolisdzeja closed 6 years ago

karolisdzeja commented 10 years ago

I want to be able to load raven.js asynchronously, but still be able to capture errors while the script is loading. (Something like how Google Analytics handles events by storing them in a variable until the library loads).

Here's what I have so far: https://gist.github.com/karolisdzeja/8010574

But doing it that way loses some of the information and detail that Raven usually provides. Is there a way to store the full error information similarly to this?

PatrickJS commented 9 years ago

+1

nathmisaki commented 8 years ago

+1

Sija commented 8 years ago

+1

sodevious commented 8 years ago

+1

benvinegar commented 8 years ago

Everyone who's commented on this – what's wrong with the solution by @karolisdzeja?

Ultimately, I'm not sure how we can add a feature to the Raven.js source that is supposed to work when Raven.js source isn't on the page. I think this will always ultimately be a custom solution; at best we could add a "how to" to our docs.

jwheare commented 8 years ago

@benvinegar the solution looks fine, but it would be better if this was officially supported and documented. I'm happier just having to trust the Sentry team over evaluating a random gist.

jwheare commented 8 years ago

Actually, a much better solution would be something like Twitter's JS SDK code: https://dev.twitter.com/web/javascript/loading

Setup a function queue on page load that then gets consumed when the external js has been loaded, replacing the proxy object. And make sure all API calls go through something like a .ready() call to the proxy.

This ensures that any call can be queued up before the js has loaded rather than just captureMessage, without having to proxy every function individually.

I'd love to be able to just load raven.js asynchronously/deferred and not have to worry.

jwheare commented 8 years ago

Other problems with the gist: it clobbers window.onerror and introduces a few uncontained global variables.

jwheare commented 8 years ago

It also doesn't make use of the full-featured traceKitWindowOnError function that raven.js uses when it loads.

oroce commented 7 years ago

I slightly redid the gist above: https://gist.github.com/oroce/ec3786ba7eff59963842220c3ffc56b4

There's no leaking variable. I kept the window.onerror handler, but feel free to use window.addEventListener('error', fn).

The biggest help at point be having traceKitWindowOnError as an exported function from Raven. Since that's the function which gets called when an error happens: https://github.com/getsentry/raven-js/blob/master/dist/raven.js#L2074

I know we wouldn't have that very proper stacktrace but we would have something better than what we have now.

benvinegar commented 7 years ago

There's other downsides with doing this:

So, you're trading potentially better performance for lower quality error reports. If async execution is important, I'd sooner recommend you bundle Raven with your own code so that it is served together.

oroce commented 7 years ago

@benvinegar you are completely right. In applications which aren't public (aka google won't reach the pages) the classic (blocking) raven way is completely fine but as soon as you have a public facing site where the google page insight points do matter, we need to optimize how we load third party code (this is the price we are willing to pay in favour of ux, speed and better search result position).

Moreover bundling raven into our bundle is a solution, but as soon as you get into optimizing your frontend code with above the fold optimizations, like splitting your bundle into multiple ones using tools like factor-bundle or you include multiple bundles to gain more speed, the above solution can be a better one imo, but I'm open for suggestions.

vinhlh commented 7 years ago

They all have trade-offs, so it would be great if we can document all available strategies, so depend on per specific web application, we will apply different strategies. With async strategy, we should load the script after onload event, instead of loading async only, to prevent blocking onload event, rendering.

/**
 * Setup Js error lazy tracking
 * - Pros: doesn't block rendering, onload event
 * - Cons: lower quality error reports for lazy errors
 *
 * @author vinhlh
 *
 * @param  {object} window
 * @param  {object} labJs
 * @param  {string} ravenCdn
 * @param  {string} sentryDsn
 */
(function(window, labJs, ravenCdn, sentryDsn) {
  var errors = [];
  var oldOnError = window.onerror;

  window.onerror = function() {
    errors.push(arguments);
    oldOnError && oldOnError.apply(this, arguments);
  };
  window.addEventListener('load', function() {
    labJs
      .script(ravenCdn)
      .wait(function() {
        window.onerror = oldOnError;
        Raven.config(sentryDsn).install();
        errors.forEach(function(args) {
          window.onerror.apply(this, args);
        });
      });
  });
})(window, $LAB, 'raven-js-3.8.1/dist/raven.js', 'https://xxx@domain.com/9');
benvinegar commented 7 years ago

I think we'll probably just document an async snippet, like the ones provided above, but mention that it comes with tradeoffs.

Just one other comment. Those tradeoffs might seem acceptable, but I deal with a lot of support tickets from users about low-fidelity errors they are experiencing that are (incorrectly) believed to derive from Raven.js. My fear is that if I encourage people to use the async approach, I'll have more and more people asking me "why is there no stack trace" and other complaints when it's because this approach is lower fidelity. I'm willing to accept that, but it's a tough pill to swallow. 😓

oroce commented 7 years ago

@benvinegar I totally get where you are coming from, I know what I'm doing so I don't expect stacktrace when there shouldn't be. For newcomers, less experienced ones or just users with different usecase it can be leading for sure.

benvinegar commented 7 years ago

@oroce – yeah, this is 100% not a concern with people in this thread, but people who might pursue this strategy without understanding the caveats properly (e.g. just copy/pasting).

I'll keep this issue open, with a plan to add the snippet to the Install docs – and I'll put a bunch of warnings all over the place.

Thanks again for your participation here / convincing me to do this.

kl0tl commented 7 years ago

Here’s the snippet we use to queue calls to Raven methods transparently: https://gist.github.com/Kl0tl/ed0a9e74462a2294f4c8842f5389d8ea.

The mock could certainly be improved but we don’t need to replicate more functionalities. Object.defineProperty allows us to hook right after Raven attaches itself to the window but maybe the script load event is enough. Would be nice to have an official way to enable this.

zanona commented 7 years ago

Hey guys, just wondering if there is anything wrong with the way Raygun does that asynchronously? I am not sure, but it seems to handle edge cases well? I might be wrong though :)

mqklin commented 7 years ago

@Kl0tl very nice, thank you

pladaria commented 7 years ago

This is very simple using a dynamic import. Still in stage3 but supported by webpack.

We simply use the promise as a queue. Once fullfiled all callbacks are executed in order.

const RavenPromise = import('raven-js'); // async load raven bundle

// initial setup
RavenPromise.then(Raven => {
    Raven.config('url-to-sentry', options).install();
}):

// exported log function
export const logMessage = (level, logger, text) => {
    RavenPromise.then(Raven => {
        Raven.captureMessage(text, {level, logger});
    });
};
kireerik commented 6 years ago

Similarly to @zanona I would also prefer to have a simple tracking code like Raygun or Google Analytics has. Here is an example for analytics.js:

<script async src="https://www.google-analytics.com/analytics.js"></script>
<script>
    window.ga = window.ga || function () {
        (ga.q = ga.q || []).push(arguments)
    }
    ga.l = +new Date

    ga('create', 'UA-XXXXX-Y', 'auto')
    ga('send', 'pageview')
</script>

@benvinegar Does something like this possible with Raven.js? Maybe in the future?

kamilogorek commented 6 years ago

@kireerik it definitely will be implemented (most likely as a documentation how-to), but I cannot give you an accurate date estimate now.

kireerik commented 6 years ago

@kamilogorek Sounds great (I don't like the workaround as a solution). No problem!

maxmilton commented 6 years ago

For anyone interested, I've put up a gist of yet another way to load ravenjs asynchronously: https://gist.github.com/MaxMilton/e2338b02b7381fc7bef2ccd96f434201

It's based on the code by @oroce but with the key differences being I use a regular <script async src'='..."> tag in the document head for better performance (browsers can schedule fetching the resource earlier) + I don't bother wrapping it in an IIFE and other little tweaks.

kireerik commented 6 years ago

@MaxMilton Nice one! I have created my own flavor based on yours:

<script async src="https://cdn.ravenjs.com/3.22.1/raven.min.js" crossorigin="anonymous" id="raven"></script>
<script>
    (function (sentryDataSourceName) {
        var raven = document.getElementById('raven')
        , isNotLoaded = true
        , errors = []
        raven.onreadystatechange = raven.onload = function () {
            if (isNotLoaded) {
                Raven.config(sentryDataSourceName).install()
                isNotLoaded = !isNotLoaded
                errors.forEach(function (error) {
                    Raven.captureException(error[4] || new Error(error[0]), {
                        extra: {
                            file: error[1]
                            , line: error[2]
                            , col: error[3]
                        }
                    })
                })
            }
        }
        window.onerror = function (message, source, lineNumber, colmnNumber, error) {
            if (isNotLoaded)
                errors.push([message, source, lineNumber, colmnNumber, error])
        }
    })('https://<key>@sentry.io/<project>')
</script>

I also have some question:

What do you think? What is the author's (@kamilogorek) opinion on this?

maxmilton commented 6 years ago

@kireerik when you put crossorigin="anonymous" on scripts it allows you to fully capture errors (from that external script) with the window.onerror event. It also prevents the browser from sending credentials with the fetch request, which is typically what you want with 3rd party resources. MDN reference 1, MDN reference 2.

You can just pass the error and it will work most of the time. The caveat being old browsers (e.g. Firefox before version 31) don't pass the columnNo or Error Object properties to the window.onerror event. So if you want really good compatibility then you need to do that extra bit. MDN reference.

EDIT: Bonus tip: turns out when you put crossorigin without any value it's treated the same as crossorigin="anonymous".

maxmilton commented 6 years ago

FYI, I've updated my previous gist to something that's much more production ready:

See the gist if you want to understand everything OR if you prefer a quick copy+paste, here's the minified version:

<!-- Sentry JS error tracking -->
<script async src="https://cdn.ravenjs.com/3.22.0/raven.min.js" crossorigin id="raven"></script>
<script>
(function(b,e,c,d){b.onerror=function(a,b,d,f,g){c||e.push([a,b,d,f,g])};b.onunhandledrejection=function(a){c||e.push([a.reason.reason||a.reason.message,a.type,JSON.stringify(a.reason)])};d.onreadystatechange=d.onload=function(){c||(c=!0,
Raven.config("___PUBLIC_DSN___").install(),
b.onunhandledrejection=function(a){Raven.captureException(Error(a.reason.reason||a.reason.message),{extra:{type:a.type,reason:JSON.stringify(a.reason)}})},e.forEach(function(a){Raven.captureException(a[4]||Error(a[0]),{extra:{file:a[1],line:a[2],col:a[3]}})}))}})(window,[],!1,document.getElementById("raven"));
</script>

<link rel="preconnect" href="https://sentry.io">

Replace ___PUBLIC_DSN___ with your DSN and paste this somewhere in the head near your closing </head> tag. Or if you're a hipster who doesn't use <head> and <body> tags anymore then just paste it near the top after any critical/app resources (e.g. CSS). Ideally it should be before any other JavaScript so you can capture errors from scripts loaded after this code.

In my quick tests there hasn't been any issues so I don't see any reason not to use this over the default synchronous version.

If anyone has an idea for a better approach I'm keen to hear it.

Edit: Sorry for editing this comment so many times. It's at a stable level now. Was fun getting it to this state! :smiley:

kamilogorek commented 6 years ago

https://docs.sentry.io/clients/javascript/install/#async-loading

dalyr95 commented 6 years ago

Once the sentry library is loaded, error reporting quality is exactly the same as loading it sync? (I assume so, just checking)

dalyr95 commented 6 years ago

Also in the docs you might want to add, you can't use Raven till the lib is loaded, maybe provide a callback function in the options so you can set user context etc?

lili21 commented 6 years ago

agree with @dalyr95 . a callback function would be useful. maybe a custom raven load event.

xt0rted commented 6 years ago

I have a similar request as @dalyr95. Right now the only way to call setUserContext() is to modify the loader snippet which isn't as clean as being able to pass in a callback on the main config object.

kamilogorek commented 6 years ago

Hello, thanks for reporting the issue.

We are in the process of working on the next major release of our SDK. Because of that, we had to put working on the current version (except major or security bugs) on hold. We'll try to get back to all the reports as soon as possible, so please be patient.

Thanks for your understanding, Cheers!

xt0rted commented 6 years ago

My solution was to add 'undefined'!=k.setup&&k.setup(); immediately after the call to .install(), then I added a function called setup to SENTRY_SDK with my post init code.

danielcompton commented 6 years ago

With the async loader, I was able to set the user context and other info by passing it as a second argument to Raven.config:

<script>
    Raven.config("https://<mydsn>@sentry.io/<projectid>", 
      {"release":"0.3.1",
       "environment":"dev",
       "user": {"id":"7b031fa0-32ff-46fe-b94b-e6bc201c3c5f",
                "organisation-id":"b1a50c41-b85e-4c50-9cec-c55ff36cf6d1"}}).install();
</script>

I think everything already exists for this, it just could be better documented.

lili21 commented 6 years ago

@danielcompton can only get user information through backend api ?