Improve with-sentry example #5727

sheerun · 2018-11-21T19:43:18Z

In response to #1852

Ability to catch server-side errors
Ability to catch errors in custom server code
Deduplicating analogous server-side and client-side errors
Reporting correct number of users in sentry for each error
Reporting BUILD_ID as release in sentry
Ability to set SENTRY_TOKEN so sentry can access source maps in production

Is it complicated? Yes. Is it necessary for proper production error handling, also yes.

I propose you merge this and I could extract this into plugins when next.js supports creating plugins for Server and App

sheerun · 2018-11-21T19:53:08Z

I think I'll replace session cookie with ip address so there are less packages in example

sheerun · 2018-11-21T19:57:14Z

or never mind, I'll still need to persist ip_address in cookies to client so errors are properly reported if next is failing to load somehow, so the complexity will be the same

Enalmada · 2018-11-21T19:59:03Z

Sentry working properly is a critical requirement before going to production with Next.js. I can't thank you enough for the time you put into this for the community.

sheerun · 2018-11-21T20:13:08Z

It took a week or two but I'm pretty happy with error reporting now :)

For sure there are some things to improve:

reported server-side stacktraces: ResolveSourceMaps Integration getsentry/sentry-javascript#1622 so I could re-enable deduplicating by stacktrace rather than error message
for some reason errors are reported multiple times in console (but only once to sentry)

I also recommend my targets-webpack-plugin so next.js websites are properly rendered by google bot (chrome 41) and legacy browsers (mobile browsers and internet explorer).

timrogers · 2018-11-21T20:20:47Z

❤️ Excited to try this!

abraxxas · 2018-11-22T08:53:17Z

Thanks a ton, for the time you put into this. Super excited to try this, this has stumped me for quite some time

abraxxas · 2018-11-22T10:39:49Z

@sheerun are errors happening in getInitialProps on the server reported correctly for you? With this code for me they are still only reported the first time they are happening. subsequent errors of the same type get lost until i restart the server. Everything works perfectly with this setup

abraxxas · 2018-11-22T10:59:00Z

Is anyone else experiencing huge memory leaks on the node server (v: 10.13.0) with this example as long as it uses the sentry request and error handlers? Not sure if this is sentry related or because of interaction between sentry and next. I observed this using memwatch

    memwatch.on('leak', (info: any) => {
        console.log('memwatch::leak')
        console.error(info)
    })

    memwatch.on('stats', (stats: any) => {
        console.log('memwatch::stats')
        console.error(Util.inspect(stats, true, null))
    })

sheerun · 2018-11-22T16:43:33Z

Maybe they are not reported because sentry ignores duplicated errors?

Enalmada · 2018-11-22T17:17:37Z

@sheerun can you elaborate on the need for targets-webpack-plugin? If nextjs isn't properly rendering for googlebot that seems like a serious problem. I am curious if there are already nextjs issues open about that so we can work towards making nextjs work perfectly out of the box?

sheerun · 2018-11-22T23:13:20Z

I don't know. By default it works with google bot but when you install practically any modern package it stops. The solution is to transcompile final bundles instead of each file individually.

abraxxas · 2018-11-23T08:44:46Z

I just double checked, it should not be because of duplicate errors. But client side error catching works perfectly now for us. I had to remove the sentry-handlers as they cause huge memory-leaks for us

sheerun · 2018-11-23T13:48:58Z

For next week I won't have time to improve this PR to fix this so maybe someone else wants to

timneutkens · 2018-11-23T16:03:06Z

examples/with-sentry/server.js

+
+    server.use(cookieParser())
+
+    server.use((req, res, next) => {


I don't think this part is needed

why? it's needed for tracking users for errors in sentry

At the very least I don't think it should use an express middleware but use getInitialProps instead

it won't work if next.js crashes before of while excuting getIntialProps

Also I can't use getInitialProps because session id for user won't be preserved at all cases (e.g. when someone does full refresh of webpage). It would require reimplementing cookies in local storage.

timneutkens · 2018-11-23T16:03:15Z

examples/with-sentry/server.js

+    // This attaches request information to sentry errors
+    server.use(Sentry.Handlers.requestHandler())
+
+    server.use(cookieParser())


Same for this

it's needed for tracking users so we get same user id on client and on server

Also to preserve the same session id for different requests so users count for given error is not artificially high

timneutkens · 2018-11-23T16:03:53Z

examples/with-sentry/server.js

+
+    server.get('*', (req, res) => handle(req, res))
+
+    // This handles errors if they are thrown before raching the app


Suggested change

// This handles errors if they are thrown before raching the app

// This handles errors if they are thrown before reaching the app

timneutkens · 2018-11-23T16:05:32Z

examples/with-sentry/pages/_app.js

+        pageProps = await Component.getInitialProps(ctx)
+      }
+    } catch (e) {
+      captureException(e, ctx)


This should be moved to _error.js instead, we use something like this for zeit.co:

static getInitialProps({ res, err }) { console.log({err}) const statusCode = res ? res.statusCode : err ? err.statusCode : null Sentry.captureException(err) return { statusCode } }

Also passing ctx is dangerous when server errors happen, as it holds req and res, which can hold auth token cookies etc

It doesn't really work and results in Synthetic errors. ctx is passed to 1st party captureException from which relevant data is extracted. I don't see how it's security issue

I tried lots of different ways to integrate sentry in the _error.js but all of them resulted in Synthetic errors

iambouamar · 2018-11-29T12:02:43Z

when i did the same steps i get

These dependencies were not found:
console in ./node_modules/@sentry/node/dist/integrations/console.js
fs in ./node_modules/@sentry/utils/fs.js, ./node_modules/lsmod/index.js
module in ./node_modules/@sentry/node/dist/integrations/console.js, 
./node_modules/@sentry/node/dist/integrations/http.js

Enalmada · 2018-11-30T07:15:17Z

I tried this out and it seems to work great. No synthetic errors! I did also comment out the server.js sentry-handlers out of fear of the memory leak and everything on the test page still seems to work. I guess they are not really needed?
I wanted to get the dsn and environment from environment variables and for some reason the es6 method didn't work for me so I did what the transpiler would have done but I am sure there is a better way. Still, perhaps it helps someone who wants the same:

const _config = require("next/config");
function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : {default: obj}; }
const _config2 = _interopRequireDefault(_config);
const _getConfig = (0, _config2.default)();
const publicRuntimeConfig = _getConfig.publicRuntimeConfig;

if (publicRuntimeConfig.SENTRY_DSN) {
    Sentry.init({
        dsn: publicRuntimeConfig.SENTRY_DSN,
        environment: publicRuntimeConfig.ENV,
        ...
    });
}

sheerun · 2018-11-30T09:34:17Z

@Enalmada Don't say that. server.js role is twofold:

It reports errors that are happening in custom server code and the code, outside of getInitialProps & render. Without it you only catch next.js-specific and react errors, not all errors. I had serious problems with my app because of errors that didn't get catched in custom server code and we potentially lost many of users because 500 pages were shown to them by express server before even reaching next.js handler, but no error reporting happened. You cannot just ignore it.
For each error in sentry it reports number of users that are affected by this error by setting sid (session id) cookie. Anyone who maintains popular app knows there will be hundreds of random errors in sentry because of variety of web-enabled devices. Knowing number of affected users is critical for selecting actually important errors to fix.

Discovering memory leaks in sentry is saddening (getsentry/sentry-javascript#1762) but it doesn't disqualify this PR as proper solution for error handling. I hope Sentry will resolve these issues soon so we can merge this.

As for catching middleware errors this is proper solution for now (after memory leaks are fixed by sentry). If you don't like it you can iterate on it in the future by e.g. having next.js manage custom server code and only allow to inject middleware in next.config.js. This way next.js would be able to catch any errors even if they happened in custom server code.

As for tracking users for errors - there's no another way to implement this for now. Only using cookies guarantees that I can see all errors that happened to single user if he/she received errors in more than one page - it's not possible to track state between different requests without cookies. Not to mention even if using getInitialProps for setting sessionId somehow preserved sessionId between requests (as suggested by @timneutkens), it won't always work because of complexity of code that has to execute for this feature to work: successful getInitialProps, serialization, app initialization and then rehydration (and also because errors can happen in custom server code). In contrast setting cookie can happen by just setting header in middleware, and reading it is handled by browser itself - there's practically nothing that can break this process, and browser automatically cares to send session cookie for all future requests. It just makes no sense to reimplement this behavior in next.js using e.g. localStorage

Enalmada · 2018-11-30T17:34:35Z

Ah, thank you for the great explanation about how these two lines (which I prematurely commented out of memory leak fear) actually do more than I realized:
server.use(Sentry.Handlers.requestHandler()); server.use(Sentry.Handlers.errorHandler());
Everyone absolutely needs to catch all errors in addition to next errors!

@abraxxas issue notes that the memory leak only happens on node 10.x so this issue should still be merged asap, just with a strong README warning to use node 8.x until that sentry issue is resolved. The existing sentry example already has the sentry and request handler so merging this with a note to use node 8 is just one more way this pull request could help everyone.

Let's get this merged because it is so much better than current example and then we can continue to work with next to fundamentally improve the situation (middleware support, etc).

abraxxas · 2018-12-06T10:37:11Z

I just implemented this in typescript and according to the types there are a few issues with this PR.

React.ErrorInfo only contains one key called componentStack and the request object does not have a params or query key

sheerun · 2018-12-06T10:43:08Z

@abraxxas Maybe you want to send PR to this PR?

abraxxas · 2018-12-06T10:53:12Z

sure will do that as soon as i have some time today, just wanted to notify everyone

…

---------------------------------------- *Sebastian Dumbs* Tel: 0699/19012945 Email:[email protected] Am Do., 6. Dez. 2018 um 11:43 Uhr schrieb Adam Stankiewicz < [email protected]>:

@abraxxas <https://github.com/abraxxas> Maybe you want to send PR to this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5727 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIbrNhvevDJTJ0pk3MHcsNuJ8uorpcl9ks5u2PTPgaJpZM4Ytx8n> .

remove nonexisting keys from request and update errorInfo handling

sheerun · 2018-12-06T11:42:57Z

I've merged @abraxxas PR to this PR so we should be fine now, thank you!

With sentry fix

timneutkens · 2018-12-10T23:00:36Z

I'm going to merge this but I think it would still be beneficial to have a lightweight version of this without a custom server / cookies / etc for if you only want to catch the errors that are rendering in _error.js (all user-side errors in Next.js), for example that's what we do for zeit.co.

sheerun · 2018-12-11T02:29:01Z

@timneutkens Maybe something like static onErrorCatched() on _error.js could be made an API that catches errors from all places in next.js?

sheerun · 2018-12-11T02:54:14Z

Maybe getInitialProps could be made the place but currently it suffers from few problems:

Render of _error.js causes _app.js to render which could've triggered error in the first place. I think _error.js should be on the same level as _app.js so to prevent such often situation
Because of something error passed to getInitialProps becomes synthetic and is missing proper stacktrace (maybe because getInitialProps of _error.js is async or maybe because async getInitialProps _app.js which it depends on is called, all the errors passed to onErrorCatched become synthetic or maybe because)
It doesn't catch errors from getInitialProps of _app.js
It doesn't catch errors that are thrown in custom server code (next.js would need to export app.getErrorHandler()) that should be attached at the end of custom server code.
It doesn't catch unhandled errors in next.js app (probably new api like static onUnhandledException() would be needed

Yankovsky · 2019-02-19T13:53:37Z

Changes from this pr have been lost in merge commit 163830c

Yankovsky · 2019-02-19T13:53:50Z

@sheerun @timneutkens ^

Timer · 2019-02-20T13:09:14Z

Feel free to resubmit a PR @Yankovsky

Improve with-sentry example

0e99c01

sheerun mentioned this pull request Nov 21, 2018

How to catch and handle errors to report logs on server side #1852

Closed

timneutkens reviewed Nov 23, 2018

View reviewed changes

remove nonexisting keys from request and update errorInfo handling

48936fb

abraxxas mentioned this pull request Dec 6, 2018

remove nonexisting keys from request and update errorInfo handling sheerun/next.js#1

Merged

Merge pull request #1 from abraxxas/with-sentry-fix

e5586dd

remove nonexisting keys from request and update errorInfo handling

abraxxas and others added 3 commits December 6, 2018 15:31

readd query and pathname

bffefed

read query and params and add pathname and query to client

6aea764

Merge pull request #2 from abraxxas/with-sentry-fix

79c6a2a

With sentry fix

timneutkens merged commit cd1d364 into vercel:master Dec 10, 2018

jesstelford mentioned this pull request Mar 3, 2019

Make with-sentry example deployable and runnable #6362

Merged

leerob mentioned this pull request Apr 23, 2019

Create simple Sentry example for exception reporting. #7119

Closed

leerob mentioned this pull request May 16, 2019

Create simple Sentry example for exception reporting. #7360

Merged

lock bot locked as resolved and limited conversation to collaborators Feb 21, 2020


		server.get('*', (req, res) => handle(req, res))

		// This handles errors if they are thrown before raching the app

Improve with-sentry example #5727

Improve with-sentry example #5727

Conversation

sheerun commented Nov 21, 2018

sheerun commented Nov 21, 2018

sheerun commented Nov 21, 2018

Enalmada commented Nov 21, 2018

sheerun commented Nov 21, 2018 • edited Loading

timrogers commented Nov 21, 2018

abraxxas commented Nov 22, 2018

abraxxas commented Nov 22, 2018 • edited Loading

abraxxas commented Nov 22, 2018

sheerun commented Nov 22, 2018

Enalmada commented Nov 22, 2018

sheerun commented Nov 22, 2018

abraxxas commented Nov 23, 2018

sheerun commented Nov 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timneutkens Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambouamar commented Nov 29, 2018 • edited Loading

Enalmada commented Nov 30, 2018 • edited Loading

sheerun commented Nov 30, 2018 • edited Loading

Enalmada commented Nov 30, 2018 • edited Loading

abraxxas commented Dec 6, 2018

sheerun commented Dec 6, 2018

abraxxas commented Dec 6, 2018 via email

sheerun commented Dec 6, 2018

timneutkens commented Dec 10, 2018 • edited Loading

sheerun commented Dec 11, 2018

sheerun commented Dec 11, 2018

Yankovsky commented Feb 19, 2019

Yankovsky commented Feb 19, 2019 • edited Loading

Timer commented Feb 20, 2019

sheerun commented Nov 21, 2018 •

edited

Loading

abraxxas commented Nov 22, 2018 •

edited

Loading

timneutkens Nov 23, 2018 •

edited

Loading

iambouamar commented Nov 29, 2018 •

edited

Loading

Enalmada commented Nov 30, 2018 •

edited

Loading

sheerun commented Nov 30, 2018 •

edited

Loading

Enalmada commented Nov 30, 2018 •

edited

Loading

timneutkens commented Dec 10, 2018 •

edited

Loading

Yankovsky commented Feb 19, 2019 •

edited

Loading