From 8eb7898827b44501dbcd855d36cdbec60fb89ebd Mon Sep 17 00:00:00 2001 From: Peter Bengtsson Date: Mon, 22 Aug 2022 18:50:18 +0200 Subject: [PATCH] send errors to Failbot in SSR rendering (#30153) * send errors to failbot in SSR rendering * wip * progress * tidying up * no point awaiting something that doens't return a promise --- lib/failbot.js | 7 ++- lib/handle-exceptions.js | 4 +- middleware/render-page.js | 8 +++ next.config.js | 4 +- pages/500.tsx | 5 -- pages/_error.tsx | 104 ++++++++++++++++++++++++++++++++++++-- 6 files changed, 117 insertions(+), 15 deletions(-) delete mode 100644 pages/500.tsx diff --git a/lib/failbot.js b/lib/failbot.js index 0189ff27a9df..46ed67babdac 100644 --- a/lib/failbot.js +++ b/lib/failbot.js @@ -33,9 +33,11 @@ async function retryingGot(url, args) { ) } -export async function report(error, metadata) { +export function report(error, metadata) { // If there's no HAYSTACK_URL set, bail early - if (!process.env.HAYSTACK_URL) return + if (!process.env.HAYSTACK_URL) { + return + } const backends = [ new HTTPBackend({ @@ -47,6 +49,7 @@ export async function report(error, metadata) { app: HAYSTACK_APP, backends, }) + return failbot.report(error, metadata) } diff --git a/lib/handle-exceptions.js b/lib/handle-exceptions.js index a6ce0adc57e7..dda3894c9a21 100644 --- a/lib/handle-exceptions.js +++ b/lib/handle-exceptions.js @@ -10,7 +10,7 @@ process.on('uncaughtException', async (err) => { console.error(err) try { - await FailBot.report(err) + FailBot.report(err) } catch (failBotError) { console.warn('Even sending the uncaughtException error to FailBot failed!') console.error(failBotError) @@ -20,7 +20,7 @@ process.on('uncaughtException', async (err) => { process.on('unhandledRejection', async (err) => { console.error(err) try { - await FailBot.report(err) + FailBot.report(err) } catch (failBotError) { console.warn('Even sending the unhandledRejection error to FailBot failed!') console.error(failBotError) diff --git a/middleware/render-page.js b/middleware/render-page.js index 6f219eaf2408..8f4a1e054254 100644 --- a/middleware/render-page.js +++ b/middleware/render-page.js @@ -1,5 +1,6 @@ import { get } from 'lodash-es' +import FailBot from '../lib/failbot.js' import patterns from '../lib/patterns.js' import getMiniTocItems from '../lib/get-mini-toc-items.js' import Page from '../lib/page.js' @@ -40,6 +41,13 @@ async function buildMiniTocItems(req) { export default async function renderPage(req, res, next) { const { context } = req + + // This is a contextualizing the request so that when this `req` is + // ultimately passed into the `Error.getInitialProps` function, + // which NextJS executes at runtime on errors, so that we can + // from there send the error to Failbot. + req.FailBot = FailBot + const { page } = context const path = req.pagePath || req.path browserCacheControl(res) diff --git a/next.config.js b/next.config.js index e427ea4c616b..bf3db7e25424 100644 --- a/next.config.js +++ b/next.config.js @@ -1,6 +1,8 @@ import fs from 'fs' -import frontmatter from 'gray-matter' import path from 'path' + +import frontmatter from 'gray-matter' + const homepage = path.posix.join(process.cwd(), 'content/index.md') const { data } = frontmatter(fs.readFileSync(homepage, 'utf8')) const productIds = data.children diff --git a/pages/500.tsx b/pages/500.tsx deleted file mode 100644 index 0b75f32263f4..000000000000 --- a/pages/500.tsx +++ /dev/null @@ -1,5 +0,0 @@ -import { GenericError } from 'components/GenericError' - -export default function Custom500() { - return -} diff --git a/pages/_error.tsx b/pages/_error.tsx index 2f6fd315f8b3..11ab2864836f 100644 --- a/pages/_error.tsx +++ b/pages/_error.tsx @@ -1,10 +1,104 @@ -import { NextPage } from 'next' -import { GenericError } from 'components/GenericError' +import type { NextPageContext } from 'next' -type Props = {} +import { GenericError } from 'components/GenericError' -const ErrorPage: NextPage = () => { +function Error() { return } -export default ErrorPage +Error.getInitialProps = async (ctx: NextPageContext) => { + // If this getInitialProps() is called in client-side rendering, + // you won't have a `.res` object. It's only present when it's + // rendered Node (SSR). That's our clue to know that, we should + // send this error to Failbot. + // In client-side, it's undefined. In server, it's a ServerResponse object. + const { err, req, res } = ctx + let statusCode = 500 + if (res?.statusCode) { + statusCode = res.statusCode + } + + // 'err' will by falsy if it's a 404 + // But note, at the time of writing this comment, we have a dedicated + // `pages/404.tsx` which takes care of 404 messages. + if (err && res && req) { + // This is a (necessary) hack! + // You can't import `../lib/failbot.js` here in this + // file because it gets imported by webpack to be used in the + // client-side JS bundle. It *could* be solved by overriding + // the webpack configuration in our `next.config.js` but this is prone + // to be fragile since ignoring code can be hard to get right + // and the more we override there, the harder it will become to + // upgrade NextJS in the future because of moving parts. + // So the solution is to essentially do what the contextualizers + // do which mutate the Express request object by attaching + // callables to it. This way it's only ever present in SSR executed + // code and doesn't need any custom webpack configuration. + const expressRequest = req as any + const FailBot = expressRequest.FailBot + if (FailBot) { + try { + // An inclusion-list of headers we're OK with sending because + // they don't contain an PII. + const OK_HEADER_KEYS = ['user-agent', 'referer', 'accept-encoding', 'accept-language'] + const reported = FailBot.report(err, { + path: req.url, + request: JSON.stringify( + { + method: expressRequest.method, + query: expressRequest.query, + language: expressRequest.language, + }, + undefined, + 2 + ), + headers: JSON.stringify( + Object.fromEntries( + Object.entries(req.headers).filter(([k]) => OK_HEADER_KEYS.includes(k)) + ), + undefined, + 2 + ), + }) + + // Within FailBot.report() (which is our wrapper for Failbot in + // the `@github/failbot` package), it might exit only because + // it has no configured backends to send to. I.e. it returns undefined. + // Otherwise, it should return `Array>`. + if (!reported) { + console.warn( + 'The FailBot.report() returned undefined which means the error was NOT sent to Failbot.' + ) + } else if ( + Array.isArray(reported) && + reported.length && + reported.every((thing) => thing instanceof Promise) + ) { + // Make sure we await the promises even though we don't care + // about the results. This makes the code cleaner rather than + // letting the eventloop take care of it which could result + // in cryptic error messages if the await does fail for some + // reason. + try { + await Promise.all(reported) + } catch (error) { + console.warn('Unable to await reported FailBot errors', error) + } + } + } catch (error) { + // This does not necessarily mean FailBot failed to send. It's + // most likely that we failed to *send to* FailBot before it + // even has a chance to use the network. This is because + // `FailBot.report` returns an array of Promises which themselves + // could go wrong, but that's a story for another try/catch. + // Basically, this catch it just to avoid other errors that + // might prevent the pretty error page to render at all. + console.warn('Failed to send error to FailBot.', error) + } + } + } + + return { statusCode, message: err?.message } +} + +export default Error