Switch error tracking to Sentry

owid · Jan 8, 2025 · 34156fa · 34156fa
1 parent c8cf78e
commit 34156fa
Show file tree

Hide file tree

Showing 38 changed files with 1,079 additions and 628 deletions.
diff --git a/README.md b/README.md
@@ -84,5 +84,3 @@ The following is an excerpt explaining the origin of this repo and what the alte
 ---
 
 Cross-browser testing provided by <a href="https://www.browserstack.com"><img src="https://3fxtqy18kygf3on3bu39kh93-wpengine.netdna-ssl.com/wp-content/themes/browserstack/img/bs-logo.svg" /> BrowserStack</a>
-
-Client-side bug tracking provided by <a href="http://www.bugsnag.com/"><img width="110" src="https://images.typeform.com/images/QKuaAssrFCq7/image/default" /></a>
diff --git a/adminSiteServer/apiRouter.ts b/adminSiteServer/apiRouter.ts
@@ -155,7 +155,7 @@ import {
     getPublishingAction,
 } from "../adminSiteClient/gdocsDeploy.js"
 import { createGdocAndInsertOwidGdocPostContent } from "../db/model/Gdoc/archieToGdoc.js"
-import { logErrorAndMaybeSendToBugsnag } from "../serverUtils/errorLog.js"
+import { logErrorAndMaybeCaptureInSentry } from "../serverUtils/errorLog.js"
 import {
     getRouteWithROTransaction,
     deleteRouteWithRWTransaction,
@@ -2158,14 +2158,15 @@ putRouteWithRWTransaction(
         await db.knexRaw(trx, `DELETE FROM dataset_tags WHERE datasetId=?`, [
             datasetId,
         ])
-        if (tagRows.length)
+        if (tagRows.length) {
             for (const tagRow of tagRows) {
                 await db.knexRaw(
                     trx,
                     `INSERT INTO dataset_tags (tagId, datasetId) VALUES (?, ?)`,
                     tagRow
                 )
             }
+        }
 
         try {
             await syncDatasetToGitRepo(trx, datasetId, {
@@ -2174,7 +2175,7 @@ putRouteWithRWTransaction(
                 commitEmail: res.locals.user.email,
             })
         } catch (err) {
-            await logErrorAndMaybeSendToBugsnag(err, req)
+            await logErrorAndMaybeCaptureInSentry(err)
             // Continue
         }
 
@@ -2240,7 +2241,7 @@ deleteRouteWithRWTransaction(
                 commitEmail: res.locals.user.email,
             })
         } catch (err: any) {
-            await logErrorAndMaybeSendToBugsnag(err, req)
+            await logErrorAndMaybeCaptureInSentry(err)
             // Continue
         }
 

diff --git a/adminSiteServer/app.ts b/adminSiteServer/app.ts
@@ -1,3 +1,6 @@
+// This must be imported first for Sentry instrumentation to work.
+import "../serverUtils/instrument.js"
+
 import { GIT_CMS_DIR } from "../gitCms/GitCmsConstants.js"
 import {
     ADMIN_SERVER_HOST,

diff --git a/adminSiteServer/appClass.tsx b/adminSiteServer/appClass.tsx
@@ -1,16 +1,11 @@
 import { simpleGit } from "simple-git"
 import express, { NextFunction } from "express"
+import * as Sentry from "@sentry/node"
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 require("express-async-errors") // todo: why the require?
 import cookieParser from "cookie-parser"
 import http from "http"
-import Bugsnag from "@bugsnag/js"
-import BugsnagPluginExpress from "@bugsnag/plugin-express"
-import {
-    BAKED_BASE_URL,
-    BUGSNAG_NODE_API_KEY,
-    ENV_IS_STAGING,
-} from "../settings/serverSettings.js"
+import { BAKED_BASE_URL, ENV_IS_STAGING } from "../settings/serverSettings.js"
 import * as db from "../db/db.js"
 import { IndexPage } from "./IndexPage.js"
 import {
@@ -66,23 +61,9 @@ export class OwidAdminApp {
 
     async startListening(adminServerPort: number, adminServerHost: string) {
         this.gitCmsBranchName = await this.getGitCmsBranchName()
-        let bugsnagMiddleware
 
         const { app } = this
 
-        if (BUGSNAG_NODE_API_KEY) {
-            Bugsnag.start({
-                apiKey: BUGSNAG_NODE_API_KEY,
-                context: "admin-server",
-                plugins: [BugsnagPluginExpress],
-                autoTrackSessions: false,
-            })
-            bugsnagMiddleware = Bugsnag.getPlugin("express")
-            // From the docs: "this must be the first piece of middleware in the
-            // stack. It can only capture errors in downstream middleware"
-            if (bugsnagMiddleware) app.use(bugsnagMiddleware.requestHandler)
-        }
-
         // since the server is running behind a reverse proxy (nginx), we need to "trust"
         // the X-Forwarded-For header in order to get the real request IP
         // https://expressjs.com/en/guide/behind-proxies.html
@@ -157,11 +138,6 @@ export class OwidAdminApp {
             }
         })
 
-        // From the docs: "this handles any errors that Express catches. This
-        // needs to go before other error handlers. BugSnag will call the `next`
-        // error handler if it exists.
-        if (bugsnagMiddleware) app.use(bugsnagMiddleware.errorHandler)
-
         if (this.options.isDev) {
             if (!this.options.isTest) {
                 // https://vitejs.dev/guide/ssr
@@ -179,6 +155,10 @@ export class OwidAdminApp {
             app.use("/", mockSiteRouter)
         }
 
+        // Add this after all routes,
+        // but before any and other error-handling middlewares are defined
+        Sentry.setupExpressErrorHandler(app)
+
         // Give full error messages, including in production
         app.use(this.errorHandler)
 

diff --git a/adminSiteServer/chartConfigR2Helpers.ts b/adminSiteServer/chartConfigR2Helpers.ts
@@ -15,7 +15,6 @@ import {
 } from "@aws-sdk/client-s3"
 import { JsonError, lazy } from "@ourworldindata/utils"
 import { Base64String, R2GrapherConfigDirectory } from "@ourworldindata/types"
-import { logErrorAndMaybeSendToBugsnag } from "../serverUtils/errorLog.js"
 
 const getS3Client: () => S3Client = lazy(
     () =>
@@ -116,7 +115,6 @@ async function saveConfigToR2(
             `Successfully uploaded object: ${params.Bucket}/${params.Key}`
         )
     } catch (err) {
-        await logErrorAndMaybeSendToBugsnag(err)
         throw new JsonError(
             `Failed to save the config to R2. Inner error: ${err}`
         )
@@ -162,7 +160,6 @@ export async function deleteGrapherConfigFromR2(
             `Successfully deleted object: ${params.Bucket}/${params.Key}`
         )
     } catch (err) {
-        await logErrorAndMaybeSendToBugsnag(err)
         throw new JsonError(
             `Failed to delete the grapher config to R2 at ${directory}/${filename}. Inner error: ${err}`
         )

diff --git a/baker/DatapageHelpers.ts b/baker/DatapageHelpers.ts
@@ -25,7 +25,7 @@ import {
 } from "@ourworldindata/types"
 import { KnexReadonlyTransaction } from "../db/db.js"
 import { parseFaqs } from "../db/model/Gdoc/rawToEnriched.js"
-import { logErrorAndMaybeSendToBugsnag } from "../serverUtils/errorLog.js"
+import { logErrorAndMaybeCaptureInSentry } from "../serverUtils/errorLog.js"
 import { getSlugForTopicTag } from "./GrapherBakingUtils.js"
 import { getShortPageCitation } from "../site/gdocs/utils.js"
 
@@ -197,8 +197,10 @@ export const getPrimaryTopic = async (
     try {
         topicSlug = await getSlugForTopicTag(knex, firstTopicTag)
     } catch {
-        await logErrorAndMaybeSendToBugsnag(
-            `Data page is using "${firstTopicTag}" as its primary tag, which we are unable to resolve to a tag in the grapher DB`
+        await logErrorAndMaybeCaptureInSentry(
+            new Error(
+                `Data page is using "${firstTopicTag}" as its primary tag, which we are unable to resolve to a tag in the grapher DB`
+            )
         )
         return undefined
     }

diff --git a/baker/DeployUtils.ts b/baker/DeployUtils.ts
@@ -1,6 +1,5 @@
 import fs from "fs-extra"
 import { BuildkiteTrigger } from "../baker/BuildkiteTrigger.js"
-import { logErrorAndMaybeSendToBugsnag, warn } from "../serverUtils/errorLog.js"
 import { DeployQueueServer } from "./DeployQueueServer.js"
 import {
     BAKED_SITE_DIR,
@@ -24,7 +23,7 @@ export const defaultCommitMessage = async (): Promise<string> => {
         const sha = await fs.readFile("public/head.txt", "utf8")
         message += `\nowid/owid-grapher@${sha}`
     } catch (err) {
-        warn(err)
+        console.warn(err)
     }
 
     return message
@@ -43,16 +42,12 @@ const triggerBakeAndDeploy = async (
     if (BUILDKITE_API_ACCESS_TOKEN) {
         const buildkite = new BuildkiteTrigger()
         if (lightningQueue?.length) {
-            await buildkite
-                .runLightningBuild(
-                    lightningQueue.map((change) => change.slug!),
-                    deployMetadata
-                )
-                .catch(logErrorAndMaybeSendToBugsnag)
+            await buildkite.runLightningBuild(
+                lightningQueue.map((change) => change.slug!),
+                deployMetadata
+            )
         } else {
-            await buildkite
-                .runFullBuild(deployMetadata)
-                .catch(logErrorAndMaybeSendToBugsnag)
+            await buildkite.runFullBuild(deployMetadata)
         }
     } else {
         // otherwise, bake locally. This is used for local development or staging servers
@@ -143,11 +138,8 @@ const getSlackMentionByEmail = async (
         const response = await slackClient.users.lookupByEmail({ email })
         return response.user?.id ? `<@${response.user.id}>` : undefined
     } catch (error) {
-        await logErrorAndMaybeSendToBugsnag(
-            `Error looking up email "${email}" in slack: ${error}`
-        )
+        throw new Error(`Error looking up email "${email}" in slack: ${error}`)
     }
-    return
 }
 
 const MAX_SUCCESSIVE_FAILURES = 2

diff --git a/baker/GrapherBaker.tsx b/baker/GrapherBaker.tsx
@@ -29,7 +29,6 @@ import {
     getRelatedResearchAndWritingForVariable,
 } from "../db/model/Post.js"
 import {
-    JsonError,
     GrapherInterface,
     OwidVariableDataMetadataDimensions,
     DimensionProperty,
@@ -54,7 +53,7 @@ import {
     resolveFaqsForVariable,
 } from "./DatapageHelpers.js"
 import { getAllImages } from "../db/model/Image.js"
-import { logErrorAndMaybeSendToBugsnag } from "../serverUtils/errorLog.js"
+import { logErrorAndMaybeCaptureInSentry } from "../serverUtils/errorLog.js"
 
 import { getTagToSlugMap } from "./GrapherBakingUtils.js"
 import { knexRaw } from "../db/db.js"
@@ -156,8 +155,8 @@ export async function renderDataPageV2(
 
     if (faqResolveErrors.length > 0) {
         for (const error of faqResolveErrors) {
-            await logErrorAndMaybeSendToBugsnag(
-                new JsonError(
+            await logErrorAndMaybeCaptureInSentry(
+                new Error(
                     `Data page error in finding FAQs for variable ${variableId}: ${error.error}`
                 )
             )

diff --git a/baker/GrapherBakingUtils.ts b/baker/GrapherBakingUtils.ts
@@ -9,7 +9,6 @@ import { BAKED_SITE_EXPORTS_BASE_URL } from "../settings/clientSettings.js"
 
 import * as db from "../db/db.js"
 import { bakeGraphersToSvgs } from "../baker/GrapherImageBaker.js"
-import { warn } from "../serverUtils/errorLog.js"
 import { mapSlugsToIds } from "../db/model/Chart.js"
 import md5 from "md5"
 import { DbPlainTag, Url } from "@ourworldindata/utils"
@@ -70,13 +69,13 @@ export const bakeGrapherUrls = async (
 
         const slug = lodash.last(Url.fromURL(url).pathname?.split("/"))
         if (!slug) {
-            warn(`Invalid chart url ${url}`)
+            console.warn(`Invalid chart url ${url}`)
             continue
         }
 
         const chartId = slugToId[slug]
         if (chartId === undefined) {
-            warn(`Couldn't find chart with slug ${slug}`)
+            console.warn(`Couldn't find chart with slug ${slug}`)
             continue
         }
 
@@ -91,7 +90,7 @@ export const bakeGrapherUrls = async (
             [chartId]
         )
         if (!rows.length) {
-            warn(`Mysteriously missing chart by id ${chartId}`)
+            console.warn(`Mysteriously missing chart by id ${chartId}`)
             continue
         }
 
@@ -167,7 +166,7 @@ export async function getTagToSlugMap(
 
 /**
  * Given a topic tag's name or ID, return its slug
- * Throws an error if no slug is found so we can log it in Bugsnag
+ * Throws an error if no slug is found so we can log it in Sentry
  */
 export async function getSlugForTopicTag(
     knex: db.KnexReadonlyTransaction,

diff --git a/baker/MultiDimBaker.tsx b/baker/MultiDimBaker.tsx
@@ -8,7 +8,6 @@ import {
 } from "@ourworldindata/types"
 import {
     MultiDimDataPageConfig,
-    JsonError,
     keyBy,
     OwidVariableWithSource,
     pick,
@@ -28,7 +27,7 @@ import {
     getPrimaryTopic,
     resolveFaqsForVariable,
 } from "./DatapageHelpers.js"
-import { logErrorAndMaybeSendToBugsnag } from "../serverUtils/errorLog.js"
+import { logErrorAndMaybeCaptureInSentry } from "../serverUtils/errorLog.js"
 import {
     getAllMultiDimDataPages,
     getMultiDimDataPageBySlug,
@@ -83,8 +82,8 @@ const getFaqEntries = async (
 
         if (faqResolveErrors.length > 0) {
             for (const error of faqResolveErrors) {
-                void logErrorAndMaybeSendToBugsnag(
-                    new JsonError(
+                void logErrorAndMaybeCaptureInSentry(
+                    new Error(
                         `MDD baking error for page "${config.title}" in finding FAQs for variable ${metadata.id}: ${error.error}`
                     )
                 )
Original file line number	Diff line number	Diff line change
Expand Up		@@ -84,5 +84,3 @@ The following is an excerpt explaining the origin of this repo and what the alte
		---

		Cross-browser testing provided by <a href="https://www.browserstack.com"><img src="https://3fxtqy18kygf3on3bu39kh93-wpengine.netdna-ssl.com/wp-content/themes/browserstack/img/bs-logo.svg" /> BrowserStack</a>

		Client-side bug tracking provided by <a href="http://www.bugsnag.com/"><img width="110" src="https://images.typeform.com/images/QKuaAssrFCq7/image/default" /></a>