-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
Improve with-sentry example #5727
Changes from 1 commit
0e99c01
48936fb
e5586dd
bffefed
6aea764
79c6a2a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
const webpack = require('webpack') | ||
const nextSourceMaps = require('@zeit/next-source-maps')() | ||
|
||
const SENTRY_DSN = '' | ||
|
||
module.exports = nextSourceMaps({ | ||
webpack: (config, { dev, isServer, buildId }) => { | ||
if (!dev) { | ||
config.plugins.push( | ||
new webpack.DefinePlugin({ | ||
'process.env.SENTRY_DSN': JSON.stringify(SENTRY_DSN), | ||
'process.env.SENTRY_RELEASE': JSON.stringify(buildId) | ||
}) | ||
) | ||
} | ||
|
||
if (!isServer) { | ||
config.resolve.alias['@sentry/node'] = '@sentry/browser' | ||
} | ||
|
||
return config | ||
} | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,32 @@ | ||
import App from 'next/app' | ||
import * as Sentry from '@sentry/browser' | ||
import { captureException } from '../utils/sentry' | ||
|
||
const SENTRY_PUBLIC_DSN = '' | ||
class MyApp extends App { | ||
// This reports errors before rendering, when fetching initial props | ||
static async getInitialProps (appContext) { | ||
const { Component, ctx } = appContext | ||
|
||
export default class MyApp extends App { | ||
constructor (...args) { | ||
super(...args) | ||
Sentry.init({dsn: SENTRY_PUBLIC_DSN}) | ||
let pageProps = {} | ||
|
||
try { | ||
if (Component.getInitialProps) { | ||
pageProps = await Component.getInitialProps(ctx) | ||
} | ||
} catch (e) { | ||
captureException(e, ctx) | ||
throw e // you can also skip re-throwing and set property on pageProps | ||
} | ||
|
||
return { | ||
pageProps | ||
} | ||
} | ||
|
||
// This reports errors thrown while rendering components | ||
componentDidCatch (error, errorInfo) { | ||
Sentry.configureScope(scope => { | ||
Object.keys(errorInfo).forEach(key => { | ||
scope.setExtra(key, errorInfo[key]) | ||
}) | ||
}) | ||
Sentry.captureException(error) | ||
|
||
// This is needed to render errors correctly in development / production | ||
captureException(error, { extra: errorInfo }) | ||
super.componentDidCatch(error, errorInfo) | ||
} | ||
} | ||
|
||
export default MyApp |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,66 @@ | ||||||
const next = require('next') | ||||||
const express = require('express') | ||||||
const cookieParser = require('cookie-parser') | ||||||
const Sentry = require('@sentry/node') | ||||||
const uuidv4 = require('uuid/v4') | ||||||
const port = parseInt(process.env.PORT, 10) || 3000 | ||||||
const dev = process.env.NODE_ENV !== 'production' | ||||||
const app = next({ dev }) | ||||||
const handle = app.getRequestHandler() | ||||||
|
||||||
require('./utils/sentry') | ||||||
|
||||||
app.prepare() | ||||||
.then(() => { | ||||||
const server = express() | ||||||
|
||||||
// This attaches request information to sentry errors | ||||||
server.use(Sentry.Handlers.requestHandler()) | ||||||
|
||||||
server.use(cookieParser()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same for this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's needed for tracking users so we get same user id on client and on server There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also to preserve the same session id for different requests so users count for given error is not artificially high |
||||||
|
||||||
server.use((req, res, next) => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this part is needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? it's needed for tracking users for errors in sentry There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the very least I don't think it should use an express middleware but use getInitialProps instead There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it won't work if next.js crashes before of while excuting getIntialProps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
const htmlPage = | ||||||
!req.path.match(/^\/(_next|static)/) && | ||||||
!req.path.match(/\.(js|map)$/) && | ||||||
req.accepts('text/html', 'text/css', 'image/png') === 'text/html' | ||||||
|
||||||
if (!htmlPage) { | ||||||
next() | ||||||
return | ||||||
} | ||||||
|
||||||
if (!req.cookies.sid || req.cookies.sid.length === 0) { | ||||||
req.cookies.sid = uuidv4() | ||||||
res.cookie('sid', req.cookies.sid) | ||||||
} | ||||||
|
||||||
next() | ||||||
}) | ||||||
|
||||||
// In production we don't want to serve sourcemaps for anyone | ||||||
if (!dev) { | ||||||
const hasSentryToken = !!process.env.SENTRY_TOKEN | ||||||
server.get(/\.map$/, (req, res, next) => { | ||||||
if (hasSentryToken && req.headers['x-sentry-token'] !== process.env.SENTRY_TOKEN) { | ||||||
res | ||||||
.status(401) | ||||||
.send( | ||||||
'Authentication access token is required to access the source map.' | ||||||
) | ||||||
return | ||||||
} | ||||||
next() | ||||||
}) | ||||||
} | ||||||
|
||||||
server.get('*', (req, res) => handle(req, res)) | ||||||
|
||||||
// This handles errors if they are thrown before raching the app | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
server.use(Sentry.Handlers.errorHandler()) | ||||||
|
||||||
server.listen(port, err => { | ||||||
if (err) throw err | ||||||
console.log(`> Ready on http://localhost:${port}`) | ||||||
}) | ||||||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
const Sentry = require('@sentry/node') | ||
const Cookie = require('js-cookie') | ||
|
||
if (process.env.SENTRY_DSN) { | ||
Sentry.init({ | ||
dsn: process.env.SENTRY_DSN, | ||
release: process.env.SENTRY_RELEASE, | ||
maxBreadcrumbs: 50, | ||
attachStacktrace: true | ||
}) | ||
} | ||
|
||
function captureException (err, { req, res, extra }) { | ||
Sentry.configureScope(scope => { | ||
if (err.message) { | ||
// De-duplication currently doesn't work correctly for SSR / browser errors | ||
// so we force deduplication by error message if it is present | ||
scope.setFingerprint([err.message]) | ||
} | ||
|
||
if (err.statusCode) { | ||
scope.setExtra('statusCode', err.statusCode) | ||
} | ||
|
||
if (res && res.statusCode) { | ||
scope.setExtra('statusCode', res.statusCode) | ||
} | ||
|
||
if (process.browser) { | ||
scope.setTag('ssr', false) | ||
|
||
// On client-side we use js-cookie package to fetch it | ||
const sessionId = Cookie.get('sid') | ||
if (sessionId) { | ||
scope.setUser({ id: sessionId }) | ||
} | ||
} else { | ||
scope.setTag('ssr', true) | ||
scope.setExtra('url', req.url) | ||
scope.setExtra('params', req.params) | ||
scope.setExtra('query', req.query) | ||
scope.setExtra('headers', req.headers) | ||
|
||
// On server-side we take session cookie directly from request | ||
if (req.cookies.sid) { | ||
scope.setUser({ id: req.cookies.sid }) | ||
} | ||
} | ||
|
||
if (extra) { | ||
Object.keys(extra).forEach(key => { | ||
scope.setExtra(key, extra[key]) | ||
}) | ||
} | ||
}) | ||
|
||
Sentry.captureException(err) | ||
} | ||
|
||
module.exports = { | ||
captureException | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to _error.js instead, we use something like this for zeit.co:
Also passing
ctx
is dangerous when server errors happen, as it holdsreq
andres
, which can hold auth token cookies etcThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried lots of different ways to integrate sentry in the _error.js but all of them resulted in Synthetic errors