Try catching JS errors and sending them to the API #1949

scruffian · 2015-12-24T00:18:01Z

This will catch JS errors in production and send them to the API.

TODOs

Disable it in the development environment
Restrict to only 1% of users.
Use uglifyjs to build the file.

To test, edit some code so it throws a JS error. You should see the API request happen in the Network tab.

To make sure you are included in logging add this to your local storage:
localStorage.setItem('log-errors', true);

props @drewblaisdell and @Tug.

dmsnell · 2015-12-24T00:24:54Z

On a related note, I just found http://yellerapp.com

blowery · 2015-12-24T18:35:35Z

public/catch-js-errors.js

+	// POST to the API
+	xhr.open( 'POST', 'https://public-api.wordpress.com/js-error', true );
+	xhr.setRequestHeader( 'Content-type', 'application/x-www-form-urlencoded' );
+	xhr.send( 'message=' + error.message + '&type=' + error.constructor && error.constructor.name + '&userAgent=' + navigator.userAgent + '&stack=' + error.stack );


should call encodeURIComponent on each of these parameters

would it not also make some sense to just send this all as structured data in the POST body? something like a JSON export or just the plain message from the exception?

artpi · 2015-12-29T16:53:18Z

Maybe we can try the service @dmsnell found? (also for 1%)
http://yellerapp.com/
Seems perfect for us!

artpi · 2016-01-05T11:03:32Z

Apparently there are other similar services:
https://trackjs.com/
https://errorception.com/
https://getsentry.com/for/javascript/

artpi · 2016-01-05T15:40:13Z

I made a PR connecting to one of these services.
It also tracks proper environment and user ID, so we know which users are affected:
#2107

apeatling · 2016-01-19T16:44:43Z

@scruffian What's the status of this one?

scruffian · 2016-01-19T17:11:15Z

@apeatling it's on my list for this week...

apeatling · 2016-01-19T17:12:30Z

👌

scruffian · 2016-01-22T00:08:49Z

@blowery would appreciate your thoughts on this...

nb · 2016-01-22T10:09:47Z

Makefile

@@ -148,12 +149,15 @@ build: install build-$(CALYPSO_ENV)

 build-css: public/style.css public/style-rtl.css public/style-debug.css public/editor.css

-build-development: build-server $(CLIENT_CONFIG_FILE) server/devdocs/search-index.js build-css
+build-inline-js:
+	@$(UGLIFYJS) -o public/catch-js-errors.js catch-js-errors.js


Do we need a separate build process for this one file? Can we fit into the current build process?

The current build process puts the files into one bundle. This needs to be a separate file for the reasons @blowery mentions here: #1868 (comment)

Even then, our current process supports building different chunks or we can make it build a separate file. The idea is to use the same process and tools like webpack, instead of using uglify on its own.

nb · 2016-01-22T10:21:20Z

Have you looked at the Sentry client for inspiration and probably some tricks?

blowery · 2016-01-22T12:52:02Z

catch-js-errors.js

+	localStorage.setItem( 'log-errors', true );
+
+	// set up handler to POST errors
+	window.onerror = debounce( function( message, scriptUrl, lineNumber, columnNumber, error ) {


need to check to see if window.onerror was already set and call the previous version after doing our work

Tug · 2016-01-22T14:51:23Z

Yep this is golden https://github.com/getsentry/raven-js/blob/master/src/raven.js#L635

I have noticed that window.onerror returns very little information and almost always errors have no stacktrace :(

blowery · 2016-01-22T15:25:08Z

Yep this is golden

Wow. That's.... wow. Much wow. Such danger. The noise must be incredible.

To elaborate, that code is replacing methods on Host objects. I would not screw with host objects, as the behavior of host objects is entirely undefined. https://es5.github.io/ has a lot of good info on all the crazy things host objects are allowed to do.

TL;DR: They can do anything they want.

Though I guess it's working for Raven?

Just gives me the willies.

rralian · 2016-01-28T14:07:55Z

One other thing we would want, but it can come separately if it doesn't make sense here, is some frequency of error events. I understand the extended mobile endpoint will do some throttling for us, so we would get a skewed reporting for frequency. But can we also hook into the analytics module somehow?

scruffian · 2016-02-01T15:22:13Z

client/catch-js-errors.js

+		xhr.open( 'POST', 'https://public-api.wordpress.com/rest/v1.1/js-error?http_envelope=1', true );
+		xhr.setRequestHeader( 'Content-type', 'application/x-www-form-urlencoded' );
+
+		params = 'client_id=39911&client_secret=cOaYKdrkgXz8xY7aysv4fU6wL6sK5J8a6ojReEIAPwggsznj4Cb6mW0nffTxtYT8&error=' + stringifyErrorForUrlParams( error );


It would be good to be able to get these from the config.

@tyxla does that in #47003, 4.5 years later 🥇

blowery · 2016-02-01T15:55:54Z

I'm confused. My understanding of what the code does is that if you have localStorage.getItem( 'log-errors' ) ==='true' then you'll fire the onerror event. Otherwise, if you are in the 1% then you'll get the localStorage item set an also fire the onerror event. This will only load once per session, which means that at first only 1% of users will have that local storage key set, but in time that will scale up, as we get more sessions, until eventually all users are in the bucket.

Ah, sure. When you do the set, you should set the value to either true or false, depending on which way the 1% falls. When you come back in, if it's already set, you don't bother recalculating it. That way it isn't a 1% chance per visit and it doesn't walk up.

blowery · 2016-02-01T15:57:07Z

client/catch-js-errors.js

+			var now = Date.now();
+
+			if ( ( now - lastCall ) < delay ) {
+				return;


probably want to stash the error we would have logged in an array or stack so we don't lose errors to the debouncer.

It would be nice to do this, but I'd like to get this PR merged. Can we handle this better in the future if we find we need it?

How will we know we're missing events? :)

scruffian · 2016-02-02T00:09:13Z

When you come back in, if it's already set, you don't bother recalculating it. That way it isn't a 1% chance per visit and it doesn't walk up.

This is what I did, but I did like the idea of it walking up over time, so that we catch more and more errors. I agree that it's better to control that ourselves though.

Tug · 2016-02-02T14:41:30Z

Maybe we could run this for all users on the wpcalypso/horizon environment first?

gravityrail · 2016-02-02T18:53:57Z

LGTM, 👍 for enabling only in horizon/staging first.

blowery · 2016-02-02T20:45:47Z

I'm happy to get this out the door to staging as is

scruffian · 2016-02-02T23:27:03Z

Thanks, I added a commit to exclude from production. Good to 🚢 ?

Tug · 2016-02-03T09:24:46Z

🚢

This required a change in how the module entries are built. Before that, webpack module system was included into the `vendor` module. Since we want to load `catch-js-errors` first, we need to load the module system before that so it is now extracted into a `common.js` file. For the next changes, I think it would be probably better to exclude this file from webpack's build and simply include it like third party services (olark...) in the page.

…eadable, and only assign 1% of users

Try catching JS errors and sending them to the API

blowery · 2016-02-04T14:50:17Z

🎉 👍 💯

scruffian added [Type] Question [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 24, 2015

scruffian self-assigned this Dec 24, 2015

scruffian mentioned this pull request Dec 24, 2015

Catch JS errors and send them to the API #1868

Closed

scruffian added the Framework label Dec 24, 2015

blowery reviewed Dec 24, 2015
View reviewed changes

artpi mentioned this pull request Jan 5, 2016

External js error logging service #2107

Closed

scruffian force-pushed the try/catch-js-errors-2 branch from d40c1e2 to 12847b7 Compare January 7, 2016 10:32

scruffian removed their assignment Jan 13, 2016

scruffian force-pushed the try/catch-js-errors-2 branch from 12847b7 to 3bffe59 Compare January 20, 2016 11:42

nb reviewed Jan 22, 2016
View reviewed changes

blowery reviewed Jan 22, 2016
View reviewed changes

scruffian reviewed Feb 1, 2016
View reviewed changes

blowery reviewed Feb 1, 2016
View reviewed changes

scruffian force-pushed the try/catch-js-errors-2 branch from 3d880f9 to 992f49a Compare February 1, 2016 18:41

scruffian self-assigned this Feb 2, 2016

Tug added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 3, 2016

scruffian and others added 7 commits February 3, 2016 10:00

Use the Make file to build catch errors

76f627a

Use Webpack to build the catch-js-errors script

52a5d18

Whitespace fixes

a144d4c

Framework: Make user assignment to the catch JS errors feature more r…

3508cea

…eadable, and only assign 1% of users

Framework: Don't catch JS errors in development

2161383

Catch JS Errors: Don't catch errors on production

644553c

scruffian force-pushed the try/catch-js-errors-2 branch from 3170866 to 644553c Compare February 3, 2016 10:00

scruffian added a commit that referenced this pull request Feb 3, 2016

Merge pull request #1949 from Automattic/try/catch-js-errors-2

4b41631

Try catching JS errors and sending them to the API

scruffian merged commit 4b41631 into master Feb 3, 2016

scruffian deleted the try/catch-js-errors-2 branch February 3, 2016 10:22

scruffian removed the [Status] Ready to Merge label Feb 3, 2016

This was referenced Feb 3, 2016

Revert "Try catching JS errors and sending them to the API" #3034

Merged

Try catching JS errors and sending them to the API #3060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try catching JS errors and sending them to the API #1949

Try catching JS errors and sending them to the API #1949

scruffian commented Dec 24, 2015

dmsnell commented Dec 24, 2015

blowery Dec 24, 2015

dmsnell Dec 24, 2015

artpi commented Dec 29, 2015

artpi commented Jan 5, 2016

artpi commented Jan 5, 2016

apeatling commented Jan 19, 2016

scruffian commented Jan 19, 2016

apeatling commented Jan 19, 2016

scruffian commented Jan 22, 2016

nb Jan 22, 2016

scruffian Jan 22, 2016

nb Jan 22, 2016

nb commented Jan 22, 2016

blowery Jan 22, 2016

Tug commented Jan 22, 2016

blowery commented Jan 22, 2016

rralian commented Jan 28, 2016

scruffian Feb 1, 2016

jsnajdr Nov 3, 2020

blowery commented Feb 1, 2016

blowery Feb 1, 2016

scruffian Feb 2, 2016

blowery Feb 2, 2016

scruffian commented Feb 2, 2016

Tug commented Feb 2, 2016

gravityrail commented Feb 2, 2016

blowery commented Feb 2, 2016

scruffian commented Feb 2, 2016

Tug commented Feb 3, 2016

blowery commented Feb 4, 2016

Try catching JS errors and sending them to the API #1949

Try catching JS errors and sending them to the API #1949

Conversation

scruffian commented Dec 24, 2015

dmsnell commented Dec 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artpi commented Dec 29, 2015

artpi commented Jan 5, 2016

artpi commented Jan 5, 2016

apeatling commented Jan 19, 2016

scruffian commented Jan 19, 2016

apeatling commented Jan 19, 2016

scruffian commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nb commented Jan 22, 2016

Choose a reason for hiding this comment

Tug commented Jan 22, 2016

blowery commented Jan 22, 2016

rralian commented Jan 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blowery commented Feb 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scruffian commented Feb 2, 2016

Tug commented Feb 2, 2016

gravityrail commented Feb 2, 2016

blowery commented Feb 2, 2016

scruffian commented Feb 2, 2016

Tug commented Feb 3, 2016

blowery commented Feb 4, 2016