feat(observability): add env-gated Sentry error tracking - #671
Conversation
samxu01
left a comment
There was a problem hiding this comment.
Review verdict: changes requested (submitted as comment — REQUEST_CHANGES 422s from this identity).
Reviewed against the #664 spec and REVIEW.md, with focus on the privacy gate. The gate itself is right on both sides: no DSN → Sentry.init never runs (unit-tested), the frontend chunk is dead-code-eliminated via the vite define, Helm defaults off with the ESO entry correctly values-gated, and the runbook covers ordering, spike alerts, and rollback. Lockfile adds resolve to registry.npmjs.org only.
Important — one change requested
backend/instrument.tsbeforeSend(and the identical scrub infrontend/src/sentry.ts): the scrub deletesuser,request.headers, andrequest.cookiesbut keepsrequest.dataandrequest.query_string. Sentry's Express request-data integration attaches the request body by default, so an unhandled error onPOST /api/auth/*would ship the login body (email + password) to Sentry, and query strings can carry tokens. Adddelete scrubbedEvent.request.data;anddelete scrubbedEvent.request.query_string;to both scrubs, and extend the two "scrubs sensitive request data" tests to includedata+query_stringin the fixture and assert they're gone. The URL + method that remain are enough for debugging.
Everything else is approve-ready as-is; no other changes needed.
|
@samxu01 review fix pushed in
Ready for the fast re-review. |
samxu01
left a comment
There was a problem hiding this comment.
Re-review verdict: APPROVE — ready to merge (submitted as comment; both APPROVE and REQUEST_CHANGES 422 from this bot identity on this PR).
Re-reviewed 5d280418: both beforeSend scrubbers now delete request.data and request.query_string in addition to user/headers/cookies, and both tests exercise a login-style body + token query with explicit removal assertions plus a strict toEqual on the surviving shape (url + method only). The privacy concern from my previous review is fully addressed; everything else is unchanged from the already-approve-ready state.
One non-blocking doc nit, fine in a follow-up: docs/runbooks/error-tracking.md §Privacy stance still says the scrub removes "the user object and all request headers and cookies" — it now also removes request bodies and query strings. Doc understates the protection (safe direction), but keep it accurate.
Closes #664
Summary
@sentry/nodebefore all backend imports and attach its Express error handler only whenSENTRY_DSNis present@sentry/reactchunk only when the compile-time frontend DSN is presenterrorTracking.enabledVerification
/v2/landingrendered with HTTP 200, no console errors, and no Sentry/ingest/envelope resources or requestsRollout note
Do not dispatch
Deploy Devuntilcommonly-dev-sentry-dsnexists in GCP Secret Manager andDEV_SENTRY_FRONTEND_DSNexists in the GitHubdevenvironment. A missing remoteRef prevents the wholeapi-keysExternalSecret from reconciling; the runbook preserves the required ordering.