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

Add options to disable timer and event target instrumentation #938

Merged

Conversation

brentvatne
Copy link
Contributor

Within react-native, it doesn't make sense for us to instrument timers because they are already wrapped in try/catch and their errors are reported to ErrorUtils. Additionally, if we don't disable the timer instrumentation then the error will be handled separately by ErrorUtils and the try/catch wrapper. This lead to, in my case, fatal errors being reported immediately (rather than on next load) and not having the correct metadata on Android because I wasn't able to modify the stack before passing it through TraceKit.

As for event targets, it doesn't seem necessary within the React Native context where these mostly don't exist -- I imagine people would want this off by default in RN.

See the related PR to react-native-sentry: getsentry/sentry-react-native#40

@HazAT
Copy link
Member

HazAT commented Apr 25, 2017

LGTM 👍
But I think @benvinegar or @LewisJEllis should give their final seal of approval.

@benvinegar
Copy link
Contributor

benvinegar commented Apr 25, 2017

I don't want to have 3 (or more) instrumentX config options, so I'd rather do this as a higher-level config option, e.g.

Raven.config('your-dsn', {
  instrument: {
    timers: false,
    eventTargets: false,
    http: false // XMLHttpRequest/fetch - better name welcome
  }
});

... kind of like how we've done with autoBreadcrumbs.

@brentvatne
Copy link
Contributor Author

That makes sense @benvinegar! I'll send PR with this change shortly

@brentvatne
Copy link
Contributor Author

@benvinegar - It seems like xhr (or http as you put it in example) instrumentation config lives on autoBreadcrumbs already. Should I leave it out of the instrument config then?

@benvinegar
Copy link
Contributor

benvinegar commented Apr 25, 2017

It still instruments with try/catch (or at least, it should).

You don't have to implement the option: I just want the API to have a place to expand to when we introduce it in the future.

@brentvatne
Copy link
Contributor Author

brentvatne commented Apr 25, 2017

@benvinegar - updated, let me now if that works for you!

edit: awkward wording, I did actually test it :P I meant if the change is OK for you

@benvinegar
Copy link
Contributor

Looks p good, I think last thing is I'd add an integration test that verifies the built-ins haven't been overridden. Our integration tests are pretty wacky, so I don't mind putting that part together.

@brentvatne
Copy link
Contributor Author

@benvinegar - would be great if you could -- I'd like to get this landed as soon as possible so Expo users can use Sentry easily so if it will take you some time I can dig into it

@brentvatne
Copy link
Contributor Author

ping @benvinegar :)

@benvinegar
Copy link
Contributor

Hey, I'll try to investigate this tomorrow.

@brentvatne
Copy link
Contributor Author

happy monday @benvinegar!

@benvinegar
Copy link
Contributor

happy monday @benvinegar!

@benvinegar
Copy link
Contributor

Looks like editing the integration tests is too tricky because you can't set a config object as-is ... I'd have to completely re-work them.

If you could quickly add some cursory tests that verify the instrument config works as expected, that's good enough for me. Here's an example of what that looks like for autoBreadcrumbs.

@brentvatne
Copy link
Contributor Author

cool, thanks! done

@benvinegar
Copy link
Contributor

I guess the last thing is: doing {instrument: { eventTargets: false }} will also disable breadcrumb recording.

As for event targets, it doesn't seem necessary within the React Native context where these mostly don't exist -- I imagine people would want this off by default in RN.

Should we re-evaluate this assertion?

@benvinegar
Copy link
Contributor

Is this something we could discuss in the Expo slack channel?

@brentvatne
Copy link
Contributor Author

@benvinegar - yeah let's do that

@brentvatne brentvatne force-pushed the @brent/instrumentation-options branch from 0772c5a to fd85919 Compare May 2, 2017 19:12
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.

Documentation needs some expanding but I can do this asynchronously.

@benvinegar benvinegar merged commit 5785911 into getsentry:master May 2, 2017
@benvinegar
Copy link
Contributor

@LewisJEllis @MaxBittker – could one of you publish 3.15.0?

@LewisJEllis
Copy link
Contributor

Published 3.15.0, see #955.

@brentvatne
Copy link
Contributor Author

🎸

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.

4 participants