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

Switch error tracking to Sentry #4409

Merged
merged 4 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .bundlewatch.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"files": [
{
"path": "./dist/assets/owid.mjs",
"maxSize": "2.6MB"
"maxSize": "2.8MB"
},
{
"path": "./dist/assets/owid.css",
Expand Down
21 changes: 21 additions & 0 deletions .github/workflows/sentry.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
name: Sentry Release
on: push

jobs:
create-release:
runs-on: ubuntu-latest

steps:
- name: Clone repository
uses: actions/checkout@v4
with:
fetch-depth: 0

- name: Create Sentry release
uses: getsentry/action-release@v1
env:
SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }}
SENTRY_ORG: ${{ secrets.SENTRY_ORG }}
with:
environment: ${{ github.ref_name == 'master' && 'production' || 'staging' }}
rakyi marked this conversation as resolved.
Show resolved Hide resolved
projects: admin website
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,6 @@ dist/
**/tsup.config.bundled*.mjs
cfstorage/
vite.*.mjs

# Sentry Config File
.env.sentry-build-plugin
2 changes: 0 additions & 2 deletions README.md
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>
14 changes: 13 additions & 1 deletion adminSiteClient/Admin.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as Sentry from "@sentry/react"
import ReactDOM from "react-dom"
import * as lodash from "lodash"
import { observable, computed, action } from "mobx"
Expand All @@ -6,6 +7,7 @@ import urljoin from "url-join"
import { AdminApp } from "./AdminApp.js"
import {
Json,
JsonError,
stringifyUnknownError,
queryParamsToStr,
} from "@ourworldindata/utils"
Expand All @@ -30,18 +32,26 @@ export class Admin {
@observable errorMessage?: ErrorMessage
basePath: string
username: string
email: string
isSuperuser: boolean
settings: ClientSettings

constructor(props: {
username: string
email: string
isSuperuser: boolean
settings: ClientSettings
}) {
this.basePath = "/admin"
this.username = props.username
this.email = props.email
this.isSuperuser = props.isSuperuser
this.settings = props.settings

Sentry.setUser({
username: this.username,
email: this.email,
})
}

@observable currentRequests: Promise<Response>[] = []
Expand Down Expand Up @@ -131,7 +141,9 @@ export class Admin {
text = await response.text()

json = JSON.parse(text)
if (json.error) throw json.error
if (json.error) {
throw new JsonError(json.error.message, json.error.status)
}
} catch (err) {
if (onFailure === "show")
this.setErrorMessage({
Expand Down
4 changes: 4 additions & 0 deletions adminSiteClient/admin.entry.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// This should be imported as early as possible so the global error handler is
// set up before any errors are thrown.
import "./instrument.js"

import "./admin.scss"
import "@ourworldindata/grapher/src/core/grapher.scss"
import "../explorerAdminClient/ExplorerCreatePage.scss"
Expand Down
12 changes: 12 additions & 0 deletions adminSiteClient/instrument.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as Sentry from "@sentry/react"
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have sentry and scope in the filename, I think

e.g.

initSentryAdminSiteClient.ts
initSentryServer.ts
initSentrySite.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to keep the name, because:

  • instrument is what is used in Sentry docs, so if you read the docs and then search for such file, you'll easily find all three of them. Scope is in the module path, i.e. the directory name, we don't usually repeat that in nested file names.
  • instrument is named by what it does, so if we ever want to add instrumentation of something else, we can.
  • We don't have to change the name, when we change providers. We also don't call files/folders as specific as mysql when we don't need to, but rather db.

import {
COMMIT_SHA,
ENV,
SENTRY_ADMIN_DSN,
} from "../settings/clientSettings.js"

Sentry.init({
dsn: SENTRY_ADMIN_DSN,
environment: ENV,
release: COMMIT_SHA,
})
12 changes: 7 additions & 5 deletions adminSiteServer/IndexPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,20 @@ import {
import { viteAssetsForAdmin } from "../site/viteUtils.js"

export const IndexPage = (props: {
email: string
username: string
isSuperuser: boolean
gitCmsBranchName: string
}) => {
const assets = viteAssetsForAdmin()
const script = `
window.isEditor = true
window.admin = new Admin({ username: "${
props.username
}", isSuperuser: ${props.isSuperuser.toString()}, settings: ${JSON.stringify(
{ ENV, GITHUB_USERNAME, DATA_API_FOR_ADMIN_UI }
)}})
window.admin = new Admin({
username: "${props.username}",
email: "${props.email}",
isSuperuser: ${props.isSuperuser.toString()},
settings: ${JSON.stringify({ ENV, GITHUB_USERNAME, DATA_API_FOR_ADMIN_UI })}
})
admin.start(document.querySelector("#app"), '${props.gitCmsBranchName}')
`

Expand Down
6 changes: 3 additions & 3 deletions adminSiteServer/apiRoutes/datasets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
assignTagsForCharts,
} from "../../db/model/Chart.js"
import { getDatasetById, setTagsForDataset } from "../../db/model/Dataset.js"
import { logErrorAndMaybeSendToBugsnag } from "../../serverUtils/errorLog.js"
import { logErrorAndMaybeCaptureInSentry } from "../../serverUtils/errorLog.js"
import { expectInt } from "../../serverUtils/serverUtil.js"
import {
syncDatasetToGitRepo,
Expand Down Expand Up @@ -299,7 +299,7 @@ export async function updateDataset(
commitEmail: _res.locals.user.email,
})
} catch (err) {
await logErrorAndMaybeSendToBugsnag(err, req)
await logErrorAndMaybeCaptureInSentry(err)
// Continue
}

Expand Down Expand Up @@ -363,7 +363,7 @@ export async function deleteDataset(
commitEmail: _res.locals.user.email,
})
} catch (err: any) {
await logErrorAndMaybeSendToBugsnag(err, req)
await logErrorAndMaybeCaptureInSentry(err)
// Continue
}

Expand Down
4 changes: 4 additions & 0 deletions adminSiteServer/app.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// This should be imported as early as possible so the global error handler is
// set up before any errors are thrown.
import "../serverUtils/instrument.js"

import { GIT_CMS_DIR } from "../gitCms/GitCmsConstants.js"
import {
ADMIN_SERVER_HOST,
Expand Down
33 changes: 7 additions & 26 deletions adminSiteServer/appClass.tsx
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -119,6 +100,7 @@ export class OwidAdminApp {
res.send(
renderToHtmlPage(
<IndexPage
email={res.locals.user.email}
username={res.locals.user.fullName}
isSuperuser={res.locals.user.isSuperuser}
gitCmsBranchName={this.gitCmsBranchName}
Expand Down Expand Up @@ -157,11 +139,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
Expand All @@ -179,6 +156,10 @@ export class OwidAdminApp {
app.use("/", mockSiteRouter)
}

// Add this after all routes, but before any other error-handling
// middlewares are defined.
Sentry.setupExpressErrorHandler(app)

// Give full error messages, including in production
app.use(this.errorHandler)

Expand Down
12 changes: 10 additions & 2 deletions adminSiteServer/authentication.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as Sentry from "@sentry/node"
import express from "express"
import crypto from "crypto"
import randomstring from "randomstring"
Expand Down Expand Up @@ -100,7 +101,7 @@ export async function authCloudflareSSOMiddleware(
res.cookie("sessionid", sessionId, {
httpOnly: true,
sameSite: "lax",
secure: ENV === "production",
secure: ENV !== "development",
ikesau marked this conversation as resolved.
Show resolved Hide resolved
})

// Prevents redirect to external URLs
Expand Down Expand Up @@ -194,6 +195,13 @@ export async function authMiddleware(
if (user?.isActive) {
res.locals.session = session
res.locals.user = user

Sentry.setUser({
id: user.id,
email: user.email,
username: user.fullName,
})

return next()
} else if (!req.path.startsWith("/admin") || req.path === "/admin/login")
return next()
Expand Down Expand Up @@ -327,7 +335,7 @@ export async function tailscaleAuthMiddleware(
res.cookie("sessionid", sessionId, {
httpOnly: true,
sameSite: "lax",
secure: ENV === "production",
secure: ENV !== "development",
})

// Save the sessionid in cookies for `authMiddleware` to log us in
Expand Down
3 changes: 0 additions & 3 deletions adminSiteServer/chartConfigR2Helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
() =>
Expand Down Expand Up @@ -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}`
)
Expand Down Expand Up @@ -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}`
)
Expand Down
8 changes: 5 additions & 3 deletions baker/DatapageHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
}
Expand Down
22 changes: 7 additions & 15 deletions baker/DeployUtils.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading