Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2628 +/- ##
==========================================
- Coverage 94.03% 93.78% -0.25%
==========================================
Files 348 349 +1
Lines 4492 4508 +16
Branches 588 588
==========================================
+ Hits 4224 4228 +4
- Misses 267 279 +12
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🔎 A preview deployment of this pull request is available at https://utilities.philanthropydatacommons.org/pdc-service-preview-2628-btkjd/ (and upstream https://pdc-service-preview-2628-btkjd.ondigitalocean.app) assuming this PR is still open. 🔍 |
bickelj
left a comment
There was a problem hiding this comment.
I know this is draft, just noticed it and thought I'd give some preliminary feedback.
a947ce5 to
921b8fc
Compare
921b8fc to
cbba7a4
Compare
cbba7a4 to
0212388
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two unresolved critical issues remain: startup failures can leave the process alive without serving, and arbitrary non-Error values may leak sensitive data to Sentry despite the documented privacy guarantee.
2 open issues
What changed in this PR
This pull request adds opt-in Sentry error reporting for HTTP 5xx responses, startup and migration failures, uncaught process errors, and Graphile Worker failures, with privacy-focused request-data restrictions and deployment documentation.
Changes:
- Initializes Sentry for the API and migration entry points.
- Reports server, startup, migration, worker, and process-level failures.
- Adds tests for 4xx/5xx reporting behavior.
- Adds Sentry configuration, dependencies, environment variables, and operational documentation.
The pull request requires changes to ensure startup failures terminate the process, preserve Sentry’s default process-level integrations, and sanitize non-Error values before reporting them.
| File | Description |
|---|---|
src/scripts/migrate.ts |
Reports migration failures and flushes Sentry events before shutdown. |
src/middleware/errorHandler.ts |
Reports HTTP 5xx errors while excluding 4xx responses. |
src/middleware/__tests__/errorHandler.unit.test.ts |
Tests Sentry reporting behavior for server and client errors. |
src/jobQueue.ts |
Reports Graphile Worker runner failures. |
src/instrument.ts |
Initializes Sentry and restricts collected request data. |
src/index.ts |
Initializes Sentry and reports startup failures. |
src/constants/time.ts |
Adds the Sentry flush timeout constant. |
package.json |
Adds the Sentry Node SDK dependency. |
package-lock.json |
Locks the Sentry SDK and transitive dependencies. |
docs/SENTRY.md |
Documents reporting scope, privacy, environments, and configuration. |
docs/HOSTING.md |
Links hosting documentation to Sentry setup guidance. |
AGENTS.md |
Documents Sentry configuration and initialization files. |
.env.example |
Adds Sentry-related environment variable examples. |
.do/deploy.template.yaml |
Adds Sentry deployment environment variables. |
Review effort: Lite
|
Re codecov: the lines changed are entry level files, and I don't think we necessarily want / need test coverage for those lines. |
Without error reporting, the team learns about failures only when a PDC user reports them. 4xx responses are left out because they mean the service correctly rejected a bad request. Issue #2497 [Enhancement]: Add log calls (e.g., Sentry) for errors or performance problems Claude-Session: 45c591f5-c6a1-4f19-ae57-70e3001990b4
0212388 to
3e24798
Compare

This is the first, errors-only stage of #2497. It follows the plan in #2497 (comment).
Sentry stays off unless
SENTRY_DSNis set, so local development and the tests are unaffected.What gets reported:
errorHandleranswers with a 5xx (4xx responses are not reported)Notes for review:
errorHandlerreports instead.src/instrument.tsturns all of that off and allows only thecontent-type,content-lengthanduser-agentheaders.node --import. SDK v11 no longer supports--requirefor loading Sentry before the rest of the app.Before merging, each deployment needs
SENTRY_DSNandSENTRY_ENVIRONMENTset (seedocs/SENTRY.md). The Sentry project doesn't exist yet, so this stays a draft until it does.🤖 Generated with Claude Code
Claude-Session: 45c591f5-c6a1-4f19-ae57-70e3001990b4