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 sentry #981

Merged
merged 1 commit into from
Feb 7, 2020
Merged

Add sentry #981

merged 1 commit into from
Feb 7, 2020

Conversation

iAdramelk
Copy link
Contributor

Test integration with Sentry. To make it work we need to add env variable SENTRY_DSN to Heroku.

@shcheklein shcheklein temporarily deployed to dvc-landing-sentry-inte-rzvicf February 7, 2020 14:02 Inactive
@iAdramelk iAdramelk temporarily deployed to dvc-landing-sentry-inte-rzvicf February 7, 2020 14:04 Inactive
@iAdramelk iAdramelk temporarily deployed to dvc-landing-sentry-inte-rzvicf February 7, 2020 14:05 Inactive
@shcheklein shcheklein merged commit 4d2194e into master Feb 7, 2020
@jorgeorpinel
Copy link
Contributor

Interesting. So Sentry is a SRE/APM dashboard? I quickly read https://blog.sentry.io/2018/03/06/the-sentry-workflow but would be great to know what this is all about 🙂

@iAdramelk
Copy link
Contributor Author

@jorgeorpinel , yep. Pretty much it. We can collect info about user's javascirpt errors with it, see information about environment, stack traces, etc.

@@ -0,0 +1,14 @@
/* eslint-env node */

const withSourceMaps = require('@zeit/next-source-maps')
Copy link
Contributor

Choose a reason for hiding this comment

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

I see @zeit/next-source-maps was also added here. Is this specifically just a nice addition for Sentry logs/traces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Source maps is a mechanism to map bundled and minified code with the original source files. If we just run sentry as it is, it will show errors like: You nave an error on line 1, symbol 3454560 in the gX method. Source maps will ideally show us original file, line and method name. (But right now it's looks like it didn't work correctly)

It's not strictly related to Sentry and is used for example by browser Dev Tools, but it make using Sentry more convenient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I did read about source maps and it seems like a great feature. I was just wondering if this was added specifically for Sentry traces or whether it can also be used on the Browser now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we should be, because I leave them enabled (there is also an option to send them directly to sentry through API and not publish on site). But in practice from that I see, I misconfigured them somehow and didn't see them anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK cool please let us know when you figure it out 🙂

Comment on lines +8 to +14
process.on('unhandledRejection', err => {
Sentry.captureException(err)
})

process.on('uncaughtException', err => {
Sentry.captureException(err)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these needed if Sentry supposedly has automated handling of uncaught exceptions and unhandled rejections? See https://docs.sentry.io/platforms/javascript/#automatically-capturing-errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for server side errors in node.js, Sentry will automatically log errors in browser, but not unhandled errors in SSR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks.

Would these also be needed separately for server.js or other build-time code? e.g. sidebar.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This catches all code in the actual pages including sidebar.js, but I think we are now missing errors from server.js because they happens before we call _document.js.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Feb 12, 2020

Choose a reason for hiding this comment

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

Yes some server code runs before the Next app and partially includes sidebar.js code I think. That's what I was wondering about. But it's probably not a big deal? Only runs when the server is being initialized, probably only when new versions are deployed to Heroku (or when they restart it automatically for whatever reason).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's pretty much my thoughts as well. And also the fact that we are planning to migrate to Gatsby and it didn't have a server. So I decided to leave it as is for now.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Thanks @iAdramelk. A few more questions here ^

@shcheklein shcheklein deleted the sentry-integration branch March 27, 2020 20:08
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.

3 participants