-
Notifications
You must be signed in to change notification settings - Fork 0
Kasper/sentry fork #29
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
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # .github/workflows/deploy.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates the Sentry integration from v9 to v10, implementing a new initialization approach using Node.js's --import flag for server-side instrumentation. The changes enable better distributed tracing between client and server, add Spotlight support for local development, and include example routes for testing the Sentry integration.
Key Changes:
- Upgraded Sentry packages from v9.32.0 to v10.32.1 and associated OpenTelemetry dependencies
- Created a new
instrument.server.tsfile that initializes Sentry before application code loads - Modified npm scripts to use
--import ./instrument.server.tsfor proper server-side initialization - Added Sentry example routes for testing error capture and distributed tracing
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated Sentry packages to v10.32.1 and added @sentry/cli; modified scripts to use --import flag |
| package-lock.json | Lock file updates for Sentry v10 and OpenTelemetry v2 dependencies |
| instrument.server.ts | New server-side Sentry initialization file using the v10 API |
| server/index.ts | Removed conditional Sentry initialization logic; now relies on --import preloading |
| server/utils/monitoring.ts | Deleted file - functionality moved to instrument.server.ts |
| app/entry.server.tsx | Integrated Sentry wrappers for request handling and error handling |
| app/entry.client.tsx | Moved client-side Sentry initialization inline; added Spotlight and reactRouterTracingIntegration |
| app/utils/monitoring.client.tsx | Deleted file - functionality moved to entry.client.tsx |
| vite.config.ts | Simplified Sentry build configuration for v10 API |
| app/routes/sentry/example-page.tsx | New test page for validating Sentry error capture and connectivity |
| app/routes/sentry/api.sentry-example-api.ts | New API route for backend error testing |
| other/Dockerfile | Added hardcoded SENTRY_ORG and SENTRY_PROJECT values; added SENTRY_RELEASE environment variable |
| .github/workflows/deploy.yml | Added Sentry release creation steps and SENTRY_ENVIRONMENT configuration |
| docs/monitoring.md | Updated documentation to reflect v10 setup and new workflow |
| .env.example | Added SENTRY_DSN and SENTRY_ENVIRONMENT examples |
| playwright.config.ts | Added SENTRY_ENVIRONMENT test configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `SENTRY_ORG` and `SENTRY_PROJECT` values. Setup up your secrets in | ||
| `SENTRY_ORG` and `SENTRY_PROJECT` values, both in | ||
| [Dockerfile](../other/Dockerfile) and in the "Create Sentry release" step of the | ||
| [`deploy`](../.github/workflows/deploy.yml) script. Setup up your secrets in |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in the documentation: "Setup up your secrets" should be "Set up your secrets".
| [`deploy`](../.github/workflows/deploy.yml) script. Setup up your secrets in | |
| [`deploy`](../.github/workflows/deploy.yml) script. Set up your secrets in |
| import * as Sentry from '@sentry/react-router' | ||
| import { requireUserWithRole } from '../../utils/permissions.server.ts' | ||
| import { type Route } from './+types/api.sentry-example-api.ts' | ||
|
|
||
| class SentryExampleBackendError extends Error { | ||
| constructor(message: string | undefined) { | ||
| super(message) | ||
| this.name = 'SentryExampleBackendError' | ||
| } | ||
| } | ||
|
|
||
| export async function loader({ request }: Route.LoaderArgs) { | ||
| await requireUserWithRole(request, 'admin') | ||
|
|
||
| await Sentry.startSpan( | ||
| { | ||
| name: 'Example Backend Span', | ||
| op: 'test', | ||
| }, | ||
| async () => { | ||
| // Simulate some backend work | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) | ||
| }, | ||
| ) | ||
|
|
||
| throw new SentryExampleBackendError( | ||
| 'This error is raised on the backend API route.', | ||
| ) | ||
| } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new Sentry example routes (example-page.tsx and api.sentry-example-api.ts) lack test coverage. Since these routes are intended to validate Sentry integration, consider adding tests to ensure they function correctly and that the error capturing works as expected.
| HONEYPOT_SECRET="super-duper-s3cret" | ||
| RESEND_API_KEY="re_blAh_blaHBlaHblahBLAhBlAh" | ||
| SENTRY_DSN="your-dsn" | ||
| SENTRY_DSN="https://be85ef67af06663b750a36fd66223526@o4505900870467584.ingest.us.sentry.io/4510528957317120" |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Sentry DSN is exposed in the .env.example file. This appears to be a real DSN token that could be used to send data to your Sentry project. Even for example files, it's better to use a placeholder value to prevent accidental usage of production/test credentials.
| SENTRY_DSN="https://be85ef67af06663b750a36fd66223526@o4505900870467584.ingest.us.sentry.io/4510528957317120" | |
| SENTRY_DSN="<your-sentry-dsn-here>" |
| SENTRY_ORG: kasper-peulen-d2924a93 | ||
| SENTRY_PROJECT: epic-rsc-stack |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Sentry organization and project identifiers are hardcoded in the GitHub Actions workflow. These should be extracted to GitHub repository variables or secrets to make this workflow reusable across different projects and prevent accidental deployments to the wrong Sentry project.
| SENTRY_ORG: kasper-peulen-d2924a93 | |
| SENTRY_PROJECT: epic-rsc-stack | |
| SENTRY_ORG: ${{ vars.SENTRY_ORG }} | |
| SENTRY_PROJECT: ${{ vars.SENTRY_PROJECT }} |
| SENTRY_ORG: kasper-peulen-d2924a93 | ||
| SENTRY_PROJECT: epic-rsc-stack |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Sentry organization and project identifiers are hardcoded in the production deployment step. These should be extracted to GitHub repository variables or secrets to make this workflow reusable across different projects and prevent accidental deployments to the wrong Sentry project.
| SENTRY_ORG: kasper-peulen-d2924a93 | |
| SENTRY_PROJECT: epic-rsc-stack | |
| SENTRY_ORG: ${{ vars.SENTRY_ORG }} | |
| SENTRY_PROJECT: ${{ vars.SENTRY_PROJECT }} |
| ENV SENTRY_ORG="kasper-peulen-d2924a93" | ||
| ENV SENTRY_PROJECT="epic-rsc-stack" |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Sentry organization and project identifiers are hardcoded in the Dockerfile. These should be configurable via build arguments or environment variables to allow different projects to use this setup without modifying the Dockerfile.
| release: { | ||
| name: process.env.COMMIT_SHA, | ||
| inject: true, | ||
| }, |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sentryConfig object has been significantly simplified from the previous version, removing important configuration options like authToken, org, project, setCommits.auto, and sourcemaps.filesToDeleteAfterUpload. While the new v10 API may handle these differently, verify that source map uploads and commit association are still working correctly. The documentation changes suggest that commit association is now handled in the GitHub Actions workflow, but source map cleanup configuration appears to be missing.
| }, | |
| }, | |
| sourcemaps: { | |
| // Ensure uploaded source maps are cleaned up from the build output | |
| filesToDeleteAfterUpload: ['dist/assets/**/*.map'], | |
| }, |
| dsn: | ||
| process.env.NODE_ENV === 'production' ? process.env.SENTRY_DSN : undefined, |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition checks if NODE_ENV is 'production' to determine if the DSN should be used, but then also checks for spotlight which is enabled in 'development'. This means Sentry won't be initialized with a DSN in development mode. If you want Sentry to work in development (with Spotlight), you should either pass the DSN in development mode too, or ensure the check is consistent with your intentions.
| dsn: | |
| process.env.NODE_ENV === 'production' ? process.env.SENTRY_DSN : undefined, | |
| dsn: process.env.SENTRY_DSN, |
| denyUrls: [ | ||
| /\/resources\/healthcheck/, | ||
| // TODO: be smarter about the public assets... | ||
| /\/favicons\//, | ||
| /\/img\//, | ||
| /\/fonts\//, | ||
| /\/favicon.ico/, | ||
| /\/site\.webmanifest/, | ||
| ], |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The denyUrls pattern '//build//' was removed from the server-side configuration but kept in the client-side configuration in app/entry.client.tsx. If this was intentional because build assets are only relevant on the client, that's fine. However, ensure this is consistent with your error tracking strategy, as server-side requests to build assets might still occur in some scenarios.
| integrations: [ | ||
| Sentry.reactRouterTracingIntegration(), | ||
| Sentry.replayIntegration(), | ||
| ], |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The browserProfilingIntegration was removed from the client-side Sentry initialization. If browser profiling was previously enabled and desired, verify this removal is intentional. If profiling is still needed, ensure it's configured correctly with the new v10 API.
Test Plan
Checklist
Screenshots