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

feat(solidstart): Add withSentry config wrapper #13784

Closed
wants to merge 10 commits into from

Conversation

andreiborza
Copy link
Member

@andreiborza andreiborza commented Sep 25, 2024

This PR adds a withSentry wrapper for SolidStart's config to build and place instrument.server.ts alongside the server build output so that it doesn't have to be placed in /public anymore to be discoverable.

It also adds an experimental option to top level import instrument.server.ts in the server entry file for platforms that currently do not support --import, overriding the start command or setting scoped NODE_OPTIONS (e.g. Vercel, Netlify (free version)).

The configuration now looks like this:

// app.config.ts
import { defineConfig } from '@solidjs/start/config';
import { sentrySolidStartVite, withSentry } from '@sentry/solidstart';

export default defineConfig(withSentry({
  // ...
  middleware: './src/middleware.ts',
  vite: {
    plugins: [
      sentrySolidStartVite({
        org: process.env.SENTRY_ORG,
        project: process.env.SENTRY_PROJECT,
        authToken: process.env.SENTRY_AUTH_TOKEN,
        // optional: if your `instrument.server.ts` file is not located inside `src`
        instrumentation: './mypath/instrument.server.ts'
      }),
    ],
  },
  // ...
}, { experimental_basicServerTracing: true }));

Note to reviewers
Unfortunately, we need both withSentry and sentrySolidStartVite because SolidStart's config allows the vite configuration to be a function that gets the current build target (i.e.server, client, server-fns) passed in and we cannot add our plugin first for this case as the function would have to be executed (and thus run all predefined plugins) before we can add our plugin.

The sentrySolidStartVite plugin is now automatically added by withSentry.

Closes: #13785

@andreiborza andreiborza added the Package: solidstart Issues related to the Sentry SolidStart SDK label Sep 25, 2024
@andreiborza andreiborza force-pushed the ab/solidstart-bundle-instrumentation-file branch 4 times, most recently from 5e1af7a to cc78ede Compare September 25, 2024 06:44
@andreiborza
Copy link
Member Author

I'm investigating my claim in the note to reviewers. I might be wrong here and we can simplify to just using withSentry.

Comment on lines 130 to 131
If your `instrument.server.ts` file is not located in the `src` folder, you can specify the path via the
`sentrySolidStartVite` plugin.
Copy link
Member

Choose a reason for hiding this comment

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

m: The option is instrumentation right? You could add this here, so it is clear where the path can be specified.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I updated the readme to make this clearer.

Comment on lines 126 to 128
Sentry relies on running `instrument.server.ts` as early as possible. Add the `sentrySolidStartVite` plugin from
`@sentry/solidstart` to your `app.config.ts`. This takes care of building `instrument.server.ts` and placing it
alongside the server entry file.
Copy link
Member

Choose a reason for hiding this comment

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

L: I would restructure this a little bit at it is hard to follow when reading it first. The first sentence says it relies on running the file as early as possible but the explanation for this comes later.

The proposed structure: do this -> why to do this -> what it does.

Suggested change
Sentry relies on running `instrument.server.ts` as early as possible. Add the `sentrySolidStartVite` plugin from
`@sentry/solidstart` to your `app.config.ts`. This takes care of building `instrument.server.ts` and placing it
alongside the server entry file.
Add the `sentrySolidStartVite` plugin from `@sentry/solidstart` to your plugins in `app.config.ts`. Sentry relies on running `instrument.server.ts` as early as possible. The `sentrySolidStartVite` plugin builds the `instrument.server.ts` file and places it alongside the server entry file.

Copy link
Member Author

@andreiborza andreiborza Sep 27, 2024

Choose a reason for hiding this comment

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

Thanks. I updated the readme and removed some of this explanation in favor of simplicity. Maybe this is better kept in the docs.


To upload source maps, configure an auth token. Auth tokens can be passed to the plugin explicitly with the `authToken`
option, with a `SENTRY_AUTH_TOKEN` environment variable, or with an `.env.sentry-build-plugin` file in the working
directory when building your project. We recommend you add the auth token to your CI/CD environment as an environment
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
directory when building your project. We recommend you add the auth token to your CI/CD environment as an environment
directory when building your project. We recommend adding the auth token to your CI/CD environment as an environment

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, updated.

@andreiborza andreiborza force-pushed the ab/solidstart-bundle-instrumentation-file branch from cc78ede to 084f7f1 Compare September 27, 2024 03:47
@andreiborza andreiborza force-pushed the ab/solidstart-bundle-instrumentation-file branch from 084f7f1 to 82e4c12 Compare September 27, 2024 03:52
Copy link

codecov bot commented Sep 27, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
231 1 230 8
View the full list of 1 ❄️ flaky tests
errorboundary.test.ts captures an exception

Flake rate in main: 29.41% (Passed 12 times, Failed 5 times)

Stack Traces | 30s run time
errorboundary.test.ts:4:1 captures an exception

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@andreiborza andreiborza requested a review from s1gr1d September 27, 2024 07:42
@andreiborza
Copy link
Member Author

Closing this for now as it needs to be reworked, using dynamic import instead of top-level-import of the instrumentation file to get around deploy platform restrictions of not being able to --import at the right time.

@s1gr1d s1gr1d self-assigned this Dec 16, 2024
s1gr1d added a commit that referenced this pull request Jan 8, 2025
…verSentry` (#14862)

This PR adds a `withSentry` wrapper for SolidStart's config to build and
place `instrument.server.ts` alongside the server build output so that
it doesn't have to be placed in `/public` anymore to be discoverable.

The setup is changed to be aligned with Nuxt. 
First, the `instrument.server.ts` file is added to the build output (the
sentry release injection file needs to be copied as well - this is not
ideal at the moment as there **could** be other imports as well, but
it's okay for now)

Then, there are two options to set up the SDK:
1. Users provide an `--import` CLI flag to their start command like
this:
```node --import ./.output/server/instrument.server.mjs .output/server/index.mjs```
2. Users can add `autoInjectServerSentry: 'top-level-import'` and the Sentry config will be imported at the top of the server entry

```typescript
// app.config.ts
import { defineConfig } from '@solidjs/start/config';
import { withSentry } from '@sentry/solidstart';

export default defineConfig(withSentry(
    { /* ... */ },
    { 
      autoInjectServerSentry: 'top-level-import' // optional
    })
 );
```

--- 
builds on top of the idea in this PR: #13784

---------

Co-authored-by: Andrei Borza <[email protected]>
s1gr1d added a commit that referenced this pull request Jan 22, 2025
…erSentry` (#14862)

This PR adds a `withSentry` wrapper for SolidStart's config to build and
place `instrument.server.ts` alongside the server build output so that
it doesn't have to be placed in `/public` anymore to be discoverable.

The setup is changed to be aligned with Nuxt.
First, the `instrument.server.ts` file is added to the build output (the
sentry release injection file needs to be copied as well - this is not
ideal at the moment as there **could** be other imports as well, but
it's okay for now)

Then, there are two options to set up the SDK:
1. Users provide an `--import` CLI flag to their start command like
this:
```node --import ./.output/server/instrument.server.mjs .output/server/index.mjs```
2. Users can add `autoInjectServerSentry: 'top-level-import'` and the Sentry config will be imported at the top of the server entry

```typescript
// app.config.ts
import { defineConfig } from '@solidjs/start/config';
import { withSentry } from '@sentry/solidstart';

export default defineConfig(withSentry(
    { /* ... */ },
    {
      autoInjectServerSentry: 'top-level-import' // optional
    })
 );
```

---
builds on top of the idea in this PR: #13784

---------

Co-authored-by: Andrei Borza <[email protected]>
s1gr1d added a commit that referenced this pull request Jan 23, 2025
…erSentry` (#14862)

This PR adds a `withSentry` wrapper for SolidStart's config to build and
place `instrument.server.ts` alongside the server build output so that
it doesn't have to be placed in `/public` anymore to be discoverable.

The setup is changed to be aligned with Nuxt.
First, the `instrument.server.ts` file is added to the build output (the
sentry release injection file needs to be copied as well - this is not
ideal at the moment as there **could** be other imports as well, but
it's okay for now)

Then, there are two options to set up the SDK:
1. Users provide an `--import` CLI flag to their start command like
this:
```node --import ./.output/server/instrument.server.mjs .output/server/index.mjs```
2. Users can add `autoInjectServerSentry: 'top-level-import'` and the Sentry config will be imported at the top of the server entry

```typescript
// app.config.ts
import { defineConfig } from '@solidjs/start/config';
import { withSentry } from '@sentry/solidstart';

export default defineConfig(withSentry(
    { /* ... */ },
    {
      autoInjectServerSentry: 'top-level-import' // optional
    })
 );
```

---
builds on top of the idea in this PR: #13784

---------

Co-authored-by: Andrei Borza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: solidstart Issues related to the Sentry SolidStart SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

instrument.server.mjs bundling
2 participants