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

Encourage safe usage of Sentry #814

Closed
wants to merge 1 commit into from

Conversation

lightswitch05
Copy link

I have come across several websites the immediately fail due to blocking Sentry. Its especially bad since it is often within the first few lines of their javascript. Encourage safe checking of the Sentry object before using it.

Here are a couple documented websites that fail completely if Sentry does not load:

I have come across several websites the immediately fail due to blocking Sentry. Its especially bad since it is often within the first few lines of their javascript. Encourage safe checking of the `Sentry` object before using it.

Here are a couple documented websites that fail completely if Sentry does not load:

* lightswitch05/hosts#40
* lightswitch05/hosts#77
@untitaker
Copy link
Member

untitaker commented Mar 5, 2019

My personal opinion on this is that if you're going to mess with the internals of a site by blocking a certain domain, removing certain HTML tags or preventing certain functions from running, you should also be prepared to implement workarounds to prevent subsequent code from breaking. However, I don't think it's reasonable to ask authors of the scripts you're breaking to deploy safeguards against cases that don't occur in regular execution.

You could argue that Sentry might be undefined if cdn.ravenjs.com is down (which would imply that an entire CDN provider is down) or just unreachable due to other local network issues, but I am fairly certain that this PR or anything like it would have never been opened in a world without adblocking. Even for its original intent, this PR is insufficient. The Sentry object is used many more times in people's codebases, and wrapping each of these uses in that if-statement is not something we will encourage anybody to do. This entire charade is also completely unnecessary for users who vendor our SDK into their own JS bundle, where your hosts file will have no effect anyway.

TL;DR: Patching code at runtime to not do certain things is harder than going to the network tab and copypasting hostnames that look weird to you.

@untitaker untitaker closed this Mar 5, 2019
@lightswitch05
Copy link
Author

lightswitch05 commented Mar 5, 2019

Wow, I didn't expect this response. I thought showing safe usage check in the docs would have been a simple change, after all the goal is to track and fix errors and its just a documentation change. cdn.ravenjs.com being down would certainly cause the same error behavior. Anyways, thank you for your time reviewing the pull request

@untitaker
Copy link
Member

So this is not a new issue: getsentry/sentry-javascript#1421. In the linked issue you have a customer of Sentry saying that this way of using the SDK is inacceptable (wrapping everything with if(Sentry)) which implies to me that this has to be solved at a different level.

Another thread here: StevenBlack/hosts#568

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

Successfully merging this pull request may close these issues.

None yet

2 participants