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.

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

docs: Document collectWindowErrors option #1050

Closed
wants to merge 1 commit into from

Conversation

kamilogorek
Copy link
Contributor

Resolves #1049

docs/config.rst Outdated
@@ -145,6 +145,18 @@ Those configuration options are documented below:
includePaths: [/https?:\/\/getsentry\.com/, /https?:\/\/cdn\.getsentry\.com/]
}

.. describe:: collectWindowErrors

By default Raven collects all uncaught errors, that has been caught by `onerror` handler.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realised that uncaught/caught wording... would appreciate native speaker input.

Copy link

@crccheck crccheck Sep 23, 2017

Choose a reason for hiding this comment

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

"uncaught" sounds good to me, but if I were to tweak things, I would say:

By default, Raven collects all uncaught errors that have been caught by the onerror handler. You can disable this behaviour by setting this flag to false. This may be useful when you only want to manually handle errors by using the captureMessage and captureException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @crccheck!

@kamilogorek kamilogorek force-pushed the collect-window-errors-docs branch from b16e4db to be56c12 Compare September 25, 2017 09:46
Copy link
Contributor

@benvinegar benvinegar left a comment

Choose a reason for hiding this comment

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

So, in the interest of aligning APIs, have we considered what form this would take in raven-node? e.g. should this be renamed collectGlobalErrors? Note that we already have captureUnhandledRejections in raven-node as the closest comparable thing, so maybe this should be captureUnhandledGlobalErrors? Should it also prevent try/catch instrumentation? Not clear that this config option does (e.g. Raven may still wrap async functions and still collect global errors).

Don't need to rush this out, the behavior is already present by just not calling .install. Let's make sure we have the naming right first.

.. describe:: collectWindowErrors

By default, Raven collects all uncaught errors that have been caught by the `onerror` handler.
You can disable this behaviour by setting this flag to `false`. This may be useful when you only want to manually handle errors by using the `captureMessage` and `captureException`.
Copy link
Contributor

Choose a reason for hiding this comment

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

by using the captureMessage and captureException.

Drop unnecessary the. Should just be "by using captureMessage and captureException".

@kamilogorek kamilogorek requested a review from a team September 26, 2017 15:40
Copy link
Member

@mitsuhiko mitsuhiko left a comment

Choose a reason for hiding this comment

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

Just needs the sentence fix, otherwise good to go.

@benvinegar
Copy link
Contributor

@mitsuhiko re-read my comments, I think this has to be re-thought

@kamilogorek
Copy link
Contributor Author

There's similar issue in raven-node now – getsentry/raven-node#307

We have 3 separate calls during installation process:

process.on('uncaughtException', ...) // global error handler
process.on('unhandledRejection', ...) // global promises handler
instrumentor.instrument(..., ...) // global breadcrumbs handler

Only unhandledRejection is configurable by using captureUnhandledRejections.

However, for raven-js, we call:

TraceKit.report.subscribe // global error handler
self._instrumentTryCatch()  // global try/catch handler
self._instrumentBreadcrumbs() // global breadcrumbs handler

And only latter 2 are configurable through autoBreadcrumbs and instrument options.

So we have:

I'd love to unify both of those setups, eg.:
captureUncaughtExceptions/captureUncaughtErrors
captureUnhandledRejections
captureBreadcrumbs/autoBreadcrumbs
instrument + instrument as a dict (as it's done now for raven-js)

Thoughts? @getsentry/javascript

@kamilogorek
Copy link
Contributor Author

More ref: #782

@kamilogorek kamilogorek deleted the collect-window-errors-docs branch January 23, 2018 13:36
@sunknudsen
Copy link

Hey guys, trying to figure out how to disable Raven.js's onerror error handling to implement my own. The goal is to add extra data that isn't necessarily available at page load.

The following code snippet doesn't appear to work as two issues are reported (Raven.js's onerror and my own).

Raven.config('__DSN__', {
  allowDuplicates: true,
  collectWindowErrors: false
}).install();

Has collectWindowErrors been deprecated?

Also, is it possible to get Raven.js to report all xhr requests that return status codes in the 4** and 5**?

Thanks!

@danielhusar
Copy link

danielhusar commented Dec 6, 2018

Looks like collectWindowErrors doesn't work any more.
You need to edit defaultIntegrations now.
This is how i have disabled the global handlers:

import { init, Integrations } from '@sentry/browser';
import { Integrations as CoreIntegrations } from '@sentry/core';

init({
  defaultIntegrations: [
    new CoreIntegrations.Dedupe(),
    new CoreIntegrations.InboundFilters(),
    new CoreIntegrations.FunctionToString(),
    new Integrations.TryCatch(),
    new Integrations.Breadcrumbs(),
    new Integrations.LinkedErrors(),
    new Integrations.UserAgent(),
  ],
});

Maybe there is a way how to just remove one default integration but I havent found it.

@kamilogorek
Copy link
Contributor Author

kamilogorek commented Dec 7, 2018

@danielhusar it's right there in the docs https://docs.sentry.io/platforms/javascript/#removing-an-integration :)

Sentry.init({
  integrations(integrations) {
    return integrations.filter(i => i.name !== 'GlobalHandlers');
  }
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants