feat: add sentry#2516
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Review SummaryNice to see Sentry getting wired up. The wizard-generated scaffold is solid, but there are a handful of issues worth addressing before merge — flagged inline. Grouping them here by priority: Higher priority
Medium priority
Low priority / nits
Looks good
|
- Guard Sentry.setUser in setuserInfo: call setUser(null) on logout/null
data, fix "undefined undefined" username via nullish coalescing
- Initialize onRouterTransitionStart as undefined instead of {} to avoid
TypeError when Next.js calls it as a function in non-production
- Gate onRequestError behind NODE_ENV production check to match the
init guard in server/edge configs
- Remove dead beforeBreadcrumb from server and edge configs — DOM events
(ui.click/ui.input) never fire in Node.js or V8 edge runtimes
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- next.config.js: extract nextConfig variable so module.exports is assigned once; eliminates fragile double-assignment left by Sentry wizard - beforeBreadcrumb: guard with instanceof Element before reading tagName; EventTarget includes Text nodes, Document, and Window — tagName is undefined on these and .toLowerCase() throws, swallowing the breadcrumb - Sentry.setUser: gate behind NODE_ENV === 'production' to match the Sentry.init guard and avoid no-op churn on every setuserInfo call in dev - _error.js: drop await on captureUnderscoreErrorException; fire-and-forget so a slow network doesn't block the error page from rendering - Dockerfile: bump base image from 18.18.2-alpine3.17 to 18.20.4-alpine3.20 to satisfy @opentelemetry/context-async-hooks@2.2.0 (requires >=18.19.0) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review summarySolid Sentry integration overall — the structure follows the SDK's expected layout and the bug-fix list in the description shows the obvious foot-guns were already caught. A few things worth resolving before merging, plus some smaller cleanups left inline. Blocking-ish (worth fixing before this lands)
Worth doing
Smaller / optional
Nice work on |
Without the token, withSentryConfig silently skips source map uploads during npm run build, leaving Sentry with minified stack traces in prod. - Dockerfile: accept SENTRY_AUTH_TOKEN as a build arg and set it as an env var so the Sentry webpack plugin can read it during the build - cloudbuild.yaml / cloudbuildProduction.yaml: pull the token from GCP Secret Manager and forward it as --build-arg to docker build Requires sentry-auth-token to be created in GCP Secret Manager and the Cloud Build service account granted roles/secretmanager.secretAccessor. See PR description for setup steps. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review summarySolid first pass at Sentry integration — the wizard-generated boilerplate has been cleaned up and the bugs called out in the PR description (the Worth addressing before merge
Smaller cleanups
Things I'd flag but aren't this PR's problem
Things I checked and they look good
|
| ARG SENTRY_AUTH_TOKEN | ||
| ENV SENTRY_AUTH_TOKEN=$SENTRY_AUTH_TOKEN |
There was a problem hiding this comment.
Secret persists in the runtime image.
ENV SENTRY_AUTH_TOKEN=… writes the token into the image's environment layer, so it remains:
- Visible via
docker inspect/docker historyon anyone who can pull the image - Set as
process.env.SENTRY_AUTH_TOKENinside the running container atnpm start
The token only needs to exist for the npm run build step. Either inline it on the RUN line, or (better) use a BuildKit secret mount so it never lands in any layer:
| ARG SENTRY_AUTH_TOKEN | |
| ENV SENTRY_AUTH_TOKEN=$SENTRY_AUTH_TOKEN | |
| ARG SENTRY_AUTH_TOKEN | |
| RUN --mount=type=secret,id=sentry_auth_token,env=SENTRY_AUTH_TOKEN npm run build |
(cloudbuild.yaml would then mount the secret via --secret id=sentry_auth_token,env=SENTRY_AUTH_TOKEN instead of --build-arg.)
Lower-effort alternative if you'd rather not touch the cloudbuild files:
ARG SENTRY_AUTH_TOKEN
RUN SENTRY_AUTH_TOKEN=$SENTRY_AUTH_TOKEN npm run buildThis keeps the token scoped to that single RUN, so it isn't baked into the image config.
| dsn: 'https://4d93b3a97e10da5bc8817c46c1c79e9b@o162121.ingest.us.sentry.io/4510349072728064', | ||
|
|
||
| // Define how likely traces are sampled. Adjust this value in production, or use tracesSampler for greater control. | ||
| tracesSampleRate: 1, |
There was a problem hiding this comment.
tracesSampleRate: 1 sends a performance transaction for every page load / route transition on the client. The same value is set in sentry.server.config.js and sentry.edge.config.js, so server-side requests are also fully sampled.
For a public marketing site this can burn through the Sentry transaction quota fast and adds visible network overhead for users. Sentry's own guidance (docs) is to start much lower in production (e.g. 0.1 or even 0.01) and tune up if needed.
Consider either lowering the constant or reading it from an env var so it can be tuned per-environment without a redeploy:
tracesSampleRate: Number(process.env.NEXT_PUBLIC_SENTRY_TRACES_SAMPLE_RATE ?? 0.1),(Same change would apply to the server + edge configs.)
| if (elementType === 'button' && target.textContent) { | ||
| messages.push(`Button Text: "${target.textContent.trim()}"`); |
There was a problem hiding this comment.
Two concerns with using target.textContent here:
-
PII leakage —
sendDefaultPii: trueis already on, but button text that contains user-specific info (e.g.Delete invoice INV-2024-…,Sign out as alice@example.com) will land in breadcrumbs and from there into the events shown to whoever is on-call. Worth deciding whether that's intended. -
Unbounded length —
textContentreturns the concatenated text of all descendants. A button containing an icon + label is fine, but a click captured on an outer wrapper (or a button whose Sentry-resolved target is a parent due to event bubbling) can produce a multi-KB string. Sentry's payload limits will eventually truncate, but it's still wasted breadcrumb space.
A small cap is usually enough:
if (elementType === 'button' && target.textContent) {
messages.push(`Button Text: "${target.textContent.trim().slice(0, 100)}"`);
}
| // This file configures the initialization of Sentry for edge features (middleware, edge routes, and so on). | ||
| // The config you add here will be used whenever one of the edge features is loaded. | ||
| // Note that this config is unrelated to the Vercel Edge Runtime and is also required when running locally. | ||
| // https://docs.sentry.io/platforms/javascript/guides/nextjs/ | ||
|
|
||
| import * as Sentry from '@sentry/nextjs'; | ||
|
|
||
| if (process.env.NODE_ENV === 'production') { | ||
| Sentry.init({ | ||
| dsn: 'https://4d93b3a97e10da5bc8817c46c1c79e9b@o162121.ingest.us.sentry.io/4510349072728064', | ||
|
|
||
| // Define how likely traces are sampled. Adjust this value in production, or use tracesSampler for greater control. | ||
| tracesSampleRate: 1, | ||
|
|
||
| // Enable sending user PII (Personally Identifiable Information) | ||
| // https://docs.sentry.io/platforms/javascript/guides/nextjs/configuration/options/#sendDefaultPii | ||
| sendDefaultPii: true, | ||
|
|
||
| maxBreadcrumbs: 50, | ||
| normalizeDepth: 10, | ||
| }); | ||
| } |
There was a problem hiding this comment.
sentry.edge.config.js and sentry.server.config.js are byte-for-byte identical except for the leading comment. Since they'll have to be kept in sync every time the init options change (DSN rotation, sampling tweaks, integrations), consider extracting the shared options into a small helper and importing them in both files — or, if the Sentry SDK allows, re-exporting one from the other:
// sentry.edge.config.js
import './sentry.server.config';Not a blocker — just flagging that the duplication will quietly drift.
| Error.getInitialProps = ({ res, err }) => { | ||
| const statusCode = res ? res.statusCode : err ? err.statusCode : 404; | ||
| Error.getInitialProps = (contextData) => { | ||
| Sentry.captureUnderscoreErrorException(contextData); |
There was a problem hiding this comment.
Minor: Sentry.captureUnderscoreErrorException runs unconditionally, but Sentry.init is gated on NODE_ENV === 'production' in instrumentation-client.js. The SDK no-ops when uninitialized so this is harmless, but it's inconsistent with the gating pattern used everywhere else in this PR (setuserInfo, onRequestError, onRouterTransitionStart). Either gate it the same way or drop the gate from the other call sites — pick one convention so future readers don't have to wonder which is right.
Review summaryOverall the Sentry wiring is clean and the bug fixes called out in the PR description (the A few things worth addressing before merge — inline comments have details: Security
Tuning
Quality / consistency
Re: the two verification gates already called out in the PR description — both are well-flagged. The |
1 participant
Description
Add Sentry error tracking to the website/accounts app.
Resolves #2480
Changes
Sentry initialization
beforeBreadcrumbon the client to enrich UI click/input events with element type,data-cy, ID, name, and button textSentry.setUseron login; cleared withSentry.setUser(null)on logout_error.jsupdated to capture errors viacaptureUnderscoreErrorException(fire-and-forget, no await)SENTRY_AUTH_TOKENwired into the CI build step for source map uploadsBug fixes (from code review)
setuserInfo(null)now correctly callsSentry.setUser(null)instead of sending{ username: "undefined undefined" }to SentrySentry.setUsergated behindNODE_ENV === 'production'to match theSentry.initguard and avoid no-op churn on everysetuserInfocall in devonRouterTransitionStartinitialized asundefinedinstead of{}to avoidTypeErrorwhen Next.js calls it in non-productiononRequestErrorgated behindNODE_ENV === 'production'to match the init guard in server/edge configsbeforeBreadcrumbguarded withinstanceof Elementbefore readingtagName—EventTargetincludesText,Document, andWindowwhich have notagName; the throw would silently swallow the entire breadcrumb pipelinebeforeBreadcrumbfrom server and edge configs (DOM events never fire in Node.js/edge runtimes)next.config.js: extractednextConfigvariable somodule.exportsis assigned once, eliminating the fragile double-assignment left by the Sentry wizardDocker / CI fixes
.tsto.jsto avoid requiringtypescriptas a production dependency during the Docker buildnode:18.18.2-alpine3.17tonode:18.20.4-alpine3.20—@sentry/nextjspulls in@opentelemetry/context-async-hooks@2.2.0which requiresnode ^18.19.0 || >=20.6.0SENTRY_AUTH_TOKENpassed as a Docker build arg incloudbuild.yamlandcloudbuildProduction.yamlso source maps are uploaded during production buildsType of change
instrumentation-client.jsis an officially supported convention in Next.js 15.3+, but this project is on Next.js^14.1.4.@sentry/nextjs@10's webpack plugin does inject this file into the client bundle for older Next.js versions, but this is undocumented behavior — if it doesn't work silently, client-side errors will go uncaptured with no build warning.Please verify on staging before merging:
window.__SENTRY__— should return the Sentry client object, notundefinedSentry.captureException(new Error('sentry test'))and confirm the event appears in the Sentry UI underwebsite-accountsIf
window.__SENTRY__isundefined, fall back to thesentry.client.config.jsconvention which@sentry/nextjsofficially supports for Next.js 14.cloudbuild.yamlandcloudbuildProduction.yamlnow passSENTRY_AUTH_TOKENas a Docker build arg via Cloud Build's Secret Manager integration. This will not work until the following is set up by someone with GCP access:Create the secret in Secret Manager:
Grant the Cloud Build service account access to the secret:
Until this is done, production builds will fail at the Build step. The
SENTRY_AUTH_TOKENvalue is the same token already stored in GitHub Actions secrets.Note
Requires
SENTRY_AUTH_TOKENto be added to repository secrets (already added for CI).